[PATCH 0/3] Three small block layer patches

[PATCH 0/3] Three small block layer patches

[Bart Van Assche]

Hi Jens,

These three patches are what I came up with while reading block layer code in
preparation of the patch series for improving zoned write performance.

Please consider this patch series for kernel v5.20.

Thanks,

Bart.

Bart Van Assche (3):
  blk-iocost: Simplify ioc_rqos_done()
  block: Rename a blk_mq_map_queue() argument
  block: Specify the operation type when calling blk_mq_map_queue()

 block/blk-iocost.c |  2 +-
 block/blk-mq.c     |  2 +-
 block/blk-mq.h     | 12 ++++++------
 3 files changed, 8 insertions(+), 8 deletions(-)

[PATCH 1/3] blk-iocost: Simplify ioc_rqos_done()

[Bart Van Assche]

Leave out the superfluous "& REQ_OP_MASK" code. The definition of req_op()
shows that that code is superfluous:

 #define req_op(req) ((req)->cmd_flags & REQ_OP_MASK)

Compile-tested only.

Cc: Tejun Heo <tj@kernel.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 block/blk-iocost.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 33a11ba971ea..b7082f2aed9c 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -2769,7 +2769,7 @@ static void ioc_rqos_done(struct rq_qos *rqos, struct request *rq)
 	if (!ioc->enabled || !rq->alloc_time_ns || !rq->start_time_ns)
 		return;
 
-	switch (req_op(rq) & REQ_OP_MASK) {
+	switch (req_op(rq)) {
 	case REQ_OP_READ:
 		pidx = QOS_RLAT;
 		rw = READ;

Re: [PATCH 1/3] blk-iocost: Simplify ioc_rqos_done()

[Tejun Heo]

On Tue, Jun 14, 2022 at 10:57:23AM -0700, Bart Van Assche wrote:
> Leave out the superfluous "& REQ_OP_MASK" code. The definition of req_op()
> shows that that code is superfluous:
> 
>  #define req_op(req) ((req)->cmd_flags & REQ_OP_MASK)
> 
> Compile-tested only.
> 
> Cc: Tejun Heo <tj@kernel.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH 1/3] blk-iocost: Simplify ioc_rqos_done()

[Christoph Hellwig]

On Tue, Jun 14, 2022 at 10:57:23AM -0700, Bart Van Assche wrote:
> Leave out the superfluous "& REQ_OP_MASK" code. The definition of req_op()
> shows that that code is superfluous:
> 
>  #define req_op(req) ((req)->cmd_flags & REQ_OP_MASK)
> 
> Compile-tested only.

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 2/3] block: Rename a blk_mq_map_queue() argument

[Bart Van Assche]

Before the introduction of blk_mq_get_hctx_type(), blk_mq_map_queue()
only used the flags from its second argument. Since the introduction of
blk_mq_get_hctx_type(), blk_mq_map_queue() uses both the operation and
the flags encoded in that argument. Rename the second argument of
blk_mq_map_queue() to make this clear.

Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 block/blk-mq.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/blk-mq.h b/block/blk-mq.h
index 2615bd58bad3..e4c6fe2c8ac8 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -86,16 +86,16 @@ static inline struct blk_mq_hw_ctx *blk_mq_map_queue_type(struct request_queue *
 	return xa_load(&q->hctx_table, q->tag_set->map[type].mq_map[cpu]);
 }
 
-static inline enum hctx_type blk_mq_get_hctx_type(unsigned int flags)
+static inline enum hctx_type blk_mq_get_hctx_type(unsigned int opf)
 {
 	enum hctx_type type = HCTX_TYPE_DEFAULT;
 
 	/*
 	 * The caller ensure that if REQ_POLLED, poll must be enabled.
 	 */
-	if (flags & REQ_POLLED)
+	if (opf & REQ_POLLED)
 		type = HCTX_TYPE_POLL;
-	else if ((flags & REQ_OP_MASK) == REQ_OP_READ)
+	else if ((opf & REQ_OP_MASK) == REQ_OP_READ)
 		type = HCTX_TYPE_READ;
 	return type;
 }
