On Thu, Jun 27, 2019 at 1:18 PM Konstantin Belousov <kostik...@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.
>
> > +/* 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.
>
> > nbp->b_xflags |= BX_ALTDATA;
> > return (0);
> > } else {
> > - panic("ufs_bmaparray: blkno out of range");
> > + /* blkno out of range */
> > + return (EINVAL);
> > }
> > /*
> > * Since this is FFS independent code, we are out of
_______________________________________________
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
