Skip to contentSkip to conversation

block: remove ioc_*_changed()

PATCH In review Tejun Heo 3 participants 9 messages Mar 19, 2012 – Mar 20, 2012

Subsystems (3)
VFS Block layer cgroup blkio
lore
Share & tools
lore

Tejun Heo

author maintainer
#0 lore raw

PATCH 1/1 block: remove ioc_*_changed()

Hello, guys.

The changed notification used by cfq is rather odd. cfq caches the associated cfqqs per cic and uses the changed notification to expire those lookup caches.

The explicit notification mechanism might make sense if determining whether the current cache is up-to-date is difficult or expensive; however, that isn't the case here. Determining whether ioprio or cgroup has changed is straight-forward and inexpensive.

This patchset updates cfq to so that it remembers the current ioprio and blkcg in the cic and determines whether cfqq's need to be reset without using the changed notification and drops the changed notification code.

0001-blkcg-add-blkcg-id.patch 0002-cfq-pass-around-cfq_io_cq-instead-of-io_context.patch 0003-cfq-don-t-use-icq_get_changed.patch 0004-block-remove-ioc_-_changed.patch

0001 adds unique u64 id to each blkcg so that policies can tell whether the associated blkcg has changed.

0002 makes cfq pass around cic internally instead of ioc. This is a cleanup in itself and necessary for the next patch.

0003 updates cfq to not use changed notification.

0004 removes code implementing changed notification.

This patchset is on top of

block/for-3.4/core 671058fb2a2aac4e70f01b316b06bc59b98bd138
+ [1] blkcg: fix percpu stat allocation and remove stats_lock, take#2

and available in the following git branch

git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git blk-ioc-remove-changed

Thanks.

 block/blk-cgroup.c        |   22 +----------
 block/blk-cgroup.h        |    3 +
 block/blk-ioc.c           |   68 ----------------------------------
 block/cfq-iosched.c       |   92 ++++++++++++++++++++++++++--------------------
 fs/ioprio.c               |    2 -
 include/linux/iocontext.h |    7 ---
 include/linux/ioprio.h    |   22 ++---------
 7 files changed, 64 insertions(+), 152 deletions(-)

--
tejun

[1] http://thread.gmane.org/gmane.linux.kernel/1263989

author maintainer
#1 lore raw

PATCH 1/4 blkcg: add blkcg->id

Add 64bit unique id to blkcg. This will be used by policies which want blkcg identity test to tell whether the associated blkcg has changed.

Signed-off-by Tejun Heo <[email protected]>
Cc Vivek Goyal <[email protected]>
---
 block/blk-cgroup.c |    3 +++
 block/blk-cgroup.h |    3 +++
 2 files changed, 6 insertions(+), 0 deletions(-)
block/blk-cgroup.c +3 -0
index b15a517..30e0730 100644
@@ -19,6 +19,7 @@
 #include <linux/slab.h>
 #include <linux/genhd.h>
 #include <linux/delay.h>
+#include <linux/atomic.h>
 #include "blk-cgroup.h"
 #include "blk.h"
 

