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

[Ming Lei]

On Thu, Nov 3, 2016 at 10:26 PM, Christoph Hellwig <hch@xxxxxx> wrote:
>> -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.

This patch uses backing fs block size to emulate physical block size
of loop block device, and the logical block size of loop is set from user
space, seems the idea is correct.

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



-- 
Ming Lei