linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] block: simplify with PAGE_SECTORS_SHIFT
@ 2023-04-21 19:58 Luis Chamberlain
  2023-04-21 19:58 ` [PATCH 1/5] dm integrity: simplify by using PAGE_SECTORS_SHIFT Luis Chamberlain
                   ` (4 more replies)
  0 siblings, 5 replies; 16+ messages in thread
From: Luis Chamberlain @ 2023-04-21 19:58 UTC (permalink / raw)
  To: axboe, agk, snitzer, philipp.reisner, lars.ellenberg,
	christoph.boehmwalder, hch, djwong, minchan, senozhatsky
  Cc: patches, linux-block, linux-mm, linux-xfs, linux-fsdevel,
	dm-devel, drbd-dev, linux-kernel, willy, hare, p.raghav,
	da.gomez, kbusch, mcgrof

A bit of block drivers have their own incantations with
PAGE_SHIFT - SECTOR_SHIFT. Just simplfy and use PAGE_SECTORS_SHIFT
all over.

Based on linux-next next-20230421.

Luis Chamberlain (5):
  dm integrity: simplify by using PAGE_SECTORS_SHIFT
  drbd: use PAGE_SECTORS_SHIFT and PAGE_SECTORS
  iomap: simplify iomap_init() with PAGE_SECTORS
  dm bufio: simplify by using PAGE_SECTORS_SHIFT
  zram: use generic PAGE_SECTORS and PAGE_SECTORS_SHIFT

 drivers/block/drbd/drbd_bitmap.c |  4 ++--
 drivers/block/zram/zram_drv.c    | 12 ++++++------
 drivers/block/zram/zram_drv.h    |  2 --
 drivers/md/dm-bufio.c            |  4 ++--
 drivers/md/dm-integrity.c        | 10 +++++-----
 fs/iomap/buffered-io.c           |  2 +-
 6 files changed, 16 insertions(+), 18 deletions(-)

-- 
2.39.2



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

* [PATCH 1/5] dm integrity: simplify by using PAGE_SECTORS_SHIFT
  2023-04-21 19:58 [PATCH 0/5] block: simplify with PAGE_SECTORS_SHIFT Luis Chamberlain
@ 2023-04-21 19:58 ` Luis Chamberlain
  2023-04-21 20:15   ` Matthew Wilcox
  2023-04-21 19:58 ` [PATCH 2/5] drbd: use PAGE_SECTORS_SHIFT and PAGE_SECTORS Luis Chamberlain
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 16+ messages in thread
From: Luis Chamberlain @ 2023-04-21 19:58 UTC (permalink / raw)
  To: axboe, agk, snitzer, philipp.reisner, lars.ellenberg,
	christoph.boehmwalder, hch, djwong, minchan, senozhatsky
  Cc: patches, linux-block, linux-mm, linux-xfs, linux-fsdevel,
	dm-devel, drbd-dev, linux-kernel, willy, hare, p.raghav,
	da.gomez, kbusch, mcgrof

The PAGE_SHIFT - SECTOR_SHIFT constant be replaced with PAGE_SECTORS_SHIFT
defined in linux/blt_types.h, which is included by linux/blkdev.h.

This produces no functional changes.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 drivers/md/dm-integrity.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
index 31838b13ea54..a179265970dd 100644
--- a/drivers/md/dm-integrity.c
+++ b/drivers/md/dm-integrity.c
@@ -752,7 +752,7 @@ static void page_list_location(struct dm_integrity_c *ic, unsigned int section,
 
 	sector = section * ic->journal_section_sectors + offset;
 
-	*pl_index = sector >> (PAGE_SHIFT - SECTOR_SHIFT);
+	*pl_index = sector >> (PAGE_SECTORS_SHIFT);
 	*pl_offset = (sector << SECTOR_SHIFT) & (PAGE_SIZE - 1);
 }
 
@@ -1077,7 +1077,7 @@ static void rw_journal_sectors(struct dm_integrity_c *ic, blk_opf_t opf,
 		return;
 	}
 
-	pl_index = sector >> (PAGE_SHIFT - SECTOR_SHIFT);
+	pl_index = sector >> (PAGE_SECTORS_SHIFT);
 	pl_offset = (sector << SECTOR_SHIFT) & (PAGE_SIZE - 1);
 
 	io_req.bi_opf = opf;
@@ -1201,7 +1201,7 @@ static void copy_from_journal(struct dm_integrity_c *ic, unsigned int section, u
 
 	sector = section * ic->journal_section_sectors + JOURNAL_BLOCK_SECTORS + offset;
 
-	pl_index = sector >> (PAGE_SHIFT - SECTOR_SHIFT);
+	pl_index = sector >> (PAGE_SECTORS_SHIFT);
 	pl_offset = (sector << SECTOR_SHIFT) & (PAGE_SIZE - 1);
 
 	io_req.bi_opf = REQ_OP_WRITE;
@@ -3765,7 +3765,7 @@ static int create_journal(struct dm_integrity_c *ic, char **error)
 	ic->commit_ids[3] = cpu_to_le64(0x4444444444444444ULL);
 
 	journal_pages = roundup((__u64)ic->journal_sections * ic->journal_section_sectors,
-				PAGE_SIZE >> SECTOR_SHIFT) >> (PAGE_SHIFT - SECTOR_SHIFT);
+				PAGE_SIZE >> SECTOR_SHIFT) >> (PAGE_SECTORS_SHIFT);
 	journal_desc_size = journal_pages * sizeof(struct page_list);
 	if (journal_pages >= totalram_pages() - totalhigh_pages() || journal_desc_size > ULONG_MAX) {
 		*error = "Journal doesn't fit into memory";
@@ -4542,7 +4542,7 @@ static int dm_integrity_ctr(struct dm_target *ti, unsigned int argc, char **argv
 			spin_lock_init(&bbs->bio_queue_lock);
 
 			sector = i * (BITMAP_BLOCK_SIZE >> SECTOR_SHIFT);
-			pl_index = sector >> (PAGE_SHIFT - SECTOR_SHIFT);
+			pl_index = sector >> (PAGE_SECTORS_SHIFT);
 			pl_offset = (sector << SECTOR_SHIFT) & (PAGE_SIZE - 1);
 
 			bbs->bitmap = lowmem_page_address(ic->journal[pl_index].page) + pl_offset;
-- 
2.39.2



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

* [PATCH 2/5] drbd: use PAGE_SECTORS_SHIFT and PAGE_SECTORS
  2023-04-21 19:58 [PATCH 0/5] block: simplify with PAGE_SECTORS_SHIFT Luis Chamberlain
  2023-04-21 19:58 ` [PATCH 1/5] dm integrity: simplify by using PAGE_SECTORS_SHIFT Luis Chamberlain
