[PATCH 0/4] cancel all throttled bios in blk_cleanup_queue()

[PATCH 0/4] cancel all throttled bios in blk_cleanup_queue()

[Yu Kuai]

If blk_cleanup_queue() is called when some io are still throttled,
such io will not be handled until the throttle is done, which is not
necessary because the queue is already dying.

This patch series handle such in blk_cleanup_queue().

Yu Kuai (4):
  Revert "blk-throttle: remove tg_drain_bios"
  blk-throtl: don't warn in tg_drain_bios()
  blk-throtl: introduce blk_throtl_cancel_bios()
  block: cancel all throttled bios in blk_cleanup_queue()

 block/blk-core.c     |  3 ++
 block/blk-throttle.c | 70 +++++++++++++++++++++++++++++++++++++++++---
 block/blk.h          |  2 ++
 3 files changed, 71 insertions(+), 4 deletions(-)

-- 
2.31.1

[PATCH 1/4] Revert "blk-throttle: remove tg_drain_bios"

[Yu Kuai]

This reverts commit 32e3374304c7c317c05a61f3ddc315dbd46424f2.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-throttle.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 7c4e7993ba97..43dee985170b 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -2412,6 +2412,28 @@ void blk_throtl_bio_endio(struct bio *bio)
 }
 #endif
 
+/*
+ * Dispatch all bios from all children tg's queued on @parent_sq.  On
+ * return, @parent_sq is guaranteed to not have any active children tg's
+ * and all bios from previously active tg's are on @parent_sq->bio_lists[].
+ */
+static void tg_drain_bios(struct throtl_service_queue *parent_sq)
+{
+	struct throtl_grp *tg;
+
+	while ((tg = throtl_rb_first(parent_sq))) {
+		struct throtl_service_queue *sq = &tg->service_queue;
+		struct bio *bio;
+
+		throtl_dequeue_tg(tg);
+
+		while ((bio = throtl_peek_queued(&sq->queued[READ])))
+			tg_dispatch_one_bio(tg, bio_data_dir(bio));
+		while ((bio = throtl_peek_queued(&sq->queued[WRITE])))
+			tg_dispatch_one_bio(tg, bio_data_dir(bio));
+	}
+}
+
 int blk_throtl_init(struct request_queue *q)
 {
 	struct throtl_data *td;
-- 
2.31.1

[PATCH 2/4] blk-throtl: don't warn in tg_drain_bios()

[Yu Kuai]

tg_drain_bios() will iterate until throtl_rb_first() return NULL,
don't warn in such situation.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-throttle.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 43dee985170b..3892ead7a0b5 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -644,12 +644,13 @@ static void throtl_pd_free(struct blkg_policy_data *pd)
 }
 
 static struct throtl_grp *
