Re: [PATCH 4/4] loop: Add LOOP_SET_FD_AND_STATUS ioctl.

[Christoph Hellwig <hch@lst.de>]

On Mon, Apr 20, 2020 at 10:04:09AM +0200, Martijn Coenen wrote:
> This allows userspace to completely setup a loop device with a single
> ioctl, removing the in-between state where the device can be partially
> configured - eg the loop device has a backing file associated with it,
> but is reading from the wrong offset.
> 
> Besides removing the intermediate state, another big benefit of this
> ioctl is that LOOP_SET_STATUS can be slow; the main reason for this
> slowness is that LOOP_SET_STATUS(64) calls blk_mq_freeze_queue() to
> freeze the associated queue; this requires waiting for RCU
> synchronization, which I've measured can take about 15-20ms on this
> device on average.
> 
> Here's setting up ~70 regular loop devices with an offset on an x86
> Android device, using LOOP_SET_FD and LOOP_SET_STATUS:
> 
> vsoc_x86:/system/apex # time for i in `seq 30 100`;
> do losetup -r -o 4096 /dev/block/loop$i com.android.adbd.apex; done
>     0m03.40s real     0m00.02s user     0m00.03s system
> 
> Here's configuring ~70 devices in the same way, but using a modified
> losetup that uses the new LOOP_SET_FD_AND_STATUS ioctl:
> 
> vsoc_x86:/system/apex # time for i in `seq 30 100`;
> do losetup -r -o 4096 /dev/block/loop$i com.android.adbd.apex; done
>     0m01.94s real     0m00.01s user     0m00.01s system
> 
> Signed-off-by: Martijn Coenen <maco@android.com>
> ---
>  drivers/block/loop.c      | 47 ++++++++++++++++++++++++++++++---------
>  include/uapi/linux/loop.h |  6 +++++
>  2 files changed, 42 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 6e656390b285..e1dbd70d6d6e 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -1065,8 +1065,9 @@ loop_set_from_status(struct loop_device *lo, const struct loop_info64 *info)
>  	return 0;
>  }
>  
> -static int loop_set_fd(struct loop_device *lo, fmode_t mode,
> -		       struct block_device *bdev, unsigned int arg)
> +static int loop_set_fd_and_status(struct loop_device *lo, fmode_t mode,
> +				  struct block_device *bdev, unsigned int fd,
> +				  const struct loop_info64 *info)
>  {
>  	struct file	*file;
>  	struct inode	*inode;
> @@ -1081,7 +1082,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
>  	__module_get(THIS_MODULE);
>  
>  	error = -EBADF;
> -	file = fget(arg);
> +	file = fget(fd);
>  	if (!file)
>  		goto out;
>  
> @@ -1090,7 +1091,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
>  	 * here to avoid changing device under exclusive owner.
>  	 */
>  	if (!(mode & FMODE_EXCL)) {
> -		claimed_bdev = bd_start_claiming(bdev, loop_set_fd);
> +		claimed_bdev = bd_start_claiming(bdev, loop_set_fd_and_status);
>  		if (IS_ERR(claimed_bdev)) {
>  			error = PTR_ERR(claimed_bdev);
>  			goto out_putf;
> @@ -1117,9 +1118,24 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
>  		lo_flags |= LO_FLAGS_READ_ONLY;
>  
>  	error = -EFBIG;
> -	size = get_loop_size(lo, file);
> +	if (info)
> +		size = get_size(info->lo_offset, info->lo_sizelimit,
> +				file);
> +	else
> +		size = get_loop_size(lo, file);
>  	if ((loff_t)(sector_t)size != size)
>  		goto out_unlock;
> +
> +	if (info) {
> +		error = loop_set_from_status(lo, info);
> +		if (error)
> +			goto out_unlock;
> +	} else {
> +		lo->transfer = NULL;
> +		lo->ioctl = NULL;
> +		lo->lo_sizelimit = 0;
> +		lo->lo_offset = 0;
> +	}

Just curious:  Can't we just pass in an on-stack info for the legacy
case and avoid all these conditionals?

>  out:
> @@ -1653,7 +1666,18 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
>  
>  	switch (cmd) {
>  	case LOOP_SET_FD:
> -		return loop_set_fd(lo, mode, bdev, arg);
> +		return loop_set_fd_and_status(lo, mode, bdev, arg, NULL);
> +	case LOOP_SET_FD_AND_STATUS: {
> +		struct loop_fd_and_status fds;
> +
> +		if (copy_from_user(&fds,
> +				   (struct loop_fd_and_status __user *) arg,
> +				   sizeof(fds)))
> +			return -EFAULT;
> +
> +		return loop_set_fd_and_status(lo, mode, bdev, fds.fd,
> +					      &fds.info);
> +	}

Can you throw in another prep patch that adds a:

	void __user *argp = (void __user *)arg;

line at the top of lo_compat_ioctl, and switches the LOOP_SET_STATUS
and LOOP_GET_STATUS case to it?

The nhere you can just do:

		if (copy_from_user(&p, argp, sizeof(fds)))
			return -EFAULT;
		return loop_set_fd_and_status(lo, mode, bdev, p.fd, &p.info);

and be done.

> +struct loop_fd_and_status {
> +	struct loop_info64	info;
> +	__u32			fd;

This should grow a

	__u32	__pad;

to avoid different struct sizes on x86-32 vs x86-64.