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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 3 Nov 2016 23:40:34 +0800
From:   Ming Lei <tom.leiming@...il.com>
To:     Christoph Hellwig <hch@....de>
Cc:     Hannes Reinecke <hare@...e.de>, Jens Axboe <axboe@...com>,
        Alexander Graf <agraf@...e.com>,
        Martin Schwidefsky <schwidefsky@...ibm.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Hannes Reinecke <hare@...e.com>
Subject: Re: [PATCH 2/2] loop: support 4k physical blocksize

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