[PATCH] blk-mq: setup blk_mq_alloc_data.cmd_flags after submit_bio_checks() is done

[PATCH] blk-mq: setup blk_mq_alloc_data.cmd_flags after submit_bio_checks() is done

[Ming Lei]

submit_bio_checks() may update bio->bi_opf, so we have to initialize
blk_mq_alloc_data.cmd_flags after submit_bio_checks() returns when
allocating new requests.

In case of using cached request, fallback to allocate new request if
cached rq's cmd_flags isn't same with bio->bi_opf after
submit_bio_checks().

Fixes: 900e080752025f00 ("block: move queue enter logic into blk_mq_submit_bio()")
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index f511db395c7f..f84044c8de3f 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2517,7 +2517,6 @@ static struct request *blk_mq_get_new_requests(struct request_queue *q,
 	struct blk_mq_alloc_data data = {
 		.q		= q,
 		.nr_tags	= 1,
-		.cmd_flags	= bio->bi_opf,
 	};
 	struct request *rq;
 
@@ -2525,6 +2524,7 @@ static struct request *blk_mq_get_new_requests(struct request_queue *q,
 		return NULL;
 	if (unlikely(!submit_bio_checks(bio)))
 		goto put_exit;
+	data.cmd_flags	= bio->bi_opf;
 	if (blk_mq_attempt_bio_merge(q, bio, nsegs, same_queue_rq))
 		goto put_exit;
 
@@ -2564,13 +2564,15 @@ static inline struct request *blk_mq_get_request(struct request_queue *q,
 			if (blk_mq_attempt_bio_merge(q, bio, nsegs,
 						same_queue_rq))
 				return NULL;
+			if (bio->bi_opf != rq->cmd_flags)
+				goto fallback;
 			plug->cached_rq = rq_list_next(rq);
 			INIT_LIST_HEAD(&rq->queuelist);
 			rq_qos_throttle(q, bio);
 			return rq;
 		}
 	}
-
+fallback:
 	return blk_mq_get_new_requests(q, plug, bio, nsegs, same_queue_rq);
 }
 
-- 
2.31.1

Re: [PATCH] blk-mq: setup blk_mq_alloc_data.cmd_flags after submit_bio_checks() is done

[Christoph Hellwig]