@@ -1622,6 +1623,7 @@ static void blkiocg_destroy(struct cgroup_subsys *subsys, struct cgroup *cgroup)
 static struct cgroup_subsys_state *
 blkiocg_create(struct cgroup_subsys *subsys, struct cgroup *cgroup)
 {
+	static atomic64_t id_seq = ATOMIC64_INIT(0);
 	struct blkio_cgroup *blkcg;
 	struct cgroup *parent = cgroup->parent;
 

@@ -1635,6 +1637,7 @@ blkiocg_create(struct cgroup_subsys *subsys, struct cgroup *cgroup)
 		return ERR_PTR(-ENOMEM);
 
 	blkcg->weight = BLKIO_WEIGHT_DEFAULT;
+	blkcg->id = atomic64_inc_return(&id_seq); /* root is 0, start from 1 */
 done:
 	spin_lock_init(&blkcg->lock);
 	INIT_HLIST_HEAD(&blkcg->blkg_list);
block/blk-cgroup.h +3 -0
index 9df5ab0..1cb8f76 100644
@@ -116,6 +116,9 @@ struct blkio_cgroup {
 	unsigned int weight;
 	spinlock_t lock;
 	struct hlist_head blkg_list;
+
+	/* for policies to test whether associated blkcg has changed */
+	uint64_t id;
 };
 
 struct blkio_group_stats {
Signature · 1 line
1.7.7.3
author maintainer
#2 lore raw

PATCH 2/4 cfq: pass around cfq_io_cq instead of io_context

In reply to Tejun Heo

Now that io_cq is managed by block core and guaranteed to exist for any in-flight request, it is easier and carries more information to pass around cfq_io_cq than io_context.

This patch updates cfq_init_prio_data(), cfq_find_alloc_queue() and cfq_get_queue() to take @cic instead of @ioc. This change removes a duplicate cfq_cic_lookup() from cfq_find_alloc_queue().

This change enables the use of cic-cached ioprio in the next patch.

Signed-off-by Tejun Heo <[email protected]>
---
 block/cfq-iosched.c |   39 +++++++++++++++++----------------------
 1 files changed, 17 insertions(+), 22 deletions(-)
block/cfq-iosched.c +17 -22
index f2387b5..9e8624e 100644
@@ -468,7 +468,7 @@ static inline int cfqg_busy_async_queues(struct cfq_data *cfqd,
 
 static void cfq_dispatch_insert(struct request_queue *, struct request *);
 static struct cfq_queue *cfq_get_queue(struct cfq_data *cfqd, bool is_sync,
-				       struct io_context *ioc, struct bio *bio,
+				       struct cfq_io_cq *cic, struct bio *bio,
 				       gfp_t gfp_mask);
 
 static inline struct cfq_io_cq *icq_to_cic(struct io_cq *icq)

@@ -2560,7 +2560,7 @@ static void cfq_exit_icq(struct io_cq *icq)
 	}
 }
 
-static void cfq_init_prio_data(struct cfq_queue *cfqq, struct io_context *ioc)
+static void cfq_init_prio_data(struct cfq_queue *cfqq, struct cfq_io_cq *cic)
 {
 	struct task_struct *tsk = current;
 	int ioprio_class;

@@ -2568,7 +2568,7 @@ static void cfq_init_prio_data(struct cfq_queue *cfqq, struct io_context *ioc)
 	if (!cfq_cfqq_prio_changed(cfqq))
 		return;
 
-	ioprio_class = IOPRIO_PRIO_CLASS(ioc->ioprio);
+	ioprio_class = IOPRIO_PRIO_CLASS(cic->icq.ioc->ioprio);
 	switch (ioprio_class) {
 	default:
 		printk(KERN_ERR "cfq: bad prio %x\n", ioprio_class);

@@ -2580,11 +2580,11 @@ static void cfq_init_prio_data(struct cfq_queue *cfqq, struct io_context *ioc)
 		cfqq->ioprio_class = task_nice_ioclass(tsk);
 		break;
 	case IOPRIO_CLASS_RT:
-		cfqq->ioprio = task_ioprio(ioc);
+		cfqq->ioprio = task_ioprio(cic->icq.ioc);
 		cfqq->ioprio_class = IOPRIO_CLASS_RT;
 		break;
 	case IOPRIO_CLASS_BE:
-		cfqq->ioprio = task_ioprio(ioc);
+		cfqq->ioprio = task_ioprio(cic->icq.ioc);
 		cfqq->ioprio_class = IOPRIO_CLASS_BE;
 		break;
 	case IOPRIO_CLASS_IDLE:

@@ -2613,8 +2613,8 @@ static void changed_ioprio(struct cfq_io_cq *cic, struct bio *bio)
 	cfqq = cic->cfqq[BLK_RW_ASYNC];
 	if (cfqq) {
 		struct cfq_queue *new_cfqq;
-		new_cfqq = cfq_get_queue(cfqd, BLK_RW_ASYNC, cic->icq.ioc,
-					 bio, GFP_ATOMIC);
+		new_cfqq = cfq_get_queue(cfqd, BLK_RW_ASYNC, cic, bio,
+					 GFP_ATOMIC);
 		if (new_cfqq) {
 			cic->cfqq[BLK_RW_ASYNC] = new_cfqq;
 			cfq_put_queue(cfqq);

@@ -2671,23 +2671,18 @@ static void changed_cgroup(struct cfq_io_cq *cic)
 #endif  /* CONFIG_CFQ_GROUP_IOSCHED */
 
 static struct cfq_queue *
-cfq_find_alloc_queue(struct cfq_data *cfqd, bool is_sync,
-		     struct io_context *ioc, struct bio *bio, gfp_t gfp_mask)
+cfq_find_alloc_queue(struct cfq_data *cfqd, bool is_sync, struct cfq_io_cq *cic,
+		     struct bio *bio, gfp_t gfp_mask)
 {
 	struct blkio_cgroup *blkcg;
 	struct cfq_queue *cfqq, *new_cfqq = NULL;
-	struct cfq_io_cq *cic;
 	struct cfq_group *cfqg;
 
 retry:
 	rcu_read_lock();
 
 	blkcg = bio_blkio_cgroup(bio);
-
 	cfqg = cfq_lookup_create_cfqg(cfqd, blkcg);
-
-	cic = cfq_cic_lookup(cfqd, ioc);
-	/* cic always exists here */
 	cfqq = cic_to_cfqq(cic, is_sync);
 
 	/*

@@ -2716,7 +2711,7 @@ retry:
 
 		if (cfqq) {
 			cfq_init_cfqq(cfqd, cfqq, current->pid, is_sync);
-			cfq_init_prio_data(cfqq, ioc);
+			cfq_init_prio_data(cfqq, cic);
 			cfq_link_cfqq_cfqg(cfqq, cfqg);
 			cfq_log_cfqq(cfqd, cfqq, "alloced");
 		} else

@@ -2746,11 +2741,11 @@ cfq_async_queue_prio(struct cfq_data *cfqd, int ioprio_class, int ioprio)
 }
 
 static struct cfq_queue *
-cfq_get_queue(struct cfq_data *cfqd, bool is_sync, struct io_context *ioc,
+cfq_get_queue(struct cfq_data *cfqd, bool is_sync, struct cfq_io_cq *cic,
 	      struct bio *bio, gfp_t gfp_mask)
 {
-	const int ioprio = task_ioprio(ioc);
-	const int ioprio_class = task_ioprio_class(ioc);
+	const int ioprio = task_ioprio(cic->icq.ioc);
+	const int ioprio_class = task_ioprio_class(cic->icq.ioc);
 	struct cfq_queue **async_cfqq = NULL;
 	struct cfq_queue *cfqq = NULL;
 

@@ -2760,7 +2755,7 @@ cfq_get_queue(struct cfq_data *cfqd, bool is_sync, struct io_context *ioc,
 	}
 
 	if (!cfqq)
-		cfqq = cfq_find_alloc_queue(cfqd, is_sync, ioc, bio, gfp_mask);
+		cfqq = cfq_find_alloc_queue(cfqd, is_sync, cic, bio, gfp_mask);
 
 	/*
 	 * pin the queue now that it's allocated, scheduler exit will prune it

@@ -3030,7 +3025,7 @@ static void cfq_insert_request(struct request_queue *q, struct request *rq)
 	struct cfq_queue *cfqq = RQ_CFQQ(rq);
 
 	cfq_log_cfqq(cfqd, cfqq, "insert_request");
-	cfq_init_prio_data(cfqq, RQ_CIC(rq)->icq.ioc);
+	cfq_init_prio_data(cfqq, RQ_CIC(rq));
 
 	rq_set_fifo_time(rq, jiffies + cfqd->cfq_fifo_expire[rq_is_sync(rq)]);
 	list_add_tail(&rq->queuelist, &cfqq->fifo);

@@ -3234,7 +3229,7 @@ static int cfq_may_queue(struct request_queue *q, int rw)
 
 	cfqq = cic_to_cfqq(cic, rw_is_sync(rw));
 	if (cfqq) {
-		cfq_init_prio_data(cfqq, cic->icq.ioc);
+		cfq_init_prio_data(cfqq, cic);
 
 		return __cfq_may_queue(cfqq);
 	}

@@ -3326,7 +3321,7 @@ cfq_set_request(struct request_queue *q, struct request *rq, struct bio *bio,
 new_queue:
 	cfqq = cic_to_cfqq(cic, is_sync);
 	if (!cfqq || cfqq == &cfqd->oom_cfqq) {
-		cfqq = cfq_get_queue(cfqd, is_sync, cic->icq.ioc, bio, gfp_mask);
+		cfqq = cfq_get_queue(cfqd, is_sync, cic, bio, gfp_mask);
 		cic_set_cfqq(cic, cfqq, is_sync);
 	} else {
 		/*
Signature · 1 line
1.7.7.3
author maintainer
#3 lore raw

PATCH 3/4 cfq: don't use icq_get_changed()

In reply to Tejun Heo

cfq caches the associated cfqq's for a given cic. The cache needs to be flushed if the cic's ioprio or blkcg has changed. It is currently done by requiring the changing action to set the respective ICQ_*_CHANGED bit in the icq and testing it from cfq_set_request(), which involves iterating through all the affected icqs.

All cfq wants to know is whether ioprio and/or blkcg have changed since the last flush and can be easily achieved by just remembering the current ioprio and blkcg ID in cic.

This patch adds cic->{ioprio|blkcg_id}, updates all ioprio users to use the remembered value instead, and updates cfq_set_request() path such that, instead of using icq_get_changed(), the current values are compared against the remembered ones and trigger appropriate flush action if not. Condition tests are moved inside both _changed functions which are now named check_ioprio_changed() and check_blkcg_changed().

ioprio.h::task_ioprio*() can't be used anymore and replaced with open-coded IOPRIO_CLASS_NONE case in cfq_async_queue_prio().

Signed-off-by Tejun Heo <[email protected]>
Cc Vivek Goyal <[email protected]>
---
 block/cfq-iosched.c    |   63 ++++++++++++++++++++++++++++++-----------------
 include/linux/ioprio.h |   22 ++++-------------
 2 files changed, 45 insertions(+), 40 deletions(-)
block/cfq-iosched.c +40 -23
index 9e8624e..7c3893d 100644
@@ -218,6 +218,10 @@ struct cfq_io_cq {
 	struct io_cq		icq;		/* must be the first member */
 	struct cfq_queue	*cfqq[2];
 	struct cfq_ttime	ttime;
+	int			ioprio;		/* the current ioprio */
+#ifdef CONFIG_CFQ_GROUP_IOSCHED
+	uint64_t		blkcg_id;	/* the current blkcg ID */
+#endif
 };
 
 /*

@@ -2568,7 +2572,7 @@ static void cfq_init_prio_data(struct cfq_queue *cfqq, struct cfq_io_cq *cic)
 	if (!cfq_cfqq_prio_changed(cfqq))
 		return;
 
-	ioprio_class = IOPRIO_PRIO_CLASS(cic->icq.ioc->ioprio);
+	ioprio_class = IOPRIO_PRIO_CLASS(cic->ioprio);
 	switch (ioprio_class) {
 	default:
 		printk(KERN_ERR "cfq: bad prio %x\n", ioprio_class);

@@ -2580,11 +2584,11 @@ static void cfq_init_prio_data(struct cfq_queue *cfqq, struct cfq_io_cq *cic)
 		cfqq->ioprio_class = task_nice_ioclass(tsk);
 		break;
 	case IOPRIO_CLASS_RT:
-		cfqq->ioprio = task_ioprio(cic->icq.ioc);
+		cfqq->ioprio = IOPRIO_PRIO_DATA(cic->ioprio);
 		cfqq->ioprio_class = IOPRIO_CLASS_RT;
 		break;
 	case IOPRIO_CLASS_BE:
-		cfqq->ioprio = task_ioprio(cic->icq.ioc);
+		cfqq->ioprio = IOPRIO_PRIO_DATA(cic->ioprio);
 		cfqq->ioprio_class = IOPRIO_CLASS_BE;
 		break;
 	case IOPRIO_CLASS_IDLE:

@@ -2602,12 +2606,17 @@ static void cfq_init_prio_data(struct cfq_queue *cfqq, struct cfq_io_cq *cic)
 	cfq_clear_cfqq_prio_changed(cfqq);
 }
 
-static void changed_ioprio(struct cfq_io_cq *cic, struct bio *bio)
+static void check_ioprio_changed(struct cfq_io_cq *cic, struct bio *bio)
 {
+	int ioprio = cic->icq.ioc->ioprio;
 	struct cfq_data *cfqd = cic_to_cfqd(cic);
 	struct cfq_queue *cfqq;
 
-	if (unlikely(!cfqd))
+	/*
+	 * Check whether ioprio has changed.  The condition may trigger
+	 * spuriously on a newly created cic but there's no harm.
+	 */
+	if (unlikely(!cfqd) || likely(cic->ioprio == ioprio))
 		return;
 
 	cfqq = cic->cfqq[BLK_RW_ASYNC];

@@ -2624,6 +2633,8 @@ static void changed_ioprio(struct cfq_io_cq *cic, struct bio *bio)
 	cfqq = cic->cfqq[BLK_RW_SYNC];
 	if (cfqq)
 		cfq_mark_cfqq_prio_changed(cfqq);
+
+	cic->ioprio = ioprio;
 }
 
 static void cfq_init_cfqq(struct cfq_data *cfqd, struct cfq_queue *cfqq,

@@ -2647,17 +2658,24 @@ static void cfq_init_cfqq(struct cfq_data *cfqd, struct cfq_queue *cfqq,
 }
 
 #ifdef CONFIG_CFQ_GROUP_IOSCHED
-static void changed_cgroup(struct cfq_io_cq *cic)
+static void check_blkcg_changed(struct cfq_io_cq *cic, struct bio *bio)
 {
-	struct cfq_queue *sync_cfqq = cic_to_cfqq(cic, 1);
 	struct cfq_data *cfqd = cic_to_cfqd(cic);
-	struct request_queue *q;
+	struct cfq_queue *sync_cfqq;
+	uint64_t id;
 
-	if (unlikely(!cfqd))
-		return;
+	rcu_read_lock();
+	id = bio_blkio_cgroup(bio)->id;
+	rcu_read_unlock();
 
-	q = cfqd->queue;
+	/*
+	 * Check whether blkcg has changed.  The condition may trigger
+	 * spuriously on a newly created cic but there's no harm.
+	 */
+	if (unlikely(!cfqd) || likely(cic->blkcg_id == id))
+		return;
 
+	sync_cfqq = cic_to_cfqq(cic, 1);
 	if (sync_cfqq) {
 		/*
 		 * Drop reference to sync queue. A new sync queue will be

@@ -2667,7 +2685,11 @@ static void changed_cgroup(struct cfq_io_cq *cic)
 		cic_set_cfqq(cic, NULL, 1);
 		cfq_put_queue(sync_cfqq);
 	}
+
+	cic->blkcg_id = id;
 }
+#else
+static inline void check_blkcg_changed(struct cfq_io_cq *cic, struct bio *bio) { }
 #endif  /* CONFIG_CFQ_GROUP_IOSCHED */
 
 static struct cfq_queue *

@@ -2731,6 +2753,9 @@ cfq_async_queue_prio(struct cfq_data *cfqd, int ioprio_class, int ioprio)
 	switch (ioprio_class) {
 	case IOPRIO_CLASS_RT:
 		return &cfqd->async_cfqq[0][ioprio];
+	case IOPRIO_CLASS_NONE:
+		ioprio = IOPRIO_NORM;
+		/* fall through */
 	case IOPRIO_CLASS_BE:
 		return &cfqd->async_cfqq[1][ioprio];
 	case IOPRIO_CLASS_IDLE:

@@ -2744,8 +2769,8 @@ static struct cfq_queue *
 cfq_get_queue(struct cfq_data *cfqd, bool is_sync, struct cfq_io_cq *cic,
 	      struct bio *bio, gfp_t gfp_mask)
 {
-	const int ioprio = task_ioprio(cic->icq.ioc);
-	const int ioprio_class = task_ioprio_class(cic->icq.ioc);
+	const int ioprio_class = IOPRIO_PRIO_CLASS(cic->ioprio);
+	const int ioprio = IOPRIO_PRIO_DATA(cic->ioprio);
 	struct cfq_queue **async_cfqq = NULL;
 	struct cfq_queue *cfqq = NULL;
 

@@ -3303,21 +3328,13 @@ cfq_set_request(struct request_queue *q, struct request *rq, struct bio *bio,
 	const int rw = rq_data_dir(rq);
 	const bool is_sync = rq_is_sync(rq);
 	struct cfq_queue *cfqq;
-	unsigned int changed;
 
 	might_sleep_if(gfp_mask & __GFP_WAIT);
 
 	spin_lock_irq(q->queue_lock);
 
-	/* handle changed notifications */
-	changed = icq_get_changed(&cic->icq);
-	if (unlikely(changed & ICQ_IOPRIO_CHANGED))
-		changed_ioprio(cic, bio);
-#ifdef CONFIG_CFQ_GROUP_IOSCHED
-	if (unlikely(changed & ICQ_CGROUP_CHANGED))
-		changed_cgroup(cic);
-#endif
-
+	check_ioprio_changed(cic, bio);
+	check_blkcg_changed(cic, bio);
 new_queue:
 	cfqq = cic_to_cfqq(cic, is_sync);
 	if (!cfqq || cfqq == &cfqd->oom_cfqq) {
include/linux/ioprio.h +5 -17
index 76dad48..beb9ce1 100644
@@ -42,26 +42,14 @@ enum {
 };
 
 /*
- * if process has set io priority explicitly, use that. if not, convert
- * the cpu scheduler nice value to an io priority
+ * Fallback BE priority
  */
 #define IOPRIO_NORM	(4)
-static inline int task_ioprio(struct io_context *ioc)
-{
-	if (ioprio_valid(ioc->ioprio))
-		return IOPRIO_PRIO_DATA(ioc->ioprio);
-
-	return IOPRIO_NORM;
-}
-
-static inline int task_ioprio_class(struct io_context *ioc)
-{
-	if (ioprio_valid(ioc->ioprio))
-		return IOPRIO_PRIO_CLASS(ioc->ioprio);
-
-	return IOPRIO_CLASS_BE;
-}
 
+/*
+ * if process has set io priority explicitly, use that. if not, convert
+ * the cpu scheduler nice value to an io priority
+ */
 static inline int task_nice_ioprio(struct task_struct *task)
 {
 	return (task_nice(task) + 20) / 5;
Signature · 1 line
1.7.7.3
author maintainer
#4 lore raw

PATCH 4/4 block: remove ioc_*_changed()

In reply to Tejun Heo

After the previous patch to cfq, there's no ioc_get_changed() user left. This patch yanks out ioc_{ioprio|cgroup|get}_changed() and all related stuff.

Signed-off-by Tejun Heo <[email protected]>
Cc Vivek Goyal <[email protected]>
---
 block/blk-cgroup.c        |   19 ------------
 block/blk-ioc.c           |   68 ---------------------------------------------
 fs/ioprio.c               |    2 +-
 include/linux/iocontext.h |    7 ----
 4 files changed, 1 insertions(+), 95 deletions(-)
block/blk-cgroup.c +0 -19
index 30e0730..a74019b 100644
@@ -47,8 +47,6 @@ static struct cgroup_subsys_state *blkiocg_create(struct cgroup_subsys *,
 						  struct cgroup *);
 static int blkiocg_can_attach(struct cgroup_subsys *, struct cgroup *,
 			      struct cgroup_taskset *);
-static void blkiocg_attach(struct cgroup_subsys *, struct cgroup *,
-			   struct cgroup_taskset *);
 static int blkiocg_pre_destroy(struct cgroup_subsys *, struct cgroup *);
 static void blkiocg_destroy(struct cgroup_subsys *, struct cgroup *);
 static int blkiocg_populate(struct cgroup_subsys *, struct cgroup *);

@@ -63,7 +61,6 @@ struct cgroup_subsys blkio_subsys = {
 	.name = "blkio",
 	.create = blkiocg_create,
 	.can_attach = blkiocg_can_attach,
-	.attach = blkiocg_attach,
 	.pre_destroy = blkiocg_pre_destroy,
 	.destroy = blkiocg_destroy,
 	.populate = blkiocg_populate,

@@ -1729,22 +1726,6 @@ static int blkiocg_can_attach(struct cgroup_subsys *ss, struct cgroup *cgrp,
 	return ret;
 }
 
-static void blkiocg_attach(struct cgroup_subsys *ss, struct cgroup *cgrp,
-			   struct cgroup_taskset *tset)
-{
-	struct task_struct *task;
-	struct io_context *ioc;
-
-	cgroup_taskset_for_each(task, cgrp, tset) {
-		/* we don't lose anything even if ioc allocation fails */
-		ioc = get_task_io_context(task, GFP_ATOMIC, NUMA_NO_NODE);
-		if (ioc) {
-			ioc_cgroup_changed(ioc);
-			put_io_context(ioc);
-		}
-	}
-}
-
 static void blkcg_bypass_start(void)
 	__acquires(&all_q_mutex)
 {
block/blk-ioc.c +0 -68

74 lines not on this page · raw message

fs/ioprio.c +1 -1
index 0f1b951..4864437 100644
@@ -50,7 +50,7 @@ int set_task_ioprio(struct task_struct *task, int ioprio)
 
 	ioc = get_task_io_context(task, GFP_ATOMIC, NUMA_NO_NODE);
 	if (ioc) {
-		ioc_ioprio_changed(ioc, ioprio);
+		ioc->ioprio = ioprio;
 		put_io_context(ioc);
 	}
include/linux/iocontext.h +0 -7
index 6f1a260..df38db2 100644
@@ -6,11 +6,7 @@
 #include <linux/workqueue.h>
 
 enum {
-	ICQ_IOPRIO_CHANGED	= 1 << 0,
-	ICQ_CGROUP_CHANGED	= 1 << 1,
 	ICQ_EXITED		= 1 << 2,
-
-	ICQ_CHANGED_MASK	= ICQ_IOPRIO_CHANGED | ICQ_CGROUP_CHANGED,
 };
 
 /*

@@ -152,9 +148,6 @@ void put_io_context_active(struct io_context *ioc);
 void exit_io_context(struct task_struct *task);
 struct io_context *get_task_io_context(struct task_struct *task,
 				       gfp_t gfp_flags, int node);
-void ioc_ioprio_changed(struct io_context *ioc, int ioprio);
-void ioc_cgroup_changed(struct io_context *ioc);
-unsigned int icq_get_changed(struct io_cq *icq);
 #else
 struct io_context;
 static inline void put_io_context(struct io_context *ioc) { }
Signature · 1 line
1.7.7.3
maintainer
#5 lore raw
In reply to Tejun Heo
Quoted 31 lines · Tejun Heo

Hello, guys.

The changed notification used by cfq is rather odd. cfq caches the associated cfqqs per cic and uses the changed notification to expire those lookup caches.

The explicit notification mechanism might make sense if determining whether the current cache is up-to-date is difficult or expensive; however, that isn't the case here. Determining whether ioprio or cgroup has changed is straight-forward and inexpensive.

This patchset updates cfq to so that it remembers the current ioprio and blkcg in the cic and determines whether cfqq's need to be reset without using the changed notification and drops the changed notification code.

0001-blkcg-add-blkcg-id.patch 0002-cfq-pass-around-cfq_io_cq-instead-of-io_context.patch 0003-cfq-don-t-use-icq_get_changed.patch 0004-block-remove-ioc_-_changed.patch

0001 adds unique u64 id to each blkcg so that policies can tell whether the associated blkcg has changed.

0002 makes cfq pass around cic internally instead of ioc. This is a cleanup in itself and necessary for the next patch.

0003 updates cfq to not use changed notification.

0004 removes code implementing changed notification.

Quoted 8 lines

This patchset is on top of

block/for-3.4/core 671058fb2a2aac4e70f01b316b06bc59b98bd138
+ [1] blkcg: fix percpu stat allocation and remove stats_lock, take#2

and available in the following git branch

git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git blk-ioc-remove-changed

Thanks Tejun, applied [1] and this one to for-3.4/core.

Signature · 1 line
Jens Axboe
#6 lore raw
In reply to Tejun Heo
Quoted 11 lines · Tejun Heo

Hello, guys.

The changed notification used by cfq is rather odd. cfq caches the associated cfqqs per cic and uses the changed notification to expire those lookup caches.

The explicit notification mechanism might make sense if determining whether the current cache is up-to-date is difficult or expensive; however, that isn't the case here. Determining whether ioprio or cgroup has changed is straight-forward and inexpensive.

Quoted 4 lines

This patchset updates cfq to so that it remembers the current ioprio and blkcg in the cic and determines whether cfqq's need to be reset without using the changed notification and drops the changed notification code.

Hi Tejun,

So this patch still breaks cic->cfqq association in asynchronous manner, when new request comes in. So it will still not solve the problem I reported where after doing IO a task changes cgroup and tries to delete the old cgroup and hangs forever as cic->cfqq is still holding a reference to cgroup?

Thanks
Vivek

author maintainer
#7 lore raw
Quoted 5 lines · Vivek Goyal

So this patch still breaks cic->cfqq association in asynchronous manner, when new request comes in. So it will still not solve the problem I reported where after doing IO a task changes cgroup and tries to delete the old cgroup and hangs forever as cic->cfqq is still holding a reference to cgroup?

That one is gonna have to be fixed from cgroup side. I already posted a RFC patchset (you were cc'd there, I think). At the moment, the blocking thing is memcg's ->pre_destroy() usage. I'll ping memcg people once more and if modification there isn't feasible, will implement a workaround in cgroup core.

Thanks.

Signature · 1 line
tejun
#8 lore raw
Quoted 7 lines · Tejun Heo
Quoted 5 lines · Vivek Goyal

So this patch still breaks cic->cfqq association in asynchronous manner, when new request comes in. So it will still not solve the problem I reported where after doing IO a task changes cgroup and tries to delete the old cgroup and hangs forever as cic->cfqq is still holding a reference to cgroup?

Quoted 5 lines

That one is gonna have to be fixed from cgroup side. I already posted a RFC patchset (you were cc'd there, I think). At the moment, the blocking thing is memcg's ->pre_destroy() usage. I'll ping memcg people once more and if modification there isn't feasible, will implement a workaround in cgroup core.

Ok, now I remember that patchset. So the idea is to delete the cgroup and drop the creation reference. Internal blkcg object might still be around and will freed when all the references are gone. Makes sense. Thanks.

Vivek