diff mbox series

[RFC] block: add meaningful macro for flush op flags

Message ID 20230512080757.387523-1-kch@nvidia.com (mailing list archive)
State Rejected, archived
Headers show
Series [RFC] block: add meaningful macro for flush op flags | expand

Commit Message

Chaitanya Kulkarni May 12, 2023, 8:07 a.m. UTC
Flush requests are implemented as REQ_OP_WRITE + REQ_OP_PREFLUSH
combination and not REQ_OP_FLUSH + REQ_PREFLUSH combination.

This unclear nature has lead to the confusion and bugs in the code for
block drivers causing more work for testing, reviews and fixes :-

1. https://lore.kernel.org/all/ZFHgefWofVt24tRl@infradead.org/
2. https://marc.info/?l=linux-block&m=168386364026498&w=2

Add a macro (name can me more meaningful) with a meaningful comment
clearing the confusion and replace the REQ_OP_WRITE | REQ_PREFLUSH with
the new macro name that also saves code repetation.

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 block/blk-flush.c                   | 2 +-
 drivers/md/bcache/request.c         | 3 +--
 drivers/md/dm-bufio.c               | 2 +-
 drivers/md/dm-integrity.c           | 2 +-
 drivers/md/dm-log.c                 | 2 +-
 drivers/md/dm-raid1.c               | 2 +-
 drivers/md/dm-snap-persistent.c     | 5 ++---
 drivers/md/dm-writecache.c          | 2 +-
 drivers/md/dm.c                     | 2 +-
 drivers/md/md.c                     | 3 +--
 drivers/md/raid5-cache.c            | 3 +--
 drivers/md/raid5-ppl.c              | 3 +--
 drivers/nvme/target/io-cmd-bdev.c   | 2 +-
 drivers/target/target_core_iblock.c | 3 +--
 include/linux/blk_types.h           | 7 +++++++
 15 files changed, 22 insertions(+), 21 deletions(-)

Comments

Christoph Hellwig May 12, 2023, 1 p.m. UTC | #1
Hell no.  This is just obsfucation.  We can look into actually exposing
REQ_OP_FLUSH at the bio level, but not something like this.
Chaitanya Kulkarni May 13, 2023, 1:09 a.m. UTC | #2
On 5/12/23 06:00, Christoph Hellwig wrote:
> Hell no.  This is just obsfucation.  We can look into actually exposing
> REQ_OP_FLUSH at the bio level, but not something like this.
>

and that's why I made it RFC, thanks for the can you please elaborate
on "exposing REQ_OP_FLUSH at the bio level" ?

I'd really like work that ...

-ck
Coly Li May 14, 2023, 11:11 a.m. UTC | #3
> 2023年5月12日 10:07,Chaitanya Kulkarni <kch@nvidia.com> 写道:
> 
> Flush requests are implemented as REQ_OP_WRITE + REQ_OP_PREFLUSH
> combination and not REQ_OP_FLUSH + REQ_PREFLUSH combination.
> 
> This unclear nature has lead to the confusion and bugs in the code for
> block drivers causing more work for testing, reviews and fixes :-
> 
> 1. https://lore.kernel.org/all/ZFHgefWofVt24tRl@infradead.org/
> 2. https://marc.info/?l=linux-block&m=168386364026498&w=2
> 
> Add a macro (name can me more meaningful) with a meaningful comment
> clearing the confusion and replace the REQ_OP_WRITE | REQ_PREFLUSH with
> the new macro name that also saves code repetation.
> 
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>

> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -455,6 +455,13 @@ enum req_flag_bits {
> #define REQ_NOMERGE_FLAGS \
> (REQ_NOMERGE | REQ_PREFLUSH | REQ_FUA)
> 
> +/*
> + * Flush requests are implemented as REQ_OP_WRITE + REQ_OP_PREFLUSH combination
> + * and not REQ_OP_FLUSH + REQ_PREFLUSH combination.
> + */
> +
> +#define REQ_FLUSH_OPF (REQ_OP_WRITE | REQ_PREFLUSH)
> +
> enum stat_group {
> STAT_READ,
> STAT_WRITE,
> -- 

Personally I like current explicit way, it is simpler than an extra macro. This is just my own points, FYI.

Thanks.

Coly Li
Yu Kuai May 15, 2023, 1:43 a.m. UTC | #4
Hi,

在 2023/05/13 9:09, Chaitanya Kulkarni 写道:
> On 5/12/23 06:00, Christoph Hellwig wrote:
>> Hell no.  This is just obsfucation.  We can look into actually exposing
>> REQ_OP_FLUSH at the bio level, but not something like this.
>>
> 
> and that's why I made it RFC, thanks for the can you please elaborate
> on "exposing REQ_OP_FLUSH at the bio level" ?

I think Christoph means that use this flag directly for bio, it's only
used for reqeust for now.

Thanks,
Kuai
> 
> I'd really like work that ...
> 
> -ck
> 
>
diff mbox series

Patch

diff --git a/block/blk-flush.c b/block/blk-flush.c
index 04698ed9bcd4..376f00257100 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -460,7 +460,7 @@  int blkdev_issue_flush(struct block_device *bdev)
 {
 	struct bio bio;
 
-	bio_init(&bio, bdev, NULL, 0, REQ_OP_WRITE | REQ_PREFLUSH);
+	bio_init(&bio, bdev, NULL, 0, REQ_FLUSH_OPF);
 	return submit_bio_wait(&bio);
 }
 EXPORT_SYMBOL(blkdev_issue_flush);
diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
index 67a2e29e0b40..ab89897a36a2 100644
--- a/drivers/md/bcache/request.c
+++ b/drivers/md/bcache/request.c
@@ -1023,8 +1023,7 @@  static void cached_dev_write(struct cached_dev *dc, struct search *s)
 			 */
 			struct bio *flush;
 
-			flush = bio_alloc_bioset(bio->bi_bdev, 0,
-						 REQ_OP_WRITE | REQ_PREFLUSH,
+			flush = bio_alloc_bioset(bio->bi_bdev, 0, REQ_FLUSH_OPF,
 						 GFP_NOIO, &dc->disk.bio_split);
 			if (!flush) {
 				s->iop.status = BLK_STS_RESOURCE;
diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
index eea977662e81..da815325842b 100644
--- a/drivers/md/dm-bufio.c
+++ b/drivers/md/dm-bufio.c
@@ -2133,7 +2133,7 @@  EXPORT_SYMBOL_GPL(dm_bufio_write_dirty_buffers);
 int dm_bufio_issue_flush(struct dm_bufio_client *c)
 {
 	struct dm_io_request io_req = {
-		.bi_opf = REQ_OP_WRITE | REQ_PREFLUSH | REQ_SYNC,
+		.bi_opf = REQ_FLUSH_OPF | REQ_SYNC,
 		.mem.type = DM_IO_KMEM,
 		.mem.ptr.addr = NULL,
 		.client = c->dm_io,
diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
index 31838b13ea54..2d90f8ad1ae5 100644
--- a/drivers/md/dm-integrity.c
+++ b/drivers/md/dm-integrity.c
@@ -1533,7 +1533,7 @@  static void dm_integrity_flush_buffers(struct dm_integrity_c *ic, bool flush_dat
 	if (!ic->meta_dev)
 		flush_data = false;
 	if (flush_data) {
-		fr.io_req.bi_opf = REQ_OP_WRITE | REQ_PREFLUSH | REQ_SYNC,
+		fr.io_req.bi_opf = REQ_FLUSH_OPF | REQ_SYNC,
 		fr.io_req.mem.type = DM_IO_KMEM,
 		fr.io_req.mem.ptr.addr = NULL,
 		fr.io_req.notify.fn = flush_notify,
diff --git a/drivers/md/dm-log.c b/drivers/md/dm-log.c
index f9f84236dfcd..2c40f865ef16 100644
--- a/drivers/md/dm-log.c
+++ b/drivers/md/dm-log.c
@@ -311,7 +311,7 @@  static int flush_header(struct log_c *lc)
 		.count = 0,
 	};
 
-	lc->io_req.bi_opf = REQ_OP_WRITE | REQ_PREFLUSH;
+	lc->io_req.bi_opf = REQ_FLUSH_OPF;
 
 	return dm_io(&lc->io_req, 1, &null_location, NULL);
 }
diff --git a/drivers/md/dm-raid1.c b/drivers/md/dm-raid1.c
index ddcb2bc4a617..7acb9a390b38 100644
--- a/drivers/md/dm-raid1.c
+++ b/drivers/md/dm-raid1.c
@@ -265,7 +265,7 @@  static int mirror_flush(struct dm_target *ti)
 	struct dm_io_region io[MAX_NR_MIRRORS];
 	struct mirror *m;
 	struct dm_io_request io_req = {
-		.bi_opf = REQ_OP_WRITE | REQ_PREFLUSH | REQ_SYNC,
+		.bi_opf = REQ_FLUSH_OPF | REQ_SYNC,
 		.mem.type = DM_IO_KMEM,
 		.mem.ptr.addr = NULL,
 		.client = ms->io_client,
diff --git a/drivers/md/dm-snap-persistent.c b/drivers/md/dm-snap-persistent.c
index 15649921f2a9..cfb7f1b92c5e 100644
--- a/drivers/md/dm-snap-persistent.c
+++ b/drivers/md/dm-snap-persistent.c
@@ -739,8 +739,7 @@  static void persistent_commit_exception(struct dm_exception_store *store,
 	/*
 	 * Commit exceptions to disk.
 	 */
-	if (ps->valid && area_io(ps, REQ_OP_WRITE | REQ_PREFLUSH | REQ_FUA |
-				 REQ_SYNC))
+	if (ps->valid && area_io(ps, REQ_FLUSH_OPF | REQ_FUA | REQ_SYNC))
 		ps->valid = 0;
 
 	/*
@@ -817,7 +816,7 @@  static int persistent_commit_merge(struct dm_exception_store *store,
 	for (i = 0; i < nr_merged; i++)
 		clear_exception(ps, ps->current_committed - 1 - i);
 
-	r = area_io(ps, REQ_OP_WRITE | REQ_PREFLUSH | REQ_FUA);
+	r = area_io(ps, REQ_FLUSH_OPF | REQ_FUA);
 	if (r < 0)
 		return r;
 
diff --git a/drivers/md/dm-writecache.c b/drivers/md/dm-writecache.c
index 074cb785eafc..538f74114d13 100644
--- a/drivers/md/dm-writecache.c
+++ b/drivers/md/dm-writecache.c
@@ -590,7 +590,7 @@  static void writecache_disk_flush(struct dm_writecache *wc, struct dm_dev *dev)
 	region.bdev = dev->bdev;
 	region.sector = 0;
 	region.count = 0;
-	req.bi_opf = REQ_OP_WRITE | REQ_PREFLUSH;
+	req.bi_opf = REQ_FLUSH_OPF;
 	req.mem.type = DM_IO_KMEM;
 	req.mem.ptr.addr = NULL;
 	req.client = wc->dm_io;
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 3b694ba3a106..a570024a747d 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1555,7 +1555,7 @@  static void __send_empty_flush(struct clone_info *ci)
 	 * the basis for the clone(s).
 	 */
 	bio_init(&flush_bio, ci->io->md->disk->part0, NULL, 0,
-		 REQ_OP_WRITE | REQ_PREFLUSH | REQ_SYNC);
+		 REQ_FLUSH_OPF | REQ_SYNC);
 
 	ci->bio = &flush_bio;
 	ci->sector_count = 0;
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 8e344b4b3444..5f72a693dc1c 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -533,8 +533,7 @@  static void submit_flushes(struct work_struct *ws)
 			atomic_inc(&rdev->nr_pending);
 			atomic_inc(&rdev->nr_pending);
 			rcu_read_unlock();
-			bi = bio_alloc_bioset(rdev->bdev, 0,
-					      REQ_OP_WRITE | REQ_PREFLUSH,
+			bi = bio_alloc_bioset(rdev->bdev, 0, REQ_FLUSH_OPF,
 					      GFP_NOIO, &mddev->bio_set);
 			bi->bi_end_io = md_end_flush;
 			bi->bi_private = rdev;
diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index 46182b955aef..692c3ed33b1f 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -1303,8 +1303,7 @@  void r5l_flush_stripe_to_raid(struct r5l_log *log)
 
 	if (!do_flush)
 		return;
-	bio_init(&log->flush_bio, log->rdev->bdev, NULL, 0,
-		  REQ_OP_WRITE | REQ_PREFLUSH);
+	bio_init(&log->flush_bio, log->rdev->bdev, NULL, 0, REQ_FLUSH_OPF);
 	log->flush_bio.bi_end_io = r5l_log_flush_endio;
 	submit_bio(&log->flush_bio);
 }
diff --git a/drivers/md/raid5-ppl.c b/drivers/md/raid5-ppl.c
index e495939bb3e0..da2012744b0d 100644
--- a/drivers/md/raid5-ppl.c
+++ b/drivers/md/raid5-ppl.c
@@ -629,8 +629,7 @@  static void ppl_do_flush(struct ppl_io_unit *io)
 		if (bdev) {
 			struct bio *bio;
 
-			bio = bio_alloc_bioset(bdev, 0,
-					       REQ_OP_WRITE | REQ_PREFLUSH,
+			bio = bio_alloc_bioset(bdev, 0, REQ_FLUSH_OPF,
 					       GFP_NOIO, &ppl_conf->flush_bs);
 			bio->bi_private = io;
 			bio->bi_end_io = ppl_flush_endio;
diff --git a/drivers/nvme/target/io-cmd-bdev.c b/drivers/nvme/target/io-cmd-bdev.c
index c2d6cea0236b..2717b64cb02f 100644
--- a/drivers/nvme/target/io-cmd-bdev.c
+++ b/drivers/nvme/target/io-cmd-bdev.c
@@ -342,7 +342,7 @@  static void nvmet_bdev_execute_flush(struct nvmet_req *req)
 		return;
 
 	bio_init(bio, req->ns->bdev, req->inline_bvec,
-		 ARRAY_SIZE(req->inline_bvec), REQ_OP_WRITE | REQ_PREFLUSH);
+		 ARRAY_SIZE(req->inline_bvec), REQ_FLUSH_OPF);
 	bio->bi_private = req;
 	bio->bi_end_io = nvmet_bio_done;
 
diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index cc838ffd1294..01984d07ff9c 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -419,8 +419,7 @@  iblock_execute_sync_cache(struct se_cmd *cmd)
 	if (immed)
 		target_complete_cmd(cmd, SAM_STAT_GOOD);
 
-	bio = bio_alloc(ib_dev->ibd_bd, 0, REQ_OP_WRITE | REQ_PREFLUSH,
-			GFP_KERNEL);
+	bio = bio_alloc(ib_dev->ibd_bd, 0, REQ_FLUSH_OPF, GFP_KERNEL);
 	bio->bi_end_io = iblock_end_io_flush;
 	if (!immed)
 		bio->bi_private = cmd;
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 740afe80f297..f3afdbb3f239 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -455,6 +455,13 @@  enum req_flag_bits {
 #define REQ_NOMERGE_FLAGS \
 	(REQ_NOMERGE | REQ_PREFLUSH | REQ_FUA)
 
+/*
+ * Flush requests are implemented as REQ_OP_WRITE + REQ_OP_PREFLUSH combination
+ * and not REQ_OP_FLUSH + REQ_PREFLUSH combination.
+ */
+
+#define REQ_FLUSH_OPF (REQ_OP_WRITE | REQ_PREFLUSH)
+
 enum stat_group {
 	STAT_READ,
 	STAT_WRITE,