linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/1] block: move the PAGE_SECTORS definition into <linux/blkdev.h>
@ 2020-08-21  2:03 Zhen Lei
  2020-08-21  4:11 ` Coly Li
  0 siblings, 1 reply; 7+ messages in thread
From: Zhen Lei @ 2020-08-21  2:03 UTC (permalink / raw)
  To: Jens Axboe, Coly Li, Kent Overstreet, Alasdair Kergon,
	Mike Snitzer, dm-devel, linux-block, linux-kernel, linux-bcache
  Cc: Zhen Lei

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@huawei.com>
---
 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(-)

diff --git a/drivers/block/brd.c b/drivers/block/brd.c
index 2723a70eb855936..24c4687694b9f49 100644
--- a/drivers/block/brd.c
+++ b/drivers/block/brd.c
@@ -26,7 +26,6 @@
 #include <linux/uaccess.h>
 
 #define PAGE_SECTORS_SHIFT	(PAGE_SHIFT - SECTOR_SHIFT)
-#define PAGE_SECTORS		(1 << PAGE_SECTORS_SHIFT)
 
 /*
  * Each block ramdisk device has a radix_tree brd_pages of pages that stores
diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
index 47a9dad880af2aa..0624a26b86453ce 100644
--- a/drivers/block/null_blk_main.c
+++ b/drivers/block/null_blk_main.c
@@ -12,7 +12,6 @@
 #include "null_blk.h"
 
 #define PAGE_SECTORS_SHIFT	(PAGE_SHIFT - SECTOR_SHIFT)
-#define PAGE_SECTORS		(1 << PAGE_SECTORS_SHIFT)
 #define SECTOR_MASK		(PAGE_SECTORS - 1)
 
 #define FREE_BATCH		16
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
diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
index 93096e524e43945..ffccce9b700c326 100644
--- a/include/linux/device-mapper.h
+++ b/include/linux/device-mapper.h
@@ -143,7 +143,6 @@ typedef size_t (*dm_dax_copy_iter_fn)(struct dm_target *ti, pgoff_t pgoff,
 		void *addr, size_t bytes, struct iov_iter *i);
 typedef int (*dm_dax_zero_page_range_fn)(struct dm_target *ti, pgoff_t pgoff,
 		size_t nr_pages);
-#define PAGE_SECTORS (PAGE_SIZE / 512)
 
 void dm_error(const char *message);
 
-- 
1.8.3



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] block: move the PAGE_SECTORS definition into <linux/blkdev.h>
  2020-08-21  2:03 [PATCH 1/1] block: move the PAGE_SECTORS definition into <linux/blkdev.h> Zhen Lei
@ 2020-08-21  4:11 ` Coly Li
  2020-08-21  6:48   ` Leizhen (ThunderTown)
  0 siblings, 1 reply; 7+ messages in thread
From: Coly Li @ 2020-08-21  4:11 UTC (permalink / raw)
  To: Zhen Lei, Jens Axboe, Kent Overstreet, Alasdair Kergon,
	Mike Snitzer, dm-devel, linux-block, linux-kernel, linux-bcache

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@huawei.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 ?

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]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] block: move the PAGE_SECTORS definition into <linux/blkdev.h>
  2020-08-21  4:11 ` Coly Li
@ 2020-08-21  6:48   ` Leizhen (ThunderTown)
  2020-08-21  7:05     ` Coly Li
  0 siblings, 1 reply; 7+ messages in thread
From: Leizhen (ThunderTown) @ 2020-08-21  6:48 UTC (permalink / raw)
  To: Coly Li, Jens Axboe, Kent Overstreet, Alasdair Kergon,
	Mike Snitzer, dm-devel, linux-block, linux-kernel, linux-bcache



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@huawei.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

> 
> 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]
> 
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] block: move the PAGE_SECTORS definition into <linux/blkdev.h>
  2020-08-21  6:48   ` Leizhen (ThunderTown)
@ 2020-08-21  7:05     ` Coly Li
  2020-09-07  7:39       ` Leizhen (ThunderTown)
  0 siblings, 1 reply; 7+ messages in thread
From: Coly Li @ 2020-08-21  7:05 UTC (permalink / raw)
  To: Leizhen (ThunderTown)
  Cc: Jens Axboe, Kent Overstreet, Alasdair Kergon, Mike Snitzer,
	dm-devel, linux-block, linux-kernel, linux-bcache

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@huawei.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@suse.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]
>>
>>
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] block: move the PAGE_SECTORS definition into <linux/blkdev.h>
  2020-08-21  7:05     ` Coly Li
