On Wed, Apr 22, 2020 at 8:19 AM Christoph Hellwig <hch@xxxxxx> wrote:
>
> 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?
Yeah, that is actually much nicer. I will rework it to that.
> 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?
Did you mean in regular lo_ioctl()? eg something like this:
@@ -1671,6 +1671,7 @@ static int lo_ioctl(struct block_device *bdev,
fmode_t mode,
unsigned int cmd, unsigned long arg)
{
struct loop_device *lo = bdev->bd_disk->private_data;
+ void __user *argp = (void __user *) arg;
int err;
switch (cmd) {
@@ -1694,21 +1695,19 @@ static int lo_ioctl(struct block_device *bdev,
fmode_t mode,
case LOOP_SET_STATUS:
err = -EPERM;
if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN)) {
- err = loop_set_status_old(lo,
- (struct loop_info __user *)arg);
+ err = loop_set_status_old(lo, argp);
}
break;
case LOOP_GET_STATUS:
- return loop_get_status_old(lo, (struct loop_info __user *) arg);
+ return loop_get_status_old(lo, argp);
case LOOP_SET_STATUS64:
err = -EPERM;
if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN)) {
- err = loop_set_status64(lo,
- (struct loop_info64 __user *) arg);
+ err = loop_set_status64(lo, argp);
}
break;
case LOOP_GET_STATUS64:
- return loop_get_status64(lo, (struct loop_info64 __user *) arg);
+ return loop_get_status64(lo, argp);
> > +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.
will do, thanks!
