blkcg: basic accounting and throttling fixes

Move __blkg_release_rcu() above blkg_alloc(). This is a pure code reorganization to prepare for the switch to percpu_ref.

Signed-off-by Tejun Heo <[email protected]>

---
 block/blk-cgroup.c | 46 +++++++++++++++++++++++-----------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

block/blk-cgroup.c +23 -23

index d3f56ba..6482be5 100644
@@ -81,6 +81,29 @@ static void blkg_free(struct blkcg_gq *blkg)
 	kfree(blkg);
 }
 
+/*
+ * A group is RCU protected, but having an rcu lock does not mean that one
+ * can access all the fields of blkg and assume these are valid.  For
+ * example, don't try to follow throtl_data and request queue links.
+ *
+ * Having a reference to blkg under an rcu allows accesses to only values
+ * local to groups like group stats and group rate limits.
+ */
+void __blkg_release_rcu(struct rcu_head *rcu_head)
+{
+	struct blkcg_gq *blkg = container_of(rcu_head, struct blkcg_gq, rcu_head);
+
+	/* release the blkcg and parent blkg refs this blkg has been holding */
+	css_put(&blkg->blkcg->css);
+	if (blkg->parent)
+		blkg_put(blkg->parent);
+
+	wb_congested_put(blkg->wb_congested);
+
+	blkg_free(blkg);
+}
+EXPORT_SYMBOL_GPL(__blkg_release_rcu);
+
 /**
  * blkg_alloc - allocate a blkg
  * @blkcg: block cgroup the new blkg is associated with

@@ -378,29 +401,6 @@ static void blkg_destroy_all(struct request_queue *q)
 }
 
 /*
- * A group is RCU protected, but having an rcu lock does not mean that one
- * can access all the fields of blkg and assume these are valid.  For
- * example, don't try to follow throtl_data and request queue links.
- *
- * Having a reference to blkg under an rcu allows accesses to only values
- * local to groups like group stats and group rate limits.
- */
-void __blkg_release_rcu(struct rcu_head *rcu_head)
-{
-	struct blkcg_gq *blkg = container_of(rcu_head, struct blkcg_gq, rcu_head);
-
-	/* release the blkcg and parent blkg refs this blkg has been holding */
-	css_put(&blkg->blkcg->css);
-	if (blkg->parent)
-		blkg_put(blkg->parent);
-
-	wb_congested_put(blkg->wb_congested);
-
-	blkg_free(blkg);
-}
-EXPORT_SYMBOL_GPL(__blkg_release_rcu);
-
-/*
  * The next function used by blk_queue_for_each_rl().  It's a bit tricky
  * because the root blkg uses @q->root_rl instead of its own rl.
  */

Signature · 1 line

2.9.5

blkcg_gq->refcnt is an atomic_t. This was due to the following two reasons.

* When blkcg_gq was added, the percpu memory allocator didn't support allocations from !GFP_KERNEL contexts. Because blkcg_gq's are created from IO issue paths, it couldn't use GFP_KERNEL allocations.

* A blkcg_gq represents the connection between a cgroup and a request_queue. Because a in-flight bio already pins both, blkcg_gq didn't usually need explicit pinning, making the use of atomic_t acceptable albeit restrictive and fragile.

Now that the percpu allocator supports !GFP_KERNEL allocations, there's no reason to keep using atomic_t refcnt. This will allow clean separation between bio and request layers helping blkcg support in blk-mq.

Signed-off-by Tejun Heo <[email protected]>

---
 block/blk-cgroup.c         | 21 ++++++++++++++++-----
 include/linux/blk-cgroup.h | 13 ++++---------
 2 files changed, 20 insertions(+), 14 deletions(-)

block/blk-cgroup.c +16 -5

index 6482be5..60a4486 100644
@@ -78,6 +78,7 @@ static void blkg_free(struct blkcg_gq *blkg)
 
 	blkg_rwstat_exit(&blkg->stat_ios);
 	blkg_rwstat_exit(&blkg->stat_bytes);