-throtl_rb_first(struct throtl_service_queue *parent_sq)
+throtl_rb_first(struct throtl_service_queue *parent_sq, bool warn)
 {
 	struct rb_node *n;
 
 	n = rb_first_cached(&parent_sq->pending_tree);
-	WARN_ON_ONCE(!n);
+	if (warn)
+		WARN_ON_ONCE(!n);
 	if (!n)
 		return NULL;
 	return rb_entry_tg(n);
@@ -667,7 +668,7 @@ static void update_min_dispatch_time(struct throtl_service_queue *parent_sq)
 {
 	struct throtl_grp *tg;
 
-	tg = throtl_rb_first(parent_sq);
+	tg = throtl_rb_first(parent_sq, true);
 	if (!tg)
 		return;
 
@@ -1236,7 +1237,7 @@ static int throtl_select_dispatch(struct throtl_service_queue *parent_sq)
 		if (!parent_sq->nr_pending)
 			break;
 
-		tg = throtl_rb_first(parent_sq);
+		tg = throtl_rb_first(parent_sq, true);
 		if (!tg)
 			break;
 
@@ -2421,7 +2422,7 @@ static void tg_drain_bios(struct throtl_service_queue *parent_sq)
 {
 	struct throtl_grp *tg;
 
-	while ((tg = throtl_rb_first(parent_sq))) {
+	while ((tg = throtl_rb_first(parent_sq, false))) {
 		struct throtl_service_queue *sq = &tg->service_queue;
 		struct bio *bio;
 
-- 
2.31.1

[PATCH 3/4] blk-throtl: introduce blk_throtl_cancel_bios()

[Yu Kuai]

This function is used to cancel all throttled bios when queue is
dying. Noted this modification is mainly from revertion of commit
b77412372b68 ("blk-throttle: remove blk_throtl_drain").

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-throttle.c | 39 +++++++++++++++++++++++++++++++++++++++
 block/blk.h          |  2 ++
 2 files changed, 41 insertions(+)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 3892ead7a0b5..4227dcf2dd3b 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -2435,6 +2435,45 @@ static void tg_drain_bios(struct throtl_service_queue *parent_sq)
 	}
 }
 
+/**
+ * blk_throtl_cancel_bios - cancel throttled bios
+ * @q: request_queue to cancel throttled bios for
+ *
+ * This function is called when queue is dying, error all currently
+ * throttled bios on @q so that user threads that are waiting for the bios
+ * can exit.
+ */
+void blk_throtl_cancel_bios(struct request_queue *q)
+{
+	struct throtl_data *td = q->td;
+	struct blkcg_gq *blkg;
+	struct cgroup_subsys_state *pos_css;
+	struct bio *bio;
+	int rw;
+
+	rcu_read_lock();
+
+	/*
+	 * Drain each tg while doing post-order walk on the blkg tree, so
+	 * that all bios are propagated to td->service_queue.  It'd be
+	 * better to walk service_queue tree directly but blkg walk is
+	 * easier.
+	 */
+	blkg_for_each_descendant_post(blkg, pos_css, td->queue->root_blkg)
+		tg_drain_bios(&blkg_to_tg(blkg)->service_queue);
+
+	/* finally, transfer bios from top-level tg's into the td */
+	tg_drain_bios(&td->service_queue);
+
+	rcu_read_unlock();
+
+	/* all bios now should be in td->service_queue, cancel them */
+	for (rw = READ; rw <= WRITE; rw++)
+		while ((bio = throtl_pop_queued(&td->service_queue.queued[rw],
+						NULL)))
+			bio_io_error(bio);
+}
+
 int blk_throtl_init(struct request_queue *q)
 {
 	struct throtl_data *td;
diff --git a/block/blk.h b/block/blk.h
index 7d2a0ba7ed21..c407581a3a19 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -287,12 +287,14 @@ int create_task_io_context(struct task_struct *task, gfp_t gfp_mask, int node);
  * Internal throttling interface
  */
 #ifdef CONFIG_BLK_DEV_THROTTLING
+extern void blk_throtl_cancel_bios(struct request_queue *q);
 extern int blk_throtl_init(struct request_queue *q);
 extern void blk_throtl_exit(struct request_queue *q);
 extern void blk_throtl_register_queue(struct request_queue *q);
 extern void blk_throtl_charge_bio_split(struct bio *bio);
 bool blk_throtl_bio(struct bio *bio);
 #else /* CONFIG_BLK_DEV_THROTTLING */
+static inline void blk_throtl_cancel_bios(struct request_queue *q) { }
 static inline int blk_throtl_init(struct request_queue *q) { return 0; }
 static inline void blk_throtl_exit(struct request_queue *q) { }
 static inline void blk_throtl_register_queue(struct request_queue *q) { }
-- 
2.31.1

[PATCH 4/4] block: cancel all throttled bios in blk_cleanup_queue()

[Yu Kuai]

Throttled bios can't be issued after queue is dying in
blk_cleanup_queue(), thus it's better to cancel them immediately
rather than waiting for throttle is done.

For example, if user thread is throttled with low bps while is
issuing large io, and the device is deleted. The user thread will
wait for a long time for io to return.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/blk-core.c b/block/blk-core.c
index 5454db2fa263..356a56318068 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -389,6 +389,9 @@ void blk_cleanup_queue(struct request_queue *q)
 
 	blk_queue_flag_set(QUEUE_FLAG_DEAD, q);
 
+	if (q->root_blkg)
+		blk_throtl_cancel_bios(q);
+
 	/* for synchronous bio-based driver finish in-flight integrity i/o */
 	blk_flush_integrity();
 
-- 
2.31.1

Re: [PATCH 4/4] block: cancel all throttled bios in blk_cleanup_queue()

[Christoph Hellwig]

On Wed, Sep 22, 2021 at 08:51:15PM +0800, Yu Kuai wrote:
> Throttled bios can't be issued after queue is dying in
> blk_cleanup_queue(), thus it's better to cancel them immediately
> rather than waiting for throttle is done.
> 
> For example, if user thread is throttled with low bps while is
> issuing large io, and the device is deleted. The user thread will
> wait for a long time for io to return.

blk_cleanup_queue is too late, this will need to go into del_gendisk
and on top of my "tear down file system I/O in del_gendisk" series.

Re: [PATCH 4/4] block: cancel all throttled bios in blk_cleanup_queue()

[yukuai (C)]

On 2021/09/22 22:34, Christoph Hellwig wrote:
> On Wed, Sep 22, 2021 at 08:51:15PM +0800, Yu Kuai wrote:
>> Throttled bios can't be issued after queue is dying in
>> blk_cleanup_queue(), thus it's better to cancel them immediately
>> rather than waiting for throttle is done.
>>
>> For example, if user thread is throttled with low bps while is
>> issuing large io, and the device is deleted. The user thread will
>> wait for a long time for io to return.
> 
> blk_cleanup_queue is too late, this will need to go into del_gendisk
> and on top of my "tear down file system I/O in del_gendisk" series.
> .
> 

Hi,

I see what your series is trying to do, it semms not easy to
collaborate. I'll wait for your series to applied if you don't mind,
or you can take over this series.

Thanks,
Kuai

Re: [PATCH 1/4] Revert "blk-throttle: remove tg_drain_bios"

[Christoph Hellwig]

On Sat, Nov 27, 2021 at 06:10:56PM +0800, Yu Kuai wrote:
> This reverts commit 32e3374304c7c317c05a61f3ddc315dbd46424f2.

Please re-add the function together with the actual users in a commit
that has a proper commit log.

[PATCH 1/4] Revert "blk-throttle: remove tg_drain_bios"

[Yu Kuai]

This reverts commit 32e3374304c7c317c05a61f3ddc315dbd46424f2.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-throttle.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 39bb6e68a9a2..230e300c5856 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -2257,6 +2257,28 @@ void blk_throtl_bio_endio(struct bio *bio)
 }
 #endif
 
+/*
+ * Dispatch all bios from all children tg's queued on @parent_sq.  On
+ * return, @parent_sq is guaranteed to not have any active children tg's
+ * and all bios from previously active tg's are on @parent_sq->bio_lists[].
+ */
+static void tg_drain_bios(struct throtl_service_queue *parent_sq)
+{
+	struct throtl_grp *tg;
+
+	while ((tg = throtl_rb_first(parent_sq))) {
+		struct throtl_service_queue *sq = &tg->service_queue;
+		struct bio *bio;
+
+		throtl_dequeue_tg(tg);
+
+		while ((bio = throtl_peek_queued(&sq->queued[READ])))
+			tg_dispatch_one_bio(tg, bio_data_dir(bio));
+		while ((bio = throtl_peek_queued(&sq->queued[WRITE])))
+			tg_dispatch_one_bio(tg, bio_data_dir(bio));
+	}
+}
+
 int blk_throtl_init(struct request_queue *q)
 {
 	struct throtl_data *td;
-- 
2.31.1