@ 2023-04-21 19:58 ` Luis Chamberlain
  2023-04-24  8:26   ` Christoph Böhmwalder
  2023-04-21 19:58 ` [PATCH 3/5] iomap: simplify iomap_init() with PAGE_SECTORS Luis Chamberlain
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 16+ messages in thread
From: Luis Chamberlain @ 2023-04-21 19:58 UTC (permalink / raw)
  To: axboe, agk, snitzer, philipp.reisner, lars.ellenberg,
	christoph.boehmwalder, hch, djwong, minchan, senozhatsky
  Cc: patches, linux-block, linux-mm, linux-xfs, linux-fsdevel,
	dm-devel, drbd-dev, linux-kernel, willy, hare, p.raghav,
	da.gomez, kbusch, mcgrof

Replace common constants with generic versions.
This produces no functional changes.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 drivers/block/drbd/drbd_bitmap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/block/drbd/drbd_bitmap.c b/drivers/block/drbd/drbd_bitmap.c
index 6ac8c54b44c7..b556e6634f13 100644
--- a/drivers/block/drbd/drbd_bitmap.c
+++ b/drivers/block/drbd/drbd_bitmap.c
@@ -1000,7 +1000,7 @@ static void bm_page_io_async(struct drbd_bm_aio_ctx *ctx, int page_nr) __must_ho
 	unsigned int len;
 
 	first_bm_sect = device->ldev->md.md_offset + device->ldev->md.bm_offset;
-	on_disk_sector = first_bm_sect + (((sector_t)page_nr) << (PAGE_SHIFT-SECTOR_SHIFT));
+	on_disk_sector = first_bm_sect + (((sector_t)page_nr) << PAGE_SECTORS_SHIFT);
 
 	/* this might happen with very small
 	 * flexible external meta data device,
@@ -1008,7 +1008,7 @@ static void bm_page_io_async(struct drbd_bm_aio_ctx *ctx, int page_nr) __must_ho
 	last_bm_sect = drbd_md_last_bitmap_sector(device->ldev);
 	if (first_bm_sect <= on_disk_sector && last_bm_sect >= on_disk_sector) {
 		sector_t len_sect = last_bm_sect - on_disk_sector + 1;
-		if (len_sect < PAGE_SIZE/SECTOR_SIZE)
+		if (len_sect < PAGE_SECTORS)
 			len = (unsigned int)len_sect*SECTOR_SIZE;
 		else
 			len = PAGE_SIZE;
-- 
2.39.2



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

* [PATCH 3/5] iomap: simplify iomap_init() with PAGE_SECTORS
  2023-04-21 19:58 [PATCH 0/5] block: simplify with PAGE_SECTORS_SHIFT Luis Chamberlain
  2023-04-21 19:58 ` [PATCH 1/5] dm integrity: simplify by using PAGE_SECTORS_SHIFT Luis Chamberlain
  2023-04-21 19:58 ` [PATCH 2/5] drbd: use PAGE_SECTORS_SHIFT and PAGE_SECTORS Luis Chamberlain
@ 2023-04-21 19:58 ` Luis Chamberlain
  2023-04-21 20:14   ` Matthew Wilcox
  2023-04-21 19:58 ` [PATCH 4/5] dm bufio: simplify by using PAGE_SECTORS_SHIFT Luis Chamberlain
  2023-04-21 19:58 ` [PATCH 5/5] zram: use generic PAGE_SECTORS and PAGE_SECTORS_SHIFT Luis Chamberlain
  4 siblings, 1 reply; 16+ messages in thread
From: Luis Chamberlain @ 2023-04-21 19:58 UTC (permalink / raw)
  To: axboe, agk, snitzer, philipp.reisner, lars.ellenberg,
	christoph.boehmwalder, hch, djwong, minchan, senozhatsky
  Cc: patches, linux-block, linux-mm, linux-xfs, linux-fsdevel,
	dm-devel, drbd-dev, linux-kernel, willy, hare, p.raghav,
	da.gomez, kbusch, mcgrof

Just use the PAGE_SECTORS generic define. This produces no functional
changes. While at it use left shift to simplify this even further.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 fs/iomap/buffered-io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 063133ec77f4..ba2824f405df 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -1819,7 +1819,7 @@ EXPORT_SYMBOL_GPL(iomap_writepages);
 
 static int __init iomap_init(void)
 {
-	return bioset_init(&iomap_ioend_bioset, 4 * (PAGE_SIZE / SECTOR_SIZE),
+	return bioset_init(&iomap_ioend_bioset, PAGE_SECTORS << 2,
 			   offsetof(struct iomap_ioend, io_inline_bio),
 			   BIOSET_NEED_BVECS);
 }
-- 
2.39.2



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

* [PATCH 4/5] dm bufio: simplify by using PAGE_SECTORS_SHIFT
  2023-04-21 19:58 [PATCH 0/5] block: simplify with PAGE_SECTORS_SHIFT Luis Chamberlain
                   ` (2 preceding siblings ...)
  2023-04-21 19:58 ` [PATCH 3/5] iomap: simplify iomap_init() with PAGE_SECTORS Luis Chamberlain
@ 2023-04-21 19:58 ` Luis Chamberlain
  2023-04-21 19:58 ` [PATCH 5/5] zram: use generic PAGE_SECTORS and PAGE_SECTORS_SHIFT Luis Chamberlain
  4 siblings, 0 replies; 16+ messages in thread
From: Luis Chamberlain @ 2023-04-21 19:58 UTC (permalink / raw)
  To: axboe, agk, snitzer, philipp.reisner, lars.ellenberg,
	christoph.boehmwalder, hch, djwong, minchan, senozhatsky
  Cc: patches, linux-block, linux-mm, linux-xfs, linux-fsdevel,
	dm-devel, drbd-dev, linux-kernel, willy, hare, p.raghav,
	da.gomez, kbusch, mcgrof

The PAGE_SHIFT - SECTOR_SHIFT constant be replaced with PAGE_SECTORS_SHIFT
defined in linux/blt_types.h, which is included by linux/blkdev.h.

This produces no functional changes.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 drivers/md/dm-bufio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
index eea977662e81..08c4730e1819 100644
--- a/drivers/md/dm-bufio.c
+++ b/drivers/md/dm-bufio.c
@@ -1152,7 +1152,7 @@ static void *alloc_buffer_data(struct dm_bufio_client *c, gfp_t gfp_mask,
 	    gfp_mask & __GFP_NORETRY) {
 		*data_mode = DATA_MODE_GET_FREE_PAGES;
 		return (void *)__get_free_pages(gfp_mask,
-						c->sectors_per_block_bits - (PAGE_SHIFT - SECTOR_SHIFT));
+						c->sectors_per_block_bits - (PAGE_SECTORS_SHIFT));
 	}
 
 	*data_mode = DATA_MODE_VMALLOC;
@@ -1190,7 +1190,7 @@ static void free_buffer_data(struct dm_bufio_client *c,
 
 	case DATA_MODE_GET_FREE_PAGES:
 		free_pages((unsigned long)data,
-			   c->sectors_per_block_bits - (PAGE_SHIFT - SECTOR_SHIFT));
+			   c->sectors_per_block_bits - (PAGE_SECTORS_SHIFT));
 		break;
 
 	case DATA_MODE_VMALLOC:
-- 
2.39.2



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

* [PATCH 5/5] zram: use generic PAGE_SECTORS and PAGE_SECTORS_SHIFT
  2023-04-21 19:58 [PATCH 0/5] block: simplify with PAGE_SECTORS_SHIFT Luis Chamberlain
                   ` (3 preceding siblings ...)
  2023-04-21 19:58 ` [PATCH 4/5] dm bufio: simplify by using PAGE_SECTORS_SHIFT Luis Chamberlain
@ 2023-04-21 19:58 ` Luis Chamberlain
  2023-04-24  2:36   ` Sergey Senozhatsky
  4 siblings, 1 reply; 16+ messages in thread
From: Luis Chamberlain @ 2023-04-21 19:58 UTC (permalink / raw)
  To: axboe, agk, snitzer, philipp.reisner, lars.ellenberg,
	christoph.boehmwalder, hch, djwong, minchan, senozhatsky
  Cc: patches, linux-block, linux-mm, linux-xfs, linux-fsdevel,
	dm-devel, drbd-dev, linux-kernel, willy, hare, p.raghav,
	da.gomez, kbusch, mcgrof

Instead of re-defining the already existing constants use the provided ones:

So replace:

 o SECTORS_PER_PAGE_SHIFT with PAGE_SECTORS_SHIFT
 o SECTORS_PER_PAGE       with PAGE_SECTORS

This produces no functional changes.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 drivers/block/zram/zram_drv.c | 12 ++++++------
 drivers/block/zram/zram_drv.h |  2 --
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index a84c4268257a..11c9b5a9ac7a 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1834,8 +1834,8 @@ static ssize_t recompress_store(struct device *dev,
 static void zram_bio_discard(struct zram *zram, struct bio *bio)
 {
 	size_t n = bio->bi_iter.bi_size;
-	u32 index = bio->bi_iter.bi_sector >> SECTORS_PER_PAGE_SHIFT;
-	u32 offset = (bio->bi_iter.bi_sector & (SECTORS_PER_PAGE - 1)) <<
+	u32 index = bio->bi_iter.bi_sector >> PAGE_SECTORS_SHIFT;
+	u32 offset = (bio->bi_iter.bi_sector & (PAGE_SECTORS - 1)) <<
 			SECTOR_SHIFT;
 
 	/*
@@ -1876,8 +1876,8 @@ static void zram_bio_read(struct zram *zram, struct bio *bio)
 
 	start_time = bio_start_io_acct(bio);
 	bio_for_each_segment(bv, bio, iter) {
-		u32 index = iter.bi_sector >> SECTORS_PER_PAGE_SHIFT;
-		u32 offset = (iter.bi_sector & (SECTORS_PER_PAGE - 1)) <<
+		u32 index = iter.bi_sector >> PAGE_SECTORS_SHIFT;
+		u32 offset = (iter.bi_sector & (PAGE_SECTORS - 1)) <<
 				SECTOR_SHIFT;
 
 		if (zram_bvec_read(zram, &bv, index, offset, bio) < 0) {
@@ -1903,8 +1903,8 @@ static void zram_bio_write(struct zram *zram, struct bio *bio)
 
 	start_time = bio_start_io_acct(bio);
 	bio_for_each_segment(bv, bio, iter) {
-		u32 index = iter.bi_sector >> SECTORS_PER_PAGE_SHIFT;
-		u32 offset = (iter.bi_sector & (SECTORS_PER_PAGE - 1)) <<
+		u32 index = iter.bi_sector >> PAGE_SECTORS_SHIFT;
+		u32 offset = (iter.bi_sector & (PAGE_SECTORS - 1)) <<
 				SECTOR_SHIFT;
 
 		if (zram_bvec_write(zram, &bv, index, offset, bio) < 0) {
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index ca7a15bd4845..9f2543af5c76 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -21,8 +21,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.39.2



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

* Re: [PATCH 3/5] iomap: simplify iomap_init() with PAGE_SECTORS
  2023-04-21 19:58 ` [PATCH 3/5] iomap: simplify iomap_init() with PAGE_SECTORS Luis Chamberlain
