Alloc batch fixes

PATCH Applied Jens Axboe 6 participants 36 messages Nov 4, 2021 – Nov 15, 2021 Block layer

lore

Share & tools

lore

Jens Axboe

author maintainer

2021-11-04 18:22 UTC #0 lore raw

v3 cover Alloc batch fixes

Hi,

Since the new feedback on v1 arrived post v2, here's a v3 that takes it all into account.

A few fixes related to the batched allocations:

- Have the requests hold a queue reference, flush them on schedule unplug as well.

- Make sure the queue matches, could be a mismatch if we're driving multiple devices.

Since v2:
- Split both alloc handlers into helpers
- Retained blk_try_enter_queue() the way that it was, don't want to inline all of bio_queue_enter(). Added justification for that in the commit message.
- Add fops based submit_bio helper
Since v1:
- Reshuffle series to do plug rq alloc helper before enter changes
- Protect submit_bio_checks() by queue enter reference as well

Signature · 1 line

Jens Axboe

Jens Axboe

author maintainer

2021-11-04 18:22 UTC #1 lore raw

PATCH v3 1/5 block: have plug stored requests hold references to the queue

Requests that were stored in the cache deliberately didn't hold an enter reference to the queue, instead we grabbed one every time we pulled a request out of there. That made for awkward logic on freeing the remainder of the cached list, if needed, where we had to artificially raise the queue usage count before each free.

Grab references up front for cached plug requests. That's safer, and also more efficient.

Fixes 47c122e35d7e ("block: pre-allocate requests if plug is started and is a batch")

Signed-off-by Jens Axboe <[email protected]>

---
 block/blk-core.c | 2 +-
 block/blk-mq.c   | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

block/blk-core.c +1 -1

index fd389a16013c..c2d267b6f910 100644
@@ -1643,7 +1643,7 @@ void blk_flush_plug(struct blk_plug *plug, bool from_schedule)
 		flush_plug_callbacks(plug, from_schedule);
 	if (!rq_list_empty(plug->mq_list))
 		blk_mq_flush_plug_list(plug, from_schedule);
-	if (unlikely(!from_schedule && plug->cached_rq))
+	if (unlikely(!rq_list_empty(plug->cached_rq)))
 		blk_mq_free_plug_rqs(plug);
 }

block/blk-mq.c +4 -3

index c68aa0a332e1..5498454c2164 100644
@@ -410,7 +410,10 @@ __blk_mq_alloc_requests_batch(struct blk_mq_alloc_data *data,
 		tag_mask &= ~(1UL << i);
 		rq = blk_mq_rq_ctx_init(data, tags, tag, alloc_time_ns);
 		rq_list_add(data->cached_rq, rq);
+		nr++;
 	}
+	/* caller already holds a reference, add for remainder */
+	percpu_ref_get_many(&data->q->q_usage_counter, nr - 1);
 	data->nr_tags -= nr;
 
 	return rq_list_pop(data->cached_rq);

@@ -630,10 +633,8 @@ void blk_mq_free_plug_rqs(struct blk_plug *plug)
 {
 	struct request *rq;
 
-	while ((rq = rq_list_pop(&plug->cached_rq)) != NULL) {
-		percpu_ref_get(&rq->q->q_usage_counter);
+	while ((rq = rq_list_pop(&plug->cached_rq)) != NULL)
 		blk_mq_free_request(rq);
-	}
 }
 
 static void req_bio_endio(struct request *rq, struct bio *bio,

Signature · 1 line

2.33.1

Christoph Hellwig

Reviewed-by on v3 1/5

2021-11-04 18:34 UTC #2 lore raw

Re: [PATCH 1/5] block: have plug stored requests hold references to the queue

Quoted 8 lines · Jens Axboe

Requests that were stored in the cache deliberately didn't hold an enter reference to the queue, instead we grabbed one every time we pulled a request out of there. That made for awkward logic on freeing the remainder of the cached list, if needed, where we had to artificially raise the queue usage count before each free.
Grab references up front for cached plug requests. That's safer, and also more efficient.

I think it would be useful to add zour explanation why the cached requests should be flushed at schedule time now here.

Otherwise this looks good:

Reviewed-by Christoph Hellwig <[email protected]>

Jens Axboe

author maintainer

2021-11-04 18:35 UTC #3 lore raw

Re: [PATCH 1/5] block: have plug stored requests hold references to the queue

Quoted 10 lines · Christoph Hellwig

Quoted 8 lines · Jens Axboe
Requests that were stored in the cache deliberately didn't hold an enter reference to the queue, instead we grabbed one every time we pulled a request out of there. That made for awkward logic on freeing the remainder of the cached list, if needed, where we had to artificially raise the queue usage count before each free.
Grab references up front for cached plug requests. That's safer, and also more efficient.

Quoted 6 lines

I think it would be useful to add zour explanation why the cached requests should be flushed at schedule time now here.
Otherwise this looks good:
Reviewed-by Christoph Hellwig <[email protected]>

Thanks, I'll add a comment when amending with your reviewed-by.

Signature · 1 line

Jens Axboe

Jens Axboe

author maintainer

2021-11-04 18:22 UTC #4 lore raw

PATCH v3 2/5 block: split request allocation components into helpers

In reply to Jens Axboe

This is in preparation for a fix, but serves as a cleanup as well moving the cached vs regular alloc logic out of blk_mq_submit_bio().

Signed-off-by Jens Axboe <[email protected]>

---
 block/blk-mq.c | 71 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 48 insertions(+), 23 deletions(-)

block/blk-mq.c +48 ~~-23~~

index 5498454c2164..dcb413297a96 100644
@@ -2478,6 +2478,51 @@ static inline unsigned short blk_plug_max_rq_count(struct blk_plug *plug)
 	return BLK_MAX_REQUEST_COUNT;
 }
 
