[PATCH Linux 2.6.12-rc4-mm2 00/03] cfq: various fixes

[PATCH Linux 2.6.12-rc4-mm2 00/03] cfq: various fixes

[Tejun Heo]

 Hello, Jens.

 This patchset is various fixes to cfq.  All patches are against
2.6.12-rc4-mm2.

 One thing that isn't fixed in this patchset but I think might be
problematic is the priority of async queue.  It seems that the async
queue doesn't receive any special attention regarding its priority.
It just follows normal rules and its priority jumps irregularly.
If I'm missing something, plz point out.

[ Start of patch descriptions ]

01_cfq_INSERT_BACK_fix.patch
	: cfq ELEVATOR_INSERT_BACK fix

	When inserting INSERT_BACK request, cfq_insert_request() calls
	cfq_dispatch_requests() repetitively until it returns zero
	indicating no request is dispatched.  This used to flush all
	the requests in the queue to the dispatch queue but, with idle
	slice implemented, the current active queue may decide to wait
	for new request using slice_timer.  When this happens, 0 is
	returned from cfq_dispatch_requests() even when other cfqq's
	have pending requests.  This breaks INSRET_BACK semantics.

	This patch adds @force argument which, when set to non-zero,
	disables idle_slice, and uses the argument when flushing
	cfqq's for INSERT_BACK.  While at it, use INT_MAX instead of
	cfq_quantum when flushing cfqq's, as we're gonna dump all the
	requests and using cfq_quantum does nothing but adding
	unnecessary iterations.

02_cfq_ioc_leak_fix.patch
	: cfq_io_context leak fix

	When a process has more than one cic's associated with it,
	only the first one was kmem_cache_free'd in the original code.
	This patch frees all cic's in cfq_free_io_context().

	While at it, remove unnecessary refcounting from cic's to ioc.
	This reference is created when each cic is created and removed
	altogether when the ioc is exited, and, thus, serves no
	purpose.

03_cfq_remove_unused_fields.patch
	: remove serveral unused fields from cfq data structures

	cfq_data->idle_start, cfq_data->end_prio and cfq_rq->end_pos
	are not used in meaningful way.  Remove'em.

[ End of patch descriptions ]

 Thanks.

--
tejun

Re: [PATCH Linux 2.6.12-rc4-mm2 01/03] cfq: cfq ELEVATOR_INSERT_BACK fix

[Tejun Heo]

01_cfq_INSERT_BACK_fix.patch

	When inserting INSERT_BACK request, cfq_insert_request() calls
	cfq_dispatch_requests() repetitively until it returns zero
	indicating no request is dispatched.  This used to flush all
	the requests in the queue to the dispatch queue but, with idle
	slice implemented, the current active queue may decide to wait
	for new request using slice_timer.  When this happens, 0 is
	returned from cfq_dispatch_requests() even when other cfqq's
	have pending requests.  This breaks INSRET_BACK semantics.

	This patch adds @force argument which, when set to non-zero,
	disables idle_slice, and uses the argument when flushing
	cfqq's for INSERT_BACK.  While at it, use INT_MAX instead of
	cfq_quantum when flushing cfqq's, as we're gonna dump all the
	requests and using cfq_quantum does nothing but adding
	unnecessary iterations.

Signed-off-by: Tejun Heo <htejun@gmail.com>

 cfq-iosched.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

