On Thu, Aug 24, 2017 at 10:06:57AM +0200, Karel Zak wrote:
> On Thu, Aug 24, 2017 at 12:03:43AM -0700, Omar Sandoval wrote:
> > From: Omar Sandoval <[email protected]>
> >
> > This is a different approach from the first attempt in f2c6df7dbf9a
> > ("loop: support 4k physical blocksize"). Rather than extending
> > LOOP_{GET,SET}_STATUS, add a separate ioctl just for setting the block
> > size.
> >
> > Signed-off-by: Omar Sandoval <[email protected]>
> > ---
> > drivers/block/loop.c | 24 ++++++++++++++++++++++++
> > include/uapi/linux/loop.h | 1 +
> > 2 files changed, 25 insertions(+)
> >
> > diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> > index 1a5b4ecf54ec..6b1d932028e3 100644
> > --- a/drivers/block/loop.c
> > +++ b/drivers/block/loop.c
> > @@ -1047,6 +1047,7 @@ static int loop_clr_fd(struct loop_device *lo)
> > memset(lo->lo_encrypt_key, 0, LO_KEY_SIZE);
> > memset(lo->lo_crypt_name, 0, LO_NAME_SIZE);
> > memset(lo->lo_file_name, 0, LO_NAME_SIZE);
> > + blk_queue_logical_block_size(lo->lo_queue, 512);
> > if (bdev) {
> > bdput(bdev);
> > invalidate_bdev(bdev);
> > @@ -1330,6 +1331,24 @@ static int loop_set_dio(struct loop_device *lo,
> > unsigned long arg)
> > return error;
> > }
> >
> > +static int loop_set_block_size(struct loop_device *lo, unsigned long arg)
> > +{
> > + if (lo->lo_state != Lo_bound)
> > + return -ENXIO;
> > +
> > + if (arg < 512 || arg > PAGE_SIZE || !is_power_of_2(arg))
> > + return -EINVAL;
>
> I'm not sure if the PAGE_SIZE is the right limit for the use-case
> where backing file is a block device ...but it's nothing important.
A bigger logical block size just means we'll send bigger I/Os down to
the backing file, so I think it should be okay.
> > + blk_mq_freeze_queue(lo->lo_queue);
> > +
> > + blk_queue_logical_block_size(lo->lo_queue, arg);
>
> Just note that this function also updates physical_block_size if the
> 'arg' is greater than the current setting. That's good thing :-)
Patch 2 sets the physical block size to PAGE_SIZE, and we cap the
logical block size to PAGE_SIZE, so we won't actually hit that case
anyways :)
> > + loop_update_dio(lo);
> > +
> > + blk_mq_unfreeze_queue(lo->lo_queue);
> > +
> > + return 0;
> > +}
> > +
> > static int lo_ioctl(struct block_device *bdev, fmode_t mode,
> > unsigned int cmd, unsigned long arg)
> > {
> > @@ -1378,6 +1397,11 @@ static int lo_ioctl(struct block_device *bdev,
> > fmode_t mode,
> > if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN))
> > err = loop_set_dio(lo, arg);
> > break;
> > + case LOOP_SET_BLOCK_SIZE:
> > + err = -EPERM;
> > + if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN))
> > + err = loop_set_block_size(lo, arg);
> > + break;
>
> I like it this ioctl idea. It makes loop based tests more comfortable
> as we can change sector size on the fly without loop device reset.
>
> I did not test it yet, but this implementation seems the best. Thanks.
Thanks! Let me know if you spot any issues when you test it.
