block: fix blk-iolatency accounting underflow

The blk-iolatency controller measures the time from rq_qos_throttle() to rq_qos_done_bio() and attributes this time to the first bio that needs to create the request. This means if a bio is plug-mergeable or bio-mergeable, it gets to bypass the blk-iolatency controller.

The recent series [1], to tag all bios w/ blkgs undermined how iolatency was determining which bios it was charging and should process in rq_qos_done_bio(). Because all bios are being tagged, this caused the atomic_t for the struct rq_wait inflight count to underflow and result in a stall.

This patch adds a new flag BIO_TRACKED to let controllers know that a bio is going through the rq_qos path. blk-iolatency now checks if this flag is set to see if it should process the bio in rq_qos_done_bio().

Overloading BLK_QUEUE_ENTERED works, but makes the flag rules confusing. BIO_THROTTLED was another candidate, but the flag is set for all bios that have gone through blk-throttle code. Overloading a flag comes with the burden of making sure that when either implementation changes, a change in setting rules for one doesn't cause a bug in the other. So here, we unfortunately opt for adding a new flag.

The blk-iolatency controller measures the time from rq_qos_throttle() to rq_qos_done_bio() and attributes this time to the first bio that needs to create the request. This means if a bio is plug-mergeable or bio-mergeable, it gets to bypass the blk-iolatency controller.

The recent series [1], to tag all bios w/ blkgs undermined how iolatency was determining which bios it was charging and should process in rq_qos_done_bio(). Because all bios are being tagged, this caused the atomic_t for the struct rq_wait inflight count to underflow and result in a stall.

This patch adds a new flag BIO_TRACKED to let controllers know that a bio is going through the rq_qos path. blk-iolatency now checks if this flag is set to see if it should process the bio in rq_qos_done_bio().

Overloading BLK_QUEUE_ENTERED works, but makes the flag rules confusing. BIO_THROTTLED was another candidate, but the flag is set for all bios that have gone through blk-throttle code. Overloading a flag comes with the burden of making sure that when either implementation changes, a change in setting rules for one doesn't cause a bug in the other. So here, we unfortunately opt for adding a new flag.

[1] https://lore.kernel.org/lkml/[email protected]/

Fixes 5cdf2e3fea5e ("blkcg: associate blkg when associating a device")

Signed-off-by Dennis Zhou <[email protected]>

Cc Josef Bacik <[email protected]>

---
 block/blk-iolatency.c     | 2 +-
 block/blk-rq-qos.h        | 5 +++++
 include/linux/blk_types.h | 1 +
 3 files changed, 7 insertions(+), 1 deletion(-)

block/blk-iolatency.c +1 -1

index bee092727cad..fc714ef402a6 100644
@@ -593,7 +593,7 @@ static void blkcg_iolatency_done_bio(struct rq_qos *rqos, struct bio *bio)
        bool enabled = false;
 
        blkg = bio->bi_blkg;
-       if (!blkg)
+       if (!blkg || !bio_flagged(bio, BIO_TRACKED))
                return;
 
        iolat = blkg_to_lat(bio->bi_blkg);

block/blk-rq-qos.h +5 -0

index fd8a0c5debd3..58f62483b537 100644
@@ -170,6 +170,11 @@ static inline void rq_qos_done_bio(struct request_queue *q, struct bio *bio)
 
 static inline void rq_qos_throttle(struct request_queue *q, struct bio *bio)
 {
+       /*
+        * BIO_TRACKED lets controllers know that a bio went through the
+        * normal rq_qos path.
+        */
+       bio_set_flag(bio, BIO_TRACKED);
        if (q->rq_qos)
                __rq_qos_throttle(q->rq_qos, bio);
 }

include/linux/blk_types.h +1 -1

index 46c005d601ac..fc99474ac968 100644
@@ -228,6 +228,7 @@ struct bio {
 #define BIO_TRACE_COMPLETION 10        /* bio_endio() should trace the final completion
                                 * of this bio. */
 #define BIO_QUEUE_ENTERED 11   /* can use blk_queue_enter_live() */
+#define BIO_TRACKED 12         /* set if bio goes through the rq_qos path */
 
 /* See BVEC_POOL_OFFSET below before adding new flags */
 
--

2.17.1

Quoted 6 lines · Dennis Zhou

The blk-iolatency controller measures the time from rq_qos_throttle() to rq_qos_done_bio() and attributes this time to the first bio that needs to create the request. This means if a bio is plug-mergeable or bio-mergeable, it gets to bypass the blk-iolatency controller.

Hi,

I have a question about merging in plug list, since plug merges are done before rq_qos_throttle(), why would plug-mergeable bios bypass the controller?

thanks,
liubo

Quoted 8 lines · Liu Bo

Quoted 6 lines · Dennis Zhou

The blk-iolatency controller measures the time from rq_qos_throttle() to rq_qos_done_bio() and attributes this time to the first bio that needs to create the request. This means if a bio is plug-mergeable or bio-mergeable, it gets to bypass the blk-iolatency controller.

Quoted 9 lines

Hi,

I have a question about merging in plug list, since plug merges are done before rq_qos_throttle(), why would plug-mergeable bios bypass the controller?

thanks,
liubo

Hi Liubo,

BIO_TRACKED is tagging the bio that is responsible for allocating a new request, so that rq_qos controllers can decide whether or not they want to process the bio any part of the way. I should have phrased that a little better in the commit message. It's not that the bio itself is bypassing the blk-iolatency controller, but the blk-iolatency controller deciding to not do anything based on the BIO_TRACKED flag. This doesn't change any of the function calls made on a bio/request.

Thanks,
Dennis