@ 2020-09-07  7:39       ` Leizhen (ThunderTown)
  2020-11-20  1:27         ` John Dorminy
  0 siblings, 1 reply; 7+ messages in thread
From: Leizhen (ThunderTown) @ 2020-09-07  7:39 UTC (permalink / raw)
  To: Coly Li
  Cc: Jens Axboe, Kent Overstreet, Alasdair Kergon, Mike Snitzer,
	dm-devel, linux-block, linux-kernel, linux-bcache

Hi, Jens Axboe, Alasdair Kergon, Mike Snitzer:
  What's your opinion?


On 2020/8/21 15:05, Coly Li wrote:
> 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@huawei.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@suse.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]
>>>
>>>
>>
> 
> 
> .
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] block: move the PAGE_SECTORS definition into <linux/blkdev.h>
  2020-09-07  7:39       ` Leizhen (ThunderTown)
@ 2020-11-20  1:27         ` John Dorminy
  2020-11-20  2:25           ` Leizhen (ThunderTown)
  0 siblings, 1 reply; 7+ messages in thread
From: John Dorminy @ 2020-11-20  1:27 UTC (permalink / raw)
  To: Leizhen (ThunderTown)
  Cc: Coly Li, Jens Axboe, Kent Overstreet, Alasdair Kergon,
	Mike Snitzer, dm-devel, linux-block, linux-kernel, linux-bcache

Greetings;

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?

Thanks!

John Dorminy


On Mon, Sep 7, 2020 at 3:40 AM Leizhen (ThunderTown)
<thunder.leizhen@huawei.com> wrote:
>
> Hi, Jens Axboe, Alasdair Kergon, Mike Snitzer:
>   What's your opinion?
>
>
> On 2020/8/21 15:05, Coly Li wrote:
> > 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@huawei.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@suse.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]
> >>>
> >>>
> >>
> >
> >
> > .
> >
>


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] block: move the PAGE_SECTORS definition into <linux/blkdev.h>
  2020-11-20  1:27         ` John Dorminy
@ 2020-11-20  2:25           ` Leizhen (ThunderTown)
  0 siblings, 0 replies; 7+ messages in thread
From: Leizhen (ThunderTown) @ 2020-11-20  2:25 UTC (permalink / raw)
  To: John Dorminy
  Cc: Coly Li, Jens Axboe, Kent Overstreet, Alasdair Kergon,
	Mike Snitzer, dm-devel, linux-block, linux-kernel, linux-bcache



On 2020/11/20 9:27, John Dorminy wrote:
> Greetings;
> 
> 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?

Actually, I'm trying to make further replacements after this patch is applied.
But there was no response except Coly Li.

> 
> Thanks!
> 
> John Dorminy
> 
> 
> On Mon, Sep 7, 2020 at 3:40 AM Leizhen (ThunderTown)
> <thunder.leizhen@huawei.com> wrote:
>>
>> Hi, Jens Axboe, Alasdair Kergon, Mike Snitzer:
>>   What's your opinion?
>>
>>
>> On 2020/8/21 15:05, Coly Li wrote:
>>> 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@huawei.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@suse.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]
>>>>>
>>>>>
>>>>
>>>
>>>
>>> .
>>>
>>
> 
> 
> .
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-11-20  2:26 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-21  2:03 [PATCH 1/1] block: move the PAGE_SECTORS definition into <linux/blkdev.h> Zhen Lei
2020-08-21  4:11 ` Coly Li
2020-08-21  6:48   ` Leizhen (ThunderTown)
2020-08-21  7:05     ` Coly Li
2020-09-07  7:39       ` Leizhen (ThunderTown)
2020-11-20  1:27         ` John Dorminy
2020-11-20  2:25           ` Leizhen (ThunderTown)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).