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

[Konstantin Belousov]

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