keep passthrough request out of the I/O schedulers

keep passthrough request out of the I/O schedulers

[Christoph Hellwig]

Hi Jens,

this is my respin of Ming's "blk-mq: handle passthrough request as really
passthrough" series.  The first patch is a slightly tweaked version of
Ming's first patch, while the 2 others are new based on the discussion.

This isn't meant to shut down the discussion on wether to use scheduler
tags for passthrough or not, but I'd like to see the bug fixed (and
a series I have that needs it unblocked).

[PATCH 1/3] blk-mq: don't queue plugged passthrough requests into scheduler

[Christoph Hellwig]

From: Ming Lei <ming.lei@redhat.com>

Passthrough) request should never be queued to the I/O scheduler,
as scheduling these opaque requests doens't make sense, and I/O
schedulers might required req->bio to be always valid.

We never let passthrough request cross scheduler before commit
1c2d2fff6dc0 ("block: wire-up support for passthrough plugging"),
restored this behavior even for passthrough requests issued under
a plug.

Reported-by: Guangwu Zhang <guazhang@redhat.com>
Closes: https://lore.kernel.org/linux-block/CAGS2=YosaYaUTEMU3uaf+y=8MqSrhL7sYsJn8EwbaM=76p_4Qg@mail.gmail.com/
Investigated-by: Yu Kuai <yukuai1@huaweicloud.com>
Fixes: 1c2d2fff6dc0 ("block: wire-up support for passthrough plugging")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
[hch: use blk_mq_insert_requests for passthrough requests,
      fix up the commit message and comments]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-mq.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index f6dad0886a2fa1..8b7e4daaa5b70d 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2711,6 +2711,7 @@ static void blk_mq_dispatch_plug_list(struct blk_plug *plug, bool from_sched)
 	struct request *requeue_list = NULL;
 	struct request **requeue_lastp = &requeue_list;
 	unsigned int depth = 0;
