* [PATCH 1/3] block: move blk_exit_queue into __blk_release_queue
2019-05-15 3:03 [PATCH 0/3] block: queue exit cleanup Ming Lei
@ 2019-05-15 3:03 ` Ming Lei
2019-05-21 12:53 ` Christoph Hellwig
2019-05-15 3:03 ` [PATCH 2/3] block: don't protect generic_make_request_checks with blk_queue_enter Ming Lei
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Ming Lei @ 2019-05-15 3:03 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Ming Lei, Bart Van Assche, Christoph Hellwig
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 2/3] block: don't protect generic_make_request_checks with blk_queue_enter
2019-05-15 3:03 [PATCH 0/3] block: queue exit cleanup Ming Lei
2019-05-15 3:03 ` [PATCH 1/3] block: move blk_exit_queue into __blk_release_queue Ming Lei
@ 2019-05-15 3:03 ` Ming Lei
2019-05-21 12:57 ` Christoph Hellwig
2019-05-15 3:03 ` [PATCH 3/3] block: rename BIO_QUEUE_ENTERED as BIO_SPLITTED Ming Lei
2019-05-29 6:52 ` [PATCH 0/3] block: queue exit cleanup Christoph Hellwig
3 siblings, 1 reply; 10+ messages in thread
From: Ming Lei @ 2019-05-15 3:03 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Ming Lei, Christoph Hellwig, Bart Van Assche
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 3/3] block: rename BIO_QUEUE_ENTERED as BIO_SPLITTED
2019-05-15 3:03 [PATCH 0/3] block: queue exit cleanup Ming Lei
2019-05-15 3:03 ` [PATCH 1/3] block: move blk_exit_queue into __blk_release_queue Ming Lei
2019-05-15 3:03 ` [PATCH 2/3] block: don't protect generic_make_request_checks with blk_queue_enter Ming Lei
@ 2019-05-15 3:03 ` Ming Lei
2019-05-15 16:01 ` Josef Bacik
2019-05-21 12:59 ` Christoph Hellwig
2019-05-29 6:52 ` [PATCH 0/3] block: queue exit cleanup Christoph Hellwig
3 siblings, 2 replies; 10+ messages in thread
From: Ming Lei @ 2019-05-15 3:03 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Ming Lei, Josef Bacik, Christoph Hellwig, Bart Van Assche
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 3/3] block: rename BIO_QUEUE_ENTERED as BIO_SPLITTED
2019-05-15 3:03 ` [PATCH 3/3] block: rename BIO_QUEUE_ENTERED as BIO_SPLITTED Ming Lei
@ 2019-05-15 16:01 ` Josef Bacik
2019-05-21 12:59 ` Christoph Hellwig
1 sibling, 0 replies; 10+ messages in thread
From: Josef Bacik @ 2019-05-15 16:01 UTC (permalink / raw)
To: Ming Lei
Cc: Jens Axboe, linux-block, Josef Bacik, Christoph Hellwig, Bart Van Assche
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 3/3] block: rename BIO_QUEUE_ENTERED as BIO_SPLITTED
2019-05-15 3:03 ` [PATCH 3/3] block: rename BIO_QUEUE_ENTERED as BIO_SPLITTED Ming Lei
2019-05-15 16:01 ` Josef Bacik
@ 2019-05-21 12:59 ` Christoph Hellwig
1 sibling, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2019-05-21 12:59 UTC (permalink / raw)
To: Ming Lei
Cc: Jens Axboe, linux-block, Josef Bacik, Christoph Hellwig, Bart Van Assche
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..
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/3] block: queue exit cleanup
2019-05-15 3:03 [PATCH 0/3] block: queue exit cleanup Ming Lei
` (2 preceding siblings ...)
2019-05-15 3:03 ` [PATCH 3/3] block: rename BIO_QUEUE_ENTERED as BIO_SPLITTED Ming Lei
@ 2019-05-29 6:52 ` Christoph Hellwig
2019-05-29 12:25 ` Jens Axboe
3 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2019-05-29 6:52 UTC (permalink / raw)
To: Ming Lei
Cc: Jens Axboe, linux-block, Josef Bacik, Christoph Hellwig, Bart Van Assche
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.
^ permalink raw reply [flat|nested] 10+ messages in thread