block, cgroup: make cfq charge async IOs to the appropriate blkcgs

cfq_get_queue()'s control flow looks like the following.

	async_cfqq = NULL;
	cfqq = NULL;

	if (!is_sync) {
		...
		async_cfqq = ...;
		cfqq = *async_cfqq;
	}

	if (!cfqq)
		cfqq = ...;

	if (!is_sync && !(*async_cfqq))
		...;

The only thing the local variable init, the second if, and the async_cfqq test in the third if achieves is to skip cfqq creation and installation if *async_cfqq was already non-NULL. This is needlessly complicated with different tests examining the same condition. Simplify it to the following.

	if (!is_sync) {
		...
		async_cfqq = ...;
		cfqq = *async_cfqq;
		if (cfqq)
			goto out;
	}

	cfqq = ...;

	if (!is_sync)
		...;

out:

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

Cc Vivek Goyal <[email protected]>

Cc Arianna Avanzini <[email protected]>

Async cfqq's (cfq_queue's) are shared across cfq_data. When cfq_get_queue() obtains a new queue from cfq_find_alloc_queue(), it stashes the pointer in cfq_data and reuses it from then on; however, the function doesn't consider that cfq_find_alloc_queue() may return the oom_cfqq under memory pressure and installs the returned queue unconditionally.

If the oom_cfqq is installed as an async cfqq, cfq_set_request() will continue calling cfq_get_queue() hoping to replace it with a proper queue; however, cfq_get_queue() will keep returning the cached queue for the slot - the oom_cfqq.

Fix it by skipping caching if the queue is the oom one.

If the cfq_queue cached in cfq_io_cq is the oom one, cfq_set_request() replaces it by invoking cfq_get_queue() again without putting the oom queue leaking the reference it was holding. While oom queues are not released through reference counting, they're still reference counted and this can theoretically lead to the reference count overflowing and incorrectly invoke the usual release path on it.

Fix it by making cfq_set_request() put the ref it was holding.

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

Cc Vivek Goyal <[email protected]>

Cc Arianna Avanzini <[email protected]>

* Some were accessing cic->cfqq[] directly. Always use cic_to_cfqq() and cic_set_cfqq().

* check_ioprio_changed() doesn't need to verify cfq_get_queue()'s return for NULL. It's always non-NULL. Simplify accordingly.

This patch doesn't cause any functional changes.

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

Cc Vivek Goyal <[email protected]>

Cc Arianna Avanzini <[email protected]>

Even when allocations fail, cfq_find_alloc_queue() always returns a valid cfq_queue by falling back to the oom cfq_queue. As such, there isn't much point in taking @gfp_mask and trying "harder" if __GFP_WAIT is set. GFP_ATOMIC allocations don't fail often and even when they do the degraded behavior is acceptable and temporary.

After all, the only reason get_request(), which ultimately determines the gfp_mask, cares about __GFP_WAIT is to guarantee request allocation, assuming IO forward progress, for callers which are willing to wait. There's no reason for cfq_find_alloc_queue() to behave differently on __GFP_WAIT when it already has a fallback mechanism.

Remove @gfp_mask from cfq_find_alloc_queue() and propagate the changes to its callers. This simplifies the function quite a bit and will help making async queues per-cfq_group.

Quoted 13 lines · Tejun Heo

Even when allocations fail, cfq_find_alloc_queue() always returns a valid cfq_queue by falling back to the oom cfq_queue. As such, there isn't much point in taking @gfp_mask and trying "harder" if __GFP_WAIT is set. GFP_ATOMIC allocations don't fail often and even when they do the degraded behavior is acceptable and temporary.

After all, the only reason get_request(), which ultimately determines the gfp_mask, cares about __GFP_WAIT is to guarantee request allocation, assuming IO forward progress, for callers which are willing to wait. There's no reason for cfq_find_alloc_queue() to behave differently on __GFP_WAIT when it already has a fallback mechanism.

Quoted 3 lines

