From owner-svn-src-all@freebsd.org Thu Jun 27 20:01:30 2019 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id A392A15CC07F; Thu, 27 Jun 2019 20:01:30 +0000 (UTC) (envelope-from asomers@gmail.com) Received: from mail-lj1-f193.google.com (mail-lj1-f193.google.com [209.85.208.193]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 33E526DC07; Thu, 27 Jun 2019 20:01:30 +0000 (UTC) (envelope-from asomers@gmail.com) Received: by mail-lj1-f193.google.com with SMTP id x25so3647362ljh.2; Thu, 27 Jun 2019 13:01:30 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Jc4DzYF5DK0JoRGxBnjUHfDk3UazJPSqip78WXkgR70=; b=P+Sf1GZuDxKmhMSCvrBXtEN6d9T1iec1qQyXjHKczmoNXNhcGTccyM6DQYgchdVRNk abSYeAS+Ka7o/Ae8Sn2+1p0tZpu1rsyrawEYMRMUg6CstZcSNc8sppAUnouk1ongBPZS KYz9ZLQHo0ZR6qGbGd54fBYNbYN1tqNv48ivJPq2DPmlmctJxFXjCqCkNFcPRZiqEQXK GCH0BE4aBsFMGi+EqHLfBLjq94a5JaleOQ9E85IJoChr/gpfCjGuSFfvHVnwoT5TvQYJ 5uhoc6qGactWRszYJ8QPynYElnyrtrmOPq3TLhpv25HuhI9hvNq61xqh/tuzQtggTILg 9/DA== X-Gm-Message-State: APjAAAUZKe8STtDwvRhOEKhMFueDeIRriBq8tKCEycPGJ+LqYpodjHpc FQSidrYVay/FGsEIYFoTn17g7YprCvtMN7usdYk= X-Google-Smtp-Source: APXvYqzH5Ft4rvMGCg69K7oL89kza2bd6n87Jvaq00OTodkh8cjEyymNU2PpJCCzrIydxy7sUdWV8SyMPvCuFgWB7a0= X-Received: by 2002:a2e:9cd1:: with SMTP id g17mr3912501ljj.234.1561665683465; Thu, 27 Jun 2019 13:01:23 -0700 (PDT) MIME-Version: 1.0 References: <201906201413.x5KEDB5u010923@repo.freebsd.org> <20190627191843.GQ8697@kib.kiev.ua> In-Reply-To: <20190627191843.GQ8697@kib.kiev.ua> From: Alan Somers Date: Thu, 27 Jun 2019 14:01:11 -0600 Message-ID: Subject: Re: svn commit: r349231 - in head/sys: kern sys ufs/ufs To: Konstantin Belousov Cc: src-committers , svn-src-all , svn-src-head Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 33E526DC07 X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.86 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_SHORT(-0.86)[-0.858,0]; REPLY(-4.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 27 Jun 2019 20:01:31 -0000 On Thu, Jun 27, 2019 at 1:18 PM Konstantin Belousov 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