+static struct request *blk_mq_get_new_requests(struct request_queue *q,
+					       struct blk_plug *plug,
+					       struct bio *bio)
+{
+	struct blk_mq_alloc_data data = {
+		.q		= q,
+		.nr_tags	= 1,
+		.cmd_flags	= bio->bi_opf,
+	};
+	struct request *rq;
+
+	if (plug) {
+		data.nr_tags = plug->nr_ios;
+		plug->nr_ios = 1;
+		data.cached_rq = &plug->cached_rq;
+	}
+
+	rq = __blk_mq_alloc_requests(&data);
+	if (rq)
+		return rq;
+
+	rq_qos_cleanup(q, bio);
+	if (bio->bi_opf & REQ_NOWAIT)
+		bio_wouldblock_error(bio);
+	return NULL;
+}
+
+static inline struct request *blk_mq_get_request(struct request_queue *q,
+						 struct blk_plug *plug,
+						 struct bio *bio)
+{
+	if (plug) {
+		struct request *rq;
+
+		rq = rq_list_peek(&plug->cached_rq);
+		if (rq) {
+			plug->cached_rq = rq_list_next(rq);
+			INIT_LIST_HEAD(&rq->queuelist);
+			return rq;
+		}
+	}
+
+	return blk_mq_get_new_requests(q, plug, bio);
+}
+
 /**
  * blk_mq_submit_bio - Create and send a request to block device.
  * @bio: Bio pointer.

@@ -2518,29 +2563,9 @@ void blk_mq_submit_bio(struct bio *bio)
 	rq_qos_throttle(q, bio);
 
 	plug = blk_mq_plug(q, bio);
-	if (plug && plug->cached_rq) {
-		rq = rq_list_pop(&plug->cached_rq);
-		INIT_LIST_HEAD(&rq->queuelist);
-	} else {
-		struct blk_mq_alloc_data data = {
-			.q		= q,
-			.nr_tags	= 1,
-			.cmd_flags	= bio->bi_opf,
-		};
-
-		if (plug) {
-			data.nr_tags = plug->nr_ios;
-			plug->nr_ios = 1;
-			data.cached_rq = &plug->cached_rq;
-		}
-		rq = __blk_mq_alloc_requests(&data);
-		if (unlikely(!rq)) {
-			rq_qos_cleanup(q, bio);
-			if (bio->bi_opf & REQ_NOWAIT)
-				bio_wouldblock_error(bio);
-			goto queue_exit;
-		}
-	}
+	rq = blk_mq_get_request(q, plug, bio);
+	if (unlikely(!rq))
+		goto queue_exit;
 
 	trace_block_getrq(bio);

Signature · 1 line

2.33.1

Christoph Hellwig

Reviewed-by on v3 2/5

2021-11-04 18:35 UTC #5 lore raw

Re: [PATCH 2/5] block: split request allocation components into helpers

Looks good,

Reviewed-by Christoph Hellwig <[email protected]>

Jens Axboe

author maintainer

2021-11-04 18:22 UTC #6 lore raw

PATCH v3 3/5 block: make blk_try_enter_queue() available for blk-mq

In reply to Jens Axboe

Just a prep patch for shifting the queue enter logic. This moves the expected fast path inline, and leaves bio_queue_enter() as an out-of-line function call. We don't want to inline the latter, as it's mostly slow path code.

Signed-off-by Jens Axboe <[email protected]>

---
 block/blk-core.c | 26 +-------------------------
 block/blk.h      | 25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 25 deletions(-)

block/blk-core.c +1 ~~-25~~

index c2d267b6f910..e00f5a2287cc 100644
@@ -386,30 +386,6 @@ void blk_cleanup_queue(struct request_queue *q)
 }
 EXPORT_SYMBOL(blk_cleanup_queue);
 
-static bool blk_try_enter_queue(struct request_queue *q, bool pm)
-{
-	rcu_read_lock();
-	if (!percpu_ref_tryget_live_rcu(&q->q_usage_counter))
-		goto fail;
-
-	/*
-	 * The code that increments the pm_only counter must ensure that the
-	 * counter is globally visible before the queue is unfrozen.
-	 */
-	if (blk_queue_pm_only(q) &&
-	    (!pm || queue_rpm_status(q) == RPM_SUSPENDED))
-		goto fail_put;
-
-	rcu_read_unlock();
-	return true;
-
-fail_put:
-	blk_queue_exit(q);
-fail:
-	rcu_read_unlock();
-	return false;
-}
-
 /**
  * blk_queue_enter() - try to increase q->q_usage_counter
  * @q: request queue pointer

@@ -442,7 +418,7 @@ int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags)
 	return 0;
 }
 
-static inline int bio_queue_enter(struct bio *bio)
+int bio_queue_enter(struct bio *bio)
 {
 	struct request_queue *q = bdev_get_queue(bio->bi_bdev);

block/blk.h +25 -0

index 7afffd548daf..f7371d3b1522 100644
@@ -55,6 +55,31 @@ void blk_free_flush_queue(struct blk_flush_queue *q);
 void blk_freeze_queue(struct request_queue *q);
 void __blk_mq_unfreeze_queue(struct request_queue *q, bool force_atomic);
 void blk_queue_start_drain(struct request_queue *q);
+int bio_queue_enter(struct bio *bio);
+
+static inline bool blk_try_enter_queue(struct request_queue *q, bool pm)
+{
+	rcu_read_lock();
+	if (!percpu_ref_tryget_live_rcu(&q->q_usage_counter))
+		goto fail;
+
+	/*
+	 * The code that increments the pm_only counter must ensure that the
+	 * counter is globally visible before the queue is unfrozen.
+	 */
+	if (blk_queue_pm_only(q) &&
+	    (!pm || queue_rpm_status(q) == RPM_SUSPENDED))
+		goto fail_put;
+
+	rcu_read_unlock();
+	return true;
+
+fail_put:
+	blk_queue_exit(q);
+fail:
+	rcu_read_unlock();
+	return false;
+}
 
 #define BIO_INLINE_VECS 4
 struct bio_vec *bvec_alloc(mempool_t *pool, unsigned short *nr_vecs,

Signature · 1 line

2.33.1

Jens Axboe

author maintainer

2021-11-04 18:22 UTC #7 lore raw

PATCH v3 4/5 block: move queue enter logic into blk_mq_submit_bio()

In reply to Jens Axboe

Retain the old logic for the fops based submit, but for our internal blk_mq_submit_bio(), move the queue entering logic into the core function itself.

We need to be a bit careful if going into the scheduler, as a scheduler or queue mappings can arbitrarily change before we have entered the queue. Have the bio scheduler mapping do that separately, it's a very cheap operation compared to actually doing merging locking and lookups.

Signed-off-by Jens Axboe <[email protected]>

---
 block/blk-core.c     | 25 +++++++++++++------------
 block/blk-mq-sched.c | 13 ++++++++++---
 block/blk-mq.c       | 36 ++++++++++++++++++++++++++----------
 block/blk.h          |  1 +
 4 files changed, 50 insertions(+), 25 deletions(-)

block/blk-core.c +13 ~~-12~~

index e00f5a2287cc..70cfac1d7fe1 100644
@@ -746,7 +746,7 @@ static inline blk_status_t blk_check_zone_append(struct request_queue *q,
 	return BLK_STS_OK;
 }
 
-static noinline_for_stack bool submit_bio_checks(struct bio *bio)
+noinline_for_stack bool submit_bio_checks(struct bio *bio)
 {
 	struct block_device *bdev = bio->bi_bdev;
 	struct request_queue *q = bdev_get_queue(bdev);

@@ -864,22 +864,23 @@ static noinline_for_stack bool submit_bio_checks(struct bio *bio)
 	return false;
 }
 
-static void __submit_bio(struct bio *bio)
+static void __submit_bio_fops(struct gendisk *disk, struct bio *bio)
 {
-	struct gendisk *disk = bio->bi_bdev->bd_disk;
-
 	if (unlikely(bio_queue_enter(bio) != 0))
 		return;
+	if (submit_bio_checks(bio) && blk_crypto_bio_prep(&bio))
+		disk->fops->submit_bio(bio);
+	blk_queue_exit(disk->queue);
+}
 
-	if (!submit_bio_checks(bio) || !blk_crypto_bio_prep(&bio))
-		goto queue_exit;
-	if (!disk->fops->submit_bio) {
+static void __submit_bio(struct bio *bio)
+{
+	struct gendisk *disk = bio->bi_bdev->bd_disk;
+
+	if (!disk->fops->submit_bio)
 		blk_mq_submit_bio(bio);
-		return;
-	}
-	disk->fops->submit_bio(bio);
-queue_exit:
-	blk_queue_exit(disk->queue);
+	else
+		__submit_bio_fops(disk, bio);
 }
 
 /*

block/blk-mq-sched.c +10 -3

index 4a6789e4398b..4be652fa38e7 100644
@@ -370,15 +370,20 @@ bool blk_mq_sched_bio_merge(struct request_queue *q, struct bio *bio,
 	bool ret = false;
 	enum hctx_type type;
 
-	if (e && e->type->ops.bio_merge)
-		return e->type->ops.bio_merge(q, bio, nr_segs);
+	if (bio_queue_enter(bio))
+		return false;
+
+	if (e && e->type->ops.bio_merge) {
+		ret = e->type->ops.bio_merge(q, bio, nr_segs);
+		goto out_put;
+	}
 
 	ctx = blk_mq_get_ctx(q);
 	hctx = blk_mq_map_queue(q, bio->bi_opf, ctx);
 	type = hctx->type;
 	if (!(hctx->flags & BLK_MQ_F_SHOULD_MERGE) ||
 	    list_empty_careful(&ctx->rq_lists[type]))
-		return false;
+		goto out_put;
 
 	/* default per sw-queue merge */
 	spin_lock(&ctx->lock);

@@ -391,6 +396,8 @@ bool blk_mq_sched_bio_merge(struct request_queue *q, struct bio *bio,
 		ret = true;
 
 	spin_unlock(&ctx->lock);
+out_put:
+	blk_queue_exit(q);
 	return ret;
 }

block/blk-mq.c +26 ~~-10~~

index dcb413297a96..875bd0c04409 100644
@@ -2478,6 +2478,13 @@ static inline unsigned short blk_plug_max_rq_count(struct blk_plug *plug)
 	return BLK_MAX_REQUEST_COUNT;
 }
 
+static inline bool blk_mq_queue_enter(struct request_queue *q, struct bio *bio)
+{
+	if (!blk_try_enter_queue(q, false) && bio_queue_enter(bio))
+		return false;
+	return true;
+}
+
 static struct request *blk_mq_get_new_requests(struct request_queue *q,
 					       struct blk_plug *plug,
 					       struct bio *bio)

@@ -2489,6 +2496,13 @@ static struct request *blk_mq_get_new_requests(struct request_queue *q,
 	};
 	struct request *rq;
 
+	if (unlikely(!blk_mq_queue_enter(q, bio)))
+		return NULL;
+	if (unlikely(!submit_bio_checks(bio)))
+		goto put_exit;
+
+	rq_qos_throttle(q, bio);
+
 	if (plug) {
 		data.nr_tags = plug->nr_ios;
 		plug->nr_ios = 1;

@@ -2502,6 +2516,8 @@ 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;
 }

@@ -2514,8 +2530,11 @@ static inline struct request *blk_mq_get_request(struct request_queue *q,
 
 		rq = rq_list_peek(&plug->cached_rq);
 		if (rq) {
+			if (unlikely(!submit_bio_checks(bio)))
+				return NULL;
 			plug->cached_rq = rq_list_next(rq);
 			INIT_LIST_HEAD(&rq->queuelist);
+			rq_qos_throttle(q, bio);
 			return rq;
 		}
 	}

@@ -2546,26 +2565,27 @@ void blk_mq_submit_bio(struct bio *bio)
 	unsigned int nr_segs = 1;
 	blk_status_t ret;
 
+	if (unlikely(!blk_crypto_bio_prep(&bio)))
+		return;
+
 	blk_queue_bounce(q, &bio);
 	if (blk_may_split(q, bio))
 		__blk_queue_split(q, &bio, &nr_segs);
 
 	if (!bio_integrity_prep(bio))
-		goto queue_exit;
+		return;
 
 	if (!blk_queue_nomerges(q) && bio_mergeable(bio)) {
 		if (blk_attempt_plug_merge(q, bio, nr_segs, &same_queue_rq))
-			goto queue_exit;
+			return;
 		if (blk_mq_sched_bio_merge(q, bio, nr_segs))
-			goto queue_exit;
+			return;
 	}
 
-	rq_qos_throttle(q, bio);
-
 	plug = blk_mq_plug(q, bio);
 	rq = blk_mq_get_request(q, plug, bio);
 	if (unlikely(!rq))
-		goto queue_exit;
+		return;
 
 	trace_block_getrq(bio);

@@ -2646,10 +2666,6 @@ void blk_mq_submit_bio(struct bio *bio)
 		/* Default case. */
 		blk_mq_sched_insert_request(rq, false, true, true);
 	}
-
-	return;
-queue_exit:
-	blk_queue_exit(q);
 }
 
 static size_t order_to_size(unsigned int order)

block/blk.h +1 -0

index f7371d3b1522..79c98ced59c8 100644
@@ -56,6 +56,7 @@ void blk_freeze_queue(struct request_queue *q);
 void __blk_mq_unfreeze_queue(struct request_queue *q, bool force_atomic);
 void blk_queue_start_drain(struct request_queue *q);
 int bio_queue_enter(struct bio *bio);
