svn commit: r349231 - in head/sys: kern sys ufs/ufs

[Konstantin Belousov]

On Thu, Jun 27, 2019 at 02:01:11PM -0600, Alan Somers wrote:
> On Thu, Jun 27, 2019 at 1:18 PM Konstantin Belousov <kostikbel at gmail.com> wrote:
> >
> > On Thu, Jun 20, 2019 at 02:13:11PM +0000, Alan Somers wrote:
> > > Author: asomers
> > > Date: Thu Jun 20 14:13:10 2019
> > > New Revision: 349231
> > > URL: https://svnweb.freebsd.org/changeset/base/349231
> > >
> > > Log:
> > >   Add FIOBMAP2 ioctl
> >
> > >
> > >   This ioctl exposes VOP_BMAP information to userland. It can be used by
> > >   programs like fragmentation analyzers and optimized cp implementations. But
> > >   I'm using it to test fusefs's VOP_BMAP implementation. The "2" in the name
> > >   distinguishes it from the similar but incompatible FIBMAP ioctls in NetBSD
> > >   and Linux.  FIOBMAP2 differs from FIBMAP in that it uses a 64-bit block
> > >   number instead of 32-bit, and it also returns runp and runb.
> > >
> > >   Reviewed by:        mckusick
> > >   MFC after:  2 weeks
> > >   Sponsored by:       The FreeBSD Foundation
> > >   Differential Revision:      https://reviews.freebsd.org/D20705
> > >
> > > Modified:
> > >   head/sys/kern/vfs_vnops.c
> > >   head/sys/sys/filio.h
> > >   head/sys/ufs/ufs/ufs_bmap.c
> > >
> > > Modified: head/sys/kern/vfs_vnops.c
> > > ==============================================================================
> > > --- head/sys/kern/vfs_vnops.c Thu Jun 20 13:59:46 2019        (r349230)
> > > +++ head/sys/kern/vfs_vnops.c Thu Jun 20 14:13:10 2019        (r349231)
> > > @@ -1458,6 +1458,25 @@ vn_stat(struct vnode *vp, struct stat *sb, struct ucre
> > >       return (0);
> > >  }
> > >
> > > +/* generic FIOBMAP2 implementation */
> > > +static int
> > > +vn_ioc_bmap2(struct file *fp, struct fiobmap2_arg *arg, struct ucred *cred)
> > I do not like the fact that internal kernel function takes the
> > user-visible structure which results in the mix of ABI and KBI.
> > Traditionally we pass explicit arguments to kern_XXX, vn_XXX and similar
> > internal implementations.
> 
> Ok.  It will increase the number of function arguments, which is ugly,
> but I can do it if you prefer.
> 
> >
> > > +{
> > > +     struct vnode *vp = fp->f_vnode;
> > Why do you pass fp to the function that
> > 1. Has vn_ namespace, i.e. operating on vnode.
> > 2. Only needs vnode to operate on.
> > Please change the argument from fp to vp.  You would need to pass f_cred
> > as additional argument, or move mac check to vn_ioctl (I think this is
> > a better approach).
> 
> Ok.
> 
> >
> > > +     daddr_t lbn = arg->bn;
> > Style: initialization in declaration.
> >
> > > +     int error;
> > > +
> > > +     vn_lock(vp, LK_SHARED | LK_RETRY);
> > > +#ifdef MAC
> > > +     error = mac_vnode_check_read(cred, fp->f_cred, vp);
> > > +     if (error == 0)
> > > +#endif
> > > +             error = VOP_BMAP(vp, lbn, NULL, &arg->bn, &arg->runp,
> > > +                     &arg->runb);
> > Wrong indent for continuation line.
> >
> > > +     VOP_UNLOCK(vp, 0);
> > > +     return (error);
> > > +}
> > > +
> > >  /*
> > >   * File table vnode ioctl routine.
> > >   */
> > > @@ -1481,6 +1500,9 @@ vn_ioctl(struct file *fp, u_long com, void *data, stru
> > >                       if (error == 0)
> > >                               *(int *)data = vattr.va_size - fp->f_offset;
> > >                       return (error);
> > > +             case FIOBMAP2:
> > > +                     return (vn_ioc_bmap2(fp, (struct fiobmap2_arg*)data,
> > Need space between fiobmap2_arg and '*'.
> > > +                             active_cred));
> > Wrong indent.
> >
> > >               case FIONBIO:
> > >               case FIOASYNC:
> > >                       return (0);
> > >
> > > Modified: head/sys/sys/filio.h
> > > ==============================================================================
> > > --- head/sys/sys/filio.h      Thu Jun 20 13:59:46 2019        (r349230)
> > > +++ head/sys/sys/filio.h      Thu Jun 20 14:13:10 2019        (r349231)
> > > @@ -62,6 +62,13 @@ struct fiodgname_arg {
> > >  /* Handle lseek SEEK_DATA and SEEK_HOLE for holey file knowledge. */
> > >  #define      FIOSEEKDATA     _IOWR('f', 97, off_t)   /* SEEK_DATA */
> > >  #define      FIOSEEKHOLE     _IOWR('f', 98, off_t)   /* SEEK_HOLE */
> > > +struct fiobmap2_arg {
> > > +     int64_t bn;
> > > +     int     runp;
> > > +     int     runb;
> > > +};
> > This structure has different layout for LP64 and ILP32, and you did not
> > provided the compat shims.
> 
> Really?  How so?  All of the fields have the same width on 64 and 32
> bit archs, and there shouldn't be any need for padding, either.
Sorry, you are right.

> 
> >
> > > +/* Get the file's bmap info for the logical block bn */
> > > +#define FIOBMAP2     _IOWR('f', 99, struct fiobmap2_arg)
> > >
> > >  #ifdef _KERNEL
> > >  #ifdef COMPAT_FREEBSD32
> > >
> > > Modified: head/sys/ufs/ufs/ufs_bmap.c
> > > ==============================================================================
> > > --- head/sys/ufs/ufs/ufs_bmap.c       Thu Jun 20 13:59:46 2019        (r349230)
> > > +++ head/sys/ufs/ufs/ufs_bmap.c       Thu Jun 20 14:13:10 2019        (r349231)
> > > @@ -200,12 +200,15 @@ ufs_bmaparray(vp, bn, bnp, nbp, runp, runb)
> > >                       *bnp = blkptrtodb(ump, ip->i_din2->di_extb[-1 - bn]);
> > >                       if (*bnp == 0)
> > >                               *bnp = -1;
> > > -                     if (nbp == NULL)
> > > -                             panic("ufs_bmaparray: mapping ext data");
> > > +                     if (nbp == NULL) {
> > > +                             /* indirect block not found */
> > > +                             return (EINVAL);
> > > +                     }
> > This (and next chunk) loose useful checks for in-kernel interfaces.
> 
> mckusick disagrees with you on this.  He thought that changing the
> panics into errors was a good idea.  Note that ufs_bmap was already
> capable of returning errors that not all callers check.
For user-callable code, this is of course the requirement.  But for in-kernel
use, IMO it makes the interfaces loose.