[PATCH 0/3] block: queue exit cleanup

[PATCH 0/3] block: queue exit cleanup

[Ming Lei]

Hi,

For fixing queue lock switching back related issue, commit 498f6650aec8
("block: Fix a race between the cgroup code and request queue initialization")
moves what blk_exit_queue() does into blk_cleanup_queue(). Turns out
it requires to protect generic_make_request_checks with blk_queue_enter,
then we have to hold the queue usage counter before
generic_make_request_checks(). This way makes generic_make_request()
code quite complicated and not easy to maintain.

After killing legacy request IO path, there isn't driver private queue
lock story, then no such issue addressed by 498f6650aec8 any more.

So revert the following commits and clean up related code much. 

	498f6650aec8 block: Fix a race between the cgroup code and request queue initialization
	37f9579f4c31 blk-mq: Avoid that submitting a bio concurrently with device removal triggers a crash
	cd4a4ae4683d block: don't use blocking queue entered for recursive bio submits
	24ecc3585348 block: Ensure that a request queue is dissociated from the cgroup controller

Ming Lei (3):
  block: move blk_exit_queue into __blk_release_queue
  block: don't protect generic_make_request_checks with blk_queue_enter
  block: rename BIO_QUEUE_ENTERED as BIO_SPLITTED

 block/blk-core.c           | 74 ++++----------------------------------
 block/blk-merge.c          | 10 +-----
 block/blk-sysfs.c          | 47 ++++++++++++++++--------
 block/blk.h                |  1 -
 include/linux/blk-cgroup.h |  4 +--
 include/linux/blk_types.h  |  2 +-
 6 files changed, 42 insertions(+), 96 deletions(-)

Cc: Josef Bacik <josef@toxicpanda.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Bart Van Assche <bvanassche@acm.org>

-- 
2.20.1

[PATCH 1/3] block: move blk_exit_queue into __blk_release_queue

[Ming Lei]

Commit 498f6650aec8 ("block: Fix a race between the cgroup code and
request queue initialization") moves what blk_exit_queue does into
blk_cleanup_queue() for fixing issue caused by changing back
queue lock.

However, after legacy request IO path is killed, driver queue lock
won't be used at all, and there isn't story for changing back
queue lock. Then the issue addressed by Commit 498f6650aec8 doesn't
exist any more.

So move move blk_exit_queue into __blk_release_queue.

This patch basically reverts the following two commits:

	498f6650aec8 block: Fix a race between the cgroup code and request queue initialization
	24ecc3585348 block: Ensure that a request queue is dissociated from the cgroup controller

Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-core.c  | 37 -------------------------------------
 block/blk-sysfs.c | 47 ++++++++++++++++++++++++++++++++---------------
 block/blk.h       |  1 -
 3 files changed, 32 insertions(+), 53 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 419d600e6637..2af1e54870e6 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -282,35 +282,6 @@ void blk_set_queue_dying(struct request_queue *q)
 }
 EXPORT_SYMBOL_GPL(blk_set_queue_dying);
 
-/* Unconfigure the I/O scheduler and dissociate from the cgroup controller. */
-void blk_exit_queue(struct request_queue *q)
-{
-	/*
-	 * Since the I/O scheduler exit code may access cgroup information,
-	 * perform I/O scheduler exit before disassociating from the block
-	 * cgroup controller.
-	 */
-	if (q->elevator) {
-		ioc_clear_queue(q);
-		elevator_exit(q, q->elevator);
-		q->elevator = NULL;
-	}
-
-	/*
-	 * Remove all references to @q from the block cgroup controller before
-	 * restoring @q->queue_lock to avoid that restoring this pointer causes
-	 * e.g. blkcg_print_blkgs() to crash.
-	 */
-	blkcg_exit_queue(q);
-
-	/*
-	 * Since the cgroup code may dereference the @q->backing_dev_info
-	 * pointer, only decrease its reference count after having removed the
-	 * association with the block cgroup controller.
-	 */
-	bdi_put(q->backing_dev_info);
-}
-
 /**
  * blk_cleanup_queue - shutdown a request queue
  * @q: request queue to shutdown
@@ -346,14 +317,6 @@ void blk_cleanup_queue(struct request_queue *q)
 	del_timer_sync(&q->backing_dev_info->laptop_mode_wb_timer);
 	blk_sync_queue(q);
 
-	/*
-	 * I/O scheduler exit is only safe after the sysfs scheduler attribute
-	 * has been removed.
-	 */
-	WARN_ON_ONCE(q->kobj.state_in_sysfs);
-
-	blk_exit_queue(q);
-
 	if (queue_is_mq(q))
 		blk_mq_exit_queue(q);
 
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index a16a02c52a85..75b5281cc577 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -840,6 +840,36 @@ static void blk_free_queue_rcu(struct rcu_head *rcu_head)
 	kmem_cache_free(blk_requestq_cachep, q);
 }
 
