Re: [PATCH 2/2] loop: support 4k physical blocksize

[Christoph Hellwig <hch@lst.de>]

> -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;
> @@ -233,6 +234,12 @@ figure_loop_size(struct loop_device *lo, loff_t offset, loff_t sizelimit)
>  		lo->lo_offset = offset;
>  	if (lo->lo_sizelimit != sizelimit)
>  		lo->lo_sizelimit = sizelimit;
> +	if (lo->lo_flags & LO_FLAGS_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);
> +	}

I've looked how the existing code uses lo_blocksize and this whole thing
confuses me to no end.  Can we have all the blocksize assignments (i.e.
including the existing lo_blocksize assignments) in one single place and
documented?  Especialy as it seems so far lo_blocksize seems to be
the "fs" blocksize as set by set_blocksize, which seems totally wrong
to be set by loop at all.

> +	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;

No need for the inner braces.  Also please find a way to have a
descriptive name for the field, be that an anonymous union, or a #define.

> +	     (lo->lo_logical_blocksize != info->lo_init[0])))

Same comment about the inner braces here.

> +		if (figure_loop_size(lo, info->lo_offset, info->lo_sizelimit,
> +				     info->lo_init[0]))
>  			return -EFBIG;
>  
>  	loop_config_discard(lo);
> @@ -1303,7 +1328,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);

I'd say drop all the arguments but lo here (maybe in a prep patch) as
passing them all seems pointless and confusing.