[PATCH] Default iosched fixes (was: Fall back io scheduler for 2.6.15?)

[PATCH] Default iosched fixes (was: Fall back io scheduler for 2.6.15?)

[Nate Diller]

My previous default iosched patch did a poor job dealing with the
'elevator=' boot-time option.  Jens' recent solution would fail if the
selected default were compiled as a module, and I find that scenario
useful for debugging.  This patch dynamically evaluates which default
to use, and emits suitable error messages when the requested scheduler
is not available.  It also indicates the compiled-in default scheduler
at registration time, and includes a version of Chuck Ebbert's 'as' ->
'anticipatory' compatability patch.

Tested for a range of boot options on 2.6.13-rc5-mm1, should apply to
any recent kernel.

Signed-off-by: Nate Diller <nate.diller@gmail.com>

--- drivers/block/elevator.c	2006-01-19 15:01:03.000000000 -0800
+++ drivers/block/elevator.c	2006-01-19 15:03:22.000000000 -0800
@@ -153,27 +153,16 @@ static int elevator_attach(request_queue

 static char chosen_elevator[16];

-static void elevator_setup_default(void)
+static int __init elevator_setup(char *str)
 {
-	struct elevator_type *e;
-
 	/*
-	 * If default has not been set, use the compiled-in selection.
+	 * Be backwards-compatible with previous kernels, so users
+	 * won't get the wrong scheduler.
 	 */
-	if (!chosen_elevator[0])
-		strcpy(chosen_elevator, CONFIG_DEFAULT_IOSCHED);
-
- 	/*
- 	 * If the given scheduler is not available, fall back to no-op.
- 	 */
- 	if (!(e = elevator_find(chosen_elevator)))
- 		strcpy(chosen_elevator, "noop");
-	elevator_put(e);
-}
-
-static int __init elevator_setup(char *str)
-{
-	strncpy(chosen_elevator, str, sizeof(chosen_elevator) - 1);
+	if (!strcmp(str, "as"))
+		strcpy(chosen_elevator, "anticipatory");
+	else
+		strncpy(chosen_elevator, str, sizeof(chosen_elevator) - 1);
 	return 0;
 }

@@ -185,14 +174,16 @@ int elevator_init(request_queue_t *q, ch
 	struct elevator_queue *eq;
 	int ret = 0;

-	elevator_setup_default();
+	if (name && !(e = elevator_get(name)))
+		return -EINVAL;

-	if (!name)
-		name = chosen_elevator;
+	if (!e && chosen_elevator[0] && !(e = elevator_get(chosen_elevator)))
+		printk("I/O scheduler %s not found\n", chosen_elevator);

-	e = elevator_get(name);
-	if (!e)
-		return -EINVAL;
+	if (!e && !(e = elevator_get(CONFIG_DEFAULT_IOSCHED))) {
+		e = elevator_get("noop");
+		printk("Default I/O scheduler not found, using no-op\n");
+	}

 	eq = kmalloc(sizeof(struct elevator_queue), GFP_KERNEL);
 	if (!eq) {
@@ -566,7 +557,9 @@ int elv_register(struct elevator_type *e
 	spin_unlock_irq(&elv_list_lock);

 	printk(KERN_INFO "io scheduler %s registered", e->elevator_name);
-	if (!strcmp(e->elevator_name, chosen_elevator))
+	if (!strcmp(e->elevator_name, chosen_elevator) ||
+			(!chosen_elevator[0] &&
+			 !strcmp(e->elevator_name, CONFIG_DEFAULT_IOSCHED)))
 		printk(" (default)");
 	printk("\n");
 	return 0;

Re: [PATCH] Default iosched fixes (was: Fall back io scheduler for 2.6.15?)

[Jens Axboe]

On Thu, Jan 19 2006, Nate Diller wrote:
> My previous default iosched patch did a poor job dealing with the
> 'elevator=' boot-time option.  Jens' recent solution would fail if the
> selected default were compiled as a module, and I find that scenario
> useful for debugging.  This patch dynamically evaluates which default
> to use, and emits suitable error messages when the requested scheduler
> is not available.  It also indicates the compiled-in default scheduler
> at registration time, and includes a version of Chuck Ebbert's 'as' ->
> 'anticipatory' compatability patch.
> 
> Tested for a range of boot options on 2.6.13-rc5-mm1, should apply to
> any recent kernel.

Your patch doesn't apply at all, the file has even been moved. Please
test a 2.6.15 or newer kernel and you'll find that the problem you
envision doesn't exist.

-- 
Jens Axboe

[PATCH 1/2][RESEND] Default iosched fixes (was: Fall back io scheduler for 2.6.15?)

[Nate Diller]

My previous default iosched patch did a poor job dealing with the
'elevator=' boot-time option.  The old behavior falls back to the
compiled-in default if the requested one is not registered at boot
time.  This patch dynamically evaluates which default
to use, and emits a suitable error message when the requested scheduler
is not available.  It also does the 'as' -> 'anticipatory' conversion
before elevator registration, which along with a modified registration
function, allows it to correctly indicate which default scheduler is
in use.

Tested for a range of boot options on 2.6.16-rc1-mm2.

Signed-off-by: Nate Diller <nate.diller@gmail.com>

---

Jens: i'm submitting this seperately from the objectionable 'modular
default scheduler' check.  Is there anything else I should change?

--- ./block/elevator.c	2006-01-20 13:47:16.000000000 -0800
+++ ./block/elevator.c	2006-01-20 14:52:53.000000000 -0800
@@ -140,35 +140,16 @@ static int elevator_attach(request_queue

 static char chosen_elevator[16];

-static void elevator_setup_default(void)
+static int __init elevator_setup(char *str)
 {
-	struct elevator_type *e;
-
-	/*
-	 * If default has not been set, use the compiled-in selection.
-	 */
-	if (!chosen_elevator[0])
-		strcpy(chosen_elevator, CONFIG_DEFAULT_IOSCHED);
-
 	/*
 	 * Be backwards-compatible with previous kernels, so users
 	 * won't get the wrong elevator.
 	 */
-	if (!strcmp(chosen_elevator, "as"))
+	if (!strcmp(str, "as"))
 		strcpy(chosen_elevator, "anticipatory");
-
- 	/*
- 	 * If the given scheduler is not available, fall back to the default
- 	 */
- 	if ((e = elevator_find(chosen_elevator)))
-		elevator_put(e);
 	else
- 		strcpy(chosen_elevator, CONFIG_DEFAULT_IOSCHED);
-}
-
-static int __init elevator_setup(char *str)
-{
-	strncpy(chosen_elevator, str, sizeof(chosen_elevator) - 1);
+		strncpy(chosen_elevator, str, sizeof(chosen_elevator) - 1);
 	return 0;
 }

@@ -185,15 +166,15 @@ int elevator_init(request_queue_t *q, ch
 	q->end_sector = 0;
 	q->boundary_rq = NULL;

-	elevator_setup_default();
-
-	if (!name)
-		name = chosen_elevator;
-
-	e = elevator_get(name);
-	if (!e)
+	if (name && !(e = elevator_get(name)))
 		return -EINVAL;

+	if (!e && !(e = elevator_get(chosen_elevator))) {
+		e = elevator_get(CONFIG_DEFAULT_IOSCHED);
+		if (*chosen_elevator)
+			printk("I/O scheduler %s not found\n", chosen_elevator);
+	}
+
 	eq = kmalloc(sizeof(struct elevator_queue), GFP_KERNEL);
 	if (!eq) {
 		elevator_put(e);
@@ -681,8 +662,10 @@ int elv_register(struct elevator_type *e
 	spin_unlock_irq(&elv_list_lock);

 	printk(KERN_INFO "io scheduler %s registered", e->elevator_name);
-	if (!strcmp(e->elevator_name, chosen_elevator))
-		printk(" (default)");
+	if (!strcmp(e->elevator_name, chosen_elevator) ||
+			(!*chosen_elevator &&
+			 !strcmp(e->elevator_name, CONFIG_DEFAULT_IOSCHED)))
+				printk(" (default)");
 	printk("\n");
 	return 0;
 }

[PATCH 2/2][RESEND] Default iosched fixes (was: Fall back io scheduler for 2.6.15?)

[Nate Diller]

Jens has decided that allowing the default scheduler to be a module is
a bug, and should not be allowed under kconfig.  However, I find that
scenario useful for debugging, and wish for the kernel to be able to
handle this situation without OOPSing, if I enable such an option in
the .config directly.  This patch dynamically checks for the presence
of the compiled-in default, and falls back to no-op, emitting a
suitable error message, when the default is not available

Tested for a range of boot options on 2.6.16-rc1-mm2.

Signed-off-by: Nate Diller <nate.diller@gmail.com>

--- ./block/elevator.c	2006-01-20 14:52:53.000000000 -0800
+++ ./block/elevator.c	2006-01-20 15:00:16.000000000 -0800
@@ -169,10 +169,12 @@ int elevator_init(request_queue_t *q, ch
 	if (name && !(e = elevator_get(name)))
 		return -EINVAL;

-	if (!e && !(e = elevator_get(chosen_elevator))) {
-		e = elevator_get(CONFIG_DEFAULT_IOSCHED);
-		if (*chosen_elevator)
-			printk("I/O scheduler %s not found\n", chosen_elevator);
+	if (!e && *chosen_elevator && !(e = elevator_get(chosen_elevator)))
+		printk("I/O scheduler %s not found\n", chosen_elevator);
+
+	if (!e && !(e = elevator_get(CONFIG_DEFAULT_IOSCHED))) {
+		printk("Default I/O scheduler not found, using no-op\n");
+		e = elevator_get("noop");
 	}

 	eq = kmalloc(sizeof(struct elevator_queue), GFP_KERNEL);

Re: [PATCH 2/2][RESEND] Default iosched fixes (was: Fall back io scheduler for 2.6.15?)

[Jens Axboe]

On Fri, Jan 20 2006, Nate Diller wrote:
> Jens has decided that allowing the default scheduler to be a module is
> a bug, and should not be allowed under kconfig.  However, I find that
> scenario useful for debugging, and wish for the kernel to be able to
> handle this situation without OOPSing, if I enable such an option in
> the .config directly.  This patch dynamically checks for the presence
> of the compiled-in default, and falls back to no-op, emitting a
> suitable error message, when the default is not available
> 
> Tested for a range of boot options on 2.6.16-rc1-mm2.

NAK, if you find it useful for testing, keep this patch along with the
patch that enables you to config the kernel that way.

-- 
Jens Axboe

Re: [PATCH 1/2][RESEND] Default iosched fixes (was: Fall back io scheduler for 2.6.15?)

[Jens Axboe]

On Fri, Jan 20 2006, Nate Diller wrote:
> My previous default iosched patch did a poor job dealing with the
> 'elevator=' boot-time option.  The old behavior falls back to the
> compiled-in default if the requested one is not registered at boot
> time.  This patch dynamically evaluates which default
> to use, and emits a suitable error message when the requested scheduler
> is not available.  It also does the 'as' -> 'anticipatory' conversion
> before elevator registration, which along with a modified registration
> function, allows it to correctly indicate which default scheduler is
> in use.

I'm a little confused by your description - what problem does this patch
actually solve? We already fall back to the default, and we already do
the "as" conversion. It does seem to cleanup the code, just curious
since your description seems to promise a little more than what it
actually adds.

-- 
Jens Axboe

Re: [PATCH 1/2][RESEND] Default iosched fixes (was: Fall back io scheduler for 2.6.15?)

[Nate Diller]

On 1/21/06, Jens Axboe <axboe@suse.de> wrote:
> On Fri, Jan 20 2006, Nate Diller wrote:
> > My previous default iosched patch did a poor job dealing with the
> > 'elevator=' boot-time option.  The old behavior falls back to the
> > compiled-in default if the requested one is not registered at boot
> > time.  This patch dynamically evaluates which default
> > to use, and emits a suitable error message when the requested scheduler
> > is not available.  It also does the 'as' -> 'anticipatory' conversion
> > before elevator registration, which along with a modified registration
> > function, allows it to correctly indicate which default scheduler is
> > in use.
>
> I'm a little confused by your description - what problem does this patch
> actually solve? We already fall back to the default, and we already do
> the "as" conversion. It does seem to cleanup the code, just curious
> since your description seems to promise a little more than what it
> actually adds.

It makes the ' (default)' printk that happens at elevator registration
time behave (more) correctly.  My original patch rather ignored that
segment of code.  The current behavior is to only print ' (default)'
when one was specified at boot time, and not if 'as' was requested
either, since it doesn't understand the 'as -> anticipatory'
conversion.  Now, it will display correctly the one selected at
compile-time, if none was specified at boot, and when the boot-time
option was 'as'.

It also handles modular defaults better; although they cannot be
specified in kconfig, a default requested at boot-time will now still
work, even if it's a module.  When the boot-time requested scheduler
is not loaded, it will fall back to the compiled-in default; when it
is, it gets used.

NATE

Re: [PATCH 1/2][RESEND] Default iosched fixes (was: Fall back io scheduler for 2.6.15?)

[Jens Axboe]

On Mon, Jan 23 2006, Nate Diller wrote:
> On 1/21/06, Jens Axboe <axboe@suse.de> wrote:
> > On Fri, Jan 20 2006, Nate Diller wrote:
> > > My previous default iosched patch did a poor job dealing with the
> > > 'elevator=' boot-time option.  The old behavior falls back to the
> > > compiled-in default if the requested one is not registered at boot
> > > time.  This patch dynamically evaluates which default
> > > to use, and emits a suitable error message when the requested scheduler
> > > is not available.  It also does the 'as' -> 'anticipatory' conversion
> > > before elevator registration, which along with a modified registration
> > > function, allows it to correctly indicate which default scheduler is
> > > in use.
> >
> > I'm a little confused by your description - what problem does this patch
> > actually solve? We already fall back to the default, and we already do
> > the "as" conversion. It does seem to cleanup the code, just curious
> > since your description seems to promise a little more than what it
> > actually adds.
> 
> It makes the ' (default)' printk that happens at elevator registration
> time behave (more) correctly.  My original patch rather ignored that
> segment of code.  The current behavior is to only print ' (default)'
> when one was specified at boot time, and not if 'as' was requested
> either, since it doesn't understand the 'as -> anticipatory'
> conversion.  Now, it will display correctly the one selected at
> compile-time, if none was specified at boot, and when the boot-time
> option was 'as'.
> 
> It also handles modular defaults better; although they cannot be
> specified in kconfig, a default requested at boot-time will now still
> work, even if it's a module.  When the boot-time requested scheduler
> is not loaded, it will fall back to the compiled-in default; when it
> is, it gets used.

Ok, thanks for the (better) explanation. I'll add your patch for
inclusion, thanks.

-- 
Jens Axboe