+	bool is_passthrough = false;
 	LIST_HEAD(list);
 
 	do {
@@ -2719,7 +2720,9 @@ static void blk_mq_dispatch_plug_list(struct blk_plug *plug, bool from_sched)
 		if (!this_hctx) {
 			this_hctx = rq->mq_hctx;
 			this_ctx = rq->mq_ctx;
-		} else if (this_hctx != rq->mq_hctx || this_ctx != rq->mq_ctx) {
+			is_passthrough = blk_rq_is_passthrough(rq);
+		} else if (this_hctx != rq->mq_hctx || this_ctx != rq->mq_ctx ||
+			   is_passthrough != blk_rq_is_passthrough(rq)) {
 			rq_list_add_tail(&requeue_lastp, rq);
 			continue;
 		}
@@ -2731,7 +2734,8 @@ static void blk_mq_dispatch_plug_list(struct blk_plug *plug, bool from_sched)
 	trace_block_unplug(this_hctx->queue, depth, !from_sched);
 
 	percpu_ref_get(&this_hctx->queue->q_usage_counter);
-	if (this_hctx->queue->elevator) {
+	/* passthrough requests should never be issued to the I/O scheduler */
+	if (this_hctx->queue->elevator && !is_passthrough) {
 		this_hctx->queue->elevator->type->ops.insert_requests(this_hctx,
 				&list, 0);
 		blk_mq_run_hw_queue(this_hctx, from_sched);
-- 
2.39.2

[PATCH 2/3] blk-mq: remove RQF_ELVPRIV

[Christoph Hellwig]

RQF_ELVPRIV is set for all non-flush requests that have RQF_ELV set.
Expand this condition in the two users of the flag and remove it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-mq-debugfs.c | 1 -
 block/blk-mq-sched.h   | 4 ++--
 block/blk-mq.c         | 6 ++----
 include/linux/blk-mq.h | 2 --
 4 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index d23a8554ec4aeb..588b7048342bee 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -248,7 +248,6 @@ static const char *const rqf_name[] = {
 	RQF_NAME(DONTPREP),
 	RQF_NAME(FAILED),
 	RQF_NAME(QUIET),
-	RQF_NAME(ELVPRIV),
 	RQF_NAME(IO_STAT),
 	RQF_NAME(PM),
 	RQF_NAME(HASHED),
diff --git a/block/blk-mq-sched.h b/block/blk-mq-sched.h
index 7c3cbad17f3052..4d8d2cd3b47396 100644
--- a/block/blk-mq-sched.h
+++ b/block/blk-mq-sched.h
@@ -58,11 +58,11 @@ static inline void blk_mq_sched_completed_request(struct request *rq, u64 now)
 
 static inline void blk_mq_sched_requeue_request(struct request *rq)
 {
-	if (rq->rq_flags & RQF_ELV) {
+	if ((rq->rq_flags & RQF_ELV) && !op_is_flush(rq->cmd_flags)) {
 		struct request_queue *q = rq->q;
 		struct elevator_queue *e = q->elevator;
 
-		if ((rq->rq_flags & RQF_ELVPRIV) && e->type->ops.requeue_request)
+		if (e->type->ops.requeue_request)
 			e->type->ops.requeue_request(rq);
 	}
 }
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 8b7e4daaa5b70d..7470c6636dc4f7 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -393,10 +393,8 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
 		RB_CLEAR_NODE(&rq->rb_node);
 
 		if (!op_is_flush(data->cmd_flags) &&
-		    e->type->ops.prepare_request) {
+		    e->type->ops.prepare_request)
 			e->type->ops.prepare_request(rq);
-			rq->rq_flags |= RQF_ELVPRIV;
-		}
 	}
 
 	return rq;
@@ -696,7 +694,7 @@ void blk_mq_free_request(struct request *rq)
 	struct request_queue *q = rq->q;
 	struct blk_mq_hw_ctx *hctx = rq->mq_hctx;
 
-	if ((rq->rq_flags & RQF_ELVPRIV) &&
+	if ((rq->rq_flags & RQF_ELV) && !op_is_flush(rq->cmd_flags) &&
 	    q->elevator->type->ops.finish_request)
 		q->elevator->type->ops.finish_request(rq);
 
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 06caacd77ed668..5529e7d28ae6bb 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -42,8 +42,6 @@ typedef __u32 __bitwise req_flags_t;
 #define RQF_FAILED		((__force req_flags_t)(1 << 10))
 /* don't warn about errors */
 #define RQF_QUIET		((__force req_flags_t)(1 << 11))
-/* elevator private data attached */
-#define RQF_ELVPRIV		((__force req_flags_t)(1 << 12))
 /* account into disk and partition IO statistics */
 #define RQF_IO_STAT		((__force req_flags_t)(1 << 13))
 /* runtime pm request */
-- 
2.39.2

[PATCH 3/3] blk-mq: make sure elevator callbacks aren't called for passthrough request

[Christoph Hellwig]

In case of q->elevator, passthrought request can still be marked as
RQF_ELV, so some elevator callbacks will be called for them.

Fix this by splitting RQF_SCHED_TAGS, which is set for all requests that
are issued on a queue that uses an I/O scheduler, and RQF_USE_SCHED for
non-flush, non-passthrough requests on such a queue.

Roughly based on two different patches from
Ming Lei <ming.lei@redhat.com>.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-mq-debugfs.c |  3 ++-
 block/blk-mq-sched.h   |  6 ++---
 block/blk-mq.c         | 53 +++++++++++++++++++++++-------------------
 block/blk-mq.h         |  6 ++---
 include/linux/blk-mq.h | 12 ++++++----
 5 files changed, 44 insertions(+), 36 deletions(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 588b7048342bee..1178d8696dcc05 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -246,6 +246,8 @@ static const char *const rqf_name[] = {
 	RQF_NAME(MIXED_MERGE),
 	RQF_NAME(MQ_INFLIGHT),
 	RQF_NAME(DONTPREP),
+	RQF_NAME(SCHED_TAGS),
+	RQF_NAME(USE_SCHED),
 	RQF_NAME(FAILED),
 	RQF_NAME(QUIET),
 	RQF_NAME(IO_STAT),
@@ -255,7 +257,6 @@ static const char *const rqf_name[] = {
 	RQF_NAME(SPECIAL_PAYLOAD),
 	RQF_NAME(ZONE_WRITE_LOCKED),
 	RQF_NAME(TIMED_OUT),
-	RQF_NAME(ELV),
 	RQF_NAME(RESV),
 };
 #undef RQF_NAME
diff --git a/block/blk-mq-sched.h b/block/blk-mq-sched.h
index 4d8d2cd3b47396..1326526bb7338c 100644
--- a/block/blk-mq-sched.h
+++ b/block/blk-mq-sched.h
@@ -37,7 +37,7 @@ static inline bool
 blk_mq_sched_allow_merge(struct request_queue *q, struct request *rq,
 			 struct bio *bio)
 {
-	if (rq->rq_flags & RQF_ELV) {
+	if (rq->rq_flags & RQF_USE_SCHED) {
 		struct elevator_queue *e = q->elevator;
 
 		if (e->type->ops.allow_merge)
@@ -48,7 +48,7 @@ blk_mq_sched_allow_merge(struct request_queue *q, struct request *rq,
 
 static inline void blk_mq_sched_completed_request(struct request *rq, u64 now)
 {
-	if (rq->rq_flags & RQF_ELV) {
+	if (rq->rq_flags & RQF_USE_SCHED) {
 		struct elevator_queue *e = rq->q->elevator;
 
 		if (e->type->ops.completed_request)
@@ -58,7 +58,7 @@ static inline void blk_mq_sched_completed_request(struct request *rq, u64 now)
 
 static inline void blk_mq_sched_requeue_request(struct request *rq)
 {
-	if ((rq->rq_flags & RQF_ELV) && !op_is_flush(rq->cmd_flags)) {
+	if (rq->rq_flags & RQF_USE_SCHED) {
 		struct request_queue *q = rq->q;
 		struct elevator_queue *e = q->elevator;
 
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 7470c6636dc4f7..e021740154feae 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -354,12 +354,12 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
 		data->rq_flags |= RQF_IO_STAT;
 	rq->rq_flags = data->rq_flags;
 
-	if (!(data->rq_flags & RQF_ELV)) {
-		rq->tag = tag;
-		rq->internal_tag = BLK_MQ_NO_TAG;
-	} else {
+	if (data->rq_flags & RQF_SCHED_TAGS) {
 		rq->tag = BLK_MQ_NO_TAG;
 		rq->internal_tag = tag;
+	} else {
+		rq->tag = tag;
+		rq->internal_tag = BLK_MQ_NO_TAG;
 	}
 	rq->timeout = 0;
 
@@ -386,14 +386,13 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
 	WRITE_ONCE(rq->deadline, 0);
 	req_ref_set(rq, 1);
 
-	if (rq->rq_flags & RQF_ELV) {
+	if (rq->rq_flags & RQF_USE_SCHED) {
 		struct elevator_queue *e = data->q->elevator;
 
 		INIT_HLIST_NODE(&rq->hash);
 		RB_CLEAR_NODE(&rq->rb_node);
 
-		if (!op_is_flush(data->cmd_flags) &&
-		    e->type->ops.prepare_request)
+		if (e->type->ops.prepare_request)
 			e->type->ops.prepare_request(rq);
 	}
 
@@ -447,26 +446,32 @@ static struct request *__blk_mq_alloc_requests(struct blk_mq_alloc_data *data)
 		data->flags |= BLK_MQ_REQ_NOWAIT;
 
 	if (q->elevator) {
-		struct elevator_queue *e = q->elevator;
-
-		data->rq_flags |= RQF_ELV;
+		/*
+		 * All requests use scheduler tags when an I/O scheduler is
+		 * enabled for the queue.
+		 */
+		data->rq_flags |= RQF_SCHED_TAGS;
 
 		/*
 		 * Flush/passthrough requests are special and go directly to the
-		 * dispatch list. Don't include reserved tags in the
-		 * limiting, as it isn't useful.
+		 * dispatch list.
 		 */
 		if (!op_is_flush(data->cmd_flags) &&
-		    !blk_op_is_passthrough(data->cmd_flags) &&
-		    e->type->ops.limit_depth &&
-		    !(data->flags & BLK_MQ_REQ_RESERVED))
-			e->type->ops.limit_depth(data->cmd_flags, data);
+		    !blk_op_is_passthrough(data->cmd_flags)) {
+			struct elevator_mq_ops *ops = &q->elevator->type->ops;
+
+			WARN_ON_ONCE(data->flags & BLK_MQ_REQ_RESERVED);
+
+			data->rq_flags |= RQF_USE_SCHED;
+			if (ops->limit_depth)
+				ops->limit_depth(data->cmd_flags, data);
+		}
 	}
 
 retry:
 	data->ctx = blk_mq_get_ctx(q);
 	data->hctx = blk_mq_map_queue(q, data->cmd_flags, data->ctx);
-	if (!(data->rq_flags & RQF_ELV))
+	if (!(data->rq_flags & RQF_SCHED_TAGS))
 		blk_mq_tag_busy(data->hctx);
 
 	if (data->flags & BLK_MQ_REQ_RESERVED)
@@ -646,10 +651,10 @@ struct request *blk_mq_alloc_request_hctx(struct request_queue *q,
 		goto out_queue_exit;
 	data.ctx = __blk_mq_get_ctx(q, cpu);
 
-	if (!q->elevator)
-		blk_mq_tag_busy(data.hctx);
+	if (q->elevator)
+		data.rq_flags |= RQF_SCHED_TAGS;
 	else
-		data.rq_flags |= RQF_ELV;
+		blk_mq_tag_busy(data.hctx);
 
 	if (flags & BLK_MQ_REQ_RESERVED)
 		data.rq_flags |= RQF_RESV;
@@ -694,7 +699,7 @@ void blk_mq_free_request(struct request *rq)
 	struct request_queue *q = rq->q;
 	struct blk_mq_hw_ctx *hctx = rq->mq_hctx;
 
-	if ((rq->rq_flags & RQF_ELV) && !op_is_flush(rq->cmd_flags) &&
+	if ((rq->rq_flags & RQF_USE_SCHED) &&
 	    q->elevator->type->ops.finish_request)
 		q->elevator->type->ops.finish_request(rq);
 
@@ -1268,7 +1273,7 @@ static void blk_add_rq_to_plug(struct blk_plug *plug, struct request *rq)
 
 	if (!plug->multiple_queues && last && last->q != rq->q)
 		plug->multiple_queues = true;
-	if (!plug->has_elevator && (rq->rq_flags & RQF_ELV))
+	if (!plug->has_elevator && (rq->rq_flags & RQF_USE_SCHED))
 		plug->has_elevator = true;
 	rq->rq_next = NULL;
 	rq_list_add(&plug->mq_list, rq);
@@ -2620,7 +2625,7 @@ static void blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx,
 		return;
 	}
 
-	if ((rq->rq_flags & RQF_ELV) || !blk_mq_get_budget_and_tag(rq)) {
+	if ((rq->rq_flags & RQF_USE_SCHED) || !blk_mq_get_budget_and_tag(rq)) {
 		blk_mq_insert_request(rq, 0);
 		blk_mq_run_hw_queue(hctx, false);
 		return;
@@ -2983,7 +2988,7 @@ void blk_mq_submit_bio(struct bio *bio)
 	}
 
 	hctx = rq->mq_hctx;
-	if ((rq->rq_flags & RQF_ELV) ||
+	if ((rq->rq_flags & RQF_USE_SCHED) ||
 	    (hctx->dispatch_busy && (q->nr_hw_queues == 1 || !is_sync))) {
 		blk_mq_insert_request(rq, 0);
 		blk_mq_run_hw_queue(hctx, true);
diff --git a/block/blk-mq.h b/block/blk-mq.h
index e876584d351634..d15981db34b958 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -226,9 +226,9 @@ static inline bool blk_mq_is_shared_tags(unsigned int flags)
 
 static inline struct blk_mq_tags *blk_mq_tags_from_data(struct blk_mq_alloc_data *data)
 {
-	if (!(data->rq_flags & RQF_ELV))
-		return data->hctx->tags;
-	return data->hctx->sched_tags;
+	if (data->rq_flags & RQF_SCHED_TAGS)
+		return data->hctx->sched_tags;
+	return data->hctx->tags;
 }
 
 static inline bool blk_mq_hctx_stopped(struct blk_mq_hw_ctx *hctx)
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 5529e7d28ae6bb..888b79633692fc 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -38,6 +38,10 @@ typedef __u32 __bitwise req_flags_t;
 #define RQF_MQ_INFLIGHT		((__force req_flags_t)(1 << 6))
 /* 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 and 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 */
@@ -57,9 +61,7 @@ typedef __u32 __bitwise req_flags_t;
 #define RQF_ZONE_WRITE_LOCKED	((__force req_flags_t)(1 << 19))
 /* ->timeout has been called, don't expire again */
 #define RQF_TIMED_OUT		((__force req_flags_t)(1 << 21))
-/* queue has elevator attached */
-#define RQF_ELV			((__force req_flags_t)(1 << 22))
-#define RQF_RESV			((__force req_flags_t)(1 << 23))
+#define RQF_RESV		((__force req_flags_t)(1 << 23))
 
 /* flags that prevent us from merging requests: */
 #define RQF_NOMERGE_FLAGS \
@@ -842,7 +844,7 @@ void blk_mq_end_request_batch(struct io_comp_batch *ib);
  */
 static inline bool blk_mq_need_time_stamp(struct request *rq)
 {
-	return (rq->rq_flags & (RQF_IO_STAT | RQF_STATS | RQF_ELV));
+	return (rq->rq_flags & (RQF_IO_STAT | RQF_STATS | RQF_USE_SCHED));
 }
 
 static inline bool blk_mq_is_reserved_rq(struct request *rq)
@@ -858,7 +860,7 @@ static inline bool blk_mq_add_to_batch(struct request *req,
 				       struct io_comp_batch *iob, int ioerror,
 				       void (*complete)(struct io_comp_batch *))
 {
-	if (!iob || (req->rq_flags & RQF_ELV) || ioerror ||
+	if (!iob || (req->rq_flags & RQF_USE_SCHED) || ioerror ||
 			(req->end_io && !blk_rq_is_passthrough(req)))
 		return false;
 
-- 
2.39.2

Re: [PATCH 2/3] blk-mq: remove RQF_ELVPRIV

[Ming Lei]

On Thu, May 18, 2023 at 07:31:00AM +0200, Christoph Hellwig wrote:
> RQF_ELVPRIV is set for all non-flush requests that have RQF_ELV set.
> Expand this condition in the two users of the flag and remove it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

Thanks,
Ming

Re: [PATCH 2/3] blk-mq: remove RQF_ELVPRIV

[Ming Lei]

On Thu, May 18, 2023 at 07:31:00AM +0200, Christoph Hellwig wrote:
> RQF_ELVPRIV is set for all non-flush requests that have RQF_ELV set.
> Expand this condition in the two users of the flag and remove it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/blk-mq-debugfs.c | 1 -
>  block/blk-mq-sched.h   | 4 ++--
>  block/blk-mq.c         | 6 ++----
>  include/linux/blk-mq.h | 2 --
>  4 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
> index d23a8554ec4aeb..588b7048342bee 100644
> --- a/block/blk-mq-debugfs.c
> +++ b/block/blk-mq-debugfs.c
> @@ -248,7 +248,6 @@ static const char *const rqf_name[] = {
>  	RQF_NAME(DONTPREP),
>  	RQF_NAME(FAILED),
>  	RQF_NAME(QUIET),
> -	RQF_NAME(ELVPRIV),
>  	RQF_NAME(IO_STAT),
>  	RQF_NAME(PM),
>  	RQF_NAME(HASHED),
> diff --git a/block/blk-mq-sched.h b/block/blk-mq-sched.h
> index 7c3cbad17f3052..4d8d2cd3b47396 100644
> --- a/block/blk-mq-sched.h
> +++ b/block/blk-mq-sched.h
> @@ -58,11 +58,11 @@ static inline void blk_mq_sched_completed_request(struct request *rq, u64 now)
>  
>  static inline void blk_mq_sched_requeue_request(struct request *rq)
>  {
> -	if (rq->rq_flags & RQF_ELV) {
> +	if ((rq->rq_flags & RQF_ELV) && !op_is_flush(rq->cmd_flags)) {
>  		struct request_queue *q = rq->q;
>  		struct elevator_queue *e = q->elevator;
>  
> -		if ((rq->rq_flags & RQF_ELVPRIV) && e->type->ops.requeue_request)
> +		if (e->type->ops.requeue_request)
>  			e->type->ops.requeue_request(rq);

The above actually changes current behavior since RQF_ELVPRIV is only set
iff the following condition is true:

	(rq->rq_flags & RQF_ELV) && !op_is_flush(rq->cmd_flags) &&
		e->type->ops.prepare_request.

>  	}
>  }
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 8b7e4daaa5b70d..7470c6636dc4f7 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -393,10 +393,8 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
>  		RB_CLEAR_NODE(&rq->rb_node);
>  
>  		if (!op_is_flush(data->cmd_flags) &&
> -		    e->type->ops.prepare_request) {
> +		    e->type->ops.prepare_request)
>  			e->type->ops.prepare_request(rq);
> -			rq->rq_flags |= RQF_ELVPRIV;
> -		}
>  	}
>  
>  	return rq;
> @@ -696,7 +694,7 @@ void blk_mq_free_request(struct request *rq)
>  	struct request_queue *q = rq->q;
>  	struct blk_mq_hw_ctx *hctx = rq->mq_hctx;
>  
> -	if ((rq->rq_flags & RQF_ELVPRIV) &&
> +	if ((rq->rq_flags & RQF_ELV) && !op_is_flush(rq->cmd_flags) &&
>  	    q->elevator->type->ops.finish_request)
>  		q->elevator->type->ops.finish_request(rq);

Same with above.

Thanks,
Ming

Re: [PATCH 2/3] blk-mq: remove RQF_ELVPRIV

[Christoph Hellwig]

On Thu, May 18, 2023 at 03:11:12PM +0800, Ming Lei wrote:
> > -		if ((rq->rq_flags & RQF_ELVPRIV) && e->type->ops.requeue_request)
> > +		if (e->type->ops.requeue_request)
> >  			e->type->ops.requeue_request(rq);
> 
> The above actually changes current behavior since RQF_ELVPRIV is only set
> iff the following condition is true:
> 
> 	(rq->rq_flags & RQF_ELV) && !op_is_flush(rq->cmd_flags) &&
> 		e->type->ops.prepare_request.

It would require an I/O scheduler that implements .requeue_request but
not .prepare_request, which doesn't exist and also is rather pointless as
this .requeue_request method would never get called in the current code.

So no, no behavior change in practice.

Re: [PATCH 2/3] blk-mq: remove RQF_ELVPRIV

[Ming Lei]

On Thu, May 18, 2023 at 03:06:32PM +0200, Christoph Hellwig wrote:
> On Thu, May 18, 2023 at 03:11:12PM +0800, Ming Lei wrote:
> > > -		if ((rq->rq_flags & RQF_ELVPRIV) && e->type->ops.requeue_request)
> > > +		if (e->type->ops.requeue_request)
> > >  			e->type->ops.requeue_request(rq);
> > 
> > The above actually changes current behavior since RQF_ELVPRIV is only set
> > iff the following condition is true:
> > 
> > 	(rq->rq_flags & RQF_ELV) && !op_is_flush(rq->cmd_flags) &&
> > 		e->type->ops.prepare_request.
> 
> It would require an I/O scheduler that implements .requeue_request but
> not .prepare_request, which doesn't exist and also is rather pointless as
> this .requeue_request method would never get called in the current code.
> 
> So no, no behavior change in practice.

Fair enough, just found that all three schedulers have implemented
e->type->ops.prepare_request.

Thanks, 
Ming

Re: [PATCH 3/3] blk-mq: make sure elevator callbacks aren't called for passthrough request

[Ming Lei]

On Thu, May 18, 2023 at 07:31:01AM +0200, Christoph Hellwig wrote:
> In case of q->elevator, passthrought request can still be marked as
> RQF_ELV, so some elevator callbacks will be called for them.
> 
> Fix this by splitting RQF_SCHED_TAGS, which is set for all requests that
> are issued on a queue that uses an I/O scheduler, and RQF_USE_SCHED for
> non-flush, non-passthrough requests on such a queue.
> 
> Roughly based on two different patches from
> Ming Lei <ming.lei@redhat.com>.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

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

Thanks,
Ming

Re: [PATCH 1/3] blk-mq: don't queue plugged passthrough requests into scheduler

[Bart Van Assche]

On 5/17/23 22:30, Christoph Hellwig wrote:
> Passthrough) request should never be queued to the I/O scheduler,

Passthrough) request -> Passthrough requests

> as scheduling these opaque requests doens't make sense, and I/O

doens't -> doesn't

> schedulers might required req->bio to be always valid.

required -> require

> We never let passthrough request cross scheduler before commit

request -> requests

> 1c2d2fff6dc0 ("block: wire-up support for passthrough plugging"),
> restored this behavior even for passthrough requests issued under
> a plug.

The above sentence needs to be edited for clarity.

Otherwise this patch looks good to me.

Thanks,

Bart.

Re: [PATCH 2/3] blk-mq: remove RQF_ELVPRIV

[Bart Van Assche]

On 5/17/23 22:31, Christoph Hellwig wrote:
> RQF_ELVPRIV is set for all non-flush requests that have RQF_ELV set.
> Expand this condition in the two users of the flag and remove it.

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

Re: [PATCH 3/3] blk-mq: make sure elevator callbacks aren't called for passthrough request

[Bart Van Assche]

On 5/17/23 22:31, Christoph Hellwig wrote:
> In case of q->elevator, passthrought request can still be marked as

passthrought -> passthrough

> +/* use hctx->sched_tags */
> +#define RQF_SCHED_TAGS		((__force req_flags_t)(1 << 8))
> +/* use and I/O scheduler for this request */
> +#define RQF_USE_SCHED		((__force req_flags_t)(1 << 9))

and -> an

Otherwise this patch looks good to me.

Thanks,

Bart.

Re: [PATCH 2/3] blk-mq: remove RQF_ELVPRIV

[Jens Axboe]

On 5/18/23 7:20 AM, Ming Lei wrote:
> On Thu, May 18, 2023 at 03:06:32PM +0200, Christoph Hellwig wrote:
>> On Thu, May 18, 2023 at 03:11:12PM +0800, Ming Lei wrote:
>>>> -		if ((rq->rq_flags & RQF_ELVPRIV) && e->type->ops.requeue_request)
>>>> +		if (e->type->ops.requeue_request)
>>>>  			e->type->ops.requeue_request(rq);
>>>
>>> The above actually changes current behavior since RQF_ELVPRIV is only set
>>> iff the following condition is true:
>>>
>>> 	(rq->rq_flags & RQF_ELV) && !op_is_flush(rq->cmd_flags) &&
>>> 		e->type->ops.prepare_request.
>>
>> It would require an I/O scheduler that implements .requeue_request but
>> not .prepare_request, which doesn't exist and also is rather pointless as
>> this .requeue_request method would never get called in the current code.
>>
>> So no, no behavior change in practice.
> 
> Fair enough, just found that all three schedulers have implemented
> e->type->ops.prepare_request.

We should probably make this requirement explicit though, seems
very fragile to depend on it just because it's the status quo.

-- 
Jens Axboe

Re: keep passthrough request out of the I/O schedulers

[Jens Axboe]

On 5/17/23 11:30 PM, Christoph Hellwig wrote:
> Hi Jens,
> 
> this is my respin of Ming's "blk-mq: handle passthrough request as really
> passthrough" series.  The first patch is a slightly tweaked version of
> Ming's first patch, while the 2 others are new based on the discussion.
> 
> This isn't meant to shut down the discussion on wether to use scheduler
> tags for passthrough or not, but I'd like to see the bug fixed (and
> a series I have that needs it unblocked).

I think the series stands fine on its own.

-- 
Jens Axboe

Re: [PATCH 1/3] blk-mq: don't queue plugged passthrough requests into scheduler

[Jens Axboe]

On Thu, 18 May 2023 07:30:59 +0200, Christoph Hellwig wrote:
> Passthrough) request should never be queued to the I/O scheduler,
> as scheduling these opaque requests doens't make sense, and I/O
> schedulers might required req->bio to be always valid.
> 
> We never let passthrough request cross scheduler before commit
> 1c2d2fff6dc0 ("block: wire-up support for passthrough plugging"),
> restored this behavior even for passthrough requests issued under
> a plug.
> 
> [...]

Applied, thanks!

[1/3] blk-mq: don't queue plugged passthrough requests into scheduler
      commit: d97217e7f024bbe9aa62aea070771234c2879358
[2/3] blk-mq: remove RQF_ELVPRIV
      commit: fdcab6cddef24a26b86d798814b3c25057e53c21
[3/3] blk-mq: make sure elevator callbacks aren't called for passthrough request
      commit: 59f86a9c69ad379650839b41bb01be213bfac9e3

Best regards,
-- 
Jens Axboe