[PATCH v2 00/10] clean up SECTOR related macros and sectors/pages conversions

[PATCH v2 00/10] clean up SECTOR related macros and sectors/pages conversions

[Zhen Lei]

v1 --> v2:
As Matthew Wilcox's suggestion, add sectors_to_npage()/npage_to_sectors()
helpers to eliminate SECTORS_PER_PAGE_SHIFT, because it's quite hard to read.
In further, I also eliminated PAGE_SECTORS_SHIFT.

I tried to eliminate all magic number "9" and "512", but it's too many, maybe
no one want to review it, so I gave up. In the process of searching, I found
the existing macro PAGE_SECTORS, it's equivalent to SECTORS_PER_PAGE. Because
PAGE_SECTORS was defined in include/linux/device-mapper.h, and SECTORS_PER_PAGE
was defined in drivers/block/zram/zram_drv.h, so I discarded SECTORS_PER_PAGE,
althrough I prefer it so much.

v1:
When I studied the code of mm/swap, I found "1 << (PAGE_SHIFT - 9)" appears
many times. So I try to clean up it.

1. Replace "1 << (PAGE_SHIFT - 9)" or similar with SECTORS_PER_PAGE
2. Replace "PAGE_SHIFT - 9" with SECTORS_PER_PAGE_SHIFT
3. Replace "9" with SECTOR_SHIFT
4. Replace "512" with SECTOR_SIZE

Zhen Lei (10):
  block: move PAGE_SECTORS definition into <linux/blkdev.h>
  zram: abolish macro SECTORS_PER_PAGE
  block: add sectors_to_npage()/npage_to_sectors() helpers
  zram: abolish macro SECTORS_PER_PAGE_SHIFT
  block: abolish macro PAGE_SECTORS_SHIFT
  mm/swap: use npage_to_sectors() and PAGE_SECTORS to clean up code
  block: use sectors_to_npage() and PAGE_SECTORS to clean up code
  md: use sectors_to_npage() and npage_to_sectors() to clean up code
  md: use existing definition RESYNC_SECTORS
  md: use PAGE_SECTORS to clean up code

 block/blk-settings.c          |  6 +++---
 block/partitions/core.c       |  5 ++---
 drivers/block/brd.c           |  7 ++-----
 drivers/block/null_blk_main.c | 10 ++++------
 drivers/block/zram/zram_drv.c |  8 ++++----
 drivers/block/zram/zram_drv.h |  2 --
 drivers/md/bcache/util.h      |  2 --
 drivers/md/dm-kcopyd.c        |  2 +-
 drivers/md/dm-table.c         |  2 +-
 drivers/md/md-bitmap.c        | 16 ++++++++--------
 drivers/md/md.c               |  6 +++---
 drivers/md/raid1.c            | 10 +++++-----
 drivers/md/raid10.c           | 28 ++++++++++++++--------------
 drivers/md/raid5-cache.c      | 11 +++++------
 drivers/md/raid5.c            |  4 ++--
 include/linux/blkdev.h        |  7 +++++--
 include/linux/device-mapper.h |  1 -
 mm/page_io.c                  |  4 ++--
 mm/swapfile.c                 | 12 ++++++------
 19 files changed, 67 insertions(+), 76 deletions(-)

-- 
2.26.0.106.g9fadedd

[PATCH v2 01/10] block: move PAGE_SECTORS definition into <linux/blkdev.h>

[Zhen Lei]

Too many duplicated PAGE_SECTORS definitions, eliminate it.

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 2fb25c348d53..30df6daa9dbc 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 8efd8778e209..25048ff15858 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 c029f7443190..55196e0f37c3 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 32868fbedc9e..934f31fc15cd 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -904,11 +904,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 af48d9da3916..83e018ed8c21 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);
 
-- 
2.26.0.106.g9fadedd

[PATCH v2 02/10] zram: abolish macro SECTORS_PER_PAGE

[Zhen Lei]

SECTORS_PER_PAGE is equivalent to PAGE_SECTORS.

