linux-kernel - Re: [PATCH 1/1] block: move the PAGE_SECTORS definition into <linux/blkdev.h>

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <8aa638b7-6cfd-bf3d-8015-fbe59f28f31f@suse.de>
Date:   Fri, 21 Aug 2020 15:05:36 +0800
From:   Coly Li <colyli@...e.de>
To:     "Leizhen (ThunderTown)" <thunder.leizhen@...wei.com>
Cc:     Jens Axboe <axboe@...nel.dk>,
        Kent Overstreet <kent.overstreet@...il.com>,
        Alasdair Kergon <agk@...hat.com>,
        Mike Snitzer <snitzer@...hat.com>,
        dm-devel <dm-devel@...hat.com>,
        linux-block <linux-block@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-bcache <linux-bcache@...r.kernel.org>
Subject: Re: [PATCH 1/1] block: move the PAGE_SECTORS definition into
 <linux/blkdev.h>

On 2020/8/21 14:48, Leizhen (ThunderTown) wrote:
> 
> 
> On 8/21/2020 12:11 PM, Coly Li wrote:
>> On 2020/8/21 10:03, Zhen Lei wrote:
>>> There are too many PAGE_SECTORS definitions, and all of them are the
>>> same. It looks a bit of a mess. So why not move it into <linux/blkdev.h>,
>>> to achieve a basic and unique definition.
>>>
>>> Signed-off-by: Zhen Lei <thunder.leizhen@...wei.com>
>>
>>
>> A lazy question about page size > 4KB: currently in bcache code the
>> sector size is assumed to be 512 sectors, if kernel page > 4KB, it is
>> possible that PAGE_SECTORS in bcache will be a number > 8 ?
> 
> Sorry, I don't fully understand your question. I known that the sector size
> can be 512 or 4K, and the PAGE_SIZE can be 4K or 64K. So even if sector size
> is 4K, isn't it greater than 8 for 64K pages?
> 
> I'm not sure if the question you're asking is the one Matthew Wilcox has
> answered before:
> https://www.spinics.net/lists/raid/msg64345.html

Thank you for the above information. Currently bcache code assumes
sector size is always 512 bytes, you may see how many "<< 9" or ">> 9"
are used. Therefore I doubt whether current code may stably work on e.g.
4Kn SSDs (this is only doubt because I don't have such SSD).

Anyway your patch is fine to me. This change to bcache doesn't make
thins worse or better, maybe it can be helpful to trigger my above
suspicious early if people do have this kind of problem on 4Kn sector SSDs.

For the bcache part of this patch, you may add,
Acked-by: Coly Li <colyli@...e.de>

Thanks.

Coly Li

>>> ---
>>>  drivers/block/brd.c           | 1 -
>>>  drivers/block/null_blk_main.c | 1 -
>>>  drivers/md/bcache/util.h      | 2 --
>>>  include/linux/blkdev.h        | 5 +++--
>>>  include/linux/device-mapper.h | 1 -
>>>  5 files changed, 3 insertions(+), 7 deletions(-)
>>>
>>
>> [snipped]
>>
>>> diff --git a/drivers/md/bcache/util.h b/drivers/md/bcache/util.h
>>> index c029f7443190805..55196e0f37c32c6 100644
>>> --- a/drivers/md/bcache/util.h
>>> +++ b/drivers/md/bcache/util.h
>>> @@ -15,8 +15,6 @@
>>>  
>>>  #include "closure.h"
>>>  
>>> -#define PAGE_SECTORS		(PAGE_SIZE / 512)
>>> -
>>>  struct closure;
>>>  
>>>  #ifdef CONFIG_BCACHE_DEBUG
>>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>>> index bb5636cc17b91a7..b068dfc5f2ef0ab 100644
>>> --- a/include/linux/blkdev.h
>>> +++ b/include/linux/blkdev.h
>>> @@ -949,11 +949,12 @@ static inline struct request_queue *bdev_get_queue(struct block_device *bdev)
>>>   * multiple of 512 bytes. Hence these two constants.
>>>   */
>>>  #ifndef SECTOR_SHIFT
>>> -#define SECTOR_SHIFT 9
>>> +#define SECTOR_SHIFT		9
>>>  #endif
>>>  #ifndef SECTOR_SIZE
>>> -#define SECTOR_SIZE (1 << SECTOR_SHIFT)
>>> +#define SECTOR_SIZE		(1 << SECTOR_SHIFT)
>>>  #endif
>>> +#define PAGE_SECTORS		(PAGE_SIZE / SECTOR_SIZE)
>>>  
>>>  /*
>>>   * blk_rq_pos()			: the current sector
>> [snipped]
>>
>>
>