Remove @gfp_mask from cfq_find_alloc_queue() and propagate the changes to its callers. This simplifies the function quite a bit and will help making async queues per-cfq_group.

Sorry, I disagree with this patch. You've changed it so that all cfqq allocations are GFP_ATOMIC, and most, if not all of them simply don't need to be.

Quoted 19 lines · Jeff Moyer

Quoted 16 lines · Tejun Heo

Even when allocations fail, cfq_find_alloc_queue() always returns a valid cfq_queue by falling back to the oom cfq_queue. As such, there isn't much point in taking @gfp_mask and trying "harder" if __GFP_WAIT is set. GFP_ATOMIC allocations don't fail often and even when they do the degraded behavior is acceptable and temporary.

After all, the only reason get_request(), which ultimately determines the gfp_mask, cares about __GFP_WAIT is to guarantee request allocation, assuming IO forward progress, for callers which are willing to wait. There's no reason for cfq_find_alloc_queue() to behave differently on __GFP_WAIT when it already has a fallback mechanism.

Remove @gfp_mask from cfq_find_alloc_queue() and propagate the changes to its callers. This simplifies the function quite a bit and will help making async queues per-cfq_group.

Quoted 3 lines

Sorry, I disagree with this patch. You've changed it so that all cfqq allocations are GFP_ATOMIC, and most, if not all of them simply don't need to be.

It occurs to me that replacing GFP_ATOMIC with GFP_NOWAIT in your patch would address my concerns, and patches 6-8 would apply almost as-is. What do you think about that?

Whatever we end up using, may be it is a good idea to use same policy for block group allocation too. Right now we use GFP_ATOMIC for blkcg allocation.

So this will be equivalent of that when memory is low, we don't provide service differentiation.

Quoted 7 lines

Quoted 3 lines

Remove @gfp_mask from cfq_find_alloc_queue() and propagate the changes to its callers. This simplifies the function quite a bit and will help making async queues per-cfq_group.

Sorry, I disagree with this patch. You've changed it so that all cfqq allocations are GFP_ATOMIC, and most, if not all of them simply don't need to be.

It occurs to me that replacing GFP_ATOMIC with GFP_NOWAIT in your patch would address my concerns, and patches 6-8 would apply almost as-is. What do you think about that?

Hey, Jeff.

Quoted 8 lines · Jeff Moyer

Quoted 7 lines

Quoted 3 lines

Remove @gfp_mask from cfq_find_alloc_queue() and propagate the changes to its callers. This simplifies the function quite a bit and will help making async queues per-cfq_group.

Sorry, I disagree with this patch. You've changed it so that all cfqq allocations are GFP_ATOMIC, and most, if not all of them simply don't need to be.

Quoted 3 lines

It occurs to me that replacing GFP_ATOMIC with GFP_NOWAIT in your patch would address my concerns, and patches 6-8 would apply almost as-is. What do you think about that?

Oh yeah, it's okay to fail these allocations under memory pressure. GFP_NOWAIT is the better pick here. It's GFP_ATOMIC mostly due to historic copy&paste anyway. I'll change them to GFP_NOWAIT.

This is necessary for making async cfq_cgroups per-cfq_group instead of per-cfq_data. While this change makes cfq_get_queue() perform RCU locking and look up cfq_group even when it reuses async queue, the extra overhead is extremely unlikely to be noticeable given that this is already sitting behind cic->cfqq[] cache and the overall cost of cfq operation.

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

Cc Vivek Goyal <[email protected]>

Cc Arianna Avanzini <[email protected]>

cfq_find_alloc_queue() checks whether a queue actually needs to be allocated, which is unnecessary as its sole caller, cfq_get_queue(), only calls it if so. Also, the oom queue fallback logic is scattered between cfq_get_queue() and cfq_find_alloc_queue(). There really isn't much going on in the latter and things can be made simpler by folding it into cfq_get_queue().