+bool submit_bio_checks(struct bio *bio);
 
 static inline bool blk_try_enter_queue(struct request_queue *q, bool pm)
 {

Signature · 1 line

2.33.1

Christoph Hellwig

2021-11-04 18:36 UTC #8 lore raw

Re: [PATCH 4/5] block: move queue enter logic into blk_mq_submit_bio()

block/blk-mq.c static inline unsigned short blk_plug_max_rq_count(struct blk_plug *plug) · line 2486

Quoted 6 lines

block/blk-mq.c +6 -0

+static inline bool blk_mq_queue_enter(struct request_queue *q, struct bio *bio)
+{
+	if (!blk_try_enter_queue(q, false) && bio_queue_enter(bio))
+		return false;
+	return true;
+}

Didn't we just agree on splitting bio_queue_enter into an inline helper and an out of line slowpath instead?

Jens Axboe

author maintainer

2021-11-04 18:37 UTC #9 lore raw

Re: [PATCH 4/5] block: move queue enter logic into blk_mq_submit_bio()

Quoted 9 lines · Christoph Hellwig

block/blk-mq.c static inline unsigned short blk_plug_max_rq_count(struct blk_plug *plug) · line 2486
Quoted 6 lines
block/blk-mq.c +6 -0
+static inline bool blk_mq_queue_enter(struct request_queue *q, struct bio *bio)
+{
+	if (!blk_try_enter_queue(q, false) && bio_queue_enter(bio))
+		return false;
+	return true;
+}
Didn't we just agree on splitting bio_queue_enter into an inline helper and an out of line slowpath instead?

See cover letter, and I also added to the commit message of this one. I do think this approach is better, as bio_queue_enter() itself is just slow path and there's no point polluting the code with 90% of what's in there.

Hence I kept it as-is.

Signature · 1 line

Jens Axboe

Christoph Hellwig

2021-11-04 18:39 UTC #10 lore raw

Re: [PATCH 4/5] block: move queue enter logic into blk_mq_submit_bio()

Quoted 11 lines · Jens Axboe

Quoted 9 lines · Christoph Hellwig
block/blk-mq.c static inline unsigned short blk_plug_max_rq_count(struct blk_plug *plug) · line 2486
Quoted 6 lines
block/blk-mq.c +6 -0
+static inline bool blk_mq_queue_enter(struct request_queue *q, struct bio *bio)
+{
+	if (!blk_try_enter_queue(q, false) && bio_queue_enter(bio))
+		return false;
+	return true;
+}
Didn't we just agree on splitting bio_queue_enter into an inline helper and an out of line slowpath instead?

Quoted 5 lines

See cover letter, and I also added to the commit message of this one. I do think this approach is better, as bio_queue_enter() itself is just slow path and there's no point polluting the code with 90% of what's in there.
Hence I kept it as-is.

Well, let me reword this then: why do you think the above is blk-mq secific and should not be used by every other caller of bio_queue_enter as well? In other words, why not rename bio_queue_enter __bio_queue_enter and make the above the public bio_queue_enter interface then?

Jens Axboe

author maintainer

2021-11-04 18:40 UTC #11 lore raw

Re: [PATCH 4/5] block: move queue enter logic into blk_mq_submit_bio()

Quoted 16 lines · Christoph Hellwig

Quoted 15 lines · Jens Axboe
Quoted 9 lines · Christoph Hellwig
block/blk-mq.c static inline unsigned short blk_plug_max_rq_count(struct blk_plug *plug) · line 2486
Quoted 6 lines
block/blk-mq.c +6 -0
+static inline bool blk_mq_queue_enter(struct request_queue *q, struct bio *bio)
+{
+	if (!blk_try_enter_queue(q, false) && bio_queue_enter(bio))
+		return false;
+	return true;
+}
Didn't we just agree on splitting bio_queue_enter into an inline helper and an out of line slowpath instead?
See cover letter, and I also added to the commit message of this one. I do think this approach is better, as bio_queue_enter() itself is just slow path and there's no point polluting the code with 90% of what's in there.

Quoted 7 lines

Quoted 1 line
Hence I kept it as-is.
Well, let me reword this then: why do you think the above is blk-mq secific and should not be used by every other caller of bio_queue_enter as well? In other words, why not rename bio_queue_enter __bio_queue_enter and make the above the public bio_queue_enter interface then?

OK, that I can agree too. I'll respin it as such. Gets the job done as well.

Signature · 1 line

Jens Axboe

Jens Axboe

author maintainer

2021-11-04 18:45 UTC #12 lore raw

Re: [PATCH 4/5] block: move queue enter logic into blk_mq_submit_bio()

Quoted 19 lines · Jens Axboe

Quoted 18 lines · Christoph Hellwig
Quoted 16 lines · Jens Axboe
Quoted 9 lines · Christoph Hellwig
Review
Quoted 6 lines
(quoted diff) +6 -0
+static inline bool blk_mq_queue_enter(struct request_queue *q, struct bio *bio)
+{
+	if (!blk_try_enter_queue(q, false) && bio_queue_enter(bio))
+		return false;
+	return true;
+}
Didn't we just agree on splitting bio_queue_enter into an inline helper and an out of line slowpath instead?
See cover letter, and I also added to the commit message of this one. I do think this approach is better, as bio_queue_enter() itself is just slow path and there's no point polluting the code with 90% of what's in there.
Hence I kept it as-is.

Quoted 8 lines

Quoted 5 lines
Well, let me reword this then: why do you think the above is blk-mq secific and should not be used by every other caller of bio_queue_enter as well? In other words, why not rename bio_queue_enter __bio_queue_enter and make the above the public bio_queue_enter interface then?
OK, that I can agree too. I'll respin it as such. Gets the job done as well.

Ala:

block/blk-core.c +1 ~~-27~~

index c2d267b6f910..0084067949d8 100644
@@ -386,30 +386,6 @@ void blk_cleanup_queue(struct request_queue *q)
 }
 EXPORT_SYMBOL(blk_cleanup_queue);
 
-static bool blk_try_enter_queue(struct request_queue *q, bool pm)
-{
-	rcu_read_lock();
-	if (!percpu_ref_tryget_live_rcu(&q->q_usage_counter))
-		goto fail;
-
-	/*
-	 * The code that increments the pm_only counter must ensure that the
-	 * counter is globally visible before the queue is unfrozen.
-	 */
-	if (blk_queue_pm_only(q) &&
-	    (!pm || queue_rpm_status(q) == RPM_SUSPENDED))
-		goto fail_put;
-
-	rcu_read_unlock();
-	return true;
-
-fail_put:
-	blk_queue_exit(q);
-fail:
-	rcu_read_unlock();
-	return false;
-}
-
 /**
  * blk_queue_enter() - try to increase q->q_usage_counter
  * @q: request queue pointer

@@ -442,10 +418,8 @@ int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags)
 	return 0;
 }
 
-static inline int bio_queue_enter(struct bio *bio)
+int __bio_queue_enter(struct request_queue *q, struct bio *bio)
 {
-	struct request_queue *q = bdev_get_queue(bio->bi_bdev);
-
 	while (!blk_try_enter_queue(q, false)) {
 		struct gendisk *disk = bio->bi_bdev->bd_disk;

block/blk.h +34 -0

index 7afffd548daf..814d9632d43e 100644
@@ -55,6 +55,40 @@ void blk_free_flush_queue(struct blk_flush_queue *q);
 void blk_freeze_queue(struct request_queue *q);
 void __blk_mq_unfreeze_queue(struct request_queue *q, bool force_atomic);
 void blk_queue_start_drain(struct request_queue *q);
+int __bio_queue_enter(struct request_queue *q, struct bio *bio);
+
+static inline bool blk_try_enter_queue(struct request_queue *q, bool pm)
+{
+	rcu_read_lock();
+	if (!percpu_ref_tryget_live_rcu(&q->q_usage_counter))
+		goto fail;
+
+	/*
+	 * The code that increments the pm_only counter must ensure that the
+	 * counter is globally visible before the queue is unfrozen.
+	 */
+	if (blk_queue_pm_only(q) &&
+	    (!pm || queue_rpm_status(q) == RPM_SUSPENDED))
+		goto fail_put;
+
+	rcu_read_unlock();
+	return true;
+
+fail_put:
+	blk_queue_exit(q);
+fail:
+	rcu_read_unlock();
+	return false;
+}
+
+static inline int bio_queue_enter(struct bio *bio)
+{
+	struct request_queue *q = bdev_get_queue(bio->bi_bdev);
+
+	if (blk_try_enter_queue(q, false))
+		return 0;
+	return __bio_queue_enter(q, bio);
+}
 
 #define BIO_INLINE_VECS 4
 struct bio_vec *bvec_alloc(mempool_t *pool, unsigned short *nr_vecs,

Signature · 1 line

Jens Axboe

Christoph Hellwig

Reviewed-by

2021-11-04 18:52 UTC #13 lore raw

Re: [PATCH 4/5] block: move queue enter logic into blk_mq_submit_bio()

Looks good:

Reviewed-by Christoph Hellwig <[email protected]>

Jens Axboe

author maintainer

2021-11-04 19:02 UTC #14 lore raw

Re: [PATCH 4/5] block: move queue enter logic into blk_mq_submit_bio()

Quoted 3 lines · Christoph Hellwig

Looks good:
Reviewed-by Christoph Hellwig <[email protected]>

So these two are now:

https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.16/block&id=c98cb5bbdab10d187aff9b4e386210eb2332af96

which is the one I sent here, and then the next one gets cleaned up to remove that queue enter helper:

https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.16/block&id=7f930eb31eeb07f1b606b3316d8ad3ab6a92905b

Can I add your reviewed-by to this last one as well? Only change is the removal of blk_mq_enter_queue() and the weird construct there, it's just bio_queue_enter() now.

Signature · 1 line

Jens Axboe

Christoph Hellwig

2021-11-04 19:04 UTC #15 lore raw

Re: [PATCH 4/5] block: move queue enter logic into blk_mq_submit_bio()

Quoted 9 lines · Jens Axboe

Quoted 3 lines · Christoph Hellwig
Looks good:
Reviewed-by Christoph Hellwig <[email protected]>
So these two are now:
https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.16/block&id=c98cb5bbdab10d187aff9b4e386210eb2332af96

Quoted 8 lines

which is the one I sent here, and then the next one gets cleaned up to remove that queue enter helper:
https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.16/block&id=7f930eb31eeb07f1b606b3316d8ad3ab6a92905b
Can I add your reviewed-by to this last one as well? Only change is the removal of blk_mq_enter_queue() and the weird construct there, it's just bio_queue_enter() now.

Sure.

Jens Axboe

author maintainer

2021-11-04 19:15 UTC #16 lore raw

Re: [PATCH 4/5] block: move queue enter logic into blk_mq_submit_bio()

Quoted 13 lines · Christoph Hellwig

Quoted 12 lines · Jens Axboe
Quoted 3 lines · Christoph Hellwig
Looks good:
Reviewed-by Christoph Hellwig <[email protected]>
So these two are now:
https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.16/block&id=c98cb5bbdab10d187aff9b4e386210eb2332af96
which is the one I sent here, and then the next one gets cleaned up to remove that queue enter helper:

Quoted 7 lines

Quoted 5 lines
https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.16/block&id=7f930eb31eeb07f1b606b3316d8ad3ab6a92905b
Can I add your reviewed-by to this last one as well? Only change is the removal of blk_mq_enter_queue() and the weird construct there, it's just bio_queue_enter() now.
Sure.

Thanks, prematurely already done, as you could tell :-)