Although I prefer SECTORS_PER_PAGE better than PAGE_SECTORS, the former
is more clearer, I think. But the latter was defined in
<linux/device-mapper.h> before, rename it may impact users.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/block/zram/zram_drv.c | 4 ++--
 drivers/block/zram/zram_drv.h | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index ebb234f36909..e2fbf7a847e7 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1551,7 +1551,7 @@ static void __zram_make_request(struct zram *zram, struct bio *bio)
 
 	index = bio->bi_iter.bi_sector >> SECTORS_PER_PAGE_SHIFT;
 	offset = (bio->bi_iter.bi_sector &
-		  (SECTORS_PER_PAGE - 1)) << SECTOR_SHIFT;
+		  (PAGE_SECTORS - 1)) << SECTOR_SHIFT;
 
 	switch (bio_op(bio)) {
 	case REQ_OP_DISCARD:
@@ -1645,7 +1645,7 @@ static int zram_rw_page(struct block_device *bdev, sector_t sector,
 	}
 
 	index = sector >> SECTORS_PER_PAGE_SHIFT;
-	offset = (sector & (SECTORS_PER_PAGE - 1)) << SECTOR_SHIFT;
+	offset = (sector & (PAGE_SECTORS - 1)) << SECTOR_SHIFT;
 
 	bv.bv_page = page;
 	bv.bv_len = PAGE_SIZE;
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index f2fd46daa760..10fdf413dd6e 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -22,7 +22,6 @@
 #include "zcomp.h"
 
 #define SECTORS_PER_PAGE_SHIFT	(PAGE_SHIFT - SECTOR_SHIFT)
-#define SECTORS_PER_PAGE	(1 << SECTORS_PER_PAGE_SHIFT)
 #define ZRAM_LOGICAL_BLOCK_SHIFT 12
 #define ZRAM_LOGICAL_BLOCK_SIZE	(1 << ZRAM_LOGICAL_BLOCK_SHIFT)
 #define ZRAM_SECTOR_PER_LOGICAL_BLOCK	\
-- 
2.26.0.106.g9fadedd

[PATCH v2 03/10] block: add sectors_to_npage()/npage_to_sectors() helpers

[Zhen Lei]

Provide the conversion of "number of sectors"/"sector number" and
"number of pages"/"page number".

Suggested-by: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 include/linux/blkdev.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 934f31fc15cd..5d8daaffc38b 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -910,6 +910,8 @@ static inline struct request_queue *bdev_get_queue(struct block_device *bdev)
 #define SECTOR_SIZE		(1 << SECTOR_SHIFT)
 #endif
 #define PAGE_SECTORS		(PAGE_SIZE / SECTOR_SIZE)
+#define sectors_to_npage(nr)	((nr) / PAGE_SECTORS)
+#define npage_to_sectors(nr)	((nr) * PAGE_SECTORS)
 
 /*
  * blk_rq_pos()			: the current sector
-- 
2.26.0.106.g9fadedd

[PATCH v2 04/10] zram: abolish macro SECTORS_PER_PAGE_SHIFT

[Zhen Lei]

The name of SECTORS_PER_PAGE_SHIFT is quite hard to read. So use
sectors_to_npage() to replace ">> SECTORS_PER_PAGE_SHIFT"

Suggested-by: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/block/zram/zram_drv.c | 4 ++--
 drivers/block/zram/zram_drv.h | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index e2fbf7a847e7..918b77f9bce4 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1549,7 +1549,7 @@ static void __zram_make_request(struct zram *zram, struct bio *bio)
 	struct bio_vec bvec;
 	struct bvec_iter iter;
 
-	index = bio->bi_iter.bi_sector >> SECTORS_PER_PAGE_SHIFT;
+	index = sectors_to_npage(bio->bi_iter.bi_sector);
 	offset = (bio->bi_iter.bi_sector &
 		  (PAGE_SECTORS - 1)) << SECTOR_SHIFT;
 
@@ -1644,7 +1644,7 @@ static int zram_rw_page(struct block_device *bdev, sector_t sector,
 		goto out;
 	}
 
-	index = sector >> SECTORS_PER_PAGE_SHIFT;
+	index = sectors_to_npage(sector);
 	offset = (sector & (PAGE_SECTORS - 1)) << SECTOR_SHIFT;
 
 	bv.bv_page = page;
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index 10fdf413dd6e..12309175d55e 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -21,7 +21,6 @@
 
 #include "zcomp.h"
 
-#define SECTORS_PER_PAGE_SHIFT	(PAGE_SHIFT - SECTOR_SHIFT)
 #define ZRAM_LOGICAL_BLOCK_SHIFT 12
 #define ZRAM_LOGICAL_BLOCK_SIZE	(1 << ZRAM_LOGICAL_BLOCK_SHIFT)
 #define ZRAM_SECTOR_PER_LOGICAL_BLOCK	\
-- 
2.26.0.106.g9fadedd

[PATCH v2 05/10] block: abolish macro PAGE_SECTORS_SHIFT

[Zhen Lei]

The name of PAGE_SECTORS_SHIFT is quite hard to read.
1. use sectors_to_npage() to replace ">> PAGE_SECTORS_SHIFT"
2. use npage_to_sectors() to replace "<< PAGE_SECTORS_SHIFT"

Suggested-by: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/block/brd.c           | 6 ++----
 drivers/block/null_blk_main.c | 9 ++++-----
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/block/brd.c b/drivers/block/brd.c
index 30df6daa9dbc..051c5a50497f 100644
--- a/drivers/block/brd.c
+++ b/drivers/block/brd.c
@@ -25,8 +25,6 @@
 
 #include <linux/uaccess.h>
 
-#define PAGE_SECTORS_SHIFT	(PAGE_SHIFT - SECTOR_SHIFT)
-
 /*
  * Each block ramdisk device has a radix_tree brd_pages of pages that stores
  * the pages containing the block device's contents. A brd page's ->index is
@@ -69,7 +67,7 @@ static struct page *brd_lookup_page(struct brd_device *brd, sector_t sector)
 	 * here, only deletes).
 	 */
 	rcu_read_lock();
-	idx = sector >> PAGE_SECTORS_SHIFT; /* sector to page index */
+	idx = sectors_to_npage(sector);
 	page = radix_tree_lookup(&brd->brd_pages, idx);
 	rcu_read_unlock();
 
@@ -108,7 +106,7 @@ static struct page *brd_insert_page(struct brd_device *brd, sector_t sector)
 	}
 
 	spin_lock(&brd->brd_lock);
