From owner-svn-src-all@freebsd.org Thu Jun 27 19:18:51 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 A024F15CAC8E; Thu, 27 Jun 2019 19:18:51 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id CDD046C1A1; Thu, 27 Jun 2019 19:18:50 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.15.2/8.15.2) with ESMTPS id x5RJIhs2047340 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NO); Thu, 27 Jun 2019 22:18:46 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua x5RJIhs2047340 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id x5RJIhBD047339; Thu, 27 Jun 2019 22:18:43 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Thu, 27 Jun 2019 22:18:43 +0300 From: Konstantin Belousov To: Alan Somers Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r349231 - in head/sys: kern sys ufs/ufs Message-ID: <20190627191843.GQ8697@kib.kiev.ua> References: <201906201413.x5KEDB5u010923@repo.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201906201413.x5KEDB5u010923@repo.freebsd.org> User-Agent: Mutt/1.12.0 (2019-05-25) X-Spam-Status: No, score=-1.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FORGED_GMAIL_RCVD,FREEMAIL_FROM, NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on tom.home 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 19:18:51 -0000 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. > +{ > + 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). > + 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. > +/* 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. > 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