Thanks for the reviews.

Signature · 1 line

Jens Axboe

Geert Uytterhoeven

2021-11-11 12:58 UTC #17 lore raw

Re: [PATCH 4/5] block: move queue enter logic into blk_mq_submit_bio()

Hi Jens,

Quoted 16 lines · Jens Axboe

Quoted 15 lines · Christoph Hellwig
Quoted 14 lines · Jens Axboe
Quoted 3 lines · Christoph Hellwig
Looks good:
Reviewed-by Christoph Hellwig <[email protected]>
So these two are now:
https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.16/block&id=c98cb5bbdab10d187aff9b4e386210eb2332af96
which is the one I sent here, and then the next one gets cleaned up to remove that queue enter helper:
https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.16/block&id=7f930eb31eeb07f1b606b3316d8ad3ab6a92905b

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
Can I add your reviewed-by to this last one as well? Only change is the removal of blk_mq_enter_queue() and the weird construct there, it's just bio_queue_enter() now.
Sure.
Thanks, prematurely already done, as you could tell :-)

The updated version is now commit 900e080752025f00 ("block: move queue enter logic into blk_mq_submit_bio()") in Linus' tree.

I have bisected failures on m68k/atari (on ARAnyM, using nfhd as the root device) to this commit, e.g.:

sd 0:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s sd 0:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current]
sd 0:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
sd 0:0:0:0: [sda] tag#0 CDB: Write(10) 2a 08 00 00 00 01 00 00 08 00
critical target error, dev sda, sector 1 op 0x1:(WRITE) flags 0x20800 phys_seg 1 prio class 0 Buffer I/O error on dev sda1, logical block 0, lost sync page write

EXT4-fs (sda1): I/O error while writing superblock
sd 0:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s sd 0:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current]
sd 0:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
sd 0:0:0:0: [sda] tag#0 CDB: Write(10) 2a 08 00 00 00 01 00 00 08 00
critical target error, dev sda, sector 1 op 0x1:(WRITE) flags 0x20800 phys_seg 1 prio class 0 Buffer I/O error on dev sda1, logical block 0, lost sync page write
EXT4-fs (sda1): I/O error while writing superblock

This may happen either when mounting the root file system (leading to an unable to mount root fs panic), or later (leading to a read-only rootfs).

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

Martin K. Petersen

2021-11-11 13:19 UTC #18 lore raw

Re: [PATCH 4/5] block: move queue enter logic into blk_mq_submit_bio()

Geert,

Quoted 6 lines

sd 0:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s sd 0:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current]
sd 0:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
sd 0:0:0:0: [sda] tag#0 CDB: Write(10) 2a 08 00 00 00 01 00 00 08 00
critical target error, dev sda, sector 1 op 0x1:(WRITE) flags 0x20800 phys_seg 1 prio class 0 Buffer I/O error on dev sda1, logical block 0, lost sync page write

Peculiar. That write command looks OK to me. I wonder if it's the FUA bit that trips it?

What does:

# dmesg | grep FUA

say?

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

Geert Uytterhoeven

2021-11-11 14:48 UTC #19 lore raw

Re: [PATCH 4/5] block: move queue enter logic into blk_mq_submit_bio()

Hi Martin,

Quoted 7 lines · Martin K. Petersen

Quoted 6 lines
sd 0:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s sd 0:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current]
sd 0:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
sd 0:0:0:0: [sda] tag#0 CDB: Write(10) 2a 08 00 00 00 01 00 00 08 00
critical target error, dev sda, sector 1 op 0x1:(WRITE) flags 0x20800 phys_seg 1 prio class 0 Buffer I/O error on dev sda1, logical block 0, lost sync page write

Quoted 8 lines

Peculiar. That write command looks OK to me. I wonder if it's the FUA bit that trips it?
What does:
# dmesg | grep FUA
say?

sd 0:0:0:0: [sda] Write cache: disabled, read cache: enabled, doesn't support DPO or FUA

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

Martin K. Petersen

2021-11-11 15:36 UTC #20 lore raw

Re: [PATCH 4/5] block: move queue enter logic into blk_mq_submit_bio()

Geert,

Quoted 1 line

Quoted 1 line
Quoted 1 line
sd 0:0:0:0: [sda] tag#0 CDB: Write(10) 2a 08 00 00 00 01 00 00 08 00

                                               ^^
                                               FUA

Quoted 2 lines

sd 0:0:0:0: [sda] Write cache: disabled, read cache: enabled, doesn't support DPO or FUA

The device is correct in rejecting the command, then. But I'm not sure how the commit in question would cause us to inadvertently set that flag...

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

Michael Schmitz

2021-11-11 21:35 UTC #21 lore raw

Re: [PATCH 4/5] block: move queue enter logic into blk_mq_submit_bio()

In reply to Geert Uytterhoeven

Hi Geert,

how easy is that to reproduce?

sd_setup_read_write_cmnd() does not validate the request's FUA flag against sdkp->DPOFUA (not suggesting that it should ...). I'd like to try and trace when such a mismatch happens.

Cheers,

Michael

Quoted 34 lines · Geert Uytterhoeven

Hi Martin,
Quoted 7 lines · Martin K. Petersen
Quoted 6 lines
sd 0:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s sd 0:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current]
sd 0:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
sd 0:0:0:0: [sda] tag#0 CDB: Write(10) 2a 08 00 00 00 01 00 00 08 00
critical target error, dev sda, sector 1 op 0x1:(WRITE) flags 0x20800 phys_seg 1 prio class 0 Buffer I/O error on dev sda1, logical block 0, lost sync page write
Quoted 8 lines
Peculiar. That write command looks OK to me. I wonder if it's the FUA bit that trips it?
What does:
# dmesg | grep FUA
say?
sd 0:0:0:0: [sda] Write cache: disabled, read cache: enabled, doesn't support DPO or FUA
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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

Geert Uytterhoeven

2021-11-12 07:37 UTC #22 lore raw

Re: [PATCH 4/5] block: move queue enter logic into blk_mq_submit_bio()

Hi Michael,

Quoted 1 line · Michael Schmitz

how easy is that to reproduce?

Fairly easy: it happens either on mounting, or after a few seconds booting into my old Debian userspace.

Quoted 3 lines

sd_setup_read_write_cmnd() does not validate the request's FUA flag against sdkp->DPOFUA (not suggesting that it should ...). I'd like to try and trace when such a mismatch happens.

Thanks!

Quoted 13 lines · Geert Uytterhoeven

Quoted 12 lines
Quoted 10 lines · Martin K. Petersen
Quoted 6 lines
sd 0:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s sd 0:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current]
sd 0:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
sd 0:0:0:0: [sda] tag#0 CDB: Write(10) 2a 08 00 00 00 01 00 00 08 00
critical target error, dev sda, sector 1 op 0x1:(WRITE) flags 0x20800 phys_seg 1 prio class 0 Buffer I/O error on dev sda1, logical block 0, lost sync page write
Peculiar. That write command looks OK to me. I wonder if it's the FUA bit that trips it?

Quoted 8 lines

Quoted 8 lines
Quoted 5 lines
What does:
# dmesg | grep FUA
say?
sd 0:0:0:0: [sda] Write cache: disabled, read cache: enabled, doesn't support DPO or FUA

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

Michael Schmitz

2021-11-12 22:34 UTC #23 lore raw

Re: [PATCH 4/5] block: move queue enter logic into blk_mq_submit_bio()

Hi Geert,

Quoted 7 lines · Geert Uytterhoeven