-	idx = sector >> PAGE_SECTORS_SHIFT;
+	idx = sectors_to_npage(sector);
 	page->index = idx;
 	if (radix_tree_insert(&brd->brd_pages, idx, page)) {
 		__free_page(page);
diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
index 25048ff15858..81485f47dcf0 100644
--- a/drivers/block/null_blk_main.c
+++ b/drivers/block/null_blk_main.c
@@ -11,7 +11,6 @@
 #include <linux/init.h>
 #include "null_blk.h"
 
-#define PAGE_SECTORS_SHIFT	(PAGE_SHIFT - SECTOR_SHIFT)
 #define SECTOR_MASK		(PAGE_SECTORS - 1)
 
 #define FREE_BATCH		16
@@ -737,7 +736,7 @@ static void null_free_sector(struct nullb *nullb, sector_t sector,
 	struct radix_tree_root *root;
 
 	root = is_cache ? &nullb->dev->cache : &nullb->dev->data;
-	idx = sector >> PAGE_SECTORS_SHIFT;
+	idx = sectors_to_npage(sector);
 	sector_bit = (sector & SECTOR_MASK);
 
 	t_page = radix_tree_lookup(root, idx);
@@ -808,7 +807,7 @@ static struct nullb_page *__null_lookup_page(struct nullb *nullb,
 	struct nullb_page *t_page;
 	struct radix_tree_root *root;
 
-	idx = sector >> PAGE_SECTORS_SHIFT;
+	idx = sectors_to_npage(sector);
 	sector_bit = (sector & SECTOR_MASK);
 
 	root = is_cache ? &nullb->dev->cache : &nullb->dev->data;
@@ -855,7 +854,7 @@ static struct nullb_page *null_insert_page(struct nullb *nullb,
 		goto out_freepage;
 
 	spin_lock_irq(&nullb->lock);
-	idx = sector >> PAGE_SECTORS_SHIFT;
+	idx = sectors_to_npage(sector);
 	t_page->page->index = idx;
 	t_page = null_radix_tree_insert(nullb, idx, t_page, !ignore_cache);
 	radix_tree_preload_end();
@@ -878,7 +877,7 @@ static int null_flush_cache_page(struct nullb *nullb, struct nullb_page *c_page)
 
 	idx = c_page->page->index;
 
-	t_page = null_insert_page(nullb, idx << PAGE_SECTORS_SHIFT, true);
+	t_page = null_insert_page(nullb, npage_to_sectors(idx), true);
 
 	__clear_bit(NULLB_PAGE_LOCK, c_page->bitmap);
 	if (test_bit(NULLB_PAGE_FREE, c_page->bitmap)) {
-- 
2.26.0.106.g9fadedd

[PATCH v2 06/10] mm/swap: use npage_to_sectors() and PAGE_SECTORS to clean up code

[Zhen Lei]

1. Replace "<<= (PAGE_SHIFT - 9)" with "*= PAGE_SECTORS"
2. Replace "<< (PAGE_SHIFT - 9)" with npage_to_sectors()

Suggested-by: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 mm/page_io.c  |  4 ++--
 mm/swapfile.c | 12 ++++++------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/mm/page_io.c b/mm/page_io.c
index 76965be1d40e..23291a49ab91 100644
--- a/mm/page_io.c
+++ b/mm/page_io.c
@@ -38,7 +38,7 @@ static struct bio *get_swap_bio(gfp_t gfp_flags,
 
 		bio->bi_iter.bi_sector = map_swap_page(page, &bdev);
 		bio_set_dev(bio, bdev);
-		bio->bi_iter.bi_sector <<= PAGE_SHIFT - 9;
+		bio->bi_iter.bi_sector *= PAGE_SECTORS;
 		bio->bi_end_io = end_io;
 
 		bio_add_page(bio, page, PAGE_SIZE * hpage_nr_pages(page), 0);
@@ -266,7 +266,7 @@ int swap_writepage(struct page *page, struct writeback_control *wbc)
 
 static sector_t swap_page_sector(struct page *page)
 {
-	return (sector_t)__page_file_index(page) << (PAGE_SHIFT - 9);
+	return npage_to_sectors((sector_t)__page_file_index(page));
 }
 
 static inline void count_swpout_vm_event(struct page *page)
diff --git a/mm/swapfile.c b/mm/swapfile.c
index 5871a2aa86a5..c8be92f972a4 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -177,8 +177,8 @@ static int discard_swap(struct swap_info_struct *si)
 
 	/* Do not discard the swap header page! */
 	se = first_se(si);
-	start_block = (se->start_block + 1) << (PAGE_SHIFT - 9);
-	nr_blocks = ((sector_t)se->nr_pages - 1) << (PAGE_SHIFT - 9);
+	start_block = npage_to_sectors(se->start_block + 1);
+	nr_blocks = npage_to_sectors((sector_t)se->nr_pages - 1);
 	if (nr_blocks) {
 		err = blkdev_issue_discard(si->bdev, start_block,
 				nr_blocks, GFP_KERNEL, 0);
@@ -188,8 +188,8 @@ static int discard_swap(struct swap_info_struct *si)
 	}
 
 	for (se = next_se(se); se; se = next_se(se)) {
-		start_block = se->start_block << (PAGE_SHIFT - 9);
-		nr_blocks = (sector_t)se->nr_pages << (PAGE_SHIFT - 9);
+		start_block = npage_to_sectors(se->start_block);
+		nr_blocks = npage_to_sectors((sector_t)se->nr_pages);
 
 		err = blkdev_issue_discard(si->bdev, start_block,
 				nr_blocks, GFP_KERNEL, 0);
@@ -240,8 +240,8 @@ static void discard_swap_cluster(struct swap_info_struct *si,
 		start_page += nr_blocks;
 		nr_pages -= nr_blocks;
 
-		start_block <<= PAGE_SHIFT - 9;
-		nr_blocks <<= PAGE_SHIFT - 9;
+		start_block *= PAGE_SECTORS;
+		nr_blocks *= PAGE_SECTORS;
 		if (blkdev_issue_discard(si->bdev, start_block,
 					nr_blocks, GFP_NOIO, 0))
 			break;
-- 
2.26.0.106.g9fadedd

[PATCH v2 07/10] block: use sectors_to_npage() and PAGE_SECTORS to clean up code

[Zhen Lei]

1. Replace "1 << (PAGE_SHIFT - 9)" with PAGE_SECTORS
2. Replace ">> (PAGE_SHIFT - 9)" with sectors_to_npage()

Suggested-by: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 block/blk-settings.c    | 6 +++---
 block/partitions/core.c | 5 ++---
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/block/blk-settings.c b/block/blk-settings.c
index 14397b4c4b53..171665ed8318 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -150,7 +150,7 @@ void blk_queue_max_hw_sectors(struct request_queue *q, unsigned int max_hw_secto
 	unsigned int max_sectors;
 
 	if ((max_hw_sectors << 9) < PAGE_SIZE) {
-		max_hw_sectors = 1 << (PAGE_SHIFT - 9);
+		max_hw_sectors = PAGE_SECTORS;
 		printk(KERN_INFO "%s: set to minimum %d\n",
 		       __func__, max_hw_sectors);
 	}
@@ -159,7 +159,7 @@ void blk_queue_max_hw_sectors(struct request_queue *q, unsigned int max_hw_secto
 	max_sectors = min_not_zero(max_hw_sectors, limits->max_dev_sectors);
 	max_sectors = min_t(unsigned int, max_sectors, BLK_DEF_MAX_SECTORS);
 	limits->max_sectors = max_sectors;
-	q->backing_dev_info->io_pages = max_sectors >> (PAGE_SHIFT - 9);
+	q->backing_dev_info->io_pages = sectors_to_npage(max_sectors);
 }
 EXPORT_SYMBOL(blk_queue_max_hw_sectors);
 
@@ -630,7 +630,7 @@ void disk_stack_limits(struct gendisk *disk, struct block_device *bdev,
 	}
 
 	t->backing_dev_info->io_pages =
-		t->limits.max_sectors >> (PAGE_SHIFT - 9);
+		sectors_to_npage(t->limits.max_sectors);
 }
 EXPORT_SYMBOL(disk_stack_limits);
 
diff --git a/block/partitions/core.c b/block/partitions/core.c
index 9ef48a8cff86..4859739a2414 100644
--- a/block/partitions/core.c
+++ b/block/partitions/core.c
@@ -640,8 +640,7 @@ void *read_part_sector(struct parsed_partitions *state, sector_t n, Sector *p)
 		return NULL;
 	}
 
-	page = read_mapping_page(mapping,
-			(pgoff_t)(n >> (PAGE_SHIFT - 9)), NULL);
+	page = read_mapping_page(mapping, (pgoff_t)sectors_to_npage(n), NULL);
 	if (IS_ERR(page))
 		goto out;
 	if (PageError(page))
@@ -649,7 +648,7 @@ void *read_part_sector(struct parsed_partitions *state, sector_t n, Sector *p)
 
 	p->v = page;
 	return (unsigned char *)page_address(page) +
-			((n & ((1 << (PAGE_SHIFT - 9)) - 1)) << SECTOR_SHIFT);
+			((n & (PAGE_SECTORS - 1)) << SECTOR_SHIFT);
 out_put_page:
 	put_page(page);
 out:
-- 
2.26.0.106.g9fadedd

[PATCH v2 08/10] md: use sectors_to_npage() and npage_to_sectors() to clean up code

[Zhen Lei]

1. Replace ">> (PAGE_SHIFT - 9)" with sectors_to_npage()
2. Replace "<< (PAGE_SHIFT - 9)" with npage_to_sectors()

Suggested-by: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/md/dm-table.c    |  2 +-
 drivers/md/raid1.c       |  2 +-
 drivers/md/raid10.c      |  2 +-
 drivers/md/raid5-cache.c | 11 +++++------
 4 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 0a2cc197f62b..e1f176bda528 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1964,7 +1964,7 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
 #endif
 
 	/* Allow reads to exceed readahead limits */
-	q->backing_dev_info->io_pages = limits->max_sectors >> (PAGE_SHIFT - 9);
+	q->backing_dev_info->io_pages = sectors_to_npage(limits->max_sectors);
 }
 
 unsigned int dm_table_get_num_targets(struct dm_table *t)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index cd810e195086..44ffe1b6d77a 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -2129,7 +2129,7 @@ static void process_checks(struct r1bio *r1_bio)
 	int vcnt;
 
 	/* Fix variable parts of all bios */
-	vcnt = (r1_bio->sectors + PAGE_SIZE / 512 - 1) >> (PAGE_SHIFT - 9);
+	vcnt = sectors_to_npage(r1_bio->sectors + PAGE_SIZE / 512 - 1);
 	for (i = 0; i < conf->raid_disks * 2; i++) {
 		blk_status_t status;
 		struct bio *b = r1_bio->bios[i];
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index ec136e44aef7..948afe720fca 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -2029,7 +2029,7 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
 	fbio->bi_iter.bi_idx = 0;
 	fpages = get_resync_pages(fbio)->pages;
 
-	vcnt = (r10_bio->sectors + (PAGE_SIZE >> 9) - 1) >> (PAGE_SHIFT - 9);
+	vcnt = sectors_to_npage(r10_bio->sectors + (PAGE_SIZE >> 9) - 1);
 	/* now find blocks with errors */
 	for (i=0 ; i < conf->copies ; i++) {
 		int  j, d;
diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index 9b6da759dca2..0b9cd810466a 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -832,8 +832,7 @@ static void r5l_append_payload_meta(struct r5l_log *log, u16 type,
 	payload = page_address(io->meta_page) + io->meta_offset;
 	payload->header.type = cpu_to_le16(type);
 	payload->header.flags = cpu_to_le16(0);
-	payload->size = cpu_to_le32((1 + !!checksum2_valid) <<
-				    (PAGE_SHIFT - 9));
+	payload->size = cpu_to_le32(npage_to_sectors(1 + !!checksum2_valid));
 	payload->location = cpu_to_le64(location);
 	payload->checksum[0] = cpu_to_le32(checksum1);
 	if (checksum2_valid)
@@ -1042,7 +1041,7 @@ int r5l_write_stripe(struct r5l_log *log, struct stripe_head *sh)
 
 	mutex_lock(&log->io_mutex);
 	/* meta + data */
-	reserve = (1 + write_disks) << (PAGE_SHIFT - 9);
+	reserve = npage_to_sectors(1 + write_disks);
 
 	if (log->r5c_journal_mode == R5C_JOURNAL_MODE_WRITE_THROUGH) {
 		if (!r5l_has_free_space(log, reserve)) {
@@ -2053,7 +2052,7 @@ r5l_recovery_verify_data_checksum_for_mb(struct r5l_log *log,
 						  le32_to_cpu(payload->size));
 			mb_offset += sizeof(struct r5l_payload_data_parity) +
 				sizeof(__le32) *
-				(le32_to_cpu(payload->size) >> (PAGE_SHIFT - 9));
+				sectors_to_npage(le32_to_cpu(payload->size));
 		}
 
 	}
@@ -2199,7 +2198,7 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
 
 		mb_offset += sizeof(struct r5l_payload_data_parity) +
 			sizeof(__le32) *
-			(le32_to_cpu(payload->size) >> (PAGE_SHIFT - 9));
+			sectors_to_npage(le32_to_cpu(payload->size));
 	}
 
 	return 0;
@@ -2916,7 +2915,7 @@ int r5c_cache_data(struct r5l_log *log, struct stripe_head *sh)
 
 	mutex_lock(&log->io_mutex);
 	/* meta + data */
-	reserve = (1 + pages) << (PAGE_SHIFT - 9);
+	reserve = npage_to_sectors(1 + pages);
 
 	if (test_bit(R5C_LOG_CRITICAL, &conf->cache_state) &&
 	    sh->log_start == MaxSector)
-- 
2.26.0.106.g9fadedd

[PATCH v2 09/10] md: use existing definition RESYNC_SECTORS

[Zhen Lei]

"RESYNC_BLOCK_SIZE/512" is equal to "RESYNC_BLOCK_SIZE >> 9", replace it
with RESYNC_SECTORS.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/md/raid10.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 948afe720fca..ac4273f804e8 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -4483,8 +4483,8 @@ static sector_t reshape_request(struct mddev *mddev, sector_t sector_nr,
 		last = conf->reshape_progress - 1;
 		sector_nr = last & ~(sector_t)(conf->geo.chunk_mask
 					       & conf->prev.chunk_mask);
-		if (sector_nr + RESYNC_BLOCK_SIZE/512 < last)
-			sector_nr = last + 1 - RESYNC_BLOCK_SIZE/512;
+		if (sector_nr + RESYNC_SECTORS < last)
+			sector_nr = last + 1 - RESYNC_SECTORS;
 	} else {
 		/* 'next' is after the last device address that we
 		 * might write to for this chunk in the new layout
@@ -4506,8 +4506,8 @@ static sector_t reshape_request(struct mddev *mddev, sector_t sector_nr,
 		last  = sector_nr | (conf->geo.chunk_mask
 				     & conf->prev.chunk_mask);
 
-		if (sector_nr + RESYNC_BLOCK_SIZE/512 <= last)
-			last = sector_nr + RESYNC_BLOCK_SIZE/512 - 1;
+		if (sector_nr + RESYNC_SECTORS <= last)
+			last = sector_nr + RESYNC_SECTORS - 1;
 	}
 
 	if (need_flush ||
-- 
2.26.0.106.g9fadedd

[PATCH v2 10/10] md: use PAGE_SECTORS to clean up code

[Zhen Lei]

Execute the following shell script:

C_FILES=`find drivers/md/ -name "*.c"`

for file in $C_FILES
do
        #with brace
        sed -i 's/(PAGE_SIZE \/ 512)/PAGE_SECTORS/g' $file
        sed -i 's/(PAGE_SIZE\/512)/PAGE_SECTORS/g' $file
        sed -i 's/(PAGE_SIZE >> 9)/PAGE_SECTORS/g' $file
        sed -i 's/(PAGE_SIZE>>9)/PAGE_SECTORS/g' $file

        #without brace
        sed -i 's/PAGE_SIZE \/ 512/PAGE_SECTORS/g' $file
        sed -i 's/PAGE_SIZE\/512/PAGE_SECTORS/g' $file
        sed -i 's/PAGE_SIZE >> 9/PAGE_SECTORS/g' $file
        sed -i 's/PAGE_SIZE>>9/PAGE_SECTORS/g' $file
done

In addition, eliminate below scripts/checkpatch.pl warning:
#44: FILE: drivers/md/dm-kcopyd.c:587:
+       unsigned nr_pages = dm_div_up(job->dests[0].count, PAGE_SECTORS);

Change to "unsigned int nr_pages".

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/md/dm-kcopyd.c |  2 +-
 drivers/md/md-bitmap.c | 16 ++++++++--------
 drivers/md/md.c        |  6 +++---
 drivers/md/raid1.c     | 10 +++++-----
 drivers/md/raid10.c    | 20 ++++++++++----------
 drivers/md/raid5.c     |  4 ++--
 6 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/drivers/md/dm-kcopyd.c b/drivers/md/dm-kcopyd.c
index 1bbe4a34ef4c..ad861a3d648e 100644
--- a/drivers/md/dm-kcopyd.c
+++ b/drivers/md/dm-kcopyd.c
@@ -584,7 +584,7 @@ static int run_io_job(struct kcopyd_job *job)
 static int run_pages_job(struct kcopyd_job *job)
 {
 	int r;
-	unsigned nr_pages = dm_div_up(job->dests[0].count, PAGE_SIZE >> 9);
+	unsigned int nr_pages = dm_div_up(job->dests[0].count, PAGE_SECTORS);
 
 	r = kcopyd_get_pages(job->kc, nr_pages, &job->pages);
 	if (!r) {
diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
index b952bd45bd6a..12ccf1c81661 100644
--- a/drivers/md/md-bitmap.c
+++ b/drivers/md/md-bitmap.c
@@ -161,7 +161,7 @@ static int read_sb_page(struct mddev *mddev, loff_t offset,
 		    || test_bit(Bitmap_sync, &rdev->flags))
 			continue;
 
-		target = offset + index * (PAGE_SIZE/512);
+		target = offset + index * PAGE_SECTORS;
 
 		if (sync_page_io(rdev, target,
 				 roundup(size, bdev_logical_block_size(rdev->bdev)),
@@ -237,17 +237,17 @@ static int write_sb_page(struct bitmap *bitmap, struct page *page, int wait)
 		if (mddev->external) {
 			/* Bitmap could be anywhere. */
 			if (rdev->sb_start + offset + (page->index
-						       * (PAGE_SIZE/512))
+						       * PAGE_SECTORS)
 			    > rdev->data_offset
 			    &&
 			    rdev->sb_start + offset
 			    < (rdev->data_offset + mddev->dev_sectors
-			     + (PAGE_SIZE/512)))
+			     + PAGE_SECTORS))
 				goto bad_alignment;
 		} else if (offset < 0) {
 			/* DATA  BITMAP METADATA  */
 			if (offset
-			    + (long)(page->index * (PAGE_SIZE/512))
+			    + (long)(page->index * PAGE_SECTORS)
 			    + size/512 > 0)
 				/* bitmap runs in to metadata */
 				goto bad_alignment;
@@ -259,7 +259,7 @@ static int write_sb_page(struct bitmap *bitmap, struct page *page, int wait)
 			/* METADATA BITMAP DATA */
 			if (rdev->sb_start
 			    + offset
-			    + page->index*(PAGE_SIZE/512) + size/512
+			    + page->index*PAGE_SECTORS + size/512
 			    > rdev->data_offset)
 				/* bitmap runs in to data */
 				goto bad_alignment;
@@ -268,7 +268,7 @@ static int write_sb_page(struct bitmap *bitmap, struct page *page, int wait)
 		}
 		md_super_write(mddev, rdev,
 			       rdev->sb_start + offset
-			       + page->index * (PAGE_SIZE/512),
+			       + page->index * PAGE_SECTORS,
 			       size,
 			       page);
 	}
@@ -1548,14 +1548,14 @@ int md_bitmap_start_sync(struct bitmap *bitmap, sector_t offset, sector_t *block
 	 * pages, otherwise resync (which is very PAGE_SIZE based) will
 	 * get confused.
 	 * So call __bitmap_start_sync repeatedly (if needed) until
-	 * At least PAGE_SIZE>>9 blocks are covered.
+	 * At least PAGE_SECTORS blocks are covered.
 	 * Return the 'or' of the result.
 	 */
 	int rv = 0;
 	sector_t blocks1;
 
 	*blocks = 0;
-	while (*blocks < (PAGE_SIZE>>9)) {
+	while (*blocks < PAGE_SECTORS) {
 		rv |= __bitmap_start_sync(bitmap, offset,
 					  &blocks1, degraded);
 		offset += blocks1;
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 271e8a587354..a7572b17cf2e 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -1736,7 +1736,7 @@ static int super_1_load(struct md_rdev *rdev, struct md_rdev *refdev, int minor_
 		__le64 *bbp;
 		int i;
 		int sectors = le16_to_cpu(sb->bblog_size);
-		if (sectors > (PAGE_SIZE / 512))
+		if (sectors > PAGE_SECTORS)
 			return -EINVAL;
 		offset = le32_to_cpu(sb->bblog_offset);
 		if (offset == 0)
@@ -2223,7 +2223,7 @@ super_1_allow_new_offset(struct md_rdev *rdev,
 	bitmap = rdev->mddev->bitmap;
 	if (bitmap && !rdev->mddev->bitmap_info.file &&
 	    rdev->sb_start + rdev->mddev->bitmap_info.offset +
-	    bitmap->storage.file_pages * (PAGE_SIZE>>9) > new_offset)
+	    bitmap->storage.file_pages * PAGE_SECTORS > new_offset)
 		return 0;
 	if (rdev->badblocks.sector + rdev->badblocks.size > new_offset)
 		return 0;
@@ -8734,7 +8734,7 @@ void md_do_sync(struct md_thread *thread)
 	/*
 	 * Tune reconstruction:
 	 */
-	window = 32 * (PAGE_SIZE / 512);
+	window = 32 * PAGE_SECTORS;
 	pr_debug("md: using %dk window, over a total of %lluk.\n",
 		 window/2, (unsigned long long)max_sectors/2);
 
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 44ffe1b6d77a..717c6e397cad 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -2021,8 +2021,8 @@ static int fix_sync_read_error(struct r1bio *r1_bio)
 		int success = 0;
 		int start;
 
-		if (s > (PAGE_SIZE>>9))
-			s = PAGE_SIZE >> 9;
+		if (s > PAGE_SECTORS)
+			s = PAGE_SECTORS;
 		do {
 			if (r1_bio->bios[d]->bi_end_io == end_sync_read) {
 				/* No rcu protection needed here devices
@@ -2129,7 +2129,7 @@ static void process_checks(struct r1bio *r1_bio)
 	int vcnt;
 
 	/* Fix variable parts of all bios */
-	vcnt = sectors_to_npage(r1_bio->sectors + PAGE_SIZE / 512 - 1);
+	vcnt = sectors_to_npage(r1_bio->sectors + PAGE_SECTORS - 1);
 	for (i = 0; i < conf->raid_disks * 2; i++) {
 		blk_status_t status;
 		struct bio *b = r1_bio->bios[i];
@@ -2264,8 +2264,8 @@ static void fix_read_error(struct r1conf *conf, int read_disk,
 		int start;
 		struct md_rdev *rdev;
 
-		if (s > (PAGE_SIZE>>9))
-			s = PAGE_SIZE >> 9;
+		if (s > PAGE_SECTORS)
+			s = PAGE_SECTORS;
 
 		do {
 			sector_t first_bad;
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index ac4273f804e8..21bc6e33c093 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -2029,7 +2029,7 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
 	fbio->bi_iter.bi_idx = 0;
 	fpages = get_resync_pages(fbio)->pages;
 
-	vcnt = sectors_to_npage(r10_bio->sectors + (PAGE_SIZE >> 9) - 1);
+	vcnt = sectors_to_npage(r10_bio->sectors + PAGE_SECTORS - 1);
 	/* now find blocks with errors */
 	for (i=0 ; i < conf->copies ; i++) {
 		int  j, d;
@@ -2163,8 +2163,8 @@ static void fix_recovery_read_error(struct r10bio *r10_bio)
 		sector_t addr;
 		int ok;
 
-		if (s > (PAGE_SIZE>>9))
-			s = PAGE_SIZE >> 9;
+		if (s > PAGE_SECTORS)
+			s = PAGE_SECTORS;
 
 		rdev = conf->mirrors[dr].rdev;
 		addr = r10_bio->devs[0].addr + sect,
@@ -2367,8 +2367,8 @@ static void fix_read_error(struct r10conf *conf, struct mddev *mddev, struct r10
 		int success = 0;
 		int start;
 
-		if (s > (PAGE_SIZE>>9))
-			s = PAGE_SIZE >> 9;
+		if (s > PAGE_SECTORS)
+			s = PAGE_SECTORS;
 
 		rcu_read_lock();
 		do {
@@ -3597,7 +3597,7 @@ static int setup_geo(struct geom *geo, struct mddev *mddev, enum geo_type new)
 	}
 	if (layout >> 19)
 		return -1;
-	if (chunk < (PAGE_SIZE >> 9) ||
+	if (chunk < PAGE_SECTORS ||
 	    !is_power_of_2(chunk))
 		return -2;
 	nc = layout & 255;
@@ -4632,8 +4632,8 @@ static sector_t reshape_request(struct mddev *mddev, sector_t sector_nr,
 
 	nr_sectors = 0;
 	pages = get_resync_pages(r10_bio->devs[0].bio)->pages;
-	for (s = 0 ; s < max_sectors; s += PAGE_SIZE >> 9) {
-		struct page *page = pages[s / (PAGE_SIZE >> 9)];
+	for (s = 0 ; s < max_sectors; s += PAGE_SECTORS) {
+		struct page *page = pages[s / PAGE_SECTORS];
 		int len = (max_sectors - s) << 9;
 		if (len > PAGE_SIZE)
 			len = PAGE_SIZE;
@@ -4789,8 +4789,8 @@ static int handle_reshape_read_error(struct mddev *mddev,
 		int success = 0;
 		int first_slot = slot;
 
-		if (s > (PAGE_SIZE >> 9))
-			s = PAGE_SIZE >> 9;
+		if (s > PAGE_SECTORS)
+			s = PAGE_SECTORS;
 
 		rcu_read_lock();
 		while (!success) {
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index ba00e9877f02..5b316538b9ea 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -8197,7 +8197,7 @@ static int raid5_check_reshape(struct mddev *mddev)
 	if (new_chunk > 0) {
 		if (!is_power_of_2(new_chunk))
 			return -EINVAL;
-		if (new_chunk < (PAGE_SIZE>>9))
+		if (new_chunk < PAGE_SECTORS)
 			return -EINVAL;
 		if (mddev->array_sectors & (new_chunk-1))
 			/* not factor of array size */
@@ -8231,7 +8231,7 @@ static int raid6_check_reshape(struct mddev *mddev)
 	if (new_chunk > 0) {
 		if (!is_power_of_2(new_chunk))
 			return -EINVAL;
-		if (new_chunk < (PAGE_SIZE >> 9))
+		if (new_chunk < PAGE_SECTORS)
 			return -EINVAL;
 		if (mddev->array_sectors & (new_chunk-1))
 			/* not factor of array size */
-- 
2.26.0.106.g9fadedd

Re: [PATCH v2 00/10] clean up SECTOR related macros and sectors/pages conversions

[Leizhen (ThunderTown)]

Hi, all:
   It seems no one take care about these patches. But I think patch 1 is need. And
the main discussion points of others is whether we should add
sectors_to_npage()/npage_to_sectors() or keep PAGE_SECTORS_SHIFT. And which marco
name do we prefer: PAGE_SECTORS vs SECTORS_PER_PAGE, PAGE_SECTORS_SHIFT vs
SECTORS_PER_PAGE_SHIFT.

Hi, Jens Axboe, Coly Li, Kent Overstreet,Alasdair Kergon. Mike Snitzer:
   Can you take a look at patch 1?

On 2020/5/7 15:50, Zhen Lei wrote:
> v1 --> v2:
> As Matthew Wilcox's suggestion, add sectors_to_npage()/npage_to_sectors()
> helpers to eliminate SECTORS_PER_PAGE_SHIFT, because it's quite hard to read.
> In further, I also eliminated PAGE_SECTORS_SHIFT.
> 
> I tried to eliminate all magic number "9" and "512", but it's too many, maybe
> no one want to review it, so I gave up. In the process of searching, I found
> the existing macro PAGE_SECTORS, it's equivalent to SECTORS_PER_PAGE. Because
> PAGE_SECTORS was defined in include/linux/device-mapper.h, and SECTORS_PER_PAGE
> was defined in drivers/block/zram/zram_drv.h, so I discarded SECTORS_PER_PAGE,
> althrough I prefer it so much.
> 
> v1:
> When I studied the code of mm/swap, I found "1 << (PAGE_SHIFT - 9)" appears
> many times. So I try to clean up it.
> 
> 1. Replace "1 << (PAGE_SHIFT - 9)" or similar with SECTORS_PER_PAGE
> 2. Replace "PAGE_SHIFT - 9" with SECTORS_PER_PAGE_SHIFT
> 3. Replace "9" with SECTOR_SHIFT
> 4. Replace "512" with SECTOR_SIZE
> 
> Zhen Lei (10):
>   block: move PAGE_SECTORS definition into <linux/blkdev.h>
>   zram: abolish macro SECTORS_PER_PAGE
>   block: add sectors_to_npage()/npage_to_sectors() helpers
>   zram: abolish macro SECTORS_PER_PAGE_SHIFT
>   block: abolish macro PAGE_SECTORS_SHIFT
>   mm/swap: use npage_to_sectors() and PAGE_SECTORS to clean up code
>   block: use sectors_to_npage() and PAGE_SECTORS to clean up code
>   md: use sectors_to_npage() and npage_to_sectors() to clean up code
>   md: use existing definition RESYNC_SECTORS
>   md: use PAGE_SECTORS to clean up code
> 
>  block/blk-settings.c          |  6 +++---
>  block/partitions/core.c       |  5 ++---
>  drivers/block/brd.c           |  7 ++-----
>  drivers/block/null_blk_main.c | 10 ++++------
>  drivers/block/zram/zram_drv.c |  8 ++++----
>  drivers/block/zram/zram_drv.h |  2 --
>  drivers/md/bcache/util.h      |  2 --
>  drivers/md/dm-kcopyd.c        |  2 +-
>  drivers/md/dm-table.c         |  2 +-
>  drivers/md/md-bitmap.c        | 16 ++++++++--------
>  drivers/md/md.c               |  6 +++---
>  drivers/md/raid1.c            | 10 +++++-----
>  drivers/md/raid10.c           | 28 ++++++++++++++--------------
>  drivers/md/raid5-cache.c      | 11 +++++------
>  drivers/md/raid5.c            |  4 ++--
>  include/linux/blkdev.h        |  7 +++++--
>  include/linux/device-mapper.h |  1 -
>  mm/page_io.c                  |  4 ++--
>  mm/swapfile.c                 | 12 ++++++------
>  19 files changed, 67 insertions(+), 76 deletions(-)
> 

Re: [PATCH v2 06/10] mm/swap: use npage_to_sectors() and PAGE_SECTORS to clean up code

[Matthew Wilcox]

On Thu, May 07, 2020 at 03:50:56PM +0800, Zhen Lei wrote:
> @@ -266,7 +266,7 @@ int swap_writepage(struct page *page, struct writeback_control *wbc)
>  
>  static sector_t swap_page_sector(struct page *page)
>  {
> -	return (sector_t)__page_file_index(page) << (PAGE_SHIFT - 9);
> +	return npage_to_sectors((sector_t)__page_file_index(page));

If you make npage_to_sectors() a proper function instead of a macro,
you can do the casting inside the function instead of in the callers
(which is prone to bugs).

Also, this is a great example of why page_to_sector() was a better name
than npage_to_sectors().  This function doesn't return a count of sectors,
it returns a sector number within the swap device.

Re: [PATCH v2 06/10] mm/swap: use npage_to_sectors() and PAGE_SECTORS to clean up code

[Matthew Wilcox]

On Thu, May 07, 2020 at 03:50:56PM +0800, Zhen Lei wrote:
> +++ b/mm/page_io.c
> @@ -38,7 +38,7 @@ static struct bio *get_swap_bio(gfp_t gfp_flags,
>  
>  		bio->bi_iter.bi_sector = map_swap_page(page, &bdev);
>  		bio_set_dev(bio, bdev);
> -		bio->bi_iter.bi_sector <<= PAGE_SHIFT - 9;
> +		bio->bi_iter.bi_sector *= PAGE_SECTORS;
>  		bio->bi_end_io = end_io;

This just doesn't look right.  Why is map_swap_page() returning a sector_t
which isn't actually a sector_t?

Re: [PATCH v2 07/10] block: use sectors_to_npage() and PAGE_SECTORS to clean up code

[Matthew Wilcox]

On Thu, May 07, 2020 at 03:50:57PM +0800, Zhen Lei wrote:
> +++ b/block/blk-settings.c
> @@ -150,7 +150,7 @@ void blk_queue_max_hw_sectors(struct request_queue *q, unsigned int max_hw_secto
>  	unsigned int max_sectors;
>  
>  	if ((max_hw_sectors << 9) < PAGE_SIZE) {
> -		max_hw_sectors = 1 << (PAGE_SHIFT - 9);
> +		max_hw_sectors = PAGE_SECTORS;

Surely this should be:

	if (max_hw_sectors < PAGE_SECTORS) {
		max_hw_sectors = PAGE_SECTORS;

... no?

> -	page = read_mapping_page(mapping,
> -			(pgoff_t)(n >> (PAGE_SHIFT - 9)), NULL);
> +	page = read_mapping_page(mapping, (pgoff_t)sectors_to_npage(n), NULL);

... again, get the type right, and you won't need the cast.

Re: [PATCH v2 06/10] mm/swap: use npage_to_sectors() and PAGE_SECTORS to clean up code

[Leizhen (ThunderTown)]

On 2020/5/15 12:06, Matthew Wilcox wrote:
> On Thu, May 07, 2020 at 03:50:56PM +0800, Zhen Lei wrote:
>> @@ -266,7 +266,7 @@ int swap_writepage(struct page *page, struct writeback_control *wbc)
>>  
>>  static sector_t swap_page_sector(struct page *page)
>>  {
>> -	return (sector_t)__page_file_index(page) << (PAGE_SHIFT - 9);
>> +	return npage_to_sectors((sector_t)__page_file_index(page));
> 
> If you make npage_to_sectors() a proper function instead of a macro,
> you can do the casting inside the function instead of in the callers
> (which is prone to bugs).

Oh, yes. __page_file_index(page) maybe called many times in marco, althouth currently
it is not. So that, not all are suitable for page_to_sector(). And for this example,
still need to use "<< PAGE_SECTORS_SHIFT".

> 
> Also, this is a great example of why page_to_sector() was a better name
> than npage_to_sectors().  This function doesn't return a count of sectors,
> it returns a sector number within the swap device.
OK, so I will change to page_to_sector()/sector_to_page().

> 
> 
> .
> 

Re: [PATCH v2 06/10] mm/swap: use npage_to_sectors() and PAGE_SECTORS to clean up code

[Leizhen (ThunderTown)]

On 2020/5/15 12:14, Matthew Wilcox wrote:
> On Thu, May 07, 2020 at 03:50:56PM +0800, Zhen Lei wrote:
>> +++ b/mm/page_io.c
>> @@ -38,7 +38,7 @@ static struct bio *get_swap_bio(gfp_t gfp_flags,
>>  
>>  		bio->bi_iter.bi_sector = map_swap_page(page, &bdev);
>>  		bio_set_dev(bio, bdev);
>> -		bio->bi_iter.bi_sector <<= PAGE_SHIFT - 9;
>> +		bio->bi_iter.bi_sector *= PAGE_SECTORS;
>>  		bio->bi_end_io = end_io;
> 
> This just doesn't look right.  Why is map_swap_page() returning a sector_t
> which isn't actually a sector_t?

I try to understand map_swap_page(). Here maybe a bug. Otherwise, it would be
better to add a temporary variable to cache the return value of map_swap_page(page, &bdev).

> 
> 
> .
> 

Re: [PATCH v2 07/10] block: use sectors_to_npage() and PAGE_SECTORS to clean up code

[Leizhen (ThunderTown)]

On 2020/5/15 12:19, Matthew Wilcox wrote:
> On Thu, May 07, 2020 at 03:50:57PM +0800, Zhen Lei wrote:
>> +++ b/block/blk-settings.c
>> @@ -150,7 +150,7 @@ void blk_queue_max_hw_sectors(struct request_queue *q, unsigned int max_hw_secto
>>  	unsigned int max_sectors;
>>  
>>  	if ((max_hw_sectors << 9) < PAGE_SIZE) {
>> -		max_hw_sectors = 1 << (PAGE_SHIFT - 9);
>> +		max_hw_sectors = PAGE_SECTORS;
> 
> Surely this should be:
> 
> 	if (max_hw_sectors < PAGE_SECTORS) {
> 		max_hw_sectors = PAGE_SECTORS;
> 
> ... no?

I've noticed this place before. "(max_hw_sectors << 9) < PAGE_SIZE" can also make sure
that max_hw_sectors is not too large, that means (max_hw_sectors << 9) may overflow.

> 
>> -	page = read_mapping_page(mapping,
>> -			(pgoff_t)(n >> (PAGE_SHIFT - 9)), NULL);
>> +	page = read_mapping_page(mapping, (pgoff_t)sectors_to_npage(n), NULL);
> 
> ... again, get the type right, and you won't need the cast.
OK, I'll consider it.

> 
> 
> .
>