@ 2023-04-21 20:14   ` Matthew Wilcox
  2023-04-21 22:02     ` Luis Chamberlain
  0 siblings, 1 reply; 16+ messages in thread
From: Matthew Wilcox @ 2023-04-21 20:14 UTC (permalink / raw)
  To: Luis Chamberlain
  Cc: axboe, agk, snitzer, philipp.reisner, lars.ellenberg,
	christoph.boehmwalder, hch, djwong, minchan, senozhatsky,
	patches, linux-block, linux-mm, linux-xfs, linux-fsdevel,
	dm-devel, drbd-dev, linux-kernel, hare, p.raghav, da.gomez,
	kbusch

On Fri, Apr 21, 2023 at 12:58:05PM -0700, Luis Chamberlain wrote:
> Just use the PAGE_SECTORS generic define. This produces no functional
> changes. While at it use left shift to simplify this even further.

How is FOO << 2 simpler than FOO * 4?

> -	return bioset_init(&iomap_ioend_bioset, 4 * (PAGE_SIZE / SECTOR_SIZE),
> +	return bioset_init(&iomap_ioend_bioset, PAGE_SECTORS << 2,



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

* Re: [PATCH 1/5] dm integrity: simplify by using PAGE_SECTORS_SHIFT
  2023-04-21 19:58 ` [PATCH 1/5] dm integrity: simplify by using PAGE_SECTORS_SHIFT Luis Chamberlain
@ 2023-04-21 20:15   ` Matthew Wilcox
  0 siblings, 0 replies; 16+ messages in thread
From: Matthew Wilcox @ 2023-04-21 20:15 UTC (permalink / raw)
  To: Luis Chamberlain
  Cc: axboe, agk, snitzer, philipp.reisner, lars.ellenberg,
	christoph.boehmwalder, hch, djwong, minchan, senozhatsky,
	patches, linux-block, linux-mm, linux-xfs, linux-fsdevel,
	dm-devel, drbd-dev, linux-kernel, hare, p.raghav, da.gomez,
	kbusch

On Fri, Apr 21, 2023 at 12:58:03PM -0700, Luis Chamberlain wrote:
> -	*pl_index = sector >> (PAGE_SHIFT - SECTOR_SHIFT);
> +	*pl_index = sector >> (PAGE_SECTORS_SHIFT);

You could/should remove the () around PAGE_SECTORS_SHIFT

(throughout)


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

* Re: [PATCH 3/5] iomap: simplify iomap_init() with PAGE_SECTORS
  2023-04-21 20:14   ` Matthew Wilcox
@ 2023-04-21 22:02     ` Luis Chamberlain
  2023-04-21 22:24       ` Jens Axboe
  2023-04-21 22:34       ` Dave Chinner
  0 siblings, 2 replies; 16+ messages in thread
From: Luis Chamberlain @ 2023-04-21 22:02 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: axboe, agk, snitzer, philipp.reisner, lars.ellenberg,
	christoph.boehmwalder, hch, djwong, minchan, senozhatsky,
	patches, linux-block, linux-mm, linux-xfs, linux-fsdevel,
	dm-devel, drbd-dev, linux-kernel, hare, p.raghav, da.gomez,
	kbusch

On Fri, Apr 21, 2023 at 09:14:00PM +0100, Matthew Wilcox wrote:
> On Fri, Apr 21, 2023 at 12:58:05PM -0700, Luis Chamberlain wrote:
> > Just use the PAGE_SECTORS generic define. This produces no functional
> > changes. While at it use left shift to simplify this even further.
> 
> How is FOO << 2 simpler than FOO * 4?
> 
> > -	return bioset_init(&iomap_ioend_bioset, 4 * (PAGE_SIZE / SECTOR_SIZE),
> > +	return bioset_init(&iomap_ioend_bioset, PAGE_SECTORS << 2,

We could just do:


-	return bioset_init(&iomap_ioend_bioset, 4 * (PAGE_SIZE / SECTOR_SIZE),
+	return bioset_init(&iomap_ioend_bioset, 4 * PAGE_SECTORS,

The shift just seemed optimal if we're just going to change it.

  Luis


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

* Re: [PATCH 3/5] iomap: simplify iomap_init() with PAGE_SECTORS
  2023-04-21 22:02     ` Luis Chamberlain
@ 2023-04-21 22:24       ` Jens Axboe
  2023-04-21 22:30         ` Luis Chamberlain
  2023-04-21 22:34       ` Dave Chinner
  1 sibling, 1 reply; 16+ messages in thread
From: Jens Axboe @ 2023-04-21 22:24 UTC (permalink / raw)
  To: Luis Chamberlain, Matthew Wilcox
  Cc: agk, snitzer, philipp.reisner, lars.ellenberg,
	christoph.boehmwalder, hch, djwong, minchan, senozhatsky,
	patches, linux-block, linux-mm, linux-xfs, linux-fsdevel,
	dm-devel, drbd-dev, linux-kernel, hare, p.raghav, da.gomez,
	kbusch

On 4/21/23 4:02 PM, Luis Chamberlain wrote:
> On Fri, Apr 21, 2023 at 09:14:00PM +0100, Matthew Wilcox wrote:
>> On Fri, Apr 21, 2023 at 12:58:05PM -0700, Luis Chamberlain wrote:
>>> Just use the PAGE_SECTORS generic define. This produces no functional
>>> changes. While at it use left shift to simplify this even further.
>>
>> How is FOO << 2 simpler than FOO * 4?
>>
>>> -	return bioset_init(&iomap_ioend_bioset, 4 * (PAGE_SIZE / SECTOR_SIZE),
>>> +	return bioset_init(&iomap_ioend_bioset, PAGE_SECTORS << 2,
> 
> We could just do:
> 
> 
> -	return bioset_init(&iomap_ioend_bioset, 4 * (PAGE_SIZE / SECTOR_SIZE),
> +	return bioset_init(&iomap_ioend_bioset, 4 * PAGE_SECTORS,
> 
> The shift just seemed optimal if we're just going to change it.

It's going to generate the same code, but the multiplication is arguably
easier to read (or harder to misread).

-- 
Jens Axboe




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

* Re: [PATCH 3/5] iomap: simplify iomap_init() with PAGE_SECTORS
  2023-04-21 22:24       ` Jens Axboe
@ 2023-04-21 22:30         ` Luis Chamberlain
  2023-04-21 22:36           ` Jens Axboe
  0 siblings, 1 reply; 16+ messages in thread
From: Luis Chamberlain @ 2023-04-21 22:30 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Matthew Wilcox, agk, snitzer, philipp.reisner, lars.ellenberg,
	christoph.boehmwalder, hch, djwong, minchan, senozhatsky,
	patches, linux-block, linux-mm, linux-xfs, linux-fsdevel,
	dm-devel, drbd-dev, linux-kernel, hare, p.raghav, da.gomez,
	kbusch

On Fri, Apr 21, 2023 at 04:24:57PM -0600, Jens Axboe wrote:
> On 4/21/23 4:02 PM, Luis Chamberlain wrote:
> > On Fri, Apr 21, 2023 at 09:14:00PM +0100, Matthew Wilcox wrote:
> >> On Fri, Apr 21, 2023 at 12:58:05PM -0700, Luis Chamberlain wrote:
> >>> Just use the PAGE_SECTORS generic define. This produces no functional
> >>> changes. While at it use left shift to simplify this even further.
> >>
> >> How is FOO << 2 simpler than FOO * 4?
> >>
> >>> -	return bioset_init(&iomap_ioend_bioset, 4 * (PAGE_SIZE / SECTOR_SIZE),
> >>> +	return bioset_init(&iomap_ioend_bioset, PAGE_SECTORS << 2,
> > 
> > We could just do:
> > 
> > 
> > -	return bioset_init(&iomap_ioend_bioset, 4 * (PAGE_SIZE / SECTOR_SIZE),
> > +	return bioset_init(&iomap_ioend_bioset, 4 * PAGE_SECTORS,
> > 
> > The shift just seemed optimal if we're just going to change it.
> 
> It's going to generate the same code, but the multiplication is arguably
> easier to read (or harder to misread).

Then let's stick with the 4 * PAGE_SECTORS. Let me know if you need another
patch.

  Luis


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

* Re: [PATCH 3/5] iomap: simplify iomap_init() with PAGE_SECTORS
  2023-04-21 22:02     ` Luis Chamberlain
  2023-04-21 22:24       ` Jens Axboe
@ 2023-04-21 22:34       ` Dave Chinner
  2023-04-24  5:55         ` Christoph Hellwig
  1 sibling, 1 reply; 16+ messages in thread
From: Dave Chinner @ 2023-04-21 22:34 UTC (permalink / raw)
  To: Luis Chamberlain
  Cc: Matthew Wilcox, axboe, agk, snitzer, philipp.reisner,
	lars.ellenberg, christoph.boehmwalder, hch, djwong, minchan,
	senozhatsky, patches, linux-block, linux-mm, linux-xfs,
	linux-fsdevel, dm-devel, drbd-dev, linux-kernel, hare, p.raghav,
	da.gomez, kbusch

On Fri, Apr 21, 2023 at 03:02:30PM -0700, Luis Chamberlain wrote:
> On Fri, Apr 21, 2023 at 09:14:00PM +0100, Matthew Wilcox wrote:
> > On Fri, Apr 21, 2023 at 12:58:05PM -0700, Luis Chamberlain wrote:
> > > Just use the PAGE_SECTORS generic define. This produces no functional
> > > changes. While at it use left shift to simplify this even further.
> > 
> > How is FOO << 2 simpler than FOO * 4?
> > 
> > > -	return bioset_init(&iomap_ioend_bioset, 4 * (PAGE_SIZE / SECTOR_SIZE),
> > > +	return bioset_init(&iomap_ioend_bioset, PAGE_SECTORS << 2,
> 
> We could just do:
> 
> 
> -	return bioset_init(&iomap_ioend_bioset, 4 * (PAGE_SIZE / SECTOR_SIZE),
> +	return bioset_init(&iomap_ioend_bioset, 4 * PAGE_SECTORS,

Yes, please.

> The shift just seemed optimal if we're just going to change it.

Nope, it's just premature optimisation at the expense of
maintainability. The compiler will optimise the multiplication into
shifts if that is the fastest way to do it for the given
architecture the code is being compiled to.

-Dave.
-- 
Dave Chinner
david@fromorbit.com


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

* Re: [PATCH 3/5] iomap: simplify iomap_init() with PAGE_SECTORS
  2023-04-21 22:30         ` Luis Chamberlain
@ 2023-04-21 22:36           ` Jens Axboe
  0 siblings, 0 replies; 16+ messages in thread
From: Jens Axboe @ 2023-04-21 22:36 UTC (permalink / raw)
  To: Luis Chamberlain
  Cc: Matthew Wilcox, agk, snitzer, philipp.reisner, lars.ellenberg,
	christoph.boehmwalder, hch, djwong, minchan, senozhatsky,
	patches, linux-block, linux-mm, linux-xfs, linux-fsdevel,
	dm-devel, drbd-dev, linux-kernel, hare, p.raghav, da.gomez,
	kbusch

On 4/21/23 4:30?PM, Luis Chamberlain wrote:
> On Fri, Apr 21, 2023 at 04:24:57PM -0600, Jens Axboe wrote:
>> On 4/21/23 4:02?PM, Luis Chamberlain wrote:
>>> On Fri, Apr 21, 2023 at 09:14:00PM +0100, Matthew Wilcox wrote:
>>>> On Fri, Apr 21, 2023 at 12:58:05PM -0700, Luis Chamberlain wrote:
>>>>> Just use the PAGE_SECTORS generic define. This produces no functional
>>>>> changes. While at it use left shift to simplify this even further.
>>>>
>>>> How is FOO << 2 simpler than FOO * 4?
>>>>
>>>>> -	return bioset_init(&iomap_ioend_bioset, 4 * (PAGE_SIZE / SECTOR_SIZE),
>>>>> +	return bioset_init(&iomap_ioend_bioset, PAGE_SECTORS << 2,
>>>
>>> We could just do:
>>>
>>>
>>> -	return bioset_init(&iomap_ioend_bioset, 4 * (PAGE_SIZE / SECTOR_SIZE),
>>> +	return bioset_init(&iomap_ioend_bioset, 4 * PAGE_SECTORS,
>>>
>>> The shift just seemed optimal if we're just going to change it.
>>
>> It's going to generate the same code, but the multiplication is arguably
>> easier to read (or harder to misread).
> 
> Then let's stick with the 4 * PAGE_SECTORS. Let me know if you need another
> patch.

Just send out a v2 at some point, you've also got a number of cases
where there are superfluous parenthesis, at least in patch 4, and Willy
pointed one out in an earlier patch too. Didn't check the last one.

This will be 6.5 anyway I think, I already sent out the changes for the
6.4 merge window.

-- 
Jens Axboe



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

* Re: [PATCH 5/5] zram: use generic PAGE_SECTORS and PAGE_SECTORS_SHIFT
  2023-04-21 19:58 ` [PATCH 5/5] zram: use generic PAGE_SECTORS and PAGE_SECTORS_SHIFT Luis Chamberlain
@ 2023-04-24  2:36   ` Sergey Senozhatsky
  0 siblings, 0 replies; 16+ messages in thread
From: Sergey Senozhatsky @ 2023-04-24  2:36 UTC (permalink / raw)
  To: Luis Chamberlain
  Cc: axboe, agk, snitzer, philipp.reisner, lars.ellenberg,
	christoph.boehmwalder, hch, djwong, minchan, senozhatsky,
	patches, linux-block, linux-mm, linux-xfs, linux-fsdevel,
	dm-devel, drbd-dev, linux-kernel, willy, hare, p.raghav,
	da.gomez, kbusch

On (23/04/21 12:58), Luis Chamberlain wrote:
> 
> Instead of re-defining the already existing constants use the provided ones:
> 
> So replace:
> 
>  o SECTORS_PER_PAGE_SHIFT with PAGE_SECTORS_SHIFT
>  o SECTORS_PER_PAGE       with PAGE_SECTORS
> 
> This produces no functional changes.
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>


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

* Re: [PATCH 3/5] iomap: simplify iomap_init() with PAGE_SECTORS
  2023-04-21 22:34       ` Dave Chinner
@ 2023-04-24  5:55         ` Christoph Hellwig
  0 siblings, 0 replies; 16+ messages in thread
From: Christoph Hellwig @ 2023-04-24  5:55 UTC (permalink / raw)
  To: Dave Chinner
  Cc: Luis Chamberlain, Matthew Wilcox, axboe, agk, snitzer,
	philipp.reisner, lars.ellenberg, christoph.boehmwalder, hch,
	djwong, minchan, senozhatsky, patches, linux-block, linux-mm,
	linux-xfs, linux-fsdevel, dm-devel, drbd-dev, linux-kernel, hare,
	p.raghav, da.gomez, kbusch

On Sat, Apr 22, 2023 at 08:34:20AM +1000, Dave Chinner wrote:
> > 
> > -	return bioset_init(&iomap_ioend_bioset, 4 * (PAGE_SIZE / SECTOR_SIZE),
> > +	return bioset_init(&iomap_ioend_bioset, 4 * PAGE_SECTORS,
> 
> Yes, please.
> 
> > The shift just seemed optimal if we're just going to change it.
> 
> Nope, it's just premature optimisation at the expense of
> maintainability. The compiler will optimise the multiplication into
> shifts if that is the fastest way to do it for the given
> architecture the code is being compiled to.

We still had cases of the compiler not doing obvious
multiplication/division to shift conversion lately.  That being said:

 1) this is an initialization path, no one actually cares
 2) we're dealing with constants here, and compilers are really good
    at constant folding

so yes, this should be using the much more readable version.



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

* Re: [PATCH 2/5] drbd: use PAGE_SECTORS_SHIFT and PAGE_SECTORS
  2023-04-21 19:58 ` [PATCH 2/5] drbd: use PAGE_SECTORS_SHIFT and PAGE_SECTORS Luis Chamberlain
@ 2023-04-24  8:26   ` Christoph Böhmwalder
  0 siblings, 0 replies; 16+ messages in thread
From: Christoph Böhmwalder @ 2023-04-24  8:26 UTC (permalink / raw)
  To: Luis Chamberlain, axboe, agk, snitzer, philipp.reisner,
	lars.ellenberg, hch, djwong, minchan, senozhatsky
  Cc: patches, linux-block, linux-mm, linux-xfs, linux-fsdevel,
	dm-devel, drbd-dev, linux-kernel, willy, hare, p.raghav,
	da.gomez, kbusch

Am 21.04.23 um 21:58 schrieb Luis Chamberlain:
> Replace common constants with generic versions.
> This produces no functional changes.
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  drivers/block/drbd/drbd_bitmap.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/block/drbd/drbd_bitmap.c b/drivers/block/drbd/drbd_bitmap.c
> index 6ac8c54b44c7..b556e6634f13 100644
> --- a/drivers/block/drbd/drbd_bitmap.c
> +++ b/drivers/block/drbd/drbd_bitmap.c
> @@ -1000,7 +1000,7 @@ static void bm_page_io_async(struct drbd_bm_aio_ctx *ctx, int page_nr) __must_ho
>  	unsigned int len;
>  
>  	first_bm_sect = device->ldev->md.md_offset + device->ldev->md.bm_offset;
> -	on_disk_sector = first_bm_sect + (((sector_t)page_nr) << (PAGE_SHIFT-SECTOR_SHIFT));
> +	on_disk_sector = first_bm_sect + (((sector_t)page_nr) << PAGE_SECTORS_SHIFT);
>  
>  	/* this might happen with very small
>  	 * flexible external meta data device,
> @@ -1008,7 +1008,7 @@ static void bm_page_io_async(struct drbd_bm_aio_ctx *ctx, int page_nr) __must_ho
>  	last_bm_sect = drbd_md_last_bitmap_sector(device->ldev);
>  	if (first_bm_sect <= on_disk_sector && last_bm_sect >= on_disk_sector) {
>  		sector_t len_sect = last_bm_sect - on_disk_sector + 1;
> -		if (len_sect < PAGE_SIZE/SECTOR_SIZE)
> +		if (len_sect < PAGE_SECTORS)
>  			len = (unsigned int)len_sect*SECTOR_SIZE;
>  		else
>  			len = PAGE_SIZE;

Acked-by: Christoph Böhmwalder <christoph.boehmwalder@linbit.com>

-- 
Christoph Böhmwalder
LINBIT | Keeping the Digital World Running
DRBD HA —  Disaster Recovery — Software defined Storage


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

end of thread, other threads:[~2023-04-24  8:26 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-21 19:58 [PATCH 0/5] block: simplify with PAGE_SECTORS_SHIFT Luis Chamberlain
2023-04-21 19:58 ` [PATCH 1/5] dm integrity: simplify by using PAGE_SECTORS_SHIFT Luis Chamberlain
2023-04-21 20:15   ` Matthew Wilcox
2023-04-21 19:58 ` [PATCH 2/5] drbd: use PAGE_SECTORS_SHIFT and PAGE_SECTORS Luis Chamberlain
2023-04-24  8:26   ` Christoph Böhmwalder
2023-04-21 19:58 ` [PATCH 3/5] iomap: simplify iomap_init() with PAGE_SECTORS Luis Chamberlain
2023-04-21 20:14   ` Matthew Wilcox
2023-04-21 22:02     ` Luis Chamberlain
2023-04-21 22:24       ` Jens Axboe
2023-04-21 22:30         ` Luis Chamberlain
2023-04-21 22:36           ` Jens Axboe
2023-04-21 22:34       ` Dave Chinner
2023-04-24  5:55         ` Christoph Hellwig
2023-04-21 19:58 ` [PATCH 4/5] dm bufio: simplify by using PAGE_SECTORS_SHIFT Luis Chamberlain
2023-04-21 19:58 ` [PATCH 5/5] zram: use generic PAGE_SECTORS and PAGE_SECTORS_SHIFT Luis Chamberlain
2023-04-24  2:36   ` Sergey Senozhatsky

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).