@@ -103,14 +103,14 @@ static inline enum hctx_type blk_mq_get_hctx_type(unsigned int flags)
 /*
  * blk_mq_map_queue() - map (cmd_flags,type) to hardware queue
  * @q: request queue
- * @flags: request command flags
+ * @opf: operation type (REQ_OP_*) and flags (e.g. REQ_POLLED).
  * @ctx: software queue cpu ctx
  */
 static inline struct blk_mq_hw_ctx *blk_mq_map_queue(struct request_queue *q,
-						     unsigned int flags,
+						     unsigned int opf,
 						     struct blk_mq_ctx *ctx)
 {
-	return ctx->hctxs[blk_mq_get_hctx_type(flags)];
+	return ctx->hctxs[blk_mq_get_hctx_type(opf)];
 }
 
 /*

Re: [PATCH 2/3] block: Rename a blk_mq_map_queue() argument

[Christoph Hellwig]

On Tue, Jun 14, 2022 at 10:57:24AM -0700, Bart Van Assche wrote:
> Before the introduction of blk_mq_get_hctx_type(), blk_mq_map_queue()
> only used the flags from its second argument. Since the introduction of
> blk_mq_get_hctx_type(), blk_mq_map_queue() uses both the operation and
> the flags encoded in that argument. Rename the second argument of
> blk_mq_map_queue() to make this clear.

Looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 3/3] block: Specify the operation type when calling blk_mq_map_queue()

[Bart Van Assche]

Since the introduction of blk_mq_get_hctx_type() the operation type in
the second argument of blk_mq_get_hctx_type() matters. Make sure that
a hardware queue of type HCTX_TYPE_DEFAULT is selected instead of a
hardware queue of type HCTX_TYPE_READ.

Cc: Ming Lei <ming.lei@redhat.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 block/blk-mq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index e9bf950983c7..9b1518ef1aa1 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2168,7 +2168,7 @@ static struct blk_mq_hw_ctx *blk_mq_get_sq_hctx(struct request_queue *q)
 	 * just causes lock contention inside the scheduler and pointless cache
 	 * bouncing.
 	 */
-	struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(q, 0, ctx);
+	struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(q, REQ_OP_WRITE, ctx);
 
 	if (!blk_mq_hctx_stopped(hctx))
 		return hctx;

Re: [PATCH 3/3] block: Specify the operation type when calling blk_mq_map_queue()

[Jens Axboe]

On 6/14/22 11:57 AM, Bart Van Assche wrote:
> Since the introduction of blk_mq_get_hctx_type() the operation type in
> the second argument of blk_mq_get_hctx_type() matters. Make sure that
> a hardware queue of type HCTX_TYPE_DEFAULT is selected instead of a
> hardware queue of type HCTX_TYPE_READ.

Just reading this commit message, it only states that we need to make
sure that a write vs read queue. But not why that is important. Can you
augment it a little bit?

-- 
Jens Axboe

Re: [PATCH 3/3] block: Specify the operation type when calling blk_mq_map_queue()

[Ming Lei]

On Tue, Jun 14, 2022 at 10:57:25AM -0700, Bart Van Assche wrote:
> Since the introduction of blk_mq_get_hctx_type() the operation type in
> the second argument of blk_mq_get_hctx_type() matters. Make sure that
> a hardware queue of type HCTX_TYPE_DEFAULT is selected instead of a
> hardware queue of type HCTX_TYPE_READ.
> 
> Cc: Ming Lei <ming.lei@redhat.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  block/blk-mq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index e9bf950983c7..9b1518ef1aa1 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2168,7 +2168,7 @@ static struct blk_mq_hw_ctx *blk_mq_get_sq_hctx(struct request_queue *q)
>  	 * just causes lock contention inside the scheduler and pointless cache
>  	 * bouncing.
>  	 */
> -	struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(q, 0, ctx);
> +	struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(q, REQ_OP_WRITE, ctx);

The change itself doesn't make a difference, since both results in choosing
HCTX_TYPE_DEFAULT, but passing REQ_OP_WRITE is a bit misleading.


Thanks,
Ming

Re: [PATCH 3/3] block: Specify the operation type when calling blk_mq_map_queue()

[Christoph Hellwig]

On Wed, Jun 15, 2022 at 08:31:21AM +0800, Ming Lei wrote:
> > -	struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(q, 0, ctx);
> > +	struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(q, REQ_OP_WRITE, ctx);
> 
> The change itself doesn't make a difference, since both results in choosing
> HCTX_TYPE_DEFAULT, but passing REQ_OP_WRITE is a bit misleading.