This patch collapses cfq_find_alloc_queue() into cfq_get_queue(). The change is fairly straight-forward with one exception - async_cfqq is now initialized to NULL and the "!is_sync" test in the last if conditional is replaced with "async_cfqq" test. This is because gcc (5.1.1) gets confused for some reason and warns that async_cfqq may be used uninitialized otherwise. Oh well, the code isn't necessarily worse this way.

This patch doesn't cause any functional difference.

The resulting code (introduced by the last patch, I know) is not ideal:

        rcu_read_lock();
        cfqg = cfq_lookup_create_cfqg(cfqd, bio_blkcg(bio));
        if (!cfqg) {
                cfqq = &cfqd->oom_cfqq;
                goto out;
        }

        if (!is_sync) {
                if (!ioprio_valid(cic->ioprio)) {
                        struct task_struct *tsk = current;
                        ioprio = task_nice_ioprio(tsk);
                        ioprio_class = task_nice_ioclass(tsk);
                }
                async_cfqq = cfq_async_queue_prio(cfqd, ioprio_class,
                ioprio);
                cfqq = *async_cfqq;
                if (cfqq)
                        goto out;
        }

As you mentioned, we don't need to lookup the cfqg for the async queue. What's more is we could fallback to the oom_cfqq even if we had an existing async cfqq. I'm guessing you structured the code this way to make the error path cleaner. I don't think it's a big deal, as it should be a rare occurrence, so...

Up until now, all async IOs were queued to async queues which are shared across the whole request_queue, which means that blkcg resource control is completely void on async IOs including all writeback IOs. It was done this way because writeback didn't support writeback and there was no way of telling which writeback IO belonged to which cgroup; however, writeback recently became cgroup aware and writeback bio's are sent down properly tagged with the blkcg's to charge them against.

This patch makes async cfq_queues per-cfq_cgroup instead of per-cfq_data so that each async IO is charged to the blkcg that it was tagged for instead of unconditionally attributing it to root.

* cfq_data->async_cfqq and ->async_idle_cfqq are moved to cfq_group and alloc / destroy paths are updated accordingly.

* cfq_link_cfqq_cfqg() no longer overrides @cfqg to root for async queues.

* check_blkcg_changed() now also invalidates async queues as they no longer stay the same across cgroups.

After this patch, cfq's proportional IO control through blkio.weight works correctly when cgroup writeback is in use.

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

Cc Vivek Goyal <[email protected]>

Cc Arianna Avanzini <[email protected]>

---
 block/cfq-iosched.c | 85 ++++++++++++++++++++++++++---------------------------
 1 file changed, 42 insertions(+), 43 deletions(-)

block/cfq-iosched.c +28 -21