Hi Michael,
Quoted 1 line · Michael Schmitz
how easy is that to reproduce?
Fairly easy: it happens either on mounting, or after a few seconds booting into my old Debian userspace.

I must be too thick for this:

EXT4-fs (sda1): Cannot load crc32c driver.
VFS: Cannot open root device "sda1" or unknown-block(8,1): error -80

(linux-block 5833291ab6de, from kernel.org)

Same config on m68k 5.15 works just fine.

But this looks resolved now, so I'll punt for now ...

Cheers,

Michael

Quoted 40 lines

Quoted 4 lines
sd_setup_read_write_cmnd() does not validate the request's FUA flag against sdkp->DPOFUA (not suggesting that it should ...). I'd like to try and trace when such a mismatch happens.
Thanks!
Quoted 13 lines · Geert Uytterhoeven
Quoted 12 lines
Quoted 10 lines · Martin K. Petersen
Quoted 6 lines
sd 0:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s sd 0:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current]
sd 0:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
sd 0:0:0:0: [sda] tag#0 CDB: Write(10) 2a 08 00 00 00 01 00 00 08 00
critical target error, dev sda, sector 1 op 0x1:(WRITE) flags 0x20800 phys_seg 1 prio class 0 Buffer I/O error on dev sda1, logical block 0, lost sync page write
Peculiar. That write command looks OK to me. I wonder if it's the FUA bit that trips it?
Quoted 8 lines
Quoted 8 lines
Quoted 5 lines
What does:
# dmesg | grep FUA
say?
sd 0:0:0:0: [sda] Write cache: disabled, read cache: enabled, doesn't support DPO or FUA
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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

Michael Schmitz

2021-11-13 07:02 UTC #24 lore raw

Re: [PATCH 4/5] block: move queue enter logic into blk_mq_submit_bio()

Hi Geert, Jens,

Quoted 13 lines · Michael Schmitz

Hi Geert,
Quoted 9 lines · Geert Uytterhoeven
Hi Michael,
Quoted 1 line · Michael Schmitz
how easy is that to reproduce?
Fairly easy: it happens either on mounting, or after a few seconds booting
into my old Debian userspace.

Quoted 8 lines

I must be too thick for this:
EXT4-fs (sda1): Cannot load crc32c driver.
VFS: Cannot open root device "sda1" or unknown-block(8,1): error -80
(linux-block 5833291ab6de, from kernel.org)
Same config on m68k 5.15 works just fine.

FWIW - I had to revert cad439fc040efe5f4381e3a7d583c5c200dbc186 to overcome this, and reproduce Geert's finding.

Cheers,

Michael

Quoted 52 lines

But this looks resolved now, so I'll punt for now ...
Cheers,
Michael
Quoted 45 lines
Quoted 4 lines
sd_setup_read_write_cmnd() does not validate the request's FUA flag against sdkp->DPOFUA (not suggesting that it should ...). I'd like to try and trace when such a mismatch happens.
Thanks!
Quoted 15 lines · Geert Uytterhoeven
Quoted 14 lines
Quoted 12 lines · Martin K. Petersen
Quoted 8 lines
sd 0:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s
sd 0:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current] sd 0:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb sd 0:0:0:0: [sda] tag#0 CDB: Write(10) 2a 08 00 00 00 01 00 00 08 00 critical target error, dev sda, sector 1 op 0x1:(WRITE) flags 0x20800 phys_seg 1 prio class 0
Buffer I/O error on dev sda1, logical block 0, lost sync page write
Peculiar. That write command looks OK to me. I wonder if it's the FUA bit that trips it?
Quoted 8 lines
Quoted 8 lines
Quoted 5 lines
What does:
# dmesg | grep FUA
say?
sd 0:0:0:0: [sda] Write cache: disabled, read cache: enabled, doesn't support DPO or FUA
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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

Geert Uytterhoeven

2021-11-13 10:06 UTC #25 lore raw

Re: [PATCH 4/5] block: move queue enter logic into blk_mq_submit_bio()

In reply to Michael Schmitz

Quoted 11 lines · Michael Schmitz

Quoted 5 lines · Geert Uytterhoeven
Quoted 1 line · Michael Schmitz
how easy is that to reproduce?
Fairly easy: it happens either on mounting, or after a few seconds booting into my old Debian userspace.
I must be too thick for this:
EXT4-fs (sda1): Cannot load crc32c driver.
VFS: Cannot open root device "sda1" or unknown-block(8,1): error -80