Well, the argument is an operationm so we better pass in a correct
operation (at some point we should look into a __bitwise annotation
or similar to make it clean).  And as 0 is REQ_OP_READ, we will end
up with the HCTX_TYPE_READ hctx IFF someone configures read queues
and uses an sq only schedule.  Which is a completely stupid but
possible setup.

Re: [PATCH 3/3] block: Specify the operation type when calling blk_mq_map_queue()

[Ming Lei]

On Wed, Jun 15, 2022 at 08:08:51AM +0200, Christoph Hellwig wrote:
> On Wed, Jun 15, 2022 at 08:31:21AM +0800, Ming Lei wrote:
> > > -	struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(q, 0, ctx);
> > > +	struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(q, REQ_OP_WRITE, ctx);
> > 
> > The change itself doesn't make a difference, since both results in choosing
> > HCTX_TYPE_DEFAULT, but passing REQ_OP_WRITE is a bit misleading.
> 
> Well, the argument is an operationm so we better pass in a correct
> operation (at some point we should look into a __bitwise annotation
> or similar to make it clean).  And as 0 is REQ_OP_READ, we will end
> up with the HCTX_TYPE_READ hctx IFF someone configures read queues
> and uses an sq only schedule.  Which is a completely stupid but
> possible setup.
> 

OK, looks here the hctx can be retrieved ctx->hctxs[HCTX_TYPE_DEFAULT]
directly.


Thanks,
Ming

Re: [PATCH 3/3] block: Specify the operation type when calling blk_mq_map_queue()

[Bart Van Assche]

On 6/14/22 23:08, Christoph Hellwig wrote:
> On Wed, Jun 15, 2022 at 08:31:21AM +0800, Ming Lei wrote:
>>> -	struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(q, 0, ctx);
>>> +	struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(q, REQ_OP_WRITE, ctx);
>>
>> The change itself doesn't make a difference, since both results in choosing
>> HCTX_TYPE_DEFAULT, but passing REQ_OP_WRITE is a bit misleading.
> 
> Well, the argument is an operationm so we better pass in a correct
> operation (at some point we should look into a __bitwise annotation
> or similar to make it clean).  And as 0 is REQ_OP_READ, we will end
> up with the HCTX_TYPE_READ hctx IFF someone configures read queues
> and uses an sq only schedule.  Which is a completely stupid but
> possible setup.

Hi Christoph,

I looked into adding a __bitwise annotation for request flags by 
introducing a new type that is called blk_mq_opf_t. Introducing such a 
type without modifying a lot of code seems difficult to me. This is what 
I ran into:
* If the type of the operation type constants (REQ_OP_READ etc.) is 
modified into blk_mq_opf_t then their type changes from 'enum req_opf' 
into type blk_mq_opf_t and sparse complains when passing e.g. 
REQ_OP_READ to a function that accepts an argument with type enum req_opf.
* If the type of the operation type constants is not modified then 
sparse complains about bitwise or-ing the operation type and a request 
flag, e.g. REQ_OP_WRITE | REQ_FUA.

I'm not sure how to solve this other than by modifying the functions 
that accept an 'opf' argument into accepting an additional argument 
(enum req_opf op + blk_mq_opf_t op_flags).

Thanks,

Bart.

Re: [PATCH 3/3] block: Specify the operation type when calling blk_mq_map_queue()

[Christoph Hellwig]

On Fri, Jun 17, 2022 at 11:25:41AM -0700, Bart Van Assche wrote:
> I looked into adding a __bitwise annotation for request flags by 
> introducing a new type that is called blk_mq_opf_t. Introducing such a type 
> without modifying a lot of code seems difficult to me. This is what I ran 
> into:
> * If the type of the operation type constants (REQ_OP_READ etc.) is 
> modified into blk_mq_opf_t then their type changes from 'enum req_opf' into 
> type blk_mq_opf_t and sparse complains when passing e.g. REQ_OP_READ to a 
> function that accepts an argument with type enum req_opf.
> * If the type of the operation type constants is not modified then sparse 
> complains about bitwise or-ing the operation type and a request flag, e.g. 
> REQ_OP_WRITE | REQ_FUA.
>
> I'm not sure how to solve this other than by modifying the functions that 
> accept an 'opf' argument into accepting an additional argument (enum 
> req_opf op + blk_mq_opf_t op_flags).

I actually always though of one type for the operation plus flags
as we basically always use the together.  That might still run into
a lot of problems, but is definitively way simpler and matches how
all the argument passing actually works.