Re: [PATCH 4/4] loop: Pass logical blocksize in 'lo_init[0]' ioctl field

From: Jeff Moyer
Date: Fri Nov 13 2015 - 16:42:40 EST


Hi Hannes,

Hannes Reinecke <hare@xxxxxxx> writes:

> The current LOOP_SET_STATUS64 ioctl has two unused fields
> 'init[2]', which can be used in conjunction with the
> LO_FLAGS_BLOCKSIZE flag to pass in the new logical blocksize.

I don't see a reason to set LO_FLAGS_BLOCKSIZE inside of the
loop_device->lo_flags. It's not a flag that gets toggled; rather, it's a
flag that indicates that we're attempting to change the block size. The
block size is the persistent state. To further clarify, I'm okay with
passing it in info->lo_flags, I just don't like that it gets also set in
the struct loop_device. I also think that you should spell out
logical_block_size, instead of having physical_block_size and
block_size. It's just easier to read, in my opinion.

If you take my suggestion to set the physical block size automatically,
I think that will clean things up some, too. I'm looking forward to v2.

And just FYI, nothing looked horrible in patch 3/4, but I'm not going
to ack it until I see the v2 posting, since I think it will change at
least a little.

Cheers,
Jeff

> Signed-off-by: Hannes Reinecke <hare@xxxxxxx>
> ---
> drivers/block/loop.c | 25 ++++++++++++++++++++-----
> 1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index d0b8754..6723e5e 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -221,7 +221,8 @@ static void __loop_update_dio(struct loop_device *lo, bool dio)
> }
>
> static int
> -figure_loop_size(struct loop_device *lo, loff_t offset, loff_t sizelimit)
> +figure_loop_size(struct loop_device *lo, loff_t offset, loff_t sizelimit,
> + loff_t logical_blocksize)
> {
> loff_t size = get_size(offset, sizelimit, lo->lo_backing_file);
> sector_t x = (sector_t)size;
> @@ -234,6 +235,8 @@ figure_loop_size(struct loop_device *lo, loff_t offset, loff_t sizelimit)
> if (lo->lo_sizelimit != sizelimit)
> lo->lo_sizelimit = sizelimit;
> if (lo->lo_flags & LO_FLAGS_BLOCKSIZE) {
> + if (lo->lo_logical_blocksize != logical_blocksize)
> + lo->lo_logical_blocksize = logical_blocksize;
> blk_queue_physical_block_size(lo->lo_queue, lo->lo_blocksize);
> blk_queue_logical_block_size(lo->lo_queue,
> lo->lo_logical_blocksize);
> @@ -1129,13 +1132,24 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
> if (err)
> return err;
>
> - if (info->lo_flags & LO_FLAGS_BLOCKSIZE)
> + if (info->lo_flags & LO_FLAGS_BLOCKSIZE) {
> lo->lo_flags |= LO_FLAGS_BLOCKSIZE;
> + if ((info->lo_init[0] != 512) &&
> + (info->lo_init[0] != 1024) &&
> + (info->lo_init[0] != 2048) &&
> + (info->lo_init[0] != 4096))
> + return -EINVAL;
> + if (info->lo_init[0] > lo->lo_blocksize)
> + return -EINVAL;
> + }
>
> if (lo->lo_offset != info->lo_offset ||
> lo->lo_sizelimit != info->lo_sizelimit ||
> - lo->lo_flags != lo_flags)
> - if (figure_loop_size(lo, info->lo_offset, info->lo_sizelimit))
> + lo->lo_flags != lo_flags ||
> + ((lo->lo_flags & LO_FLAGS_BLOCKSIZE) &&
> + (lo->lo_logical_blocksize != info->lo_init[0])))
> + if (figure_loop_size(lo, info->lo_offset, info->lo_sizelimit,
> + info->lo_init[0]))
> return -EFBIG;
>
> loop_config_discard(lo);
> @@ -1320,7 +1334,8 @@ static int loop_set_capacity(struct loop_device *lo)
> if (unlikely(lo->lo_state != Lo_bound))
> return -ENXIO;
>
> - return figure_loop_size(lo, lo->lo_offset, lo->lo_sizelimit);
> + return figure_loop_size(lo, lo->lo_offset, lo->lo_sizelimit,
> + lo->lo_logical_blocksize);
> }
>
> static int loop_set_dio(struct loop_device *lo, unsigned long arg)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/