<div dir="ltr"><div><div>Greetings;<br><br></div>There are a lot of uses of PAGE_SIZE/SECTOR_SIZE scattered around, and it seems like a medium improvement to be able to refer to it as PAGE_SECTORS everywhere instead of just inside dm, bcache, and null_blk. Did this change progress forward somewhere?<br><br></div>Thanks!<br><div><div><div><div><div><br></div><div>John Dorminy<br><br></div></div></div></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Sep 7, 2020 at 3:40 AM Leizhen (ThunderTown) <<a href="mailto:thunder.leizhen@huawei.com">thunder.leizhen@huawei.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi, Jens Axboe, Alasdair Kergon, Mike Snitzer:<br>
  What's your opinion?<br>
<br>
<br>
On 2020/8/21 15:05, Coly Li wrote:<br>
> On 2020/8/21 14:48, Leizhen (ThunderTown) wrote:<br>
>><br>
>><br>
>> On 8/21/2020 12:11 PM, Coly Li wrote:<br>
>>> On 2020/8/21 10:03, Zhen Lei wrote:<br>
>>>> There are too many PAGE_SECTORS definitions, and all of them are the<br>
>>>> same. It looks a bit of a mess. So why not move it into <linux/blkdev.h>,<br>
>>>> to achieve a basic and unique definition.<br>
>>>><br>
>>>> Signed-off-by: Zhen Lei <<a href="mailto:thunder.leizhen@huawei.com" target="_blank">thunder.leizhen@huawei.com</a>><br>
>>><br>
>>><br>
>>> A lazy question about page size > 4KB: currently in bcache code the<br>
>>> sector size is assumed to be 512 sectors, if kernel page > 4KB, it is<br>
>>> possible that PAGE_SECTORS in bcache will be a number > 8 ?<br>
>><br>
>> Sorry, I don't fully understand your question. I known that the sector size<br>
>> can be 512 or 4K, and the PAGE_SIZE can be 4K or 64K. So even if sector size<br>
>> is 4K, isn't it greater than 8 for 64K pages?<br>
>><br>
>> I'm not sure if the question you're asking is the one Matthew Wilcox has<br>
>> answered before:<br>
>> <a href="https://www.spinics.net/lists/raid/msg64345.html" rel="noreferrer" target="_blank">https://www.spinics.net/lists/raid/msg64345.html</a><br>
> <br>
> Thank you for the above information. Currently bcache code assumes<br>
> sector size is always 512 bytes, you may see how many "<< 9" or ">> 9"<br>
> are used. Therefore I doubt whether current code may stably work on e.g.<br>
> 4Kn SSDs (this is only doubt because I don't have such SSD).<br>
> <br>
> Anyway your patch is fine to me. This change to bcache doesn't make<br>
> thins worse or better, maybe it can be helpful to trigger my above<br>
> suspicious early if people do have this kind of problem on 4Kn sector SSDs.<br>
> <br>
> For the bcache part of this patch, you may add,<br>
> Acked-by: Coly Li <<a href="mailto:colyli@suse.de" target="_blank">colyli@suse.de</a>><br>
> <br>
> Thanks.<br>
> <br>
> Coly Li<br>
> <br>
>>>> ---<br>
>>>>  drivers/block/brd.c           | 1 -<br>
>>>>  drivers/block/null_blk_main.c | 1 -<br>
>>>>  drivers/md/bcache/util.h      | 2 --<br>
>>>>  include/linux/blkdev.h        | 5 +++--<br>
>>>>  include/linux/device-mapper.h | 1 -<br>
>>>>  5 files changed, 3 insertions(+), 7 deletions(-)<br>
>>>><br>
>>><br>
>>> [snipped]<br>
>>><br>
>>>> diff --git a/drivers/md/bcache/util.h b/drivers/md/bcache/util.h<br>
>>>> index c029f7443190805..55196e0f37c32c6 100644<br>
>>>> --- a/drivers/md/bcache/util.h<br>
>>>> +++ b/drivers/md/bcache/util.h<br>
>>>> @@ -15,8 +15,6 @@<br>
>>>>  <br>
>>>>  #include "closure.h"<br>
>>>>  <br>
>>>> -#define PAGE_SECTORS              (PAGE_SIZE / 512)<br>
>>>> -<br>
>>>>  struct closure;<br>
>>>>  <br>
>>>>  #ifdef CONFIG_BCACHE_DEBUG<br>
>>>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h<br>
>>>> index bb5636cc17b91a7..b068dfc5f2ef0ab 100644<br>
>>>> --- a/include/linux/blkdev.h<br>
>>>> +++ b/include/linux/blkdev.h<br>
>>>> @@ -949,11 +949,12 @@ static inline struct request_queue *bdev_get_queue(struct block_device *bdev)<br>
>>>>   * multiple of 512 bytes. Hence these two constants.<br>
>>>>   */<br>
>>>>  #ifndef SECTOR_SHIFT<br>
>>>> -#define SECTOR_SHIFT 9<br>
>>>> +#define SECTOR_SHIFT              9<br>
>>>>  #endif<br>
>>>>  #ifndef SECTOR_SIZE<br>
>>>> -#define SECTOR_SIZE (1 << SECTOR_SHIFT)<br>
>>>> +#define SECTOR_SIZE               (1 << SECTOR_SHIFT)<br>
>>>>  #endif<br>
>>>> +#define PAGE_SECTORS              (PAGE_SIZE / SECTOR_SIZE)<br>
>>>>  <br>
>>>>  /*<br>
>>>>   * blk_rq_pos()                   : the current sector<br>
>>> [snipped]<br>
>>><br>
>>><br>
>><br>
> <br>
> <br>
> .<br>
> <br>
<br>
</blockquote></div>