On Fri, Nov 12, 2021 at 04:11:37PM +0800, Ming Lei wrote:
> @@ -2564,13 +2564,15 @@ static inline struct request *blk_mq_get_request(struct request_queue *q,
>  			if (blk_mq_attempt_bio_merge(q, bio, nsegs,
>  						same_queue_rq))
>  				return NULL;
> +			if (bio->bi_opf != rq->cmd_flags)
> +				goto fallback;

I think this deserves a comment, as this means a read prealloc
can only be used for reads, and no fua can be set if the preallocating
I/O didn't use fua, etc.

What are the pitfalls of just chanigng cmd_flags?

Re: [PATCH] blk-mq: setup blk_mq_alloc_data.cmd_flags after submit_bio_checks() is done

[Ming Lei]

On Fri, Nov 12, 2021 at 09:21:40AM +0100, Christoph Hellwig wrote:
> On Fri, Nov 12, 2021 at 04:11:37PM +0800, Ming Lei wrote:
> > @@ -2564,13 +2564,15 @@ static inline struct request *blk_mq_get_request(struct request_queue *q,
> >  			if (blk_mq_attempt_bio_merge(q, bio, nsegs,
> >  						same_queue_rq))
> >  				return NULL;
> > +			if (bio->bi_opf != rq->cmd_flags)
> > +				goto fallback;
> 
> I think this deserves a comment, as this means a read prealloc

OK.

> can only be used for reads, and no fua can be set if the preallocating
> I/O didn't use fua, etc.
> 
> What are the pitfalls of just chanigng cmd_flags?

Then we need to check cmd_flags carefully, such as hctx->type has to
be same, flush & passthrough flags has to be same, that said all
->cmd_flags used for allocating rqs have to be same with the following
bio->bi_opf.

In usual cases, I guess all IOs submitted from same plug batch should be
same type. If not, we can switch to change cmd_flags.


Thanks,
Ming

Re: [PATCH] blk-mq: setup blk_mq_alloc_data.cmd_flags after submit_bio_checks() is done

[Christoph Hellwig]

On Fri, Nov 12, 2021 at 04:37:19PM +0800, Ming Lei wrote:
> > can only be used for reads, and no fua can be set if the preallocating
> > I/O didn't use fua, etc.
> > 
> > What are the pitfalls of just chanigng cmd_flags?
> 
> Then we need to check cmd_flags carefully, such as hctx->type has to
> be same, flush & passthrough flags has to be same, that said all
> ->cmd_flags used for allocating rqs have to be same with the following
> bio->bi_opf.
> 
> In usual cases, I guess all IOs submitted from same plug batch should be
> same type. If not, we can switch to change cmd_flags.

Jens: is this a limit fitting into your use cases?

I guess as a quick fix this rejecting different flags is probably the
best we can do for now, but I suspect we'll want to eventually relax
them.

Re: [PATCH] blk-mq: setup blk_mq_alloc_data.cmd_flags after submit_bio_checks() is done

[Ming Lei]

On Fri, Nov 12, 2021 at 09:44:41AM +0100, Christoph Hellwig wrote:
> On Fri, Nov 12, 2021 at 04:37:19PM +0800, Ming Lei wrote:
> > > can only be used for reads, and no fua can be set if the preallocating
> > > I/O didn't use fua, etc.
> > > 
> > > What are the pitfalls of just chanigng cmd_flags?
> > 
> > Then we need to check cmd_flags carefully, such as hctx->type has to
> > be same, flush & passthrough flags has to be same, that said all
> > ->cmd_flags used for allocating rqs have to be same with the following
> > bio->bi_opf.
> > 
> > In usual cases, I guess all IOs submitted from same plug batch should be
> > same type. If not, we can switch to change cmd_flags.
> 
> Jens: is this a limit fitting into your use cases?
> 
> I guess as a quick fix this rejecting different flags is probably the
> best we can do for now, but I suspect we'll want to eventually relax
> them.

rw mixed workload will be affected, so I think we need to switch to
change cmd_flags, how about the following patch?

From 9ab77b7adee768272944c20b7cffc8abdb85a35b Mon Sep 17 00:00:00 2001
From: Ming Lei <ming.lei@redhat.com>
Date: Fri, 12 Nov 2021 08:14:38 +0800
Subject: [PATCH] blk-mq: fix filesystem I/O request allocation

submit_bio_checks() may update bio->bi_opf, so we have to initialize
blk_mq_alloc_data.cmd_flags with bio->bi_opf after submit_bio_checks()
returns when allocating new request.

In case of using cached request, fallback to allocate new request if
cached rq isn't compatible with the incoming bio, otherwise change
rq->cmd_flags with incoming bio->bi_opf.

Fixes: 900e080752025f00 ("block: move queue enter logic into blk_mq_submit_bio()")
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq.c | 39 ++++++++++++++++++++++++++++++---------
 block/blk-mq.h | 26 +++++++++++++++-----------
 2 files changed, 45 insertions(+), 20 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index f511db395c7f..3ab34c4f20da 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2521,12 +2521,8 @@ static struct request *blk_mq_get_new_requests(struct request_queue *q,
 	};
 	struct request *rq;
 
-	if (unlikely(bio_queue_enter(bio)))
-		return NULL;
-	if (unlikely(!submit_bio_checks(bio)))
-		goto put_exit;
 	if (blk_mq_attempt_bio_merge(q, bio, nsegs, same_queue_rq))
-		goto put_exit;
+		return NULL;
 
 	rq_qos_throttle(q, bio);
 
@@ -2543,19 +2539,32 @@ static struct request *blk_mq_get_new_requests(struct request_queue *q,
 	rq_qos_cleanup(q, bio);
 	if (bio->bi_opf & REQ_NOWAIT)
 		bio_wouldblock_error(bio);
-put_exit:
-	blk_queue_exit(q);
+
 	return NULL;
 }
 
+static inline bool blk_mq_can_use_cached_rq(struct request *rq,
+		struct bio *bio)
+{
+	if (blk_mq_get_hctx_type(bio->bi_opf) != rq->mq_hctx->type)
+		return false;
+
+	if (op_is_flush(rq->cmd_flags) != op_is_flush(bio->bi_opf))
+		return false;
+
+	return true;
+}
+
 static inline struct request *blk_mq_get_request(struct request_queue *q,
 						 struct blk_plug *plug,
 						 struct bio *bio,
 						 unsigned int nsegs,
 						 bool *same_queue_rq)
 {
+	struct request *rq;
+	bool checked = false;
+
 	if (plug) {
-		struct request *rq;
 
 		rq = rq_list_peek(&plug->cached_rq);
 		if (rq && rq->q == q) {
@@ -2564,6 +2573,10 @@ static inline struct request *blk_mq_get_request(struct request_queue *q,
 			if (blk_mq_attempt_bio_merge(q, bio, nsegs,
 						same_queue_rq))
 				return NULL;
+			checked = true;
+			if (!blk_mq_can_use_cached_rq(rq, bio))
+				goto fallback;
+			rq->cmd_flags = bio->bi_opf;
 			plug->cached_rq = rq_list_next(rq);
 			INIT_LIST_HEAD(&rq->queuelist);
 			rq_qos_throttle(q, bio);
@@ -2571,7 +2584,15 @@ static inline struct request *blk_mq_get_request(struct request_queue *q,
 		}
 	}
 
-	return blk_mq_get_new_requests(q, plug, bio, nsegs, same_queue_rq);
+fallback:
+	if (unlikely(bio_queue_enter(bio)))
+		return NULL;
+	if (!checked && !submit_bio_checks(bio))
+		return NULL;
+	rq = blk_mq_get_new_requests(q, plug, bio, nsegs, same_queue_rq);
+	if (!rq)
+		blk_queue_exit(q);
+	return rq;
 }
 
 /**
diff --git a/block/blk-mq.h b/block/blk-mq.h
index cb0b5482ca5e..4d92fc1124ae 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -89,15 +89,7 @@ static inline struct blk_mq_hw_ctx *blk_mq_map_queue_type(struct request_queue *
 	return q->queue_hw_ctx[q->tag_set->map[type].mq_map[cpu]];
 }
 
-/*
- * blk_mq_map_queue() - map (cmd_flags,type) to hardware queue
- * @q: request queue
- * @flags: request command flags
- * @ctx: software queue cpu ctx
- */
-static inline struct blk_mq_hw_ctx *blk_mq_map_queue(struct request_queue *q,
-						     unsigned int flags,
-						     struct blk_mq_ctx *ctx)
+static inline enum hctx_type blk_mq_get_hctx_type(unsigned int flags)
 {
 	enum hctx_type type = HCTX_TYPE_DEFAULT;
 
@@ -108,8 +100,20 @@ static inline struct blk_mq_hw_ctx *blk_mq_map_queue(struct request_queue *q,
 		type = HCTX_TYPE_POLL;
 	else if ((flags & REQ_OP_MASK) == REQ_OP_READ)
 		type = HCTX_TYPE_READ;
-	
-	return ctx->hctxs[type];
+	return type;
+}
+
+/*
+ * blk_mq_map_queue() - map (cmd_flags,type) to hardware queue
+ * @q: request queue
+ * @flags: request command flags
+ * @ctx: software queue cpu ctx
+ */
+static inline struct blk_mq_hw_ctx *blk_mq_map_queue(struct request_queue *q,
+						     unsigned int flags,
+						     struct blk_mq_ctx *ctx)
+{
+	return ctx->hctxs[blk_mq_get_hctx_type(flags)];
 }
 
 /*
-- 
2.31.1


-- 
Ming

Re: [PATCH] blk-mq: setup blk_mq_alloc_data.cmd_flags after submit_bio_checks() is done

[Geert Uytterhoeven]

Hi Ming,

On Fri, Nov 12, 2021 at 1:47 PM Ming Lei <ming.lei@redhat.com> wrote:
> On Fri, Nov 12, 2021 at 09:44:41AM +0100, Christoph Hellwig wrote:
> > On Fri, Nov 12, 2021 at 04:37:19PM +0800, Ming Lei wrote:
> > > > can only be used for reads, and no fua can be set if the preallocating
> > > > I/O didn't use fua, etc.
> > > >
> > > > What are the pitfalls of just chanigng cmd_flags?
> > >
> > > Then we need to check cmd_flags carefully, such as hctx->type has to
> > > be same, flush & passthrough flags has to be same, that said all
> > > ->cmd_flags used for allocating rqs have to be same with the following
> > > bio->bi_opf.
> > >
> > > In usual cases, I guess all IOs submitted from same plug batch should be
> > > same type. If not, we can switch to change cmd_flags.
> >
> > Jens: is this a limit fitting into your use cases?
> >
> > I guess as a quick fix this rejecting different flags is probably the
> > best we can do for now, but I suspect we'll want to eventually relax
> > them.
>
> rw mixed workload will be affected, so I think we need to switch to
> change cmd_flags, how about the following patch?
>
> From 9ab77b7adee768272944c20b7cffc8abdb85a35b Mon Sep 17 00:00:00 2001
> From: Ming Lei <ming.lei@redhat.com>
> Date: Fri, 12 Nov 2021 08:14:38 +0800
> Subject: [PATCH] blk-mq: fix filesystem I/O request allocation
>
> submit_bio_checks() may update bio->bi_opf, so we have to initialize
> blk_mq_alloc_data.cmd_flags with bio->bi_opf after submit_bio_checks()
> returns when allocating new request.
>
> In case of using cached request, fallback to allocate new request if
> cached rq isn't compatible with the incoming bio, otherwise change
> rq->cmd_flags with incoming bio->bi_opf.
>
> Fixes: 900e080752025f00 ("block: move queue enter logic into blk_mq_submit_bio()")
> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Thanks, this alternative patch seems to work fine, too.
Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] blk-mq: setup blk_mq_alloc_data.cmd_flags after submit_bio_checks() is done

[Jens Axboe]

On 11/12/21 1:44 AM, Christoph Hellwig wrote:
> On Fri, Nov 12, 2021 at 04:37:19PM +0800, Ming Lei wrote:
>>> can only be used for reads, and no fua can be set if the preallocating
>>> I/O didn't use fua, etc.
>>>
>>> What are the pitfalls of just chanigng cmd_flags?
>>
>> Then we need to check cmd_flags carefully, such as hctx->type has to
>> be same, flush & passthrough flags has to be same, that said all
>> ->cmd_flags used for allocating rqs have to be same with the following
>> bio->bi_opf.
>>
>> In usual cases, I guess all IOs submitted from same plug batch should be
>> same type. If not, we can switch to change cmd_flags.
> 
> Jens: is this a limit fitting into your use cases?

It's not a big problem for my use case, as that ones all the same type
of IO. And in general I don't expect a lot of mixed uses here, I guess
the main issue might be a plug for writes and an initial meta data read
will "polute" the request cache with requests that we won't be using.
But probably not a big enough thing to worry about in terms of
efficiency.

> I guess as a quick fix this rejecting different flags is probably the
> best we can do for now, but I suspect we'll want to eventually relax
> them.

Agree on both.

-- 
Jens Axboe

Re: [PATCH] blk-mq: setup blk_mq_alloc_data.cmd_flags after submit_bio_checks() is done

[Jens Axboe]

On 11/12/21 5:47 AM, Ming Lei wrote:
> On Fri, Nov 12, 2021 at 09:44:41AM +0100, Christoph Hellwig wrote:
>> On Fri, Nov 12, 2021 at 04:37:19PM +0800, Ming Lei wrote:
>>>> can only be used for reads, and no fua can be set if the preallocating
>>>> I/O didn't use fua, etc.
>>>>
>>>> What are the pitfalls of just chanigng cmd_flags?
>>>
>>> Then we need to check cmd_flags carefully, such as hctx->type has to
>>> be same, flush & passthrough flags has to be same, that said all
>>> ->cmd_flags used for allocating rqs have to be same with the following
>>> bio->bi_opf.
>>>
>>> In usual cases, I guess all IOs submitted from same plug batch should be
>>> same type. If not, we can switch to change cmd_flags.
>>
>> Jens: is this a limit fitting into your use cases?
>>
>> I guess as a quick fix this rejecting different flags is probably the
>> best we can do for now, but I suspect we'll want to eventually relax
>> them.
> 
> rw mixed workload will be affected, so I think we need to switch to
> change cmd_flags, how about the following patch?
> 
> From 9ab77b7adee768272944c20b7cffc8abdb85a35b Mon Sep 17 00:00:00 2001
> From: Ming Lei <ming.lei@redhat.com>
> Date: Fri, 12 Nov 2021 08:14:38 +0800
> Subject: [PATCH] blk-mq: fix filesystem I/O request allocation
> 
> submit_bio_checks() may update bio->bi_opf, so we have to initialize
> blk_mq_alloc_data.cmd_flags with bio->bi_opf after submit_bio_checks()
> returns when allocating new request.
> 
> In case of using cached request, fallback to allocate new request if
> cached rq isn't compatible with the incoming bio, otherwise change
> rq->cmd_flags with incoming bio->bi_opf.
> 
> Fixes: 900e080752025f00 ("block: move queue enter logic into blk_mq_submit_bio()")
> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-mq.c | 39 ++++++++++++++++++++++++++++++---------
>  block/blk-mq.h | 26 +++++++++++++++-----------
>  2 files changed, 45 insertions(+), 20 deletions(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index f511db395c7f..3ab34c4f20da 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2521,12 +2521,8 @@ static struct request *blk_mq_get_new_requests(struct request_queue *q,
>  	};
>  	struct request *rq;
>  
> -	if (unlikely(bio_queue_enter(bio)))
> -		return NULL;
> -	if (unlikely(!submit_bio_checks(bio)))
> -		goto put_exit;
>  	if (blk_mq_attempt_bio_merge(q, bio, nsegs, same_queue_rq))
> -		goto put_exit;
> +		return NULL;
>  
>  	rq_qos_throttle(q, bio);
>  
> @@ -2543,19 +2539,32 @@ static struct request *blk_mq_get_new_requests(struct request_queue *q,
>  	rq_qos_cleanup(q, bio);
>  	if (bio->bi_opf & REQ_NOWAIT)
>  		bio_wouldblock_error(bio);
> -put_exit:
> -	blk_queue_exit(q);
> +
>  	return NULL;
>  }
>  
> +static inline bool blk_mq_can_use_cached_rq(struct request *rq,
> +		struct bio *bio)
> +{
> +	if (blk_mq_get_hctx_type(bio->bi_opf) != rq->mq_hctx->type)
> +		return false;
> +
> +	if (op_is_flush(rq->cmd_flags) != op_is_flush(bio->bi_opf))
> +		return false;
> +
> +	return true;

I think we should just check if hctx is the same, that should be enough.
We don't need to match the type, just disallow if hw queue has changed.


-- 
Jens Axboe

Re: [PATCH] blk-mq: setup blk_mq_alloc_data.cmd_flags after submit_bio_checks() is done

[Ming Lei]

On Fri, Nov 12, 2021 at 08:47:01AM -0700, Jens Axboe wrote:
> On 11/12/21 5:47 AM, Ming Lei wrote:
> > On Fri, Nov 12, 2021 at 09:44:41AM +0100, Christoph Hellwig wrote:
> >> On Fri, Nov 12, 2021 at 04:37:19PM +0800, Ming Lei wrote:
> >>>> can only be used for reads, and no fua can be set if the preallocating
> >>>> I/O didn't use fua, etc.
> >>>>
> >>>> What are the pitfalls of just chanigng cmd_flags?
> >>>
> >>> Then we need to check cmd_flags carefully, such as hctx->type has to
> >>> be same, flush & passthrough flags has to be same, that said all
> >>> ->cmd_flags used for allocating rqs have to be same with the following
> >>> bio->bi_opf.
> >>>
> >>> In usual cases, I guess all IOs submitted from same plug batch should be
> >>> same type. If not, we can switch to change cmd_flags.
> >>
> >> Jens: is this a limit fitting into your use cases?
> >>
> >> I guess as a quick fix this rejecting different flags is probably the
> >> best we can do for now, but I suspect we'll want to eventually relax
> >> them.
> > 
> > rw mixed workload will be affected, so I think we need to switch to
> > change cmd_flags, how about the following patch?
> > 
> > From 9ab77b7adee768272944c20b7cffc8abdb85a35b Mon Sep 17 00:00:00 2001
> > From: Ming Lei <ming.lei@redhat.com>
> > Date: Fri, 12 Nov 2021 08:14:38 +0800
> > Subject: [PATCH] blk-mq: fix filesystem I/O request allocation
> > 
> > submit_bio_checks() may update bio->bi_opf, so we have to initialize
> > blk_mq_alloc_data.cmd_flags with bio->bi_opf after submit_bio_checks()
> > returns when allocating new request.
> > 
> > In case of using cached request, fallback to allocate new request if
> > cached rq isn't compatible with the incoming bio, otherwise change
> > rq->cmd_flags with incoming bio->bi_opf.
> > 
> > Fixes: 900e080752025f00 ("block: move queue enter logic into blk_mq_submit_bio()")
> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  block/blk-mq.c | 39 ++++++++++++++++++++++++++++++---------
> >  block/blk-mq.h | 26 +++++++++++++++-----------
> >  2 files changed, 45 insertions(+), 20 deletions(-)
> > 
> > diff --git a/block/blk-mq.c b/block/blk-mq.c
> > index f511db395c7f..3ab34c4f20da 100644
> > --- a/block/blk-mq.c
> > +++ b/block/blk-mq.c
> > @@ -2521,12 +2521,8 @@ static struct request *blk_mq_get_new_requests(struct request_queue *q,
> >  	};
> >  	struct request *rq;
> >  
> > -	if (unlikely(bio_queue_enter(bio)))
> > -		return NULL;
> > -	if (unlikely(!submit_bio_checks(bio)))
> > -		goto put_exit;
> >  	if (blk_mq_attempt_bio_merge(q, bio, nsegs, same_queue_rq))
> > -		goto put_exit;
> > +		return NULL;
> >  
> >  	rq_qos_throttle(q, bio);
> >  
> > @@ -2543,19 +2539,32 @@ static struct request *blk_mq_get_new_requests(struct request_queue *q,
> >  	rq_qos_cleanup(q, bio);
> >  	if (bio->bi_opf & REQ_NOWAIT)
> >  		bio_wouldblock_error(bio);
> > -put_exit:
> > -	blk_queue_exit(q);
> > +
> >  	return NULL;
> >  }
> >  
> > +static inline bool blk_mq_can_use_cached_rq(struct request *rq,
> > +		struct bio *bio)
> > +{
> > +	if (blk_mq_get_hctx_type(bio->bi_opf) != rq->mq_hctx->type)
> > +		return false;
> > +
> > +	if (op_is_flush(rq->cmd_flags) != op_is_flush(bio->bi_opf))
> > +		return false;
> > +
> > +	return true;
> 
> I think we should just check if hctx is the same, that should be enough.
> We don't need to match the type, just disallow if hw queue has changed.

But bio doesn't have hw queue. Figuring out exact hw queue seems
necessary and needs more cpu cycles than getting hctx type.


Thanks,
Ming

Re: [PATCH] blk-mq: setup blk_mq_alloc_data.cmd_flags after submit_bio_checks() is done

[Jens Axboe]

On 11/12/21 9:05 AM, Ming Lei wrote:
> On Fri, Nov 12, 2021 at 08:47:01AM -0700, Jens Axboe wrote:
>> On 11/12/21 5:47 AM, Ming Lei wrote:
>>> On Fri, Nov 12, 2021 at 09:44:41AM +0100, Christoph Hellwig wrote:
>>>> On Fri, Nov 12, 2021 at 04:37:19PM +0800, Ming Lei wrote:
>>>>>> can only be used for reads, and no fua can be set if the preallocating
>>>>>> I/O didn't use fua, etc.
>>>>>>
>>>>>> What are the pitfalls of just chanigng cmd_flags?
>>>>>
>>>>> Then we need to check cmd_flags carefully, such as hctx->type has to
>>>>> be same, flush & passthrough flags has to be same, that said all
>>>>> ->cmd_flags used for allocating rqs have to be same with the following
>>>>> bio->bi_opf.
>>>>>
>>>>> In usual cases, I guess all IOs submitted from same plug batch should be
>>>>> same type. If not, we can switch to change cmd_flags.
>>>>
>>>> Jens: is this a limit fitting into your use cases?
>>>>
>>>> I guess as a quick fix this rejecting different flags is probably the
>>>> best we can do for now, but I suspect we'll want to eventually relax
>>>> them.
>>>
>>> rw mixed workload will be affected, so I think we need to switch to
>>> change cmd_flags, how about the following patch?
>>>
>>> From 9ab77b7adee768272944c20b7cffc8abdb85a35b Mon Sep 17 00:00:00 2001
>>> From: Ming Lei <ming.lei@redhat.com>
>>> Date: Fri, 12 Nov 2021 08:14:38 +0800
>>> Subject: [PATCH] blk-mq: fix filesystem I/O request allocation
>>>
>>> submit_bio_checks() may update bio->bi_opf, so we have to initialize
>>> blk_mq_alloc_data.cmd_flags with bio->bi_opf after submit_bio_checks()
>>> returns when allocating new request.
>>>
>>> In case of using cached request, fallback to allocate new request if
>>> cached rq isn't compatible with the incoming bio, otherwise change
>>> rq->cmd_flags with incoming bio->bi_opf.
>>>
>>> Fixes: 900e080752025f00 ("block: move queue enter logic into blk_mq_submit_bio()")
>>> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
>>> Cc: Christoph Hellwig <hch@lst.de>
>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>>> ---
>>>  block/blk-mq.c | 39 ++++++++++++++++++++++++++++++---------
>>>  block/blk-mq.h | 26 +++++++++++++++-----------
>>>  2 files changed, 45 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>> index f511db395c7f..3ab34c4f20da 100644
>>> --- a/block/blk-mq.c
>>> +++ b/block/blk-mq.c
>>> @@ -2521,12 +2521,8 @@ static struct request *blk_mq_get_new_requests(struct request_queue *q,
>>>  	};
>>>  	struct request *rq;
>>>  
>>> -	if (unlikely(bio_queue_enter(bio)))
>>> -		return NULL;
>>> -	if (unlikely(!submit_bio_checks(bio)))
>>> -		goto put_exit;
>>>  	if (blk_mq_attempt_bio_merge(q, bio, nsegs, same_queue_rq))
>>> -		goto put_exit;
>>> +		return NULL;
>>>  
>>>  	rq_qos_throttle(q, bio);
>>>  
>>> @@ -2543,19 +2539,32 @@ static struct request *blk_mq_get_new_requests(struct request_queue *q,
>>>  	rq_qos_cleanup(q, bio);
>>>  	if (bio->bi_opf & REQ_NOWAIT)
>>>  		bio_wouldblock_error(bio);
>>> -put_exit:
>>> -	blk_queue_exit(q);
>>> +
>>>  	return NULL;
>>>  }
>>>  
>>> +static inline bool blk_mq_can_use_cached_rq(struct request *rq,
>>> +		struct bio *bio)
>>> +{
>>> +	if (blk_mq_get_hctx_type(bio->bi_opf) != rq->mq_hctx->type)
>>> +		return false;
>>> +
>>> +	if (op_is_flush(rq->cmd_flags) != op_is_flush(bio->bi_opf))
>>> +		return false;
>>> +
>>> +	return true;
>>
>> I think we should just check if hctx is the same, that should be enough.
>> We don't need to match the type, just disallow if hw queue has changed.
> 
> But bio doesn't have hw queue. Figuring out exact hw queue seems
> necessary and needs more cpu cycles than getting hctx type.

Thinking about it, if opf and request_queue matches, that should be
enough.

-- 
Jens Axboe

Re: [PATCH] blk-mq: setup blk_mq_alloc_data.cmd_flags after submit_bio_checks() is done

[Ming Lei]

On Fri, Nov 12, 2021 at 09:08:39AM -0700, Jens Axboe wrote:
> On 11/12/21 9:05 AM, Ming Lei wrote:
> > On Fri, Nov 12, 2021 at 08:47:01AM -0700, Jens Axboe wrote:
> >> On 11/12/21 5:47 AM, Ming Lei wrote:
> >>> On Fri, Nov 12, 2021 at 09:44:41AM +0100, Christoph Hellwig wrote:
> >>>> On Fri, Nov 12, 2021 at 04:37:19PM +0800, Ming Lei wrote:
> >>>>>> can only be used for reads, and no fua can be set if the preallocating
> >>>>>> I/O didn't use fua, etc.
> >>>>>>
> >>>>>> What are the pitfalls of just chanigng cmd_flags?
> >>>>>
> >>>>> Then we need to check cmd_flags carefully, such as hctx->type has to
> >>>>> be same, flush & passthrough flags has to be same, that said all
> >>>>> ->cmd_flags used for allocating rqs have to be same with the following
> >>>>> bio->bi_opf.
> >>>>>
> >>>>> In usual cases, I guess all IOs submitted from same plug batch should be
> >>>>> same type. If not, we can switch to change cmd_flags.
> >>>>
> >>>> Jens: is this a limit fitting into your use cases?
> >>>>
> >>>> I guess as a quick fix this rejecting different flags is probably the
> >>>> best we can do for now, but I suspect we'll want to eventually relax
> >>>> them.
> >>>
> >>> rw mixed workload will be affected, so I think we need to switch to
> >>> change cmd_flags, how about the following patch?
> >>>
> >>> From 9ab77b7adee768272944c20b7cffc8abdb85a35b Mon Sep 17 00:00:00 2001
> >>> From: Ming Lei <ming.lei@redhat.com>
> >>> Date: Fri, 12 Nov 2021 08:14:38 +0800
> >>> Subject: [PATCH] blk-mq: fix filesystem I/O request allocation
> >>>
> >>> submit_bio_checks() may update bio->bi_opf, so we have to initialize
> >>> blk_mq_alloc_data.cmd_flags with bio->bi_opf after submit_bio_checks()
> >>> returns when allocating new request.
> >>>
> >>> In case of using cached request, fallback to allocate new request if
> >>> cached rq isn't compatible with the incoming bio, otherwise change
> >>> rq->cmd_flags with incoming bio->bi_opf.
> >>>
> >>> Fixes: 900e080752025f00 ("block: move queue enter logic into blk_mq_submit_bio()")
> >>> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> >>> Cc: Christoph Hellwig <hch@lst.de>
> >>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> >>> ---
> >>>  block/blk-mq.c | 39 ++++++++++++++++++++++++++++++---------
> >>>  block/blk-mq.h | 26 +++++++++++++++-----------
> >>>  2 files changed, 45 insertions(+), 20 deletions(-)
> >>>
> >>> diff --git a/block/blk-mq.c b/block/blk-mq.c
> >>> index f511db395c7f..3ab34c4f20da 100644
> >>> --- a/block/blk-mq.c
> >>> +++ b/block/blk-mq.c
> >>> @@ -2521,12 +2521,8 @@ static struct request *blk_mq_get_new_requests(struct request_queue *q,
> >>>  	};
> >>>  	struct request *rq;
> >>>  
> >>> -	if (unlikely(bio_queue_enter(bio)))
> >>> -		return NULL;
> >>> -	if (unlikely(!submit_bio_checks(bio)))
> >>> -		goto put_exit;
> >>>  	if (blk_mq_attempt_bio_merge(q, bio, nsegs, same_queue_rq))
> >>> -		goto put_exit;
> >>> +		return NULL;
> >>>  
> >>>  	rq_qos_throttle(q, bio);
> >>>  
> >>> @@ -2543,19 +2539,32 @@ static struct request *blk_mq_get_new_requests(struct request_queue *q,
> >>>  	rq_qos_cleanup(q, bio);
> >>>  	if (bio->bi_opf & REQ_NOWAIT)
> >>>  		bio_wouldblock_error(bio);
> >>> -put_exit:
> >>> -	blk_queue_exit(q);
> >>> +
> >>>  	return NULL;
> >>>  }
> >>>  
> >>> +static inline bool blk_mq_can_use_cached_rq(struct request *rq,
> >>> +		struct bio *bio)
> >>> +{
> >>> +	if (blk_mq_get_hctx_type(bio->bi_opf) != rq->mq_hctx->type)
> >>> +		return false;
> >>> +
> >>> +	if (op_is_flush(rq->cmd_flags) != op_is_flush(bio->bi_opf))
> >>> +		return false;
> >>> +
> >>> +	return true;
> >>
> >> I think we should just check if hctx is the same, that should be enough.
> >> We don't need to match the type, just disallow if hw queue has changed.
> > 
> > But bio doesn't have hw queue. Figuring out exact hw queue seems
> > necessary and needs more cpu cycles than getting hctx type.
> 
> Thinking about it, if opf and request_queue matches, that should be
> enough.

I think that is same with hctx->type check: POLLED & OP needs to be
same between the request and bio, and op_is_flush(), or could you
explain how to run the exact check on opf?


Thanks,
Ming

Re: [PATCH] blk-mq: setup blk_mq_alloc_data.cmd_flags after submit_bio_checks() is done

[Jens Axboe]

On 11/12/21 9:17 AM, Ming Lei wrote:
> On Fri, Nov 12, 2021 at 09:08:39AM -0700, Jens Axboe wrote:
>> On 11/12/21 9:05 AM, Ming Lei wrote:
>>> On Fri, Nov 12, 2021 at 08:47:01AM -0700, Jens Axboe wrote:
>>>> On 11/12/21 5:47 AM, Ming Lei wrote:
>>>>> On Fri, Nov 12, 2021 at 09:44:41AM +0100, Christoph Hellwig wrote:
>>>>>> On Fri, Nov 12, 2021 at 04:37:19PM +0800, Ming Lei wrote:
>>>>>>>> can only be used for reads, and no fua can be set if the preallocating
>>>>>>>> I/O didn't use fua, etc.
>>>>>>>>
>>>>>>>> What are the pitfalls of just chanigng cmd_flags?
>>>>>>>
>>>>>>> Then we need to check cmd_flags carefully, such as hctx->type has to
>>>>>>> be same, flush & passthrough flags has to be same, that said all
>>>>>>> ->cmd_flags used for allocating rqs have to be same with the following
>>>>>>> bio->bi_opf.
>>>>>>>
>>>>>>> In usual cases, I guess all IOs submitted from same plug batch should be
>>>>>>> same type. If not, we can switch to change cmd_flags.
>>>>>>
>>>>>> Jens: is this a limit fitting into your use cases?
>>>>>>
>>>>>> I guess as a quick fix this rejecting different flags is probably the
>>>>>> best we can do for now, but I suspect we'll want to eventually relax
>>>>>> them.
>>>>>
>>>>> rw mixed workload will be affected, so I think we need to switch to
>>>>> change cmd_flags, how about the following patch?
>>>>>
>>>>> From 9ab77b7adee768272944c20b7cffc8abdb85a35b Mon Sep 17 00:00:00 2001
>>>>> From: Ming Lei <ming.lei@redhat.com>
>>>>> Date: Fri, 12 Nov 2021 08:14:38 +0800
>>>>> Subject: [PATCH] blk-mq: fix filesystem I/O request allocation
>>>>>
>>>>> submit_bio_checks() may update bio->bi_opf, so we have to initialize
>>>>> blk_mq_alloc_data.cmd_flags with bio->bi_opf after submit_bio_checks()
>>>>> returns when allocating new request.
>>>>>
>>>>> In case of using cached request, fallback to allocate new request if
>>>>> cached rq isn't compatible with the incoming bio, otherwise change
>>>>> rq->cmd_flags with incoming bio->bi_opf.
>>>>>
>>>>> Fixes: 900e080752025f00 ("block: move queue enter logic into blk_mq_submit_bio()")
>>>>> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
>>>>> Cc: Christoph Hellwig <hch@lst.de>
>>>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>>>>> ---
>>>>>  block/blk-mq.c | 39 ++++++++++++++++++++++++++++++---------
>>>>>  block/blk-mq.h | 26 +++++++++++++++-----------
>>>>>  2 files changed, 45 insertions(+), 20 deletions(-)
>>>>>
>>>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>>>> index f511db395c7f..3ab34c4f20da 100644
>>>>> --- a/block/blk-mq.c
>>>>> +++ b/block/blk-mq.c
>>>>> @@ -2521,12 +2521,8 @@ static struct request *blk_mq_get_new_requests(struct request_queue *q,
>>>>>  	};
>>>>>  	struct request *rq;
>>>>>  
>>>>> -	if (unlikely(bio_queue_enter(bio)))
>>>>> -		return NULL;
>>>>> -	if (unlikely(!submit_bio_checks(bio)))
>>>>> -		goto put_exit;
>>>>>  	if (blk_mq_attempt_bio_merge(q, bio, nsegs, same_queue_rq))
>>>>> -		goto put_exit;
>>>>> +		return NULL;
>>>>>  
>>>>>  	rq_qos_throttle(q, bio);
>>>>>  
>>>>> @@ -2543,19 +2539,32 @@ static struct request *blk_mq_get_new_requests(struct request_queue *q,
>>>>>  	rq_qos_cleanup(q, bio);
>>>>>  	if (bio->bi_opf & REQ_NOWAIT)
>>>>>  		bio_wouldblock_error(bio);
>>>>> -put_exit:
>>>>> -	blk_queue_exit(q);
>>>>> +
>>>>>  	return NULL;
>>>>>  }
>>>>>  
>>>>> +static inline bool blk_mq_can_use_cached_rq(struct request *rq,
>>>>> +		struct bio *bio)
>>>>> +{
>>>>> +	if (blk_mq_get_hctx_type(bio->bi_opf) != rq->mq_hctx->type)
>>>>> +		return false;
>>>>> +
>>>>> +	if (op_is_flush(rq->cmd_flags) != op_is_flush(bio->bi_opf))
>>>>> +		return false;
>>>>> +
>>>>> +	return true;
>>>>
>>>> I think we should just check if hctx is the same, that should be enough.
>>>> We don't need to match the type, just disallow if hw queue has changed.
>>>
>>> But bio doesn't have hw queue. Figuring out exact hw queue seems
>>> necessary and needs more cpu cycles than getting hctx type.
>>
>> Thinking about it, if opf and request_queue matches, that should be
>> enough.
> 
> I think that is same with hctx->type check: POLLED & OP needs to be
> same between the request and bio, and op_is_flush(), or could you
> explain how to run the exact check on opf?

I took a look at it, and I think your approach of checking the type is
indeed the best one.

-- 
Jens Axboe