+	percpu_ref_exit(&blkg->refcnt);
 	kfree(blkg);
 }
 

@@ -89,7 +90,7 @@ static void blkg_free(struct blkcg_gq *blkg)
  * Having a reference to blkg under an rcu allows accesses to only values
  * local to groups like group stats and group rate limits.
  */
-void __blkg_release_rcu(struct rcu_head *rcu_head)
+static void blkg_release_rcu(struct rcu_head *rcu_head)
 {
 	struct blkcg_gq *blkg = container_of(rcu_head, struct blkcg_gq, rcu_head);
 

@@ -102,7 +103,13 @@ void __blkg_release_rcu(struct rcu_head *rcu_head)
 
 	blkg_free(blkg);
 }
-EXPORT_SYMBOL_GPL(__blkg_release_rcu);
+
+static void blkg_release(struct percpu_ref *refcnt)
+{
+	struct blkcg_gq *blkg = container_of(refcnt, struct blkcg_gq, refcnt);
+
+	call_rcu(&blkg->rcu_head, blkg_release_rcu);
+}
 
 /**
  * blkg_alloc - allocate a blkg

@@ -123,6 +130,11 @@ static struct blkcg_gq *blkg_alloc(struct blkcg *blkcg, struct request_queue *q,
 	if (!blkg)
 		return NULL;
 
+	if (percpu_ref_init(&blkg->refcnt, blkg_release, 0, gfp_mask)) {
+		kfree(blkg);
+		return NULL;
+	}
+
 	if (blkg_rwstat_init(&blkg->stat_bytes, gfp_mask) ||
 	    blkg_rwstat_init(&blkg->stat_ios, gfp_mask))
 		goto err_free;

@@ -130,7 +142,6 @@ static struct blkcg_gq *blkg_alloc(struct blkcg *blkcg, struct request_queue *q,
 	blkg->q = q;
 	INIT_LIST_HEAD(&blkg->q_node);
 	blkg->blkcg = blkcg;
-	atomic_set(&blkg->refcnt, 1);
 
 	/* root blkg uses @q->root_rl, init rl only for !root blkgs */
 	if (blkcg != &blkcg_root) {

@@ -266,7 +277,7 @@ static struct blkcg_gq *blkg_create(struct blkcg *blkcg,
 		return blkg;
 
 	/* @blkg failed fully initialized, use the usual release path */
-	blkg_put(blkg);
+	percpu_ref_kill(&blkg->refcnt);
 	return ERR_PTR(ret);
 
 err_put_congested:

@@ -373,7 +384,7 @@ static void blkg_destroy(struct blkcg_gq *blkg)
 	 * Put the reference taken at the time of creation so that when all
 	 * queues are gone, group can be destroyed.
 	 */
-	blkg_put(blkg);
+	percpu_ref_kill(&blkg->refcnt);
 }
 
 /**

include/linux/blk-cgroup.h +4 -9

index 8bbc371..c0d4736 100644
@@ -19,7 +19,7 @@
 #include <linux/seq_file.h>
 #include <linux/radix-tree.h>
 #include <linux/blkdev.h>
-#include <linux/atomic.h>
+#include <linux/percpu-refcount.h>
 
 /* percpu_counter batch for blkg_[rw]stats, per-cpu drift doesn't matter */
 #define BLKG_STAT_CPU_BATCH	(INT_MAX / 2)

@@ -123,7 +123,7 @@ struct blkcg_gq {
 	struct request_list		rl;
 
 	/* reference count */
-	atomic_t			refcnt;
+	struct percpu_ref		refcnt;
 
 	/* is this blkg online? protected by both blkcg and q locks */
 	bool				online;

@@ -355,21 +355,16 @@ static inline int blkg_path(struct blkcg_gq *blkg, char *buf, int buflen)
  */
 static inline void blkg_get(struct blkcg_gq *blkg)
 {
-	WARN_ON_ONCE(atomic_read(&blkg->refcnt) <= 0);
-	atomic_inc(&blkg->refcnt);
+	percpu_ref_get(&blkg->refcnt);
 }
 
-void __blkg_release_rcu(struct rcu_head *rcu);
-
 /**
  * blkg_put - put a blkg reference
  * @blkg: blkg to put
  */
 static inline void blkg_put(struct blkcg_gq *blkg)
 {
-	WARN_ON_ONCE(atomic_read(&blkg->refcnt) <= 0);
-	if (atomic_dec_and_test(&blkg->refcnt))
-		call_rcu(&blkg->rcu_head, __blkg_release_rcu);
+	percpu_ref_put(&blkg->refcnt);
 }
 
 /**

Signature · 1 line

2.9.5

From c856a199ec70e4022e997609f1b17b9106408777 Mon Sep 17 00:00:00 2001
From: Tejun Heo <[email protected]>
Date: Mon, 13 Nov 2017 12:11:57 -0800

On the legacy request_queue, each blkcg_gq has a dedicated request_list that requests for the cgroup are allocated from. Each request points to the originating request_list and cgroup membership can be determined by examining which cgroup the associated request_list belongs to.

This doesn't work for blk-mq which doesn't use request_list. Let's associate each request with the blkcg_gq directly. Except for minor wrapper changes, this doesn't affect the legacy request_queue and will allow blk-mq to track each request's cgroup association.

v2: Build fix for !CONFIG_BLK_CGROUP.

Signed-off-by Tejun Heo <[email protected]>

---
 block/blk-mq.c             | 2 +-
 include/linux/blk-cgroup.h | 7 +++++--
 include/linux/blkdev.h     | 2 +-
 3 files changed, 7 insertions(+), 4 deletions(-)

block/blk-mq.c +1 -1

index 98a1860..af958c4 100644
@@ -306,7 +306,7 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
 	rq->part = NULL;
 	rq->start_time = jiffies;
 #ifdef CONFIG_BLK_CGROUP
-	rq->rl = NULL;
+	rq->blkg = NULL;
 	set_start_time_ns(rq);
 	rq->io_start_time_ns = 0;
 #endif

include/linux/blk-cgroup.h +0 -0

index c0d4736..47db75a 100644

include/linux/blk-cgroup.h +4 -1

 /**

@@ -474,7 +474,10 @@ static inline void blk_rq_set_rl(struct request *rq, struct request_list *rl)
  */
 static inline struct request_list *blk_rq_rl(struct request *rq)
 {
-	return rq->rl;
+	if (!rq->blkg->parent)
+		return &rq->q->root_rl;
+	else
+		return &rq->blkg->rl;
 }
 
 struct request_list *__blk_queue_next_rl(struct request_list *rl,

include/linux/blkdev.h +1 -1

index 8da6637..6ec1067 100644
@@ -206,7 +206,7 @@ struct request {
 	unsigned long start_time;
 	struct blk_issue_stat issue_stat;
 #ifdef CONFIG_BLK_CGROUP
-	struct request_list *rl;		/* rl this rq is alloced from */
+	struct blkcg_gq *blkg;			/* blkg of this rq */
 	unsigned long long start_time_ns;
 	unsigned long long io_start_time_ns;    /* when passed to hardware */
 #endif

Signature · 1 line

2.9.5

After patch 5, we don't really need the blk_put_rl, blk_get_rl, blk_rq_set_rl functions. I'd like to delete them and only keep blk_rq_rl. The name blk_rq_set_rl is confusing too.

blkcg_bio_issue_check() open codes blkcg_gq lookup and creation using blkg_lookup() and blkg_lookup_create(). Refactor the code so that

* blkg_lookup_create() is renamed to blkcg_lookup_create_locked().

* blkg_lookup_create() is now a new function which encapsulates the RCU protected lookup and queue_locked protected creation.

* blkg_lookup_create() is guaranteed to return a non-NULL blkcg_gq. The NULL checks are removed from the users.

This is pure refactoring and doesn't introduce any functional changes.

Signed-off-by Tejun Heo <[email protected]>

---
 block/blk-cgroup.c         |  6 +++---
 block/blk-throttle.c       |  2 +-
 include/linux/blk-cgroup.h | 32 +++++++++++++++++++++-----------
 3 files changed, 25 insertions(+), 15 deletions(-)

block/blk-cgroup.c +3 -3

index 60a4486..490b0a6 100644
@@ -290,7 +290,7 @@ static struct blkcg_gq *blkg_create(struct blkcg *blkcg,
 }
 
 /**
- * blkg_lookup_create - lookup blkg, try to create one if not there
+ * blkg_lookup_create_locked - lookup blkg, try to create one if not there
  * @blkcg: blkcg of interest
  * @q: request_queue of interest
  *

@@ -303,8 +303,8 @@ static struct blkcg_gq *blkg_create(struct blkcg *blkcg,
  * value on error.  If @q is dead, returns ERR_PTR(-EINVAL).  If @q is not
  * dead and bypassing, returns ERR_PTR(-EBUSY).
  */
-struct blkcg_gq *blkg_lookup_create(struct blkcg *blkcg,
-				    struct request_queue *q)
+struct blkcg_gq *blkg_lookup_create_locked(struct blkcg *blkcg,
+					   struct request_queue *q)
 {
 	struct blkcg_gq *blkg;
 

block/blk-throttle.c +1 -1

index 8631763..1e6916b 100644
@@ -2123,7 +2123,7 @@ bool blk_throtl_bio(struct request_queue *q, struct blkcg_gq *blkg,
 		    struct bio *bio)
 {
 	struct throtl_qnode *qn = NULL;
-	struct throtl_grp *tg = blkg_to_tg(blkg ?: q->root_blkg);
+	struct throtl_grp *tg = blkg_to_tg(blkg);
 	struct throtl_service_queue *sq;
 	bool rw = bio_data_dir(bio);
 	bool throttled = false;

include/linux/blk-cgroup.h +21 -11

index b5721d32..1de5158 100644
@@ -172,8 +172,8 @@ extern struct cgroup_subsys_state * const blkcg_root_css;
 
 struct blkcg_gq *blkg_lookup_slowpath(struct blkcg *blkcg,
 				      struct request_queue *q, bool update_hint);
-struct blkcg_gq *blkg_lookup_create(struct blkcg *blkcg,
-				    struct request_queue *q);
+struct blkcg_gq *blkg_lookup_create_locked(struct blkcg *blkcg,
+					   struct request_queue *q);
 int blkcg_init_queue(struct request_queue *q);
 void blkcg_drain_queue(struct request_queue *q);
 void blkcg_exit_queue(struct request_queue *q);

@@ -680,6 +680,24 @@ static inline bool blk_throtl_bio(struct request_queue *q, struct blkcg_gq *blkg
 				  struct bio *bio) { return false; }
 #endif
 
+static inline struct blkcg_gq *blkg_lookup_create(struct blkcg *blkcg,
+						  struct request_queue *q)
+{
+	struct blkcg_gq *blkg;
+
+	blkg = blkg_lookup(blkcg, q);
+	if (likely(blkg))
+		return blkg;
+
+	spin_lock_irq(q->queue_lock);
+	blkg = blkg_lookup_create_locked(blkcg, q);
+	spin_unlock_irq(q->queue_lock);
+	if (likely(!IS_ERR(blkg)))
+		return blkg;
+	else
+		return q->root_blkg;
+}
+
 static inline bool blkcg_bio_issue_check(struct request_queue *q,
 					 struct bio *bio)
 {

@@ -693,19 +711,11 @@ static inline bool blkcg_bio_issue_check(struct request_queue *q,
 	/* associate blkcg if bio hasn't attached one */
 	bio_associate_blkcg(bio, &blkcg->css);
 
-	blkg = blkg_lookup(blkcg, q);
-	if (unlikely(!blkg)) {
-		spin_lock_irq(q->queue_lock);
-		blkg = blkg_lookup_create(blkcg, q);
-		if (IS_ERR(blkg))
-			blkg = NULL;
-		spin_unlock_irq(q->queue_lock);
-	}
+	blkg = blkg_lookup_create(blkcg, q);
 
 	throtl = blk_throtl_bio(q, blkg, bio);
 
 	if (!throtl) {
-		blkg = blkg ?: q->root_blkg;
 		blkg_rwstat_add(&blkg->stat_bytes, bio->bi_opf,
 				bio->bi_iter.bi_size);
 		blkg_rwstat_add(&blkg->stat_ios, bio->bi_opf, 1);

Signature · 1 line

2.9.5

blkcg accounting is currently bio based, which is silly for request based request_queues. This is silly as the number of bios doesn't have much to do with the actual number of IOs issued to the underlying device (can be significantly higher or lower) and may change depending on the implementation details on how the bios are issued (e.g. from the recent split-bios-while-issuing change).

request based request_queues have QUEUE_FLAG_IO_STAT set by default which controls the gendisk accounting. Do cgroup accounting for those request_queues together with gendisk accounting on request completion.

This makes cgroup accounting consistent with gendisk accounting and what's happening on the system.

Signed-off-by Tejun Heo <[email protected]>

---
 block/blk-core.c           |  3 +++
 include/linux/blk-cgroup.h | 18 +++++++++++++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

block/blk-core.c +3 -0

index 048be4a..ad23b96 100644
@@ -2429,6 +2429,7 @@ void blk_account_io_completion(struct request *req, unsigned int bytes)
 		cpu = part_stat_lock();
 		part = req->part;
 		part_stat_add(cpu, part, sectors[rw], bytes >> 9);
+		blkcg_account_io_completion(req, bytes);
 		part_stat_unlock();
 	}
 }

@@ -2454,6 +2455,8 @@ void blk_account_io_done(struct request *req)
 		part_round_stats(req->q, cpu, part);
 		part_dec_in_flight(req->q, part, rw);
 
+		blkcg_account_io_done(req);
+
 		hd_struct_put(part);
 		part_stat_unlock();
 	}

include/linux/blk-cgroup.h +0 -0

index 96eed0f..f2f9691 100644

include/linux/blk-cgroup.h +15 -0

 		blkg_rwstat_add(&blkg->stat_bytes, bio->bi_opf,
 				bio->bi_iter.bi_size);
 		blkg_rwstat_add(&blkg->stat_ios, bio->bi_opf, 1);

@@ -764,6 +765,17 @@ static inline void blk_rq_disassociate_blkg(struct request *rq)
 	rq->blkg = NULL;
 }
 
+static inline void blkcg_account_io_completion(struct request *rq,
+					       unsigned int bytes)
+{
+	blkg_rwstat_add(&rq->blkg->stat_bytes, rq_data_dir(rq), bytes);
+}
+
+static inline void blkcg_account_io_done(struct request *rq)
+{
+	blkg_rwstat_add(&rq->blkg->stat_ios, rq_data_dir(rq), 1);
+}
+
 #else	/* CONFIG_BLK_CGROUP */
 
 struct blkcg {

@@ -823,6 +835,10 @@ static inline bool blkcg_bio_issue_check(struct request_queue *q,
 static inline void blk_rq_associate_blkg(struct request *rq, struct blkcg *blkcg) { }
 static inline void blk_rq_disassociate_blkg(struct request *rq) { }
 
+static inline void blkcg_account_io_completion(struct request *rq,
+					       unsigned int bytes) { }
+static inline void blkcg_account_io_done(struct request *rq) { }
+
 #define blk_queue_for_each_rl(rl, q)	\
 	for ((rl) = &(q)->root_rl; (rl); (rl) = NULL)
 

Signature · 1 line

2.9.5

Quoted 49 lines · Tejun Heo

blkcg accounting is currently bio based, which is silly for request based request_queues. This is silly as the number of bios doesn't have much to do with the actual number of IOs issued to the underlying device (can be significantly higher or lower) and may change depending on the implementation details on how the bios are issued (e.g. from the recent split-bios-while-issuing change).

request based request_queues have QUEUE_FLAG_IO_STAT set by default which controls the gendisk accounting. Do cgroup accounting for those request_queues together with gendisk accounting on request completion.

This makes cgroup accounting consistent with gendisk accounting and what's happening on the system.

Signed-off-by Tejun Heo <[email protected]>

---
 block/blk-core.c           |  3 +++
 include/linux/blk-cgroup.h | 18 +++++++++++++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

block/blk-core.c +3 -0

index 048be4a..ad23b96 100644
@@ -2429,6 +2429,7 @@ void blk_account_io_completion(struct request *req, unsigned int bytes)
 		cpu = part_stat_lock();
 		part = req->part;
 		part_stat_add(cpu, part, sectors[rw], bytes >> 9);
+		blkcg_account_io_completion(req, bytes);
 		part_stat_unlock();
 	}
 }

@@ -2454,6 +2455,8 @@ void blk_account_io_done(struct request *req)
 		part_round_stats(req->q, cpu, part);
 		part_dec_in_flight(req->q, part, rw);
 
+		blkcg_account_io_done(req);
+
 		hd_struct_put(part);
 		part_stat_unlock();
 	}

include/linux/blk-cgroup.h +0 -0

index 96eed0f..f2f9691 100644
@@ -715,7 +715,8 @@ static inline bool blkcg_bio_issue_check(struct request_queue *q,
 
 	throtl = blk_throtl_bio(q, blkg, bio);
 

include/linux/blk-cgroup.h static inline bool blkcg_bio_issue_check(struct request_queue *q, · line 719

Quoted 3 lines

include/linux/blk-cgroup.h +2 -1

-	if (!throtl) {
+	/* if @q does io stat, blkcg stats are updated together with them */
+	if (!blk_queue_io_stat(q) && !throtl) {

Reviewed-by Shaohua Li <[email protected]>

One nitpick, can we use q->request_fn to determine request based queue? I think that is more reliable and usual way for this.

BIO_THROTTLED is used to mark already throttled bios so that a bio doesn't get throttled multiple times. The flag gets set when the bio starts getting dispatched from blk-throtl and cleared when it leaves blk-throtl.

Unfortunately, this doesn't work when the request_queue decides to split or requeue the bio and ends up throttling the same IO multiple times. This condition gets easily triggered and often leads to multiple times lower bandwidth limit being enforced than configured.

Fix it by always setting BIO_THROTTLED for bios recursing to the same request_queue and clearing only when a bio leaves the current level.

Signed-off-by Tejun Heo <[email protected]>

---
 block/blk-core.c           | 10 +++++++---
 block/blk-throttle.c       |  8 --------
 include/linux/blk-cgroup.h | 20 ++++++++++++++++++++
 3 files changed, 27 insertions(+), 11 deletions(-)

block/blk-core.c +0 -2

index ad23b96..f0e3157 100644
@@ -2216,11 +2216,15 @@ blk_qc_t generic_make_request(struct bio *bio)
 			 */
 			bio_list_init(&lower);
 			bio_list_init(&same);
-			while ((bio = bio_list_pop(&bio_list_on_stack[0])) != NULL)
-				if (q == bio->bi_disk->queue)

block/blk-core.c +0 -0

 			/* now assemble so we handle the lowest level first */
 			bio_list_merge(&bio_list_on_stack[0], &lower);
 			bio_list_merge(&bio_list_on_stack[0], &same);

block/blk-throttle.c +0 -8

index 1e6916b..76579b2 100644
@@ -2223,14 +2223,6 @@ bool blk_throtl_bio(struct request_queue *q, struct blkcg_gq *blkg,
 out_unlock:
 	spin_unlock_irq(q->queue_lock);
 out:
-	/*
-	 * As multiple blk-throtls may stack in the same issue path, we
-	 * don't want bios to leave with the flag set.  Clear the flag if
-	 * being issued.
-	 */
-	if (!throttled)
-		bio_clear_flag(bio, BIO_THROTTLED);
-
 #ifdef CONFIG_BLK_DEV_THROTTLING_LOW
 	if (throttled || !td->track_bio_latency)
 		bio->bi_issue_stat.stat |= SKIP_LATENCY;

include/linux/blk-cgroup.h +20 -0

index f2f9691..bed0416 100644
@@ -675,9 +675,29 @@ static inline void blkg_rwstat_add_aux(struct blkg_rwstat *to,
 #ifdef CONFIG_BLK_DEV_THROTTLING
 extern bool blk_throtl_bio(struct request_queue *q, struct blkcg_gq *blkg,
 			   struct bio *bio);
+
+static inline void blkcg_bio_repeat_q_level(struct bio *bio)
+{
+	/*
+	 * @bio is queued while processing a previous bio which was already
+	 * throttled.  Don't throttle it again.
+	 */
+	bio_set_flag(bio, BIO_THROTTLED);
+}
+
+static inline void blkcg_bio_leave_q_level(struct bio *bio)
+{
+	/*
+	 * @bio may get throttled at multiple q levels, clear THROTTLED
+	 * when leaving the current one.
+	 */
+	bio_clear_flag(bio, BIO_THROTTLED);
+}
 #else
 static inline bool blk_throtl_bio(struct request_queue *q, struct blkcg_gq *blkg,
 				  struct bio *bio) { return false; }
+static inline void blkcg_bio_repeat_q_level(struct bio *bio) { }
+static inline void biocg_bio_leave_q_level(struct bio *bio) { }
 #endif
 
 static inline struct blkcg_gq *blkg_lookup_create(struct blkcg *blkcg,

Signature · 1 line

2.9.5

Thanks for looking into this while I was absence. I don't understand how this works. Assume a bio will be splitted into 2 small bios. In generic_make_request, we charge the whole bio. 'q->make_request_fn' will dispatch the first small bio, and call generic_make_request for the second small bio. Then generic_make_request charge the second small bio and we add the second small bio to current->bio_list[0] (please check the order). In above

code the patch changed, we pop the second small bio and set BIO_THROTTLED for it. But this is already too late, because generic_make_request already charged the second small bio.

Did you look at my original patch
(https://marc.info/?l=linux-block&m=150791825327628&w=2), anything wrong?

You're right. If we wanna take this approach, we need to keep the throttled flag while cloning. The clearing part is still correct tho. Without that, I get 1/4 bw limit enforced. Hmm... I'm not quite sure where that 1/4 is coming from tho. Will investigate more.

Hello,

Quoted 4 lines · Tejun Heo

You're right. If we wanna take this approach, we need to keep the throttled flag while cloning. The clearing part is still correct tho. Without that, I get 1/4 bw limit enforced. Hmm... I'm not quite sure where that 1/4 is coming from tho. Will investigate more.

Okay, this is because when we spiit, the split bio is the first part which gets issued and then the orignal bio is wound forward and requeued. So, for the splits, the original bio is the one which gets trimmed in the front and requeued, so not clearing BIO_THROTTLED is enough. I think we should still copy BIO_THROTTLED on clones so that we don't get suprises w/ other bio drivers.

I'm not sure how you are going to make this correct. The mechanism is very fragile. So for example, 'q->make_request_fn(q, bio)' could just queue the bio somewhere and handle in other context (both dm and md do this). The bio will be called again with generic_make_request. In this case, the second time shouldn't throttle the bio. The bio could be called again with generic_make_request but with bdev changed. In this case, the second time should throttle the bio (against the new bdev). There are a lot of different usages of bio. I'd rather not depend on generic_make_request dispatches new bio immediately. That's why I add a bdev in my patch.

Hello,

Quoted 4 lines · Tejun Heo

You're right. If we wanna take this approach, we need to keep the throttled flag while cloning. The clearing part is still correct tho. Without that, I get 1/4 bw limit enforced. Hmm... I'm not quite sure where that 1/4 is coming from tho. Will investigate more.

Okay, this is because when we spiit, the split bio is the first part which gets issued and then the orignal bio is wound forward and requeued. So, for the splits, the original bio is the one which gets trimmed in the front and requeued, so not clearing BIO_THROTTLED is enough. I think we should still copy BIO_THROTTLED on clones so that we don't get suprises w/ other bio drivers.