+/* Unconfigure the I/O scheduler and dissociate from the cgroup controller. */
+static void blk_exit_queue(struct request_queue *q)
+{
+	/*
+	 * Since the I/O scheduler exit code may access cgroup information,
+	 * perform I/O scheduler exit before disassociating from the block
+	 * cgroup controller.
+	 */
+	if (q->elevator) {
+		ioc_clear_queue(q);
+		elevator_exit(q, q->elevator);
+		q->elevator = NULL;
+	}
+
+	/*
+	 * Remove all references to @q from the block cgroup controller before
+	 * restoring @q->queue_lock to avoid that restoring this pointer causes
+	 * e.g. blkcg_print_blkgs() to crash.
+	 */
+	blkcg_exit_queue(q);
+
+	/*
+	 * Since the cgroup code may dereference the @q->backing_dev_info
+	 * pointer, only decrease its reference count after having removed the
+	 * association with the block cgroup controller.
+	 */
+	bdi_put(q->backing_dev_info);
+}
+
+
 /**
  * __blk_release_queue - release a request queue
  * @work: pointer to the release_work member of the request queue to be released
@@ -860,23 +890,10 @@ static void __blk_release_queue(struct work_struct *work)
 		blk_stat_remove_callback(q, q->poll_cb);
 	blk_stat_free_callback(q->poll_cb);
 
-	if (!blk_queue_dead(q)) {
-		/*
-		 * Last reference was dropped without having called
-		 * blk_cleanup_queue().
-		 */
-		WARN_ONCE(blk_queue_init_done(q),
-			  "request queue %p has been registered but blk_cleanup_queue() has not been called for that queue\n",
-			  q);
-		blk_exit_queue(q);
-	}
-
-	WARN(blk_queue_root_blkg(q),
-	     "request queue %p is being released but it has not yet been removed from the blkcg controller\n",
-	     q);
-
 	blk_free_queue_stats(q->stats);
 
+	blk_exit_queue(q);
+
 	blk_queue_free_zone_bitmaps(q);
 
 	if (queue_is_mq(q))
diff --git a/block/blk.h b/block/blk.h
index e27fd1512e4b..91b3581b7c7a 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -50,7 +50,6 @@ struct blk_flush_queue *blk_alloc_flush_queue(struct request_queue *q,
 		int node, int cmd_size, gfp_t flags);
 void blk_free_flush_queue(struct blk_flush_queue *q);
 
-void blk_exit_queue(struct request_queue *q);
 void blk_rq_bio_prep(struct request_queue *q, struct request *rq,
 			struct bio *bio);
 void blk_freeze_queue(struct request_queue *q);
-- 
2.20.1

[PATCH 2/3] block: don't protect generic_make_request_checks with blk_queue_enter

[Ming Lei]

