[PATCH v2 00/12] block: Catch missing debugfs flag array members

[PATCH v2 00/12] block: Catch missing debugfs flag array members

[John Garry]

Currently we rely on the developer to add the appropriate entry to the
debugfs flag array when we add a new member.

This has shown to be error prone.

Add compile-time assertions that we are not missing flag array entries.

A limitation of this approach is that if a non-end-of-array entry was now
later removed from a flag name array, we could not detect that at build
time. But this is unlikely to occur. To actually detect that, we could
make the flag name array entries a flag and name tuple. That would just
add extra complexity and slow the code, which I am not sure if is really
required.

Christoph Hellwig (1):
  block: remove QUEUE_FLAG_STOPPED

John Garry (11):
  block: Make QUEUE_FLAG_x as an enum
  block: Add build-time assert for size of blk_queue_flag_name[]
  block: Catch possible entries missing from hctx_state_name[]
  block: Catch possible entries missing from hctx_flag_name[]
  block: Catch possible entries missing from alloc_policy_name[]
  block: Add missing entries from cmd_flag_name[]
  block: Catch possible entries missing from cmd_flag_name[]
  block: Use enum to define RQF_x bit indexes
  block: Simplify definition of RQF_NAME()
  block: Add zone write plugging entry to rqf_name[]
  block: Catch possible entries missing from rqf_name[]

 block/blk-mq-debugfs.c    |  26 +++++++--
 include/linux/blk-mq.h    | 109 +++++++++++++++++++++++++-------------
 include/linux/blk_types.h |   1 +
 include/linux/blkdev.h    |  31 +++++------
 4 files changed, 109 insertions(+), 58 deletions(-)

-- 
2.31.1

[PATCH v2 01/12] block: remove QUEUE_FLAG_STOPPED

[John Garry]

From: Christoph Hellwig <hch@lst.de>

