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

[Scott Long]

> On Jun 27, 2019, at 3:09 PM, Konstantin Belousov <kostikbel at gmail.com> wrote:
> 
> 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.

I’m firmly in agreement on changing panics into errors.  When callers are
encountered that don’t properly check and propagate the errors, they
should be fixed too.  I’m not saying that it’s an easy process and that
we should just delete all panics, but it’s a goal that should be vigorously
pursued.

Scott