Now a063057d7c73 ("block: Fix a race between request queue removal and
the block cgroup controller") has been reverted, and blkcg_exit_queue()
won't be called in blk_cleanup_queue() any more.

So don't need to protect generic_make_request_checks() with
blk_queue_enter(), then the total mess can be cleaned.

37f9579f4c31 ("blk-mq: Avoid that submitting a bio concurrently with device
removal triggers a crash") is reverted.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-core.c | 37 ++++++-------------------------------
 1 file changed, 6 insertions(+), 31 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 2af1e54870e6..bca63e545f05 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -956,22 +956,8 @@ blk_qc_t generic_make_request(struct bio *bio)
 	 * yet.
 	 */
 	struct bio_list bio_list_on_stack[2];
-	blk_mq_req_flags_t flags = 0;
-	struct request_queue *q = bio->bi_disk->queue;
 	blk_qc_t ret = BLK_QC_T_NONE;
 
-	if (bio->bi_opf & REQ_NOWAIT)
-		flags = BLK_MQ_REQ_NOWAIT;
-	if (bio_flagged(bio, BIO_QUEUE_ENTERED))
-		blk_queue_enter_live(q);
-	else if (blk_queue_enter(q, flags) < 0) {
-		if (!blk_queue_dying(q) && (bio->bi_opf & REQ_NOWAIT))
-			bio_wouldblock_error(bio);
-		else
-			bio_io_error(bio);
-		return ret;
-	}
-
 	if (!generic_make_request_checks(bio))
 		goto out;
 
@@ -1008,22 +994,11 @@ blk_qc_t generic_make_request(struct bio *bio)
 	bio_list_init(&bio_list_on_stack[0]);
 	current->bio_list = bio_list_on_stack;
 	do {
-		bool enter_succeeded = true;
-
-		if (unlikely(q != bio->bi_disk->queue)) {
-			if (q)
-				blk_queue_exit(q);
-			q = bio->bi_disk->queue;
-			flags = 0;
-			if (bio->bi_opf & REQ_NOWAIT)
-				flags = BLK_MQ_REQ_NOWAIT;
-			if (blk_queue_enter(q, flags) < 0) {
-				enter_succeeded = false;
-				q = NULL;
-			}
-		}
+		struct request_queue *q = bio->bi_disk->queue;
+		blk_mq_req_flags_t flags = bio->bi_opf & REQ_NOWAIT ?
+			BLK_MQ_REQ_NOWAIT : 0;
 
-		if (enter_succeeded) {
+		if (likely(blk_queue_enter(q, flags) == 0)) {
 			struct bio_list lower, same;
 
 			/* Create a fresh bio_list for all subordinate requests */
@@ -1031,6 +1006,8 @@ blk_qc_t generic_make_request(struct bio *bio)
 			bio_list_init(&bio_list_on_stack[0]);
 			ret = q->make_request_fn(q, bio);
 
+			blk_queue_exit(q);
+
 			/* sort new bios into those for a lower level
 			 * and those for the same level
 			 */
@@ -1057,8 +1034,6 @@ blk_qc_t generic_make_request(struct bio *bio)
 	current->bio_list = NULL; /* deactivate */
 
 out:
-	if (q)
-		blk_queue_exit(q);
 	return ret;
 }
 EXPORT_SYMBOL(generic_make_request);
-- 
2.20.1

[PATCH 3/3] block: rename BIO_QUEUE_ENTERED as BIO_SPLITTED

[Ming Lei]

cd4a4ae4683d ("block: don't use blocking queue entered for recursive
bio submits") introduces BIO_QUEUE_ENTERED to avoid blocking queue entered
for recursive bio submits. Now there isn't such use any more. The only
one use is for cgroup accounting on splitted bio, so rename it
as BIO_SPLITTED.

Cc: Josef Bacik <josef@toxicpanda.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-merge.c          | 10 +---------
 include/linux/blk-cgroup.h |  4 ++--
 include/linux/blk_types.h  |  2 +-
 3 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 21e87a714a73..5fd81cd86928 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -332,15 +332,7 @@ void blk_queue_split(struct request_queue *q, struct bio **bio)
 		/* there isn't chance to merge the splitted bio */
 		split->bi_opf |= REQ_NOMERGE;
 
-		/*
-		 * Since we're recursing into make_request here, ensure
-		 * that we mark this bio as already having entered the queue.
-		 * If not, and the queue is going away, we can get stuck
-		 * forever on waiting for the queue reference to drop. But
-		 * that will never happen, as we're already holding a
-		 * reference to it.
-		 */
-		bio_set_flag(*bio, BIO_QUEUE_ENTERED);
+		bio_set_flag(*bio, BIO_SPLITTED);
 
 		bio_chain(split, *bio);
 		trace_block_split(q, split, (*bio)->bi_iter.bi_sector);
diff --git a/include/linux/blk-cgroup.h b/include/linux/blk-cgroup.h
index 76c61318fda5..a24c9a04f79f 100644
--- a/include/linux/blk-cgroup.h
+++ b/include/linux/blk-cgroup.h
@@ -792,11 +792,11 @@ static inline bool blkcg_bio_issue_check(struct request_queue *q,
 
 	if (!throtl) {
 		/*
-		 * If the bio is flagged with BIO_QUEUE_ENTERED it means this
+		 * If the bio is flagged with BIO_SPLITTED it means this
 		 * is a split bio and we would have already accounted for the
 		 * size of the bio.
 		 */
-		if (!bio_flagged(bio, BIO_QUEUE_ENTERED))
+		if (!bio_flagged(bio, BIO_SPLITTED))
 			blkg_rwstat_add(&blkg->stat_bytes, bio->bi_opf,
 					bio->bi_iter.bi_size);
 		blkg_rwstat_add(&blkg->stat_ios, bio->bi_opf, 1);
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index be418275763c..d7235009f3a7 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -229,7 +229,7 @@ enum {
 				 * throttling rules. Don't do it again. */
 	BIO_TRACE_COMPLETION,	/* bio_endio() should trace the final completion
 				 * of this bio. */
-	BIO_QUEUE_ENTERED,	/* can use blk_queue_enter_live() */
+	BIO_SPLITTED,		/* splitted bio */
 	BIO_TRACKED,		/* set if bio goes through the rq_qos path */
 	BIO_FLAG_LAST
 };
-- 
2.20.1

Re: [PATCH 3/3] block: rename BIO_QUEUE_ENTERED as BIO_SPLITTED

[Josef Bacik]

On Wed, May 15, 2019 at 11:03:10AM +0800, Ming Lei wrote:
> cd4a4ae4683d ("block: don't use blocking queue entered for recursive
> bio submits") introduces BIO_QUEUE_ENTERED to avoid blocking queue entered
> for recursive bio submits. Now there isn't such use any more. The only
> one use is for cgroup accounting on splitted bio, so rename it
> as BIO_SPLITTED.
> 
> Cc: Josef Bacik <josef@toxicpanda.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Bart Van Assche <bvanassche@acm.org>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH 1/3] block: move blk_exit_queue into __blk_release_queue

[Christoph Hellwig]

Looks good,

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

Re: [PATCH 2/3] block: don't protect generic_make_request_checks with blk_queue_enter

[Christoph Hellwig]

Looks good,

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

Re: [PATCH 3/3] block: rename BIO_QUEUE_ENTERED as BIO_SPLITTED

[Christoph Hellwig]

On Wed, May 15, 2019 at 11:03:10AM +0800, Ming Lei wrote:
> cd4a4ae4683d ("block: don't use blocking queue entered for recursive
> bio submits") introduces BIO_QUEUE_ENTERED to avoid blocking queue entered
> for recursive bio submits. Now there isn't such use any more. The only
> one use is for cgroup accounting on splitted bio, so rename it
> as BIO_SPLITTED.

Actually - this now is only used for accounting.  What about renaming the
flag to BIO_ACCOUNTED and just test and set it in blkcg_bio_issue_check?

That function looks way too big to be inline while we're at it..

Re: [PATCH 0/3] block: queue exit cleanup

[Christoph Hellwig]

Jens, do you plan to pick this up (at last patches 1 and 2)?  It
fixes a NULL pointer deref and a md raid issue.

Re: [PATCH 0/3] block: queue exit cleanup

[Jens Axboe]

On 5/29/19 12:52 AM, Christoph Hellwig wrote:
> Jens, do you plan to pick this up (at last patches 1 and 2)?  It
> fixes a NULL pointer deref and a md raid issue.

Agree, applied 1-2.

-- 
Jens Axboe