I know ;-) (or :-(
Please cherry-pick commit beaaaa37c664e9af ("crypto: api - Fix boot-up crash when crypto manager is disabled"), which is now upstream.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

Michael Schmitz

2021-11-13 22:11 UTC #26 lore raw

Re: [PATCH 4/5] block: move queue enter logic into blk_mq_submit_bio()

In reply to Geert Uytterhoeven

Hi Geert, Martin,

Quoted 5 lines · Geert Uytterhoeven

Hi Michael,
Quoted 1 line · Michael Schmitz
how easy is that to reproduce?

Quoted 8 lines

Fairly easy: it happens either on mounting, or after a few seconds booting into my old Debian userspace.
Quoted 3 lines
sd_setup_read_write_cmnd() does not validate the request's FUA flag against sdkp->DPOFUA (not suggesting that it should ...). I'd like to try and trace when such a mismatch happens.
Thanks!

Also happens with the pata-falcon driver (which stats it doesnt support DPO or FUA).

Seen twice, once on boot, once on reboot (FUA bit not set in the SCSI command descriptor when the disk doesn't support that, to avoid a panic, otherwise still 5833291ab6de). Each time when a remount operation happens:

EXT4-fs (sda1): re-mounted. Opts: . Quota mode: disabled.

------------[ cut here ]------------
WARNING: CPU: 0 PID: 695 at drivers/scsi/sd.c:1271 sd_init_command+0x6aa/0x886
Modules linked in:
CPU: 0 PID: 695 Comm: mount Not tainted 5.15.0-atari-fpuemu-clean-signalfixes-remove-set_fs-ataflopfix4+ #88
Stack from 00be1c10:
        00be1c10 0035cc02 0035cc02 002cac7a 00379b57 000004f7 00000009 
00ad80a4
        002cacd4 00379b57 000004f7 0020ac5e 00000009 00000000 00000000 
00209d00
        00000008 00000000 00000001 00000000 00000001 00ab6400 0020ac5e 
00379b57
        000004f7 00000009 00000000 00adc800 00add9d0 00ab6400 00000001 
001269c4
        00003a98 00ad8000 00adc800 00ad80a4 00ab6400 00adc800 00000008 
00020801
        00000000 00000008 00000000 00000001 00000001 00201876 00ad80a4 
00ad80a4
Call Trace: [<002cac7a>] __warn+0x9e/0xb6
 [<002cacd4>] warn_slowpath_fmt+0x42/0x62
 [<0020ac5e>] sd_init_command+0x6aa/0x886
 [<00209d00>] max_write_same_blocks_store+0x6c/0x88
 [<0020ac5e>] sd_init_command+0x6aa/0x886
 [<001269c4>] ext4_get_group_desc+0x0/0xb0
 [<00003a98>] setup_frame+0xba/0x20a
 [<00020801>] _I_CALL_TOP+0xf21/0x1900
 [<00201876>] scsi_queue_rq+0x4f8/0x67e
 [<00194aaa>] __blk_mq_try_issue_directly+0x8a/0xf8
 [<00194adc>] __blk_mq_try_issue_directly+0xbc/0xf8
 [<00194fd0>] blk_mq_try_issue_directly+0x28/0x7a
 [<0019584c>] blk_mq_submit_bio+0x3fc/0x422
 [<0018c6f6>] __submit_bio+0x0/0x78
 [<0018b5cc>] bio_list_pop+0x0/0x1a
 [<00092800>] btf_check_type_match+0x274/0x2e4
 [<00020801>] _I_CALL_TOP+0xf21/0x1900
 [<00002000>] _start+0x0/0x8
 [<0018c7dc>] submit_bio_noacct+0x6e/0x152
 [<00020801>] _I_CALL_TOP+0xf21/0x1900
 [<000f21fc>] test_bit+0x0/0x12
 [<00051f68>] ktime_get_real_seconds+0x0/0x30
 [<000f21fc>] test_bit+0x0/0x12
 [<00051f68>] ktime_get_real_seconds+0x0/0x30
 [<000f329c>] submit_bh_wbc.isra.54+0x178/0x182
 [<000f21fc>] test_bit+0x0/0x12
 [<000f32b8>] submit_bh+0x12/0x18
 [<00020800>] _I_CALL_TOP+0xf20/0x1900
 [<000f362e>] __sync_dirty_buffer+0x78/0xaa
 [<00020800>] _I_CALL_TOP+0xf20/0x1900
 [<00158684>] test_bit+0x0/0x12
 [<0015a508>] ext4_commit_super+0xa8/0xf8
 [<00020800>] _I_CALL_TOP+0xf20/0x1900
 [<0015be82>] ext4_setup_super+0x132/0x1ac
 [<0015c970>] ext4_remount+0x49c/0x57e
 [<00004003>] do_rt_sigreturn+0xe1/0x1b8
 [<000dbe3e>] dentry_lru_isolate_shrink+0x0/0x1c
 [<000efaf0>] legacy_reconfigure+0x30/0x42
 [<000ce664>] reconfigure_super+0xae/0x178
 [<000e4a38>] path_mount+0x274/0x61c
 [<000e4e20>] do_mount+0x40/0x5c
 [<000e50c4>] sys_mount+0xf8/0x10e
 [<00002922>] syscall+0x8/0xc
 [<0018c00c>] blk_cleanup_queue+0x5a/0xc6

---[ end trace b0cd975622ec08c4 ]---

EXT4-fs (sda1): re-mounted. Opts: errors=remount-ro. Quota mode: disabled. ext4 filesystem being remounted at / supports timestamps until 2038 (0x7fffffff)

------------[ cut here ]------------
WARNING: CPU: 0 PID: 1174 at drivers/scsi/sd.c:1271 sd_init_command+0x6aa/0x886
Modules linked in:
CPU: 0 PID: 1174 Comm: umount Tainted: G        W 5.15.0-atari-fpuemu-clean-signalfixes-remove-set_fs-ataflopfix4+ #88
Stack from 00b6dc70:
        00b6dc70 0035cc02 0035cc02 002cac7a 00379b57 000004f7 00000009 
00ad80a4
        002cacd4 00379b57 000004f7 0020ac5e 00000009 00000000 00000000 
00209d00
        00000008 00000000 00000001 00000000 00000001 00ab6400 0020ac5e 
00379b57
        000004f7 00000009 00000000 00adc800 00add9d0 00ab6400 00000001 
00000000
        00003a98 00ad8000 00adc800 00ad80a4 00ab6400 00adc800 00000008 
00023801
        00000000 00000008 00000000 00000001 00000001 00201876 00ad80a4 
00ad80a4
Call Trace: [<002cac7a>] __warn+0x9e/0xb6
 [<002cacd4>] warn_slowpath_fmt+0x42/0x62
 [<0020ac5e>] sd_init_command+0x6aa/0x886
 [<00209d00>] max_write_same_blocks_store+0x6c/0x88
 [<0020ac5e>] sd_init_command+0x6aa/0x886
 [<00003a98>] setup_frame+0xba/0x20a
 [<00023801>] fp_fadd+0x1d5/0x1e4
 [<00201876>] scsi_queue_rq+0x4f8/0x67e
 [<00194aaa>] __blk_mq_try_issue_directly+0x8a/0xf8
 [<00194adc>] __blk_mq_try_issue_directly+0xbc/0xf8
 [<00194fd0>] blk_mq_try_issue_directly+0x28/0x7a
 [<0019584c>] blk_mq_submit_bio+0x3fc/0x422
 [<0018c6f6>] __submit_bio+0x0/0x78
 [<0018b5cc>] bio_list_pop+0x0/0x1a
 [<00092800>] btf_check_type_match+0x274/0x2e4
 [<00023801>] fp_fadd+0x1d5/0x1e4
 [<00002000>] _start+0x0/0x8
 [<0018c7dc>] submit_bio_noacct+0x6e/0x152
 [<00023801>] fp_fadd+0x1d5/0x1e4
 [<000f21fc>] test_bit+0x0/0x12
 [<000f21fc>] test_bit+0x0/0x12
 [<000f329c>] submit_bh_wbc.isra.54+0x178/0x182
 [<00158684>] test_bit+0x0/0x12
 [<000f21fc>] test_bit+0x0/0x12
 [<000f32b8>] submit_bh+0x12/0x18
 [<00020800>] _I_CALL_TOP+0xf20/0x1900
 [<000f362e>] __sync_dirty_buffer+0x78/0xaa
 [<00020800>] _I_CALL_TOP+0xf20/0x1900
 [<00158684>] test_bit+0x0/0x12
 [<00158684>] test_bit+0x0/0x12
 [<0015a508>] ext4_commit_super+0xa8/0xf8
 [<00020800>] _I_CALL_TOP+0xf20/0x1900
 [<00158684>] test_bit+0x0/0x12
 [<0015c80e>] ext4_remount+0x33a/0x57e
 [<00004003>] do_rt_sigreturn+0xe1/0x1b8
 [<000dbe3e>] dentry_lru_isolate_shrink+0x0/0x1c
 [<000efaf0>] legacy_reconfigure+0x30/0x42
 [<000ce664>] reconfigure_super+0xae/0x178
 [<000e3ab0>] path_umount+0x17c/0x2e8
 [<000e3c62>] ksys_umount+0x46/0x54
 [<000e3c8a>] sys_oldumount+0xa/0xe
 [<00002922>] syscall+0x8/0xc
 [<0018c00c>] blk_cleanup_queue+0x5a/0xc6

---[ end trace b0cd975622ec08c5 ]---

EXT4-fs (sda1): re-mounted. Opts: (null). Quota mode: disabled. EXT4-fs (sda1): re-mounted. Opts: errors=remount-ro. Quota mode: disabled.

Not sure this helps much.

Cheers,

Michael

Quoted 34 lines

Quoted 14 lines · Geert Uytterhoeven
Quoted 12 lines
Quoted 10 lines · Martin K. Petersen
Quoted 6 lines
sd 0:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s sd 0:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current]
sd 0:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
sd 0:0:0:0: [sda] tag#0 CDB: Write(10) 2a 08 00 00 00 01 00 00 08 00
critical target error, dev sda, sector 1 op 0x1:(WRITE) flags 0x20800 phys_seg 1 prio class 0 Buffer I/O error on dev sda1, logical block 0, lost sync page write
Peculiar. That write command looks OK to me. I wonder if it's the FUA bit that trips it?
Quoted 8 lines
Quoted 8 lines
Quoted 5 lines
What does:
# dmesg | grep FUA
say?
sd 0:0:0:0: [sda] Write cache: disabled, read cache: enabled, doesn't support DPO or FUA
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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

Ming Lei

2021-11-11 13:45 UTC #27 lore raw

Re: [PATCH 4/5] block: move queue enter logic into blk_mq_submit_bio()

In reply to Geert Uytterhoeven

Quoted 49 lines · Geert Uytterhoeven

Hi Jens,
Quoted 16 lines · Jens Axboe
Quoted 15 lines · Christoph Hellwig
Quoted 14 lines · Jens Axboe
Quoted 3 lines · Christoph Hellwig
Looks good:
Reviewed-by Christoph Hellwig <[email protected]>
So these two are now:
https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.16/block&id=c98cb5bbdab10d187aff9b4e386210eb2332af96
which is the one I sent here, and then the next one gets cleaned up to remove that queue enter helper:
https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.16/block&id=7f930eb31eeb07f1b606b3316d8ad3ab6a92905b
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Can I add your reviewed-by to this last one as well? Only change is the removal of blk_mq_enter_queue() and the weird construct there, it's just bio_queue_enter() now.
Sure.
Thanks, prematurely already done, as you could tell :-)
The updated version is now commit 900e080752025f00 ("block: move queue enter logic into blk_mq_submit_bio()") in Linus' tree.
I have bisected failures on m68k/atari (on ARAnyM, using nfhd as the root device) to this commit, e.g.:
sd 0:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s sd 0:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current]
sd 0:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
sd 0:0:0:0: [sda] tag#0 CDB: Write(10) 2a 08 00 00 00 01 00 00 08 00
critical target error, dev sda, sector 1 op 0x1:(WRITE) flags 0x20800 phys_seg 1 prio class 0 Buffer I/O error on dev sda1, logical block 0, lost sync page write
EXT4-fs (sda1): I/O error while writing superblock
sd 0:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s sd 0:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current]
sd 0:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
sd 0:0:0:0: [sda] tag#0 CDB: Write(10) 2a 08 00 00 00 01 00 00 08 00
critical target error, dev sda, sector 1 op 0x1:(WRITE) flags 0x20800 phys_seg 1 prio class 0 Buffer I/O error on dev sda1, logical block 0, lost sync page write
EXT4-fs (sda1): I/O error while writing superblock

Quoted 3 lines

This may happen either when mounting the root file system (leading to an unable to mount root fs panic), or later (leading to a read-only rootfs).

BTW, today I just found that hang in blk_mq_freeze_queue_wait() is caused by commit 900e080752025f00, and the following patch can fix it:

- blk-mq: don't grab ->q_usage_counter in blk_mq_sched_bio_merge

https://lore.kernel.org/linux-block/[email protected]/T/#m759b88fda094a65ebf29bc81b780967cdaf9cf28

Maybe you can try the above patch.

Thanks,
Ming

Geert Uytterhoeven

2021-11-11 14:51 UTC #28 lore raw

Re: [PATCH 4/5] block: move queue enter logic into blk_mq_submit_bio()

Hi Ming,

Quoted 52 lines · Ming Lei

Quoted 47 lines · Geert Uytterhoeven
Quoted 16 lines · Jens Axboe
Quoted 15 lines · Christoph Hellwig
Quoted 14 lines · Jens Axboe
Quoted 3 lines · Christoph Hellwig
Looks good:
Reviewed-by Christoph Hellwig <[email protected]>
So these two are now:
https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.16/block&id=c98cb5bbdab10d187aff9b4e386210eb2332af96
which is the one I sent here, and then the next one gets cleaned up to remove that queue enter helper:
https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.16/block&id=7f930eb31eeb07f1b606b3316d8ad3ab6a92905b
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Can I add your reviewed-by to this last one as well? Only change is the removal of blk_mq_enter_queue() and the weird construct there, it's just bio_queue_enter() now.
Sure.
Thanks, prematurely already done, as you could tell :-)
The updated version is now commit 900e080752025f00 ("block: move queue enter logic into blk_mq_submit_bio()") in Linus' tree.
I have bisected failures on m68k/atari (on ARAnyM, using nfhd as the root device) to this commit, e.g.:
sd 0:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s sd 0:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current]
sd 0:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
sd 0:0:0:0: [sda] tag#0 CDB: Write(10) 2a 08 00 00 00 01 00 00 08 00
critical target error, dev sda, sector 1 op 0x1:(WRITE) flags 0x20800 phys_seg 1 prio class 0 Buffer I/O error on dev sda1, logical block 0, lost sync page write
EXT4-fs (sda1): I/O error while writing superblock
sd 0:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s sd 0:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current]
sd 0:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
sd 0:0:0:0: [sda] tag#0 CDB: Write(10) 2a 08 00 00 00 01 00 00 08 00
critical target error, dev sda, sector 1 op 0x1:(WRITE) flags 0x20800 phys_seg 1 prio class 0 Buffer I/O error on dev sda1, logical block 0, lost sync page write
EXT4-fs (sda1): I/O error while writing superblock
Quoted 3 lines
This may happen either when mounting the root file system (leading to an unable to mount root fs panic), or later (leading to a read-only rootfs).