Index: blk-fixes/drivers/block/cfq-iosched.c
===================================================================
--- blk-fixes.orig/drivers/block/cfq-iosched.c	2005-05-25 01:35:16.000000000 +0900
+++ blk-fixes/drivers/block/cfq-iosched.c	2005-05-25 01:35:16.000000000 +0900
@@ -991,7 +991,7 @@ cfq_prio_to_maxrq(struct cfq_data *cfqd,
 /*
  * get next queue for service
  */
-static struct cfq_queue *cfq_select_queue(struct cfq_data *cfqd)
+static struct cfq_queue *cfq_select_queue(struct cfq_data *cfqd, int force)
 {
 	unsigned long now = jiffies;
 	struct cfq_queue *cfqq;
@@ -1012,7 +1012,8 @@ static struct cfq_queue *cfq_select_queu
 	 */
 	if (!RB_EMPTY(&cfqq->sort_list))
 		goto keep_queue;
-	else if (cfq_cfqq_sync(cfqq) && time_before(now, cfqq->slice_end)) {
+	else if (!force && cfq_cfqq_sync(cfqq) &&
+		 time_before(now, cfqq->slice_end)) {
 		if (cfq_arm_slice_timer(cfqd, cfqq))
 			return NULL;
 	}
@@ -1078,7 +1079,8 @@ __cfq_dispatch_requests(struct cfq_data 
 	return dispatched;
 }
 
-static int cfq_dispatch_requests(request_queue_t *q, int max_dispatch)
+static int
+cfq_dispatch_requests(request_queue_t *q, int max_dispatch, int force)
 {
 	struct cfq_data *cfqd = q->elevator->elevator_data;
 	struct cfq_queue *cfqq;
@@ -1086,7 +1088,7 @@ static int cfq_dispatch_requests(request
 	if (!cfqd->busy_queues)
 		return 0;
 
-	cfqq = cfq_select_queue(cfqd);
+	cfqq = cfq_select_queue(cfqd, force);
 	if (cfqq) {
 		cfqq->wait_request = 0;
 		cfqq->must_dispatch = 0;
@@ -1172,7 +1174,7 @@ dispatch:
 		return rq;
 	}
 
-	if (cfq_dispatch_requests(q, cfqd->cfq_quantum))
+	if (cfq_dispatch_requests(q, cfqd->cfq_quantum, 0))
 		goto dispatch;
 
 	return NULL;
@@ -1707,7 +1709,7 @@ cfq_insert_request(request_queue_t *q, s
 
 	switch (where) {
 		case ELEVATOR_INSERT_BACK:
-			while (cfq_dispatch_requests(q, cfqd->cfq_quantum))
+			while (cfq_dispatch_requests(q, INT_MAX, 1))
 				;
 			list_add_tail(&rq->queuelist, &q->queue_head);
 			break;

Re: [PATCH Linux 2.6.12-rc4-mm2 02/03] cfq: cfq_io_context leak fix

[Tejun Heo]

02_cfq_ioc_leak_fix.patch

	When a process has more than one cic's associated with it,
	only the first one was kmem_cache_free'd in the original code.
	This patch frees all cic's in cfq_free_io_context().

	While at it, remove unnecessary refcounting from cic's to ioc.
	This reference is created when each cic is created and removed
	altogether when the ioc is exited, and, thus, serves no
	purpose.

Signed-off-by: Tejun Heo <htejun@gmail.com>

 cfq-iosched.c |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)

Index: blk-fixes/drivers/block/cfq-iosched.c
===================================================================
--- blk-fixes.orig/drivers/block/cfq-iosched.c	2005-05-25 01:35:16.000000000 +0900
+++ blk-fixes/drivers/block/cfq-iosched.c	2005-05-25 01:35:16.000000000 +0900
@@ -1238,6 +1238,14 @@ cfq_find_cfq_hash(struct cfq_data *cfqd,
 
 static void cfq_free_io_context(struct cfq_io_context *cic)
 {
+	struct cfq_io_context *__cic;
+	struct list_head *entry, *next;
+
+	list_for_each_safe(entry, next, &cic->list) {
+		__cic = list_entry(entry, struct cfq_io_context, list);
+		kmem_cache_free(cfq_ioc_pool, __cic);
+	}
+
 	kmem_cache_free(cfq_ioc_pool, cic);
 }
 
@@ -1260,7 +1268,6 @@ static void cfq_exit_single_io_context(s
 
 	cfq_put_queue(cic->cfqq);
 	cic->cfqq = NULL;
-	put_io_context(cic->ioc);
 	spin_unlock(q->queue_lock);
 }
 
@@ -1271,7 +1278,7 @@ static void cfq_exit_single_io_context(s
 static void cfq_exit_io_context(struct cfq_io_context *cic)
 {
 	struct cfq_io_context *__cic;
-	struct list_head *entry, *nxt;
+	struct list_head *entry;
 	unsigned long flags;
 
 	local_irq_save(flags);
@@ -1279,10 +1286,8 @@ static void cfq_exit_io_context(struct c
 	/*
 	 * put the reference this task is holding to the various queues
 	 */
-	list_for_each_safe(entry, nxt, &cic->list) {
+	list_for_each(entry, &cic->list) {
 		__cic = list_entry(entry, struct cfq_io_context, list);
-
-		list_del(&__cic->list);
 		cfq_exit_single_io_context(__cic);
 	}
 
@@ -1471,8 +1476,6 @@ cfq_get_io_context(struct cfq_data *cfqd
 		ioc->cic = cic;
 		ioc->set_ioprio = cfq_ioc_set_ioprio;
 		cic->ioc = ioc;
-		atomic_inc(&ioc->refcount);
-
 		cic->key = cfqd;
 		atomic_inc(&cfqd->ref);
 	} else {
@@ -1509,8 +1512,6 @@ cfq_get_io_context(struct cfq_data *cfqd
 			goto err;
 
 		__cic->ioc = ioc;
-		atomic_inc(&ioc->refcount);
-
 		__cic->key = cfqd;
 		atomic_inc(&cfqd->ref);
 		list_add(&__cic->list, &cic->list);

Re: [PATCH Linux 2.6.12-rc4-mm2 03/03] cfq: remove serveral unused fields from cfq data structures

[Tejun Heo]

03_cfq_remove_unused_fields.patch

	cfq_data->idle_start, cfq_data->end_prio and cfq_rq->end_pos
	are not used in meaningful way.  Remove'em.

Signed-off-by: Tejun Heo <htejun@gmail.com>

 cfq-iosched.c |   12 ++----------
 1 files changed, 2 insertions(+), 10 deletions(-)

Index: blk-fixes/drivers/block/cfq-iosched.c
===================================================================
--- blk-fixes.orig/drivers/block/cfq-iosched.c	2005-05-25 01:35:16.000000000 +0900
+++ blk-fixes/drivers/block/cfq-iosched.c	2005-05-25 01:35:17.000000000 +0900
@@ -143,11 +143,10 @@ struct cfq_data {
 	 */
 	struct timer_list idle_slice_timer;
 	struct work_struct unplug_work;
-	unsigned long idle_start;
 
 	struct cfq_queue *active_queue;
 	struct cfq_io_context *active_cic;
-	int cur_prio, cur_end_prio, end_prio;
+	int cur_prio, cur_end_prio;
 	unsigned int dispatch_slice;
 
 	struct timer_list idle_class_timer;
@@ -234,8 +233,6 @@ struct cfq_rq {
 	struct cfq_queue *cfq_queue;
 	struct cfq_io_context *io_context;
 
-	sector_t end_pos;
-
 	unsigned in_flight : 1;
 	unsigned accounted : 1;
 	unsigned is_sync   : 1;
@@ -770,12 +767,9 @@ static int cfq_get_next_prio_level(struc
 		cfqd->cur_end_prio = cfqd->cur_prio;
 		cfqd->cur_prio = 0;
 	}
-	if (cfqd->cur_end_prio > cfqd->end_prio)
-		cfqd->end_prio = cfqd->cur_end_prio;
-	if (cfqd->end_prio == CFQ_PRIO_LISTS) {
+	if (cfqd->cur_end_prio == CFQ_PRIO_LISTS) {
 		cfqd->cur_prio = 0;
 		cfqd->cur_end_prio = 0;
-		cfqd->end_prio = 0;
 	}
 
 	return prio;
@@ -876,7 +870,6 @@ static int cfq_arm_slice_timer(struct cf
 	if (!timer_pending(&cfqd->idle_slice_timer)) {
 		unsigned long slice_left = cfqq->slice_end - 1;
 
-		cfqd->idle_start = jiffies;
 		cfqd->idle_slice_timer.expires = min(jiffies + cfqd->cfq_slice_idle, slice_left);
 		add_timer(&cfqd->idle_slice_timer);
 	}
@@ -926,7 +919,6 @@ static void cfq_dispatch_sort(request_qu
 	cfq_del_crq_rb(crq);
 	cfq_remove_merge_hints(q, crq);
 
-	crq->end_pos = crq->request->sector + crq->request->nr_sectors;
 	crq->in_flight = 1;
 	crq->requeued = 0;
 	cfqq->in_flight++;

Re: [PATCH Linux 2.6.12-rc4-mm2 00/03] cfq: various fixes

[Jens Axboe]

On Wed, May 25 2005, Tejun Heo wrote:
>  Hello, Jens.
> 
>  This patchset is various fixes to cfq.  All patches are against
> 2.6.12-rc4-mm2.
> 
>  One thing that isn't fixed in this patchset but I think might be
> problematic is the priority of async queue.  It seems that the async
> queue doesn't receive any special attention regarding its priority.
> It just follows normal rules and its priority jumps irregularly.
> If I'm missing something, plz point out.
> 
> [ Start of patch descriptions ]
> 
> 01_cfq_INSERT_BACK_fix.patch
> 	: cfq ELEVATOR_INSERT_BACK fix
> 
> 	When inserting INSERT_BACK request, cfq_insert_request() calls
> 	cfq_dispatch_requests() repetitively until it returns zero
> 	indicating no request is dispatched.  This used to flush all
> 	the requests in the queue to the dispatch queue but, with idle
> 	slice implemented, the current active queue may decide to wait
> 	for new request using slice_timer.  When this happens, 0 is
> 	returned from cfq_dispatch_requests() even when other cfqq's
> 	have pending requests.  This breaks INSRET_BACK semantics.
> 
> 	This patch adds @force argument which, when set to non-zero,
> 	disables idle_slice, and uses the argument when flushing
> 	cfqq's for INSERT_BACK.  While at it, use INT_MAX instead of
> 	cfq_quantum when flushing cfqq's, as we're gonna dump all the
> 	requests and using cfq_quantum does nothing but adding
> 	unnecessary iterations.
> 
> 02_cfq_ioc_leak_fix.patch
> 	: cfq_io_context leak fix
> 
> 	When a process has more than one cic's associated with it,
> 	only the first one was kmem_cache_free'd in the original code.
> 	This patch frees all cic's in cfq_free_io_context().
> 
> 	While at it, remove unnecessary refcounting from cic's to ioc.
> 	This reference is created when each cic is created and removed
> 	altogether when the ioc is exited, and, thus, serves no
> 	purpose.
> 
> 03_cfq_remove_unused_fields.patch
> 	: remove serveral unused fields from cfq data structures
> 
> 	cfq_data->idle_start, cfq_data->end_prio and cfq_rq->end_pos
> 	are not used in meaningful way.  Remove'em.

All three patches look good, thanks!

-- 
Jens Axboe