QUEUE_FLAG_STOPPED is entirely unused.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 block/blk-mq-debugfs.c | 1 -
 include/linux/blkdev.h | 2 --
 2 files changed, 3 deletions(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 344f9e503bdb..03d0409e5018 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -79,7 +79,6 @@ static int queue_pm_only_show(void *data, struct seq_file *m)
 
 #define QUEUE_FLAG_NAME(name) [QUEUE_FLAG_##name] = #name
 static const char *const blk_queue_flag_name[] = {
-	QUEUE_FLAG_NAME(STOPPED),
 	QUEUE_FLAG_NAME(DYING),
 	QUEUE_FLAG_NAME(NOMERGES),
 	QUEUE_FLAG_NAME(SAME_COMP),
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index dce4a6bf7307..942ad4e0f231 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -588,7 +588,6 @@ struct request_queue {
 };
 
 /* Keep blk_queue_flag_name[] in sync with the definitions below */
-#define QUEUE_FLAG_STOPPED	0	/* queue is stopped */
 #define QUEUE_FLAG_DYING	1	/* queue being torn down */
 #define QUEUE_FLAG_NOMERGES     3	/* disable merge attempts */
 #define QUEUE_FLAG_SAME_COMP	4	/* complete on same CPU-group */
@@ -608,7 +607,6 @@ struct request_queue {
 void blk_queue_flag_set(unsigned int flag, struct request_queue *q);
 void blk_queue_flag_clear(unsigned int flag, struct request_queue *q);
 
-#define blk_queue_stopped(q)	test_bit(QUEUE_FLAG_STOPPED, &(q)->queue_flags)
 #define blk_queue_dying(q)	test_bit(QUEUE_FLAG_DYING, &(q)->queue_flags)
 #define blk_queue_init_done(q)	test_bit(QUEUE_FLAG_INIT_DONE, &(q)->queue_flags)
 #define blk_queue_nomerges(q)	test_bit(QUEUE_FLAG_NOMERGES, &(q)->queue_flags)
-- 
2.31.1

[PATCH v2 02/12] block: Make QUEUE_FLAG_x as an enum

[John Garry]

This will allow us better keep in sync with blk_queue_flag_name[].

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 include/linux/blkdev.h | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 942ad4e0f231..bb521745c702 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -588,19 +588,22 @@ struct request_queue {
 };
 
 /* Keep blk_queue_flag_name[] in sync with the definitions below */
-#define QUEUE_FLAG_DYING	1	/* queue being torn down */
-#define QUEUE_FLAG_NOMERGES     3	/* disable merge attempts */
-#define QUEUE_FLAG_SAME_COMP	4	/* complete on same CPU-group */
-#define QUEUE_FLAG_FAIL_IO	5	/* fake timeout */
-#define QUEUE_FLAG_NOXMERGES	9	/* No extended merges */
-#define QUEUE_FLAG_SAME_FORCE	12	/* force complete on same CPU */
-#define QUEUE_FLAG_INIT_DONE	14	/* queue is initialized */
-#define QUEUE_FLAG_STATS	20	/* track IO start and completion times */
-#define QUEUE_FLAG_REGISTERED	22	/* queue has been registered to a disk */
-#define QUEUE_FLAG_QUIESCED	24	/* queue has been quiesced */
-#define QUEUE_FLAG_RQ_ALLOC_TIME 27	/* record rq->alloc_time_ns */
-#define QUEUE_FLAG_HCTX_ACTIVE	28	/* at least one blk-mq hctx is active */
-#define QUEUE_FLAG_SQ_SCHED     30	/* single queue style io dispatch */
+enum {
+	QUEUE_FLAG_DYING		= 0,	/* queue being torn down */
+	QUEUE_FLAG_NOMERGES,			/* disable merge attempts */
+	QUEUE_FLAG_SAME_COMP,			/* complete on same CPU-group */
+	QUEUE_FLAG_FAIL_IO,			/* fake timeout */
+	QUEUE_FLAG_NOXMERGES,			/* No extended merges */
+	QUEUE_FLAG_SAME_FORCE,			/* force complete on same CPU */
+	QUEUE_FLAG_INIT_DONE,			/* queue is initialized */
+	QUEUE_FLAG_STATS,			/* track IO start and completion times */
+	QUEUE_FLAG_REGISTERED,			/* queue has been registered to a disk */
+	QUEUE_FLAG_QUIESCED,			/* queue has been quiesced */
+	QUEUE_FLAG_RQ_ALLOC_TIME,		/* record rq->alloc_time_ns */
+	QUEUE_FLAG_HCTX_ACTIVE,			/* at least one blk-mq hctx is active */
+	QUEUE_FLAG_SQ_SCHED,			/* single queue style io dispatch */
+	QUEUE_FLAG_MAX
+};
 
 #define QUEUE_FLAG_MQ_DEFAULT	(1UL << QUEUE_FLAG_SAME_COMP)
 
-- 
2.31.1

Re: [PATCH v2 02/12] block: Make QUEUE_FLAG_x as an enum

[Bart Van Assche]

On 7/11/24 1:23 AM, John Garry wrote:
> +enum {
> +	QUEUE_FLAG_DYING		= 0,	/* queue being torn down */

Please leave out " = 0" since it is redundant.

Thanks,

Bart.

Re: [PATCH v2 02/12] block: Make QUEUE_FLAG_x as an enum

[John Garry]

On 11/07/2024 19:05, Bart Van Assche wrote:
> On 7/11/24 1:23 AM, John Garry wrote:
>> +enum {
>> +    QUEUE_FLAG_DYING        = 0,    /* queue being torn down */
> 
> Please leave out " = 0" since it is redundant.

ok. I think that assigning the base member to zero is 50/50 habit and 
paranoia for me.

I'll look at the rest of your comments next week.

Thanks,
John

[PATCH v2 03/12] block: Add build-time assert for size of blk_queue_flag_name[]

[John Garry]

Assert that we are not missing flag entries in blk_queue_flag_name[].

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 block/blk-mq-debugfs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 03d0409e5018..9e18ba6b1c4d 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -5,6 +5,7 @@
 
 #include <linux/kernel.h>
 #include <linux/blkdev.h>
+#include <linux/build_bug.h>
 #include <linux/debugfs.h>
 
 #include "blk.h"
@@ -99,6 +100,7 @@ static int queue_state_show(void *data, struct seq_file *m)
 {
 	struct request_queue *q = data;
 
+	BUILD_BUG_ON(ARRAY_SIZE(blk_queue_flag_name) != QUEUE_FLAG_MAX);
 	blk_flags_show(m, q->queue_flags, blk_queue_flag_name,
 		       ARRAY_SIZE(blk_queue_flag_name));
 	seq_puts(m, "\n");
-- 
2.31.1

[PATCH v2 04/12] block: Catch possible entries missing from hctx_state_name[]

[John Garry]

Add a build-time assert that we are not missing entries from
hctx_state_name[]. For this, add a "max" entry for BLK_MQ_S_x flags.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 block/blk-mq-debugfs.c | 1 +
 include/linux/blk-mq.h | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 9e18ba6b1c4d..fca8b82464b4 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -165,6 +165,7 @@ static int hctx_state_show(void *data, struct seq_file *m)
 {
 	struct blk_mq_hw_ctx *hctx = data;
 
+	BUILD_BUG_ON(ARRAY_SIZE(hctx_state_name) != BLK_MQ_S_MAX);
 	blk_flags_show(m, hctx->state, hctx_state_name,
 		       ARRAY_SIZE(hctx_state_name));
 	seq_puts(m, "\n");
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 89ba6b16fe8b..225e51698470 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -664,12 +664,14 @@ enum {
 	BLK_MQ_F_ALLOC_POLICY_START_BIT = 8,
 	BLK_MQ_F_ALLOC_POLICY_BITS = 1,
 
+	/* Keep hctx_state_name[] in sync with the definitions below */
 	BLK_MQ_S_STOPPED	= 0,
 	BLK_MQ_S_TAG_ACTIVE	= 1,
 	BLK_MQ_S_SCHED_RESTART	= 2,
 
 	/* hw queue is inactive after all its CPUs become offline */
 	BLK_MQ_S_INACTIVE	= 3,
+	BLK_MQ_S_MAX,
 
 	BLK_MQ_MAX_DEPTH	= 10240,
 
-- 
2.31.1

Re: [PATCH v2 04/12] block: Catch possible entries missing from hctx_state_name[]

[Bart Van Assche]

On 7/11/24 1:23 AM, John Garry wrote:
> diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
> index 89ba6b16fe8b..225e51698470 100644
> --- a/include/linux/blk-mq.h
> +++ b/include/linux/blk-mq.h
> @@ -664,12 +664,14 @@ enum {
>   	BLK_MQ_F_ALLOC_POLICY_START_BIT = 8,
>   	BLK_MQ_F_ALLOC_POLICY_BITS = 1,
>   
> +	/* Keep hctx_state_name[] in sync with the definitions below */
>   	BLK_MQ_S_STOPPED	= 0,
>   	BLK_MQ_S_TAG_ACTIVE	= 1,
>   	BLK_MQ_S_SCHED_RESTART	= 2,
>   
>   	/* hw queue is inactive after all its CPUs become offline */
>   	BLK_MQ_S_INACTIVE	= 3,
> +	BLK_MQ_S_MAX,

Please create a new "enum {" section for the BLK_MQ_S_ constants.
That will make it more clear that these are unrelated to the other
constants defined above and below the BLK_MQ_S_ constants.

Thanks,

Bart.

Re: [PATCH v2 04/12] block: Catch possible entries missing from hctx_state_name[]

[John Garry]

On 11/07/2024 19:08, Bart Van Assche wrote:
> On 7/11/24 1:23 AM, John Garry wrote:
>> diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
>> index 89ba6b16fe8b..225e51698470 100644
>> --- a/include/linux/blk-mq.h
>> +++ b/include/linux/blk-mq.h
>> @@ -664,12 +664,14 @@ enum {
>>       BLK_MQ_F_ALLOC_POLICY_START_BIT = 8,
>>       BLK_MQ_F_ALLOC_POLICY_BITS = 1,
>> +    /* Keep hctx_state_name[] in sync with the definitions below */
>>       BLK_MQ_S_STOPPED    = 0,
>>       BLK_MQ_S_TAG_ACTIVE    = 1,
>>       BLK_MQ_S_SCHED_RESTART    = 2,
>>       /* hw queue is inactive after all its CPUs become offline */
>>       BLK_MQ_S_INACTIVE    = 3,
>> +    BLK_MQ_S_MAX,
> 
> Please create a new "enum {" section for the BLK_MQ_S_ constants.
> That will make it more clear that these are unrelated to the other
> constants defined above and below the BLK_MQ_S_ constants.

ok, fine.

If we are going to start breaking up this misc enum, then where do 
BLK_MQ_MAX_DEPTH and BLK_MQ_CPU_WORK_BATCH really belong? I'd be more 
inclined to have a separate #define for each of them. I am not sure how 
they ever ended up in this same enum.

BTW, BLK_MQ_CPU_WORK_BATCH is only used in block/blk-mq.c, so I don't 
see why it is even in a public API.

Re: [PATCH v2 04/12] block: Catch possible entries missing from hctx_state_name[]

[Bart Van Assche]

On 7/15/24 12:54 AM, John Garry wrote:
> BTW, BLK_MQ_CPU_WORK_BATCH is only used in block/blk-mq.c, so I don't 
> see why it is even in a public API.

I'm in favor of moving this constant into block/blk-mq.c.

Thanks,

Bart.

Re: [PATCH v2 04/12] block: Catch possible entries missing from hctx_state_name[]

[John Garry]

On 15/07/2024 18:28, Bart Van Assche wrote:
> On 7/15/24 12:54 AM, John Garry wrote:
>> BTW, BLK_MQ_CPU_WORK_BATCH is only used in block/blk-mq.c, so I don't 
>> see why it is even in a public API.
> 
> I'm in favor of moving this constant into block/blk-mq.c.

I was actually thinking of moving it to block/blk-mq.h, as much blk-mq 
internal defines/prototypes/inline functions are located there.

[PATCH v2 05/12] block: Catch possible entries missing from hctx_flag_name[]

[John Garry]

Refresh values BLK_MQ_F_x enum, and then re-arrange members in
hctx_flag_name[] to match that enum.

An entry for NO_SCHED_BY_DEFAULT is missing, so add one.

Finally add a BUILD_BUG_ON() call to ensure that we are not missing entries
in hctx_flag_name[].

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 block/blk-mq-debugfs.c |  8 ++++++--
 include/linux/blk-mq.h | 10 ++++++----
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index fca8b82464b4..b9bf4b25b267 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -183,10 +183,11 @@ static const char *const alloc_policy_name[] = {
 static const char *const hctx_flag_name[] = {
 	HCTX_FLAG_NAME(SHOULD_MERGE),
 	HCTX_FLAG_NAME(TAG_QUEUE_SHARED),
-	HCTX_FLAG_NAME(BLOCKING),
-	HCTX_FLAG_NAME(NO_SCHED),
 	HCTX_FLAG_NAME(STACKING),
 	HCTX_FLAG_NAME(TAG_HCTX_SHARED),
+	HCTX_FLAG_NAME(BLOCKING),
+	HCTX_FLAG_NAME(NO_SCHED),
+	HCTX_FLAG_NAME(NO_SCHED_BY_DEFAULT),
 };
 #undef HCTX_FLAG_NAME
 
@@ -195,6 +196,9 @@ static int hctx_flags_show(void *data, struct seq_file *m)
 	struct blk_mq_hw_ctx *hctx = data;
 	const int alloc_policy = BLK_MQ_FLAG_TO_ALLOC_POLICY(hctx->flags);
 
+	BUILD_BUG_ON(ARRAY_SIZE(hctx_flag_name) !=
+			BLK_MQ_F_ALLOC_POLICY_START_BIT);
+
 	seq_puts(m, "alloc_policy=");
 	if (alloc_policy < ARRAY_SIZE(alloc_policy_name) &&
 	    alloc_policy_name[alloc_policy])
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 225e51698470..6a41d5097dd8 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -644,6 +644,7 @@ struct blk_mq_ops {
 #endif
 };
 
+/* Keep hctx_flag_name[] in sync with the definitions below */
 enum {
 	BLK_MQ_F_SHOULD_MERGE	= 1 << 0,
 	BLK_MQ_F_TAG_QUEUE_SHARED = 1 << 1,
@@ -653,15 +654,16 @@ enum {
 	 */
 	BLK_MQ_F_STACKING	= 1 << 2,
 	BLK_MQ_F_TAG_HCTX_SHARED = 1 << 3,
-	BLK_MQ_F_BLOCKING	= 1 << 5,
+	BLK_MQ_F_BLOCKING	= 1 << 4,
 	/* Do not allow an I/O scheduler to be configured. */
-	BLK_MQ_F_NO_SCHED	= 1 << 6,
+	BLK_MQ_F_NO_SCHED	= 1 << 5,
+
 	/*
 	 * Select 'none' during queue registration in case of a single hwq
 	 * or shared hwqs instead of 'mq-deadline'.
 	 */
-	BLK_MQ_F_NO_SCHED_BY_DEFAULT	= 1 << 7,
-	BLK_MQ_F_ALLOC_POLICY_START_BIT = 8,
+	BLK_MQ_F_NO_SCHED_BY_DEFAULT	= 1 << 6,
+	BLK_MQ_F_ALLOC_POLICY_START_BIT = 7,
 	BLK_MQ_F_ALLOC_POLICY_BITS = 1,
 
 	/* Keep hctx_state_name[] in sync with the definitions below */
-- 
2.31.1

Re: [PATCH v2 05/12] block: Catch possible entries missing from hctx_flag_name[]

[Bart Van Assche]

On 7/11/24 1:23 AM, John Garry wrote:
> -	BLK_MQ_F_NO_SCHED_BY_DEFAULT	= 1 << 7,
> -	BLK_MQ_F_ALLOC_POLICY_START_BIT = 8,
> +	BLK_MQ_F_NO_SCHED_BY_DEFAULT	= 1 << 6,
> +	BLK_MQ_F_ALLOC_POLICY_START_BIT = 7,
>   	BLK_MQ_F_ALLOC_POLICY_BITS = 1,
>   
>   	/* Keep hctx_state_name[] in sync with the definitions below */

The patch description does not explain why 
BLK_MQ_F_ALLOC_POLICY_START_BIT is modified. Please include
an explanation for this change in the patch description.

Thanks,

Bart.

Re: [PATCH v2 05/12] block: Catch possible entries missing from hctx_flag_name[]

[John Garry]

On 11/07/2024 19:10, Bart Van Assche wrote:
> On 7/11/24 1:23 AM, John Garry wrote:
>> -    BLK_MQ_F_NO_SCHED_BY_DEFAULT    = 1 << 7,
>> -    BLK_MQ_F_ALLOC_POLICY_START_BIT = 8,
>> +    BLK_MQ_F_NO_SCHED_BY_DEFAULT    = 1 << 6,
>> +    BLK_MQ_F_ALLOC_POLICY_START_BIT = 7,
>>       BLK_MQ_F_ALLOC_POLICY_BITS = 1,
>>       /* Keep hctx_state_name[] in sync with the definitions below */
> 
> The patch description does not explain why 
> BLK_MQ_F_ALLOC_POLICY_START_BIT is modified. Please include
> an explanation for this change in the patch description

Can do.

[PATCH v2 06/12] block: Catch possible entries missing from alloc_policy_name[]

[John Garry]

Make BLK_TAG_ALLOC_x an enum and add a "max" entry.

Also add a BUILD_BUG_ON() call to ensure that we are not missing entries
in hctx_flag_name[].

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 block/blk-mq-debugfs.c |  1 +
 include/linux/blk-mq.h | 10 ++++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index b9bf4b25b267..91396f205f0d 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -198,6 +198,7 @@ static int hctx_flags_show(void *data, struct seq_file *m)
 
 	BUILD_BUG_ON(ARRAY_SIZE(hctx_flag_name) !=
 			BLK_MQ_F_ALLOC_POLICY_START_BIT);
+	BUILD_BUG_ON(ARRAY_SIZE(alloc_policy_name) != BLK_TAG_ALLOC_MAX);
 
 	seq_puts(m, "alloc_policy=");
 	if (alloc_policy < ARRAY_SIZE(alloc_policy_name) &&
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 6a41d5097dd8..454008397e64 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -278,8 +278,14 @@ enum blk_eh_timer_return {
 	BLK_EH_RESET_TIMER,
 };
 
-#define BLK_TAG_ALLOC_FIFO 0 /* allocate starting from 0 */
-#define BLK_TAG_ALLOC_RR 1 /* allocate starting from last allocated tag */
+/* Keep alloc_policy_name[] in sync with the definitions below */
+enum {
+	/* allocate starting from 0 */
+	BLK_TAG_ALLOC_FIFO	= 0,
+	/* allocate starting from last allocated tag */
+	BLK_TAG_ALLOC_RR,
+	BLK_TAG_ALLOC_MAX
+};
 
 /**
  * struct blk_mq_hw_ctx - State for a hardware queue facing the hardware
-- 
2.31.1

Re: [PATCH v2 06/12] block: Catch possible entries missing from alloc_policy_name[]

[Bart Van Assche]

On 7/11/24 1:23 AM, John Garry wrote:
> Make BLK_TAG_ALLOC_x an enum and add a "max" entry.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

[PATCH v2 07/12] block: Add missing entries from cmd_flag_name[]

[John Garry]

Add missing entries for req_flag_bits.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 block/blk-mq-debugfs.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 91396f205f0d..cb22e78c1070 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -230,8 +230,13 @@ static const char *const cmd_flag_name[] = {
 	CMD_FLAG_NAME(RAHEAD),
 	CMD_FLAG_NAME(BACKGROUND),
 	CMD_FLAG_NAME(NOWAIT),
-	CMD_FLAG_NAME(NOUNMAP),
 	CMD_FLAG_NAME(POLLED),
+	CMD_FLAG_NAME(ALLOC_CACHE),
+	CMD_FLAG_NAME(SWAP),
+	CMD_FLAG_NAME(DRV),
+	CMD_FLAG_NAME(FS_PRIVATE),
+	CMD_FLAG_NAME(ATOMIC),
+	CMD_FLAG_NAME(NOUNMAP),
 };
 #undef CMD_FLAG_NAME
 
-- 
2.31.1

Re: [PATCH v2 07/12] block: Add missing entries from cmd_flag_name[]

[Bart Van Assche]

On 7/11/24 1:23 AM, John Garry wrote:
> Add missing entries for req_flag_bits.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

[PATCH v2 08/12] block: Catch possible entries missing from cmd_flag_name[]

[John Garry]

Also add a BUILD_BUG_ON() call to ensure that we are not missing entries
in cmd_flag_name[].

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 block/blk-mq-debugfs.c    | 2 ++
 include/linux/blk_types.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index cb22e78c1070..971457b0a441 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -280,6 +280,8 @@ int __blk_mq_debugfs_rq_show(struct seq_file *m, struct request *rq)
 	const enum req_op op = req_op(rq);
 	const char *op_str = blk_op_str(op);
 
+	BUILD_BUG_ON(ARRAY_SIZE(cmd_flag_name) != __REQ_NR_BITS);
+
 	seq_printf(m, "%p {.op=", rq);
 	if (strcmp(op_str, "UNKNOWN") == 0)
 		seq_printf(m, "%u", op);
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 632edd71f8c6..36ed96133217 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -354,6 +354,7 @@ enum req_op {
 	REQ_OP_LAST		= (__force blk_opf_t)36,
 };
 
+/* Keep cmd_flag_name[] in sync with the definitions below */
 enum req_flag_bits {
 	__REQ_FAILFAST_DEV =	/* no driver retries of device errors */
 		REQ_OP_BITS,
-- 
2.31.1

Re: [PATCH v2 08/12] block: Catch possible entries missing from cmd_flag_name[]

[Bart Van Assche]

On 7/11/24 1:23 AM, John Garry wrote:
> Also add a BUILD_BUG_ON() call to ensure that we are not missing entries
> in cmd_flag_name[].

Please leave out the word "also" from the description since it's
confusing. Anyway:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

[PATCH v2 09/12] block: Use enum to define RQF_x bit indexes

[John Garry]

Similar to what we do for enum req_flag_bits, divide the definition of
RQF_x flags into an enum to declare the bits and an actual flag.

Tweak some comments to not spill onto new lines.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 include/linux/blk-mq.h | 86 ++++++++++++++++++++++++++----------------
 1 file changed, 54 insertions(+), 32 deletions(-)

diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 454008397e64..4b300f902d8f 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -27,38 +27,60 @@ typedef enum rq_end_io_ret (rq_end_io_fn)(struct request *, blk_status_t);
  * request flags */
 typedef __u32 __bitwise req_flags_t;
 
-/* drive already may have started this one */
-#define RQF_STARTED		((__force req_flags_t)(1 << 1))
-/* request for flush sequence */
-#define RQF_FLUSH_SEQ		((__force req_flags_t)(1 << 4))
-/* merge of different types, fail separately */
-#define RQF_MIXED_MERGE		((__force req_flags_t)(1 << 5))
-/* don't call prep for this one */
-#define RQF_DONTPREP		((__force req_flags_t)(1 << 7))
-/* use hctx->sched_tags */
-#define RQF_SCHED_TAGS		((__force req_flags_t)(1 << 8))
-/* use an I/O scheduler for this request */
-#define RQF_USE_SCHED		((__force req_flags_t)(1 << 9))
-/* vaguely specified driver internal error.  Ignored by the block layer */
-#define RQF_FAILED		((__force req_flags_t)(1 << 10))
-/* don't warn about errors */
-#define RQF_QUIET		((__force req_flags_t)(1 << 11))
-/* account into disk and partition IO statistics */
-#define RQF_IO_STAT		((__force req_flags_t)(1 << 13))
-/* runtime pm request */
-#define RQF_PM			((__force req_flags_t)(1 << 15))
-/* on IO scheduler merge hash */
-#define RQF_HASHED		((__force req_flags_t)(1 << 16))
-/* track IO completion time */
-#define RQF_STATS		((__force req_flags_t)(1 << 17))
-/* Look at ->special_vec for the actual data payload instead of the
-   bio chain. */
-#define RQF_SPECIAL_PAYLOAD	((__force req_flags_t)(1 << 18))
-/* The request completion needs to be signaled to zone write pluging. */
-#define RQF_ZONE_WRITE_PLUGGING	((__force req_flags_t)(1 << 20))
-/* ->timeout has been called, don't expire again */
-#define RQF_TIMED_OUT		((__force req_flags_t)(1 << 21))
-#define RQF_RESV		((__force req_flags_t)(1 << 23))
+enum {
+	/* drive already may have started this one */
+	__RQF_STARTED		=	0,
+	/* request for flush sequence */
+	__RQF_FLUSH_SEQ,
+	/* merge of different types, fail separately */
+	__RQF_MIXED_MERGE,
+	/* don't call prep for this one */
+	__RQF_DONTPREP,
+	/* use hctx->sched_tags */
+	__RQF_SCHED_TAGS,
+	/* use an I/O scheduler for this request */
+	__RQF_USE_SCHED,
+	/* vaguely specified driver internal error.  Ignored by block layer */
+	__RQF_FAILED,
+	/* don't warn about errors */
+	__RQF_QUIET,
+	/* account into disk and partition IO statistics */
+	__RQF_IO_STAT,
+	/* runtime pm request */
+	__RQF_PM,
+	/* on IO scheduler merge hash */
+	__RQF_HASHED,
+	/* track IO completion time */
+	__RQF_STATS,
+	/* Look at ->special_vec for the actual data payload instead of the
+	   bio chain. */
+	__RQF_SPECIAL_PAYLOAD,
+	/* request completion needs to be signaled to zone write plugging. */
+	__RQF_ZONE_WRITE_PLUGGING,
+	/* ->timeout has been called, don't expire again */
+	__RQF_TIMED_OUT,
+	__RQF_RESV,
+	__RQF_BITS
+};
+
+#define RQF_STARTED		((__force req_flags_t)(1 << __RQF_STARTED))
+#define RQF_FLUSH_SEQ		((__force req_flags_t)(1 << __RQF_FLUSH_SEQ))
+#define RQF_MIXED_MERGE		((__force req_flags_t)(1 << __RQF_MIXED_MERGE))
+#define RQF_DONTPREP		((__force req_flags_t)(1 << __RQF_DONTPREP))
+#define RQF_SCHED_TAGS		((__force req_flags_t)(1 << __RQF_SCHED_TAGS))
+#define RQF_USE_SCHED		((__force req_flags_t)(1 << __RQF_USE_SCHED))
+#define RQF_FAILED		((__force req_flags_t)(1 << __RQF_FAILED))
+#define RQF_QUIET		((__force req_flags_t)(1 << __RQF_QUIET))
+#define RQF_IO_STAT		((__force req_flags_t)(1 << __RQF_IO_STAT))
+#define RQF_PM			((__force req_flags_t)(1 << __RQF_PM))
+#define RQF_HASHED		((__force req_flags_t)(1 << __RQF_HASHED))
+#define RQF_STATS		((__force req_flags_t)(1 << __RQF_STATS))
+#define RQF_SPECIAL_PAYLOAD	\
+			((__force req_flags_t)(1 << __RQF_SPECIAL_PAYLOAD))
+#define RQF_ZONE_WRITE_PLUGGING	\
+			((__force req_flags_t)(1 << __RQF_ZONE_WRITE_PLUGGING))
+#define RQF_TIMED_OUT		((__force req_flags_t)(1 << __RQF_TIMED_OUT))
+#define RQF_RESV		((__force req_flags_t)(1 << __RQF_RESV))
 
 /* flags that prevent us from merging requests: */
 #define RQF_NOMERGE_FLAGS \
-- 
2.31.1

Re: [PATCH v2 09/12] block: Use enum to define RQF_x bit indexes

[Bart Van Assche]

On 7/11/24 1:23 AM, John Garry wrote:
> +enum {
> +	/* drive already may have started this one */
> +	__RQF_STARTED		=	0,

Why " = 0"? I think this is redundant and can be left out.
Additionally, this enum definition would be easier to read if the
comments would start next to "," instead of occurring on a line of
their own.

Thanks,

Bart.

Re: [PATCH v2 09/12] block: Use enum to define RQF_x bit indexes

[John Garry]

On 11/07/2024 19:13, Bart Van Assche wrote:
> On 7/11/24 1:23 AM, John Garry wrote:
>> +enum {
>> +    /* drive already may have started this one */
>> +    __RQF_STARTED        =    0,
> 
> Why " = 0"? 

As before. I can drop it.

I think this is redundant and can be left out.
> Additionally, this enum definition would be easier to read if the
> comments would start next to "," instead of occurring on a line of
> their own.

ok, I can put the comment next to the value in cases where we will not 
spill over 80 characters.

[PATCH v2 10/12] block: Simplify definition of RQF_NAME()

[John Garry]

Now that we have a bit index for RQF_x in __RQF_x, use __RQF_x to simplify
the definition of RQF_NAME() by not using ilog2((__force u32()).

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 block/blk-mq-debugfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 971457b0a441..305c53459fb5 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -240,7 +240,7 @@ static const char *const cmd_flag_name[] = {
 };
 #undef CMD_FLAG_NAME
 
-#define RQF_NAME(name) [ilog2((__force u32)RQF_##name)] = #name
+#define RQF_NAME(name) [__RQF_##name] = #name
 static const char *const rqf_name[] = {
 	RQF_NAME(STARTED),
 	RQF_NAME(FLUSH_SEQ),
-- 
2.31.1

Re: [PATCH v2 10/12] block: Simplify definition of RQF_NAME()

[Bart Van Assche]

On 7/11/24 1:23 AM, John Garry wrote:
> Now that we have a bit index for RQF_x in __RQF_x, use __RQF_x to simplify
> the definition of RQF_NAME() by not using ilog2((__force u32()).

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

[PATCH v2 11/12] block: Add zone write plugging entry to rqf_name[]

[John Garry]

Add missing entry.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 block/blk-mq-debugfs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 305c53459fb5..34ed099c3429 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -255,6 +255,7 @@ static const char *const rqf_name[] = {
 	RQF_NAME(HASHED),
 	RQF_NAME(STATS),
 	RQF_NAME(SPECIAL_PAYLOAD),
+	RQF_NAME(ZONE_WRITE_PLUGGING),
 	RQF_NAME(TIMED_OUT),
 	RQF_NAME(RESV),
 };
-- 
2.31.1

Re: [PATCH v2 11/12] block: Add zone write plugging entry to rqf_name[]

[Bart Van Assche]

On 7/11/24 1:23 AM, John Garry wrote:
> Add missing entry.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

[PATCH v2 12/12] block: Catch possible entries missing from rqf_name[]

[John Garry]

Also add a BUILD_BUG_ON() call to ensure that we are not missing entries
in rqf_name[].

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 block/blk-mq-debugfs.c | 1 +
 include/linux/blk-mq.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 34ed099c3429..5463697a8442 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -282,6 +282,7 @@ int __blk_mq_debugfs_rq_show(struct seq_file *m, struct request *rq)
 	const char *op_str = blk_op_str(op);
 
 	BUILD_BUG_ON(ARRAY_SIZE(cmd_flag_name) != __REQ_NR_BITS);
+	BUILD_BUG_ON(ARRAY_SIZE(rqf_name) != __RQF_BITS);
 
 	seq_printf(m, "%p {.op=", rq);
 	if (strcmp(op_str, "UNKNOWN") == 0)
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 4b300f902d8f..023e5b9f6758 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -27,6 +27,7 @@ typedef enum rq_end_io_ret (rq_end_io_fn)(struct request *, blk_status_t);
  * request flags */
 typedef __u32 __bitwise req_flags_t;
 
+/* Keep rqf_name[] in sync with the definitions below */
 enum {
 	/* drive already may have started this one */
 	__RQF_STARTED		=	0,
-- 
2.31.1

Re: [PATCH v2 12/12] block: Catch possible entries missing from rqf_name[]

[Bart Van Assche]

On 7/11/24 1:23 AM, John Garry wrote:
> Also add a BUILD_BUG_ON() call to ensure that we are not missing entries
> in rqf_name[].

Please leave out the word "also" from the description since it's
confusing. Anyway:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>