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

[Jeff Moyer <jmoyer@redhat.com>]

Hi Hannes,

Hannes Reinecke <hare@suse.de> 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@suse.de>
> ---
>  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)