[PATCH v2 0/3] Three small block layer patches

[PATCH v2 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.

Changes compared to v1:
- Modified patch 3/3 as suggested by Ming Lei.

Bart Van Assche (3):
  blk-iocost: Simplify ioc_rqos_done()
  block: Rename a blk_mq_map_queue() argument
  block: Improve blk_mq_get_sq_hctx()

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

[PATCH v2 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>
Acked-by: Tejun Heo <tj@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
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 v2 1/3] blk-iocost: Simplify ioc_rqos_done()

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH v2 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>
Reviewed-by: 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 v2 2/3] block: Rename a blk_mq_map_queue() argument

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH v2 3/3] block: Improve blk_mq_get_sq_hctx()

[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
blk_mq_get_sq_hctx() selects a hardware queue of type HCTX_TYPE_DEFAULT
instead of a hardware queue of type HCTX_TYPE_READ.

Cc: Ming Lei <ming.lei@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
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..7a5558bbc7f6 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 = ctx->hctxs[HCTX_TYPE_DEFAULT];
 
 	if (!blk_mq_hctx_stopped(hctx))
 		return hctx;

Re: [PATCH v2 3/3] block: Improve blk_mq_get_sq_hctx()

[Jens Axboe]

On 6/15/22 3:01 PM, 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
> blk_mq_get_sq_hctx() selects a hardware queue of type HCTX_TYPE_DEFAULT
> instead of a hardware queue of type HCTX_TYPE_READ.

You ignored my comment here. A good changelog explains WHY a change is
made, not what the change does. For that, I can just read the change.

So please change the commit message to say WHY it's important. Or if
it's not, make it clear that it is a cosmetic thing.

-- 
Jens Axboe

Re: [PATCH v2 3/3] block: Improve blk_mq_get_sq_hctx()

[Ming Lei]

On Wed, Jun 15, 2022 at 02:01:36PM -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
> blk_mq_get_sq_hctx() selects a hardware queue of type HCTX_TYPE_DEFAULT
> instead of a hardware queue of type HCTX_TYPE_READ.
> 
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Christoph Hellwig <hch@lst.de>
> 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..7a5558bbc7f6 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 = ctx->hctxs[HCTX_TYPE_DEFAULT];
>  
>  	if (!blk_mq_hctx_stopped(hctx))
>  		return hctx;

Looks fine, since the original behavior is to return hctx with HCTX_TYPE_DEFAULT
before commit 5d05426e2d5f ("blk-mq: don't touch ->tagset in blk_mq_get_sq_hctx").

Reviewed-by: Ming Lei <ming.lei@redhat.com>



thanks,
Ming