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@xxxxxxxxxxx>
> ---
> 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.