Quoted 8 lines

BTW, today I just found that hang in blk_mq_freeze_queue_wait() is caused by commit 900e080752025f00, and the following patch can fix it:
- blk-mq: don't grab ->q_usage_counter in blk_mq_sched_bio_merge
https://lore.kernel.org/linux-block/[email protected]/T/#m759b88fda094a65ebf29bc81b780967cdaf9cf28
Maybe you can try the above patch.

Thanks! I have applied both patches, but it doesn't make a difference.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

Ming Lei

2021-11-11 15:24 UTC #29 lore raw

Re: [PATCH 4/5] block: move queue enter logic into blk_mq_submit_bio()

Hi Geert,

Quoted 58 lines · Geert Uytterhoeven

Hi Ming,
Quoted 55 lines · Ming Lei
Quoted 47 lines · Geert Uytterhoeven
Quoted 16 lines · Jens Axboe
Quoted 15 lines · Christoph Hellwig
Quoted 14 lines · Jens Axboe
Quoted 3 lines · Christoph Hellwig
Looks good:
Reviewed-by Christoph Hellwig <[email protected]>
So these two are now:
https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.16/block&id=c98cb5bbdab10d187aff9b4e386210eb2332af96
which is the one I sent here, and then the next one gets cleaned up to remove that queue enter helper:
https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.16/block&id=7f930eb31eeb07f1b606b3316d8ad3ab6a92905b
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Can I add your reviewed-by to this last one as well? Only change is the removal of blk_mq_enter_queue() and the weird construct there, it's just bio_queue_enter() now.
Sure.
Thanks, prematurely already done, as you could tell :-)
The updated version is now commit 900e080752025f00 ("block: move queue enter logic into blk_mq_submit_bio()") in Linus' tree.
I have bisected failures on m68k/atari (on ARAnyM, using nfhd as the root device) to this commit, e.g.:
sd 0:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s sd 0:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current]
sd 0:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
sd 0:0:0:0: [sda] tag#0 CDB: Write(10) 2a 08 00 00 00 01 00 00 08 00
critical target error, dev sda, sector 1 op 0x1:(WRITE) flags 0x20800 phys_seg 1 prio class 0 Buffer I/O error on dev sda1, logical block 0, lost sync page write
EXT4-fs (sda1): I/O error while writing superblock
sd 0:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s sd 0:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current]
sd 0:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
sd 0:0:0:0: [sda] tag#0 CDB: Write(10) 2a 08 00 00 00 01 00 00 08 00
critical target error, dev sda, sector 1 op 0x1:(WRITE) flags 0x20800 phys_seg 1 prio class 0 Buffer I/O error on dev sda1, logical block 0, lost sync page write
EXT4-fs (sda1): I/O error while writing superblock
Quoted 3 lines
This may happen either when mounting the root file system (leading to an unable to mount root fs panic), or later (leading to a read-only rootfs).
BTW, today I just found that hang in blk_mq_freeze_queue_wait() is caused by commit 900e080752025f00, and the following patch can fix it:

Quoted 7 lines

Quoted 5 lines
- blk-mq: don't grab ->q_usage_counter in blk_mq_sched_bio_merge
https://lore.kernel.org/linux-block/[email protected]/T/#m759b88fda094a65ebf29bc81b780967cdaf9cf28
Maybe you can try the above patch.
Thanks! I have applied both patches, but it doesn't make a difference.

Thanks for your test!

Can you try the following patch?

block/blk-mq.c +1 -1

index f511db395c7f..a5ab2f2e9f67 100644
@@ -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;

Signature · 1 line

Ming

Jens Axboe

author maintainer

2021-11-11 22:17 UTC #30 lore raw

Re: [PATCH 4/5] block: move queue enter logic into blk_mq_submit_bio()

Quoted 87 lines · Ming Lei

Hi Geert,
Quoted 58 lines · Geert Uytterhoeven
Hi Ming,
Quoted 55 lines · Ming Lei
Quoted 47 lines · Geert Uytterhoeven
Quoted 16 lines · Jens Axboe
Quoted 15 lines · Christoph Hellwig
Quoted 14 lines · Jens Axboe
Quoted 3 lines · Christoph Hellwig
Looks good:
Reviewed-by Christoph Hellwig <[email protected]>
So these two are now:
https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.16/block&id=c98cb5bbdab10d187aff9b4e386210eb2332af96
which is the one I sent here, and then the next one gets cleaned up to remove that queue enter helper:
https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.16/block&id=7f930eb31eeb07f1b606b3316d8ad3ab6a92905b
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Can I add your reviewed-by to this last one as well? Only change is the removal of blk_mq_enter_queue() and the weird construct there, it's just bio_queue_enter() now.
Sure.
Thanks, prematurely already done, as you could tell :-)
The updated version is now commit 900e080752025f00 ("block: move queue enter logic into blk_mq_submit_bio()") in Linus' tree.
I have bisected failures on m68k/atari (on ARAnyM, using nfhd as the root device) to this commit, e.g.:
sd 0:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s sd 0:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current]
sd 0:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
sd 0:0:0:0: [sda] tag#0 CDB: Write(10) 2a 08 00 00 00 01 00 00 08 00
critical target error, dev sda, sector 1 op 0x1:(WRITE) flags 0x20800 phys_seg 1 prio class 0 Buffer I/O error on dev sda1, logical block 0, lost sync page write
EXT4-fs (sda1): I/O error while writing superblock
sd 0:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s sd 0:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current]
sd 0:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
sd 0:0:0:0: [sda] tag#0 CDB: Write(10) 2a 08 00 00 00 01 00 00 08 00
critical target error, dev sda, sector 1 op 0x1:(WRITE) flags 0x20800 phys_seg 1 prio class 0 Buffer I/O error on dev sda1, logical block 0, lost sync page write
EXT4-fs (sda1): I/O error while writing superblock
Quoted 3 lines
This may happen either when mounting the root file system (leading to an unable to mount root fs panic), or later (leading to a read-only rootfs).
BTW, today I just found that hang in blk_mq_freeze_queue_wait() is caused by commit 900e080752025f00, and the following patch can fix it:
Quoted 7 lines
Quoted 5 lines
- blk-mq: don't grab ->q_usage_counter in blk_mq_sched_bio_merge
https://lore.kernel.org/linux-block/[email protected]/T/#m759b88fda094a65ebf29bc81b780967cdaf9cf28
Maybe you can try the above patch.
Thanks! I have applied both patches, but it doesn't make a difference.
Thanks for your test!
Can you try the following patch?
block/blk-mq.c +0 -1
index f511db395c7f..a5ab2f2e9f67 100644
@@ -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;
 

block/blk-mq.c static struct request *blk_mq_get_new_requests(struct request_queue *q, · line 2529

Quoted 6 lines

block/blk-mq.c +1 -0

@@ -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;

That's definitely a real fix, akin to the other pre-enter variants, this one just post checks. Geert, can you give this a whirl?

Ming, would you mind sending this as a real patch?

Signature · 1 line

Jens Axboe

Ming Lei

2021-11-12 00:44 UTC #31 lore raw

Re: [PATCH 4/5] block: move queue enter logic into blk_mq_submit_bio()

Quoted 98 lines · Jens Axboe

Quoted 87 lines · Ming Lei
Hi Geert,
Quoted 58 lines · Geert Uytterhoeven
Hi Ming,
Quoted 55 lines · Ming Lei
Quoted 47 lines · Geert Uytterhoeven
Quoted 16 lines · Jens Axboe
Quoted 15 lines · Christoph Hellwig
Quoted 14 lines · Jens Axboe
Quoted 3 lines · Christoph Hellwig
Looks good:
Reviewed-by Christoph Hellwig <[email protected]>
So these two are now:
https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.16/block&id=c98cb5bbdab10d187aff9b4e386210eb2332af96
which is the one I sent here, and then the next one gets cleaned up to remove that queue enter helper:
https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.16/block&id=7f930eb31eeb07f1b606b3316d8ad3ab6a92905b
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Can I add your reviewed-by to this last one as well? Only change is the removal of blk_mq_enter_queue() and the weird construct there, it's just bio_queue_enter() now.
Sure.
Thanks, prematurely already done, as you could tell :-)
The updated version is now commit 900e080752025f00 ("block: move queue enter logic into blk_mq_submit_bio()") in Linus' tree.
I have bisected failures on m68k/atari (on ARAnyM, using nfhd as the root device) to this commit, e.g.:
sd 0:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s sd 0:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current]
sd 0:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
sd 0:0:0:0: [sda] tag#0 CDB: Write(10) 2a 08 00 00 00 01 00 00 08 00
critical target error, dev sda, sector 1 op 0x1:(WRITE) flags 0x20800 phys_seg 1 prio class 0 Buffer I/O error on dev sda1, logical block 0, lost sync page write
EXT4-fs (sda1): I/O error while writing superblock
sd 0:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s sd 0:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current]
sd 0:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
sd 0:0:0:0: [sda] tag#0 CDB: Write(10) 2a 08 00 00 00 01 00 00 08 00
critical target error, dev sda, sector 1 op 0x1:(WRITE) flags 0x20800 phys_seg 1 prio class 0 Buffer I/O error on dev sda1, logical block 0, lost sync page write
EXT4-fs (sda1): I/O error while writing superblock
Quoted 3 lines
This may happen either when mounting the root file system (leading to an unable to mount root fs panic), or later (leading to a read-only rootfs).
BTW, today I just found that hang in blk_mq_freeze_queue_wait() is caused by commit 900e080752025f00, and the following patch can fix it:
Quoted 7 lines
Quoted 5 lines
- blk-mq: don't grab ->q_usage_counter in blk_mq_sched_bio_merge
https://lore.kernel.org/linux-block/[email protected]/T/#m759b88fda094a65ebf29bc81b780967cdaf9cf28
Maybe you can try the above patch.
Thanks! I have applied both patches, but it doesn't make a difference.
Thanks for your test!
Can you try the following patch?
block/blk-mq.c +0 -1
index f511db395c7f..a5ab2f2e9f67 100644
@@ -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;
 