index 393befb..fded8d7 100644
@@ -291,6 +291,11 @@ struct cfq_group {
 	struct cfq_ttime ttime;
 	struct cfqg_stats stats;	/* stats for this cfqg */
 	struct cfqg_stats dead_stats;	/* stats pushed from dead children */
+
+	/* async queue for each priority case */
+	struct cfq_queue *async_cfqq[2][IOPRIO_BE_NR];
+	struct cfq_queue *async_idle_cfqq;
+
 };
 
 struct cfq_io_cq {

@@ -356,12 +361,6 @@ struct cfq_data {
 	struct cfq_queue *active_queue;
 	struct cfq_io_cq *active_cic;
 
-	/*
-	 * async queue for each priority case
-	 */
-	struct cfq_queue *async_cfqq[2][IOPRIO_BE_NR];
-	struct cfq_queue *async_idle_cfqq;
-
 	sector_t last_position;
 
 	/*

@@ -387,6 +386,7 @@ struct cfq_data {
 };
 
 static struct cfq_group *cfq_get_next_cfqg(struct cfq_data *cfqd);
+static void cfq_put_queue(struct cfq_queue *cfqq);
 
 static struct cfq_rb_root *st_for(struct cfq_group *cfqg,
 					    enum wl_class_t class,

@@ -1556,13 +1556,26 @@ static void cfq_pd_init(struct blkcg_gq *blkg)
 
 static void cfq_pd_offline(struct blkcg_gq *blkg)
 {
+	struct cfq_group *cfqg = blkg_to_cfqg(blkg);
+	int i;
+
+	for (i = 0; i < IOPRIO_BE_NR; i++) {
+		if (cfqg->async_cfqq[0][i])
+			cfq_put_queue(cfqg->async_cfqq[0][i]);
+		if (cfqg->async_cfqq[1][i])
+			cfq_put_queue(cfqg->async_cfqq[1][i]);
+	}
+
+	if (cfqg->async_idle_cfqq)
+		cfq_put_queue(cfqg->async_idle_cfqq);
+
 	/*
 	 * @blkg is going offline and will be ignored by
 	 * blkg_[rw]stat_recursive_sum().  Transfer stats to the parent so
 	 * that they don't get lost.  If IOs complete after this point, the
 	 * stats for them will be lost.  Oh well...
 	 */
-	cfqg_stats_xfer_dead(blkg_to_cfqg(blkg));
+	cfqg_stats_xfer_dead(cfqg);
 }
 
 /* offset delta from cfqg->stats to cfqg->dead_stats */

@@ -1625,10 +1638,6 @@ static struct cfq_group *cfq_lookup_create_cfqg(struct cfq_data *cfqd,
 
 static void cfq_link_cfqq_cfqg(struct cfq_queue *cfqq, struct cfq_group *cfqg)
 {
-	/* Currently, all async queues are mapped to root group */
-	if (!cfq_cfqq_sync(cfqq))
-		cfqg = cfqq->cfqd->root_group;
-
 	cfqq->cfqg = cfqg;
 	/* cfqq reference on cfqg */
 	cfqg_get(cfqg);

@@ -3541,7 +3550,7 @@ static void cfq_init_cfqq(struct cfq_data *cfqd, struct cfq_queue *cfqq,
 static void check_blkcg_changed(struct cfq_io_cq *cic, struct bio *bio)
 {
 	struct cfq_data *cfqd = cic_to_cfqd(cic);
-	struct cfq_queue *sync_cfqq;
+	struct cfq_queue *cfqq;
 	uint64_t serial_nr;
 
 	rcu_read_lock();

@@ -3555,15 +3564,22 @@ static void check_blkcg_changed(struct cfq_io_cq *cic, struct bio *bio)
 	if (unlikely(!cfqd) || likely(cic->blkcg_serial_nr == serial_nr))
 		return;
 
-	sync_cfqq = cic_to_cfqq(cic, 1);
-	if (sync_cfqq) {
-		/*
-		 * Drop reference to sync queue. A new sync queue will be
-		 * assigned in new group upon arrival of a fresh request.
-		 */
-		cfq_log_cfqq(cfqd, sync_cfqq, "changed cgroup");
-		cic_set_cfqq(cic, NULL, 1);
-		cfq_put_queue(sync_cfqq);
+	/*
+	 * Drop reference to queues.  New queues will be assigned in new
+	 * group upon arrival of fresh requests.
+	 */
+	cfqq = cic_to_cfqq(cic, false);
+	if (cfqq) {
+		cfq_log_cfqq(cfqd, cfqq, "changed cgroup");

But above is not true for buffered writes, we will not associate io context. Instead only cgroup information will be sent down and io context of submitter will be used.

So any thread which is forced to submit buffered write for some other cgroup, will have its sync queue also reset (Because CFQ will think

that cgroup of submitter has changed). Not sure how often it will happen, but if it happens frequenty, this might show up in profiles. I had

mentioned this in the past and IIUC you said we will have to carry writeback information all the way into lower layers. May be that's an optimzation for later. So nothing new here, just trying to understand the current situation.

Also I am wondring if this cgroup and io context information is carried through dm layer or not. I guess it might be a separate discussion. It has come for discussion internally in the past. So this might be a good time to get attention of dm developers on these upcoming changes. (CC dm-devel).

Up until now, all async IOs were queued to async queues which are shared across the whole request_queue, which means that blkcg resource control is completely void on async IOs including all writeback IOs. It was done this way because writeback didn't support writeback and there was no way of telling which writeback IO belonged to which cgroup; however, writeback recently became cgroup aware and writeback bio's are sent down properly tagged with the blkcg's to charge them against.

This patch makes async cfq_queues per-cfq_cgroup instead of per-cfq_data so that each async IO is charged to the blkcg that it was tagged for instead of unconditionally attributing it to root.

* cfq_data->async_cfqq and ->async_idle_cfqq are moved to cfq_group and alloc / destroy paths are updated accordingly.

* cfq_link_cfqq_cfqg() no longer overrides @cfqg to root for async queues.

* check_blkcg_changed() now also invalidates async queues as they no longer stay the same across cgroups.

After this patch, cfq's proportional IO control through blkio.weight works correctly when cgroup writeback is in use.

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

Cc Vivek Goyal <[email protected]>

Cc Arianna Avanzini <[email protected]>

Hello,

cfq has always charged all async IOs to the root cgroup. It didn't have much choice as writeback didn't know about cgroups and there was no way to tell who to blame for a given writeback IO. writeback finally grew support for cgroups and now tags each writeback IO with the appropriate cgroup to charge it against.

This patchset updates cfq so that it follows the blkcg each bio is tagged with. Async cfq_queues are now shared across cfq_group, which is per-cgroup, instead of per-request_queue cfq_data. This makes all IOs follow the weight based IO resource distribution implemented by cfq.

This patchset contains the following 8 patches.

0001-cfq-iosched-simplify-control-flow-in-cfq_get_queue.patch 0002-cfq-iosched-fix-async-oom-queue-handling.patch 0003-cfq-iosched-fix-oom-cfq_queue-ref-leak-in-cfq_set_re.patch 0004-cfq-iosched-minor-cleanups.patch 0005-cfq-iosched-remove-gfp_mask-from-cfq_find_alloc_queu.patch 0006-cfq-iosched-move-cfq_group-determination-from-cfq_fi.patch 0007-cfq-iosched-fold-cfq_find_alloc_queue-into-cfq_get_q.patch 0008-cfq-iosched-charge-async-IOs-to-the-appropriate-blkc.patch

Assuming you're ok with dropping patch 5, I'll review patches 6-8 once

they've been reworked to account for that. I took a look at them, and they look OK to me. But, if they are going to change, I'd rather wait to ack the final versions.

Hey, Jeff.

Quoted 1 line · Jeff Moyer

Assuming you're ok with dropping patch 5, I'll review patches 6-8 once

I'll update it to use GPF_NOWAIT. Can't drop it easily as that would complicate group lookup more complicated w/ pinning and all.

Quoted 3 lines

they've been reworked to account for that. I took a look at them, and they look OK to me. But, if they are going to change, I'd rather wait to ack the final versions.

Hmm... the changes will be simple s/GFP_ATOMIC/GFP_NOWAIT. Shouldn't affect reviewing, I think.

Yeah, I sent this email before realizing that GFP_NOWAIT would work out. I've reviewed the rest of the series. Looks good to me! You did a nice job splitting things up into easily reviewable pieces, so thanks for that.

From b848495d2c987e960d1f7794966d82c05fcefc6d Mon Sep 17 00:00:00 2001
From: Tejun Heo <[email protected]>
Date: Tue, 9 Jun 2015 13:19:40 +0900

blkcg performs several allocations to track IOs per cgroup and enforce resource control. Most of these allocations are performed lazily on demand in the IO path and thus can't involve reclaim path. Currently, these allocations use GFP_ATOMIC; however, blkcg can gracefully deal with occassional failures of these allocations by punting IOs to the root cgroup and there's no reason to reach into the emergency reserve.

This patch replaces GFP_ATOMIC with GFP_NOWAIT for the following allocations.

* bdi_writeback_congested and blkcg_gq allocations in blkg_create().

* radix tree node allocations for blkcg->blkg_tree.

* cfq_queue allocation on ioprio changes.

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

Suggested-by Jeff Moyer <[email protected]>

Suggested-by Vivek Goyal <[email protected]>