There have been numerous reports of problems with nbd and cfq. Deadline
gives better performance for nbd, anyway, so let's use it by default.
--
Paul
On Fri, 08 Feb 2008 11:47:42 -0500 Paul Clements wrote:
> There have been numerous reports of problems with nbd and cfq. Deadline
> gives better performance for nbd, anyway, so let's use it by default.
so what happens with this patch on cfq-only or as-only kernels?
---
~Randy
On Fri, 8 Feb 2008 09:33:41 -0800 Randy Dunlap <[email protected]> wrote:
> On Fri, 08 Feb 2008 11:47:42 -0500 Paul Clements wrote:
>
> > There have been numerous reports of problems with nbd and cfq. Deadline
> > gives better performance for nbd, anyway, so let's use it by default.
Please define "problems". If it's just "slowness" then we can live with
that, but I'd hope that Jens is aware and that it's understood.
It it's "hangs" or "oopses" then we panic.
> so what happens with this patch on cfq-only or as-only kernels?
I assume the elevator_init() call fails and the default elevator continues
to be used. Perhaps an informative printk is needed.
Andrew Morton wrote:
> On Fri, 8 Feb 2008 09:33:41 -0800 Randy Dunlap <[email protected]> wrote:
>
>> On Fri, 08 Feb 2008 11:47:42 -0500 Paul Clements wrote:
>>
>>> There have been numerous reports of problems with nbd and cfq. Deadline
>>> gives better performance for nbd, anyway, so let's use it by default.
>
> Please define "problems". If it's just "slowness" then we can live with
> that, but I'd hope that Jens is aware and that it's understood.
>
> It it's "hangs" or "oopses" then we panic.
The two problems I have experienced (which may already be fixed):
1) nbd hangs with cfq on RHEL 5 (2.6.18) -- this may well have been fixed
There's a similar debian bug that has been filed as well:
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=447638
2) nbd performs about 10% better (the last time I tested) with deadline
vs. cfq (the overhead of cfq doesn't provide much advantage to nbd [not
being a real disk], and you end up going through the I/O scheduler on
the nbd server anyway, so it makes sense that deadline is better with nbd)
There have been posts to nbd-general mailing list about problems with
cfq and nbd also.
--
Paul
On Fri, Feb 08 2008, Paul Clements wrote:
> Andrew Morton wrote:
> >On Fri, 8 Feb 2008 09:33:41 -0800 Randy Dunlap <[email protected]>
> >wrote:
> >
> >>On Fri, 08 Feb 2008 11:47:42 -0500 Paul Clements wrote:
> >>
> >>>There have been numerous reports of problems with nbd and cfq. Deadline
> >>>gives better performance for nbd, anyway, so let's use it by default.
> >
> >Please define "problems". If it's just "slowness" then we can live with
> >that, but I'd hope that Jens is aware and that it's understood.
> >
> >It it's "hangs" or "oopses" then we panic.
>
> The two problems I have experienced (which may already be fixed):
>
> 1) nbd hangs with cfq on RHEL 5 (2.6.18) -- this may well have been fixed
>
> There's a similar debian bug that has been filed as well:
>
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=447638
>
> 2) nbd performs about 10% better (the last time I tested) with deadline
> vs. cfq (the overhead of cfq doesn't provide much advantage to nbd [not
> being a real disk], and you end up going through the I/O scheduler on
> the nbd server anyway, so it makes sense that deadline is better with nbd)
>
> There have been posts to nbd-general mailing list about problems with
> cfq and nbd also.
I'm fine with that, it's one of those things we'll do automatically when
we have some sort of disk profile system setup. Devices without seek
penalties should not use AS or CFQ.
Asking for a non-existing elevator is not an issue, but it may trigger
both printks and a switch to another elevator. So if you ask for
"deadline" and it's modular, you'll get cfq again if it's the default.
Your patch looks bad though, you forget to exit the old elevator. And
you don't check the return value of elevator_init().
All in all, your patch definitely needs more work before it can be
included.
--
Jens Axboe
On Fri, Feb 08 2008, Jens Axboe wrote:
> On Fri, Feb 08 2008, Paul Clements wrote:
> > Andrew Morton wrote:
> > >On Fri, 8 Feb 2008 09:33:41 -0800 Randy Dunlap <[email protected]>
> > >wrote:
> > >
> > >>On Fri, 08 Feb 2008 11:47:42 -0500 Paul Clements wrote:
> > >>
> > >>>There have been numerous reports of problems with nbd and cfq. Deadline
> > >>>gives better performance for nbd, anyway, so let's use it by default.
> > >
> > >Please define "problems". If it's just "slowness" then we can live with
> > >that, but I'd hope that Jens is aware and that it's understood.
> > >
> > >It it's "hangs" or "oopses" then we panic.
> >
> > The two problems I have experienced (which may already be fixed):
> >
> > 1) nbd hangs with cfq on RHEL 5 (2.6.18) -- this may well have been fixed
> >
> > There's a similar debian bug that has been filed as well:
> >
> > http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=447638
> >
> > 2) nbd performs about 10% better (the last time I tested) with deadline
> > vs. cfq (the overhead of cfq doesn't provide much advantage to nbd [not
> > being a real disk], and you end up going through the I/O scheduler on
> > the nbd server anyway, so it makes sense that deadline is better with nbd)
> >
> > There have been posts to nbd-general mailing list about problems with
> > cfq and nbd also.
>
> I'm fine with that, it's one of those things we'll do automatically when
> we have some sort of disk profile system setup. Devices without seek
> penalties should not use AS or CFQ.
>
> Asking for a non-existing elevator is not an issue, but it may trigger
> both printks and a switch to another elevator. So if you ask for
> "deadline" and it's modular, you'll get cfq again if it's the default.
>
> Your patch looks bad though, you forget to exit the old elevator. And
> you don't check the return value of elevator_init().
>
> All in all, your patch definitely needs more work before it can be
> included.
argr and please don't CC closed lists on lkml, thanks! it's hugely
annoying to everyone that replies in the thread.
--
Jens Axboe
Jens Axboe wrote:
> On Fri, Feb 08 2008, Jens Axboe wrote:
>> On Fri, Feb 08 2008, Paul Clements wrote:
>>> Andrew Morton wrote:
>>>> On Fri, 8 Feb 2008 09:33:41 -0800 Randy Dunlap <[email protected]>
>>>> wrote:
>>>>
>>>>> On Fri, 08 Feb 2008 11:47:42 -0500 Paul Clements wrote:
>>>>>
>>>>>> There have been numerous reports of problems with nbd and cfq. Deadline
>>>>>> gives better performance for nbd, anyway, so let's use it by default.
>>>> Please define "problems". If it's just "slowness" then we can live with
>>>> that, but I'd hope that Jens is aware and that it's understood.
>>>>
>>>> It it's "hangs" or "oopses" then we panic.
>>> The two problems I have experienced (which may already be fixed):
>>>
>>> 1) nbd hangs with cfq on RHEL 5 (2.6.18) -- this may well have been fixed
>>>
>>> There's a similar debian bug that has been filed as well:
>>>
>>> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=447638
>>>
>>> 2) nbd performs about 10% better (the last time I tested) with deadline
>>> vs. cfq (the overhead of cfq doesn't provide much advantage to nbd [not
>>> being a real disk], and you end up going through the I/O scheduler on
>>> the nbd server anyway, so it makes sense that deadline is better with nbd)
>>>
>>> There have been posts to nbd-general mailing list about problems with
>>> cfq and nbd also.
>> I'm fine with that, it's one of those things we'll do automatically when
>> we have some sort of disk profile system setup. Devices without seek
>> penalties should not use AS or CFQ.
>>
>> Asking for a non-existing elevator is not an issue, but it may trigger
>> both printks and a switch to another elevator. So if you ask for
>> "deadline" and it's modular, you'll get cfq again if it's the default.
>>
>> Your patch looks bad though, you forget to exit the old elevator. And
>> you don't check the return value of elevator_init().
>>
>> All in all, your patch definitely needs more work before it can be
>> included.
Thanks Jens. This one should be better.
--
Paul
On Fri, 08 Feb 2008 16:23:01 -0500
Paul Clements <[email protected]> wrote:
> --- ./drivers/block/nbd.c.max_nbd_killed 2008-02-07 16:46:24.000000000 -0500
> +++ ./drivers/block/nbd.c 2008-02-08 16:13:01.000000000 -0500
> @@ -667,6 +667,12 @@ static int __init nbd_init(void)
> put_disk(disk);
> goto out;
> }
> + if (elevator_init(disk->queue, "deadline") != 0) {
> + if (elevator_init(disk->queue, "noop") != 0) {
> + put_disk(disk);
> + goto out;
> + }
> + }
> }
- if the user doesn't have deadline or noop configured, NBD will now
fail. That's a non-backward-compatible change.
- when it fails, it will fail silently. Puzzled and angry users.
- when it fails, it will inappropriately return -ENOMEM.
On Feb 8, 2008 1:41 PM, Paul Clements <[email protected]> wrote:
> Andrew Morton wrote:
> > On Fri, 8 Feb 2008 09:33:41 -0800 Randy Dunlap <[email protected]> wrote:
> >
> >> On Fri, 08 Feb 2008 11:47:42 -0500 Paul Clements wrote:
> >>
> >>> There have been numerous reports of problems with nbd and cfq. Deadline
> >>> gives better performance for nbd, anyway, so let's use it by default.
> >
> > Please define "problems". If it's just "slowness" then we can live with
> > that, but I'd hope that Jens is aware and that it's understood.
> >
> > It it's "hangs" or "oopses" then we panic.
>
> The two problems I have experienced (which may already be fixed):
>
> 1) nbd hangs with cfq on RHEL 5 (2.6.18) -- this may well have been fixed
It has been fixed in RHEL5 (but not yet released AFAIK):
https://bugzilla.redhat.com/show_bug.cgi?id=241540#c43
On a slightly related performance note; I'm seeing that for NBD
devices both max_hw_sectors_kb and max_sectors_kb are 127. If I set
it to be 256 with the following patch I see a 25% improvement in
overall throughput:
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 5def9c5..ed63e2f 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -764,6 +764,7 @@ static int __init nbd_init(void)
put_disk(disk);
goto out;
}
+ blk_queue_max_sectors(disk->queue, 512);
}
if (register_blkdev(NBD_MAJOR, "nbd")) {
Any chance we can take steps to make NBD not be artificially slow?
Any other recommendations for improving NBD throughput?
regards,
Mike
Take 3...this should address all the issues.
On Sat, 09 Feb 2008 08:30:40 -0500
Paul Clements <[email protected]> wrote:
> + old_e = disk->queue->elevator;
> + if (elevator_init(disk->queue, "deadline") == 0 ||
> + elevator_init(disk->queue, "noop") == 0) {
> + elevator_exit(old_e);
> + }
> }
afacit elevator_init() will not trigger a request_module(). And you really
do want to trigger the request_module() here. Perhaps the block layer
should provide a means of doing so?
On Tue, Feb 12 2008, Andrew Morton wrote:
> On Sat, 09 Feb 2008 08:30:40 -0500
> Paul Clements <[email protected]> wrote:
>
> > + old_e = disk->queue->elevator;
> > + if (elevator_init(disk->queue, "deadline") == 0 ||
> > + elevator_init(disk->queue, "noop") == 0) {
> > + elevator_exit(old_e);
> > + }
> > }
>
> afacit elevator_init() will not trigger a request_module(). And you really
> do want to trigger the request_module() here. Perhaps the block layer
> should provide a means of doing so?
Good point, I think elevator_get() should do that automatically. Does
this look sane?
diff --git a/block/elevator.c b/block/elevator.c
index bafbae0..88318c3 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -134,6 +134,21 @@ static struct elevator_type *elevator_get(const char *name)
spin_lock(&elv_list_lock);
e = elevator_find(name);
+ if (!e) {
+ char elv[ELV_NAME_MAX + strlen("-iosched")];
+
+ spin_unlock(&elv_list_lock);
+
+ if (!strcmp(name, "anticipatory"))
+ sprintf(elv, "as-iosched");
+ else
+ sprintf(elv, "%s-iosched", name);
+
+ request_module(elv);
+ spin_lock(&elv_list_lock);
+ e = elevator_find(name);
+ }
+
if (e && !try_module_get(e->elevator_owner))
e = NULL;
--
Jens Axboe
On Mon, 18 Feb 2008 19:16:30 +0100 Jens Axboe <[email protected]> wrote:
> On Tue, Feb 12 2008, Andrew Morton wrote:
> > On Sat, 09 Feb 2008 08:30:40 -0500
> > Paul Clements <[email protected]> wrote:
> >
> > > + old_e = disk->queue->elevator;
> > > + if (elevator_init(disk->queue, "deadline") == 0 ||
> > > + elevator_init(disk->queue, "noop") == 0) {
> > > + elevator_exit(old_e);
> > > + }
> > > }
> >
> > afacit elevator_init() will not trigger a request_module(). And you really
> > do want to trigger the request_module() here. Perhaps the block layer
> > should provide a means of doing so?
>
> Good point, I think elevator_get() should do that automatically. Does
> this look sane?
>
> diff --git a/block/elevator.c b/block/elevator.c
> index bafbae0..88318c3 100644
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -134,6 +134,21 @@ static struct elevator_type *elevator_get(const char *name)
> spin_lock(&elv_list_lock);
>
> e = elevator_find(name);
> + if (!e) {
> + char elv[ELV_NAME_MAX + strlen("-iosched")];
> +
> + spin_unlock(&elv_list_lock);
> +
> + if (!strcmp(name, "anticipatory"))
> + sprintf(elv, "as-iosched");
> + else
> + sprintf(elv, "%s-iosched", name);
> +
> + request_module(elv);
> + spin_lock(&elv_list_lock);
> + e = elevator_find(name);
> + }
> +
> if (e && !try_module_get(e->elevator_owner))
> e = NULL;
Looks nice and simple. There might be some of the usual ordering problems
when this is called during boot, maybe is-initramfs-available-yet problems,
etc. But it's unlikely to make things regress from where they are now.
Should we emit a warning if the desired elevator wasn't available?
On Mon, Feb 18 2008, Andrew Morton wrote:
> On Mon, 18 Feb 2008 19:16:30 +0100 Jens Axboe <[email protected]> wrote:
>
> > On Tue, Feb 12 2008, Andrew Morton wrote:
> > > On Sat, 09 Feb 2008 08:30:40 -0500
> > > Paul Clements <[email protected]> wrote:
> > >
> > > > + old_e = disk->queue->elevator;
> > > > + if (elevator_init(disk->queue, "deadline") == 0 ||
> > > > + elevator_init(disk->queue, "noop") == 0) {
> > > > + elevator_exit(old_e);
> > > > + }
> > > > }
> > >
> > > afacit elevator_init() will not trigger a request_module(). And you really
> > > do want to trigger the request_module() here. Perhaps the block layer
> > > should provide a means of doing so?
> >
> > Good point, I think elevator_get() should do that automatically. Does
> > this look sane?
> >
> > diff --git a/block/elevator.c b/block/elevator.c
> > index bafbae0..88318c3 100644
> > --- a/block/elevator.c
> > +++ b/block/elevator.c
> > @@ -134,6 +134,21 @@ static struct elevator_type *elevator_get(const char *name)
> > spin_lock(&elv_list_lock);
> >
> > e = elevator_find(name);
> > + if (!e) {
> > + char elv[ELV_NAME_MAX + strlen("-iosched")];
> > +
> > + spin_unlock(&elv_list_lock);
> > +
> > + if (!strcmp(name, "anticipatory"))
> > + sprintf(elv, "as-iosched");
> > + else
> > + sprintf(elv, "%s-iosched", name);
> > +
> > + request_module(elv);
> > + spin_lock(&elv_list_lock);
> > + e = elevator_find(name);
> > + }
> > +
> > if (e && !try_module_get(e->elevator_owner))
> > e = NULL;
>
> Looks nice and simple. There might be some of the usual ordering problems
> when this is called during boot, maybe is-initramfs-available-yet problems,
> etc. But it's unlikely to make things regress from where they are now.
Isn't request_module() and below robust enough to handle that?
> Should we emit a warning if the desired elevator wasn't available?
Hmm, not sure. Either the request came from a driver, in which case
it'll be notified that we could not load that elevator. Or it'll come
through sysfs online switching, in which case we already print a
warning.
--
Jens Axboe
On Tue, Feb 19 2008, Jens Axboe wrote:
> On Mon, Feb 18 2008, Andrew Morton wrote:
> > On Mon, 18 Feb 2008 19:16:30 +0100 Jens Axboe <[email protected]> wrote:
> >
> > > On Tue, Feb 12 2008, Andrew Morton wrote:
> > > > On Sat, 09 Feb 2008 08:30:40 -0500
> > > > Paul Clements <[email protected]> wrote:
> > > >
> > > > > + old_e = disk->queue->elevator;
> > > > > + if (elevator_init(disk->queue, "deadline") == 0 ||
> > > > > + elevator_init(disk->queue, "noop") == 0) {
> > > > > + elevator_exit(old_e);
> > > > > + }
> > > > > }
> > > >
> > > > afacit elevator_init() will not trigger a request_module(). And you really
> > > > do want to trigger the request_module() here. Perhaps the block layer
> > > > should provide a means of doing so?
> > >
> > > Good point, I think elevator_get() should do that automatically. Does
> > > this look sane?
> > >
> > > diff --git a/block/elevator.c b/block/elevator.c
> > > index bafbae0..88318c3 100644
> > > --- a/block/elevator.c
> > > +++ b/block/elevator.c
> > > @@ -134,6 +134,21 @@ static struct elevator_type *elevator_get(const char *name)
> > > spin_lock(&elv_list_lock);
> > >
> > > e = elevator_find(name);
> > > + if (!e) {
> > > + char elv[ELV_NAME_MAX + strlen("-iosched")];
> > > +
> > > + spin_unlock(&elv_list_lock);
> > > +
> > > + if (!strcmp(name, "anticipatory"))
> > > + sprintf(elv, "as-iosched");
> > > + else
> > > + sprintf(elv, "%s-iosched", name);
> > > +
> > > + request_module(elv);
> > > + spin_lock(&elv_list_lock);
> > > + e = elevator_find(name);
> > > + }
> > > +
> > > if (e && !try_module_get(e->elevator_owner))
> > > e = NULL;
> >
> > Looks nice and simple. There might be some of the usual ordering problems
> > when this is called during boot, maybe is-initramfs-available-yet problems,
> > etc. But it's unlikely to make things regress from where they are now.
>
> Isn't request_module() and below robust enough to handle that?
BTW, I've verified that it works as expected (at least after boot):
carl:/sys/block/sda/queue # cat scheduler
noop [cfq]
carl:/sys/block/sda/queue # echo anticipatory > scheduler
carl:/sys/block/sda/queue # dmesg
[...]
io scheduler anticipatory registered
carl:/sys/block/sda/queue # cat scheduler
noop cfq [anticipatory]
So it properly loads as-iosched instead of failing, like it would have
done before and required the user to do a modprobe as-iosched first.
carl:/sys/block/sda/queue # echo foobar > scheduler
-bash: echo: write error: Invalid argument
carl:/sys/block/sda/queue # dmesg
[...]
elevator: type foobar not found
--
Jens Axboe
On Tue, 19 Feb 2008 10:24:28 +0100 Jens Axboe <[email protected]> wrote:
> On Tue, Feb 19 2008, Jens Axboe wrote:
> > On Mon, Feb 18 2008, Andrew Morton wrote:
> > > > +
> > > > if (e && !try_module_get(e->elevator_owner))
> > > > e = NULL;
> > >
> > > Looks nice and simple. There might be some of the usual ordering problems
> > > when this is called during boot, maybe is-initramfs-available-yet problems,
> > > etc. But it's unlikely to make things regress from where they are now.
> >
> > Isn't request_module() and below robust enough to handle that?
>
> BTW, I've verified that it works as expected (at least after boot):
>
> carl:/sys/block/sda/queue # cat scheduler
> noop [cfq]
> carl:/sys/block/sda/queue # echo anticipatory > scheduler
> carl:/sys/block/sda/queue # dmesg
> [...]
> io scheduler anticipatory registered
> carl:/sys/block/sda/queue # cat scheduler
> noop cfq [anticipatory]
>
> So it properly loads as-iosched instead of failing, like it would have
> done before and required the user to do a modprobe as-iosched first.
>
> carl:/sys/block/sda/queue # echo foobar > scheduler
> -bash: echo: write error: Invalid argument
> carl:/sys/block/sda/queue # dmesg
> [...]
> elevator: type foobar not found
Looks promising - let's run with it. If there _are_ startup ordering
problems then we won't be any worse off than we are now.
otoh, the system must have _some_ io scheduler installed when talking to
disks, so perhaps there won't be any such problems at all.
On Tue, Feb 19 2008, Andrew Morton wrote:
> On Tue, 19 Feb 2008 10:24:28 +0100 Jens Axboe <[email protected]> wrote:
>
> > On Tue, Feb 19 2008, Jens Axboe wrote:
> > > On Mon, Feb 18 2008, Andrew Morton wrote:
> > > > > +
> > > > > if (e && !try_module_get(e->elevator_owner))
> > > > > e = NULL;
> > > >
> > > > Looks nice and simple. There might be some of the usual ordering problems
> > > > when this is called during boot, maybe is-initramfs-available-yet problems,
> > > > etc. But it's unlikely to make things regress from where they are now.
> > >
> > > Isn't request_module() and below robust enough to handle that?
> >
> > BTW, I've verified that it works as expected (at least after boot):
> >
> > carl:/sys/block/sda/queue # cat scheduler
> > noop [cfq]
> > carl:/sys/block/sda/queue # echo anticipatory > scheduler
> > carl:/sys/block/sda/queue # dmesg
> > [...]
> > io scheduler anticipatory registered
> > carl:/sys/block/sda/queue # cat scheduler
> > noop cfq [anticipatory]
> >
> > So it properly loads as-iosched instead of failing, like it would have
> > done before and required the user to do a modprobe as-iosched first.
> >
> > carl:/sys/block/sda/queue # echo foobar > scheduler
> > -bash: echo: write error: Invalid argument
> > carl:/sys/block/sda/queue # dmesg
> > [...]
> > elevator: type foobar not found
>
> Looks promising - let's run with it. If there _are_ startup ordering
> problems then we won't be any worse off than we are now.
I've merged it for inclusion, since it tests fine here.
> otoh, the system must have _some_ io scheduler installed when talking to
> disks, so perhaps there won't be any such problems at all.
We are guarenteed to always have noop available, since you cannot
de-select that.
--
Jens Axboe