block/blk-mq.c static struct request *blk_mq_get_new_requests(struct request_queue *q, · line 2529
Quoted 6 lines
block/blk-mq.c +1 -0
@@ -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;
That's definitely a real fix, akin to the other pre-enter variants, this one just post checks. Geert, can you give this a whirl?

Quoted 1 line

Ming, would you mind sending this as a real patch?

Hi Jens,

The above patch may not be enough, since submit_bio_checks() is done in case of using cached request, so how about the following patch(un-tested)?

block/blk-mq.c +4 -2

index f511db395c7f..f84044c8de3f 100644
@@ -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);
 }

Signature · 1 line

Ming

Geert Uytterhoeven

Tested-by Applied

2021-11-12 07:51 UTC #32 lore raw

Re: [PATCH 4/5] block: move queue enter logic into blk_mq_submit_bio()

Hi Ming,

Quoted 70 lines · Ming Lei

Quoted 69 lines · Jens Axboe
Quoted 68 lines · Ming Lei
Quoted 56 lines · Geert Uytterhoeven
Quoted 55 lines · Ming Lei
Quoted 47 lines · Geert Uytterhoeven
Quoted 16 lines · Jens Axboe
Quoted 15 lines · Christoph Hellwig
Quoted 14 lines · Jens Axboe
Quoted 3 lines · Christoph Hellwig
Looks good:
Reviewed-by Christoph Hellwig <[email protected]>
So these two are now:
https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.16/block&id=c98cb5bbdab10d187aff9b4e386210eb2332af96
which is the one I sent here, and then the next one gets cleaned up to remove that queue enter helper:
https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.16/block&id=7f930eb31eeb07f1b606b3316d8ad3ab6a92905b
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Can I add your reviewed-by to this last one as well? Only change is the removal of blk_mq_enter_queue() and the weird construct there, it's just bio_queue_enter() now.
Sure.
Thanks, prematurely already done, as you could tell :-)
The updated version is now commit 900e080752025f00 ("block: move queue enter logic into blk_mq_submit_bio()") in Linus' tree.
I have bisected failures on m68k/atari (on ARAnyM, using nfhd as the root device) to this commit, e.g.:
sd 0:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s sd 0:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current]
sd 0:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
sd 0:0:0:0: [sda] tag#0 CDB: Write(10) 2a 08 00 00 00 01 00 00 08 00
critical target error, dev sda, sector 1 op 0x1:(WRITE) flags 0x20800 phys_seg 1 prio class 0 Buffer I/O error on dev sda1, logical block 0, lost sync page write
EXT4-fs (sda1): I/O error while writing superblock
sd 0:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s sd 0:0:0:0: [sda] tag#0 Sense Key : Illegal Request [current]
sd 0:0:0:0: [sda] tag#0 Add. Sense: Invalid field in cdb
sd 0:0:0:0: [sda] tag#0 CDB: Write(10) 2a 08 00 00 00 01 00 00 08 00
critical target error, dev sda, sector 1 op 0x1:(WRITE) flags 0x20800 phys_seg 1 prio class 0 Buffer I/O error on dev sda1, logical block 0, lost sync page write
EXT4-fs (sda1): I/O error while writing superblock
Quoted 3 lines
This may happen either when mounting the root file system (leading to an unable to mount root fs panic), or later (leading to a read-only rootfs).
BTW, today I just found that hang in blk_mq_freeze_queue_wait() is caused by commit 900e080752025f00, and the following patch can fix it:
Quoted 7 lines
Quoted 5 lines
- blk-mq: don't grab ->q_usage_counter in blk_mq_sched_bio_merge
https://lore.kernel.org/linux-block/[email protected]/T/#m759b88fda094a65ebf29bc81b780967cdaf9cf28
Maybe you can try the above patch.
Thanks! I have applied both patches, but it doesn't make a difference.
Thanks for your test!
Can you try the following patch?

[...]

Quoted 2 lines

Quoted 2 lines
That's definitely a real fix, akin to the other pre-enter variants, this one just post checks. Geert, can you give this a whirl?

With both of

blk-mq: don't grab ->q_usage_counter in blk_mq_sched_bio_merge
blk-mq: rename blk_attempt_bio_merge

applied, and the version above, I no longer saw the error, but the boot sometimes hangs after:

ext3 filesystem being remounted at / supports timestamps until
2038 (0x7fffffff)

I don't know how easy that is to trigger: it hung on my first try, but the second and third tries it booted fully into old Debian userspace.

Quoted 4 lines

Quoted 1 line
Ming, would you mind sending this as a real patch?
The above patch may not be enough, since submit_bio_checks() is done in case of using cached request, so how about the following patch(un-tested)?

Worked fine in five subsequent boots. Thanks!

Tested-by Geert Uytterhoeven <[email protected]>

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

Michael Schmitz

2021-11-15 20:36 UTC #33 lore raw

Re: [PATCH 4/5] block: move queue enter logic into blk_mq_submit_bio()

On 12/11/21 20:51, Geert Uytterhoeven wrote:
[...]

Quoted 34 lines

Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
Quoted 10 lines
Quoted 8 lines
BTW, today I just found that hang in blk_mq_freeze_queue_wait() is caused by commit 900e080752025f00, and the following patch can fix it:
- blk-mq: don't grab ->q_usage_counter in blk_mq_sched_bio_merge
https://lore.kernel.org/linux-block/[email protected]/T/#m759b88fda094a65ebf29bc81b780967cdaf9cf28
Maybe you can try the above patch.
Thanks! I have applied both patches, but it doesn't make a difference.
Thanks for your test!
Can you try the following patch?
[...]
Quoted 2 lines
Quoted 2 lines
That's definitely a real fix, akin to the other pre-enter variants, this one just post checks. Geert, can you give this a whirl?
With both of
blk-mq: don't grab ->q_usage_counter in blk_mq_sched_bio_merge
blk-mq: rename blk_attempt_bio_merge
applied, and the version above, I no longer saw the error, but the boot sometimes hangs after:
ext3 filesystem being remounted at / supports timestamps until
2038 (0x7fffffff)
I don't know how easy that is to trigger: it hung on my first try, but the second and third tries it booted fully into old Debian userspace.

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
Ming, would you mind sending this as a real patch?
The above patch may not be enough, since submit_bio_checks() is done in case of using cached request, so how about the following patch(un-tested)?
Worked fine in five subsequent boots. Thanks!
Tested-by Geert Uytterhoeven <[email protected]>

For good measure: block-5.16-2021-11-13 tested fine running IO stress tests on a mix of IDE and SCSI disks, only one of those supporting DPO/FUA.

Using

WARN_ON(rq->cmd_flags & REQ_FUA && !sdkp->DPOFUA);

in sd.c:sd_setup_read_write_cmnd(), nothing seen in the logs.

Cheers,

Michael

Quoted 12 lines

Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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

Jens Axboe

author maintainer

2021-11-04 18:22 UTC #34 lore raw

PATCH v3 5/5 block: ensure cached plug request matches the current queue

In reply to Jens Axboe

If we're driving multiple devices, we could have pre-populated the cache for a different device. Ensure that the empty request matches the current queue.

Fixes 47c122e35d7e ("block: pre-allocate requests if plug is started and is a batch")

Signed-off-by Jens Axboe <[email protected]>

---
 block/blk-mq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

block/blk-mq.c +1 -1

index 875bd0c04409..6c8d02bd1b06 100644
@@ -2529,7 +2529,7 @@ static inline struct request *blk_mq_get_request(struct request_queue *q,
 		struct request *rq;
 
 		rq = rq_list_peek(&plug->cached_rq);
-		if (rq) {
+		if (rq && rq->q == q) {
 			if (unlikely(!submit_bio_checks(bio)))
 				return NULL;
 			plug->cached_rq = rq_list_next(rq);

Signature · 1 line

2.33.1

Christoph Hellwig

Reviewed-by on v3 5/5

2021-11-04 18:36 UTC #35 lore raw

Re: [PATCH 5/5] block: ensure cached plug request matches the current queue

Quoted 6 lines · Jens Axboe

If we're driving multiple devices, we could have pre-populated the cache for a different device. Ensure that the empty request matches the current queue.
Fixes 47c122e35d7e ("block: pre-allocate requests if plug is started and is a batch")
Signed-off-by Jens Axboe <[email protected]>

Looks good,

Reviewed-by Christoph Hellwig <[email protected]>