Date: Thu, 27 Jun 2019 15:48:23 -0600 From: Scott Long <scottl@samsco.org> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Alan Somers <asomers@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, svn-src-head <svn-src-head@freebsd.org> Subject: Re: svn commit: r349231 - in head/sys: kern sys ufs/ufs Message-ID: <0988866D-918B-4800-ADE7-938E1EBD729C@samsco.org> In-Reply-To: <20190627210940.GR8697@kib.kiev.ua> References: <201906201413.x5KEDB5u010923@repo.freebsd.org> <20190627191843.GQ8697@kib.kiev.ua> <CAOtMX2hO3ed=GevyDmzh8VOVT3Zw5yYNKpCBENrWSm_w2nuM3g@mail.gmail.com> <20190627210940.GR8697@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
> On Jun 27, 2019, at 3:09 PM, Konstantin Belousov <kostikbel@gmail.com> = wrote: >=20 > 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@gmail.com> wrote: >>>=20 >>> 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 >>>>=20 >>>> Log: >>>> Add FIOBMAP2 ioctl >>>=20 >>>>=20 >>>> 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. >>>>=20 >>>> Reviewed by: mckusick >>>> MFC after: 2 weeks >>>> Sponsored by: The FreeBSD Foundation >>>> Differential Revision: https://reviews.freebsd.org/D20705 >>>>=20 >>>> Modified: >>>> head/sys/kern/vfs_vnops.c >>>> head/sys/sys/filio.h >>>> head/sys/ufs/ufs/ufs_bmap.c >>>>=20 >>>> Modified: head/sys/kern/vfs_vnops.c >>>> = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D >>>> --- 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); >>>> } >>>>=20 >>>> +/* 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. >>=20 >> Ok. It will increase the number of function arguments, which is = ugly, >> but I can do it if you prefer. >>=20 >>>=20 >>>> +{ >>>> + struct vnode *vp =3D 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). >>=20 >> Ok. >>=20 >>>=20 >>>> + daddr_t lbn =3D arg->bn; >>> Style: initialization in declaration. >>>=20 >>>> + int error; >>>> + >>>> + vn_lock(vp, LK_SHARED | LK_RETRY); >>>> +#ifdef MAC >>>> + error =3D mac_vnode_check_read(cred, fp->f_cred, vp); >>>> + if (error =3D=3D 0) >>>> +#endif >>>> + error =3D VOP_BMAP(vp, lbn, NULL, &arg->bn, = &arg->runp, >>>> + &arg->runb); >>> Wrong indent for continuation line. >>>=20 >>>> + 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 =3D=3D 0) >>>> *(int *)data =3D 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. >>>=20 >>>> case FIONBIO: >>>> case FIOASYNC: >>>> return (0); >>>>=20 >>>> Modified: head/sys/sys/filio.h >>>> = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D >>>> --- 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. >>=20 >> 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. >=20 >>=20 >>>=20 >>>> +/* Get the file's bmap info for the logical block bn */ >>>> +#define FIOBMAP2 _IOWR('f', 99, struct fiobmap2_arg) >>>>=20 >>>> #ifdef _KERNEL >>>> #ifdef COMPAT_FREEBSD32 >>>>=20 >>>> Modified: head/sys/ufs/ufs/ufs_bmap.c >>>> = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D >>>> --- 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 =3D blkptrtodb(ump, = ip->i_din2->di_extb[-1 - bn]); >>>> if (*bnp =3D=3D 0) >>>> *bnp =3D -1; >>>> - if (nbp =3D=3D NULL) >>>> - panic("ufs_bmaparray: mapping ext = data"); >>>> + if (nbp =3D=3D NULL) { >>>> + /* indirect block not found */ >>>> + return (EINVAL); >>>> + } >>> This (and next chunk) loose useful checks for in-kernel interfaces. >>=20 >> 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. I=E2=80=99m firmly in agreement on changing panics into errors. When = callers are encountered that don=E2=80=99t properly check and propagate the errors, = they should be fixed too. I=E2=80=99m not saying that it=E2=80=99s an easy = process and that we should just delete all panics, but it=E2=80=99s a goal that should be = vigorously pursued. Scott
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?0988866D-918B-4800-ADE7-938E1EBD729C>