This patch adds an interface to check lld's busy status
from the block layer.
This resolves a performance problem on request stacking devices below.


Some drivers like scsi mid layer stop dispatching request when
they detect busy state on its low-level device like host/bus/device.
It allows other requests to stay in the I/O scheduler's queue
for a chance of merging.

Request stacking drivers like request-based dm should follow
the same logic.
However, there is no generic interface for the stacked device
to check if the underlying device(s) are busy.
If the request stacking driver dispatches and submits requests to
the busy underlying device, the requests will stay in
the underlying device's queue without a chance of merging.
This causes performance problem on burst I/O load.

With this patch, busy state of the underlying device is exported
via the state flag of queue's backing_dev_info. So the request
stacking driver can check it and stop dispatching requests if busy.

The underlying device driver must set/clear the flag appropriately:
ON: when the device driver can't process requests immediately.
OFF: when the device driver can process requests immediately,
including abnormal situations where the device driver needs
to kill all requests.


Signed-off-by: Kiyoshi Ueda <k-ueda@ct.jp.nec.com>
Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
---
include/linux/backing-dev.h | 8 ++++++++
mm/backing-dev.c | 12 ++++++++++++
2 files changed, 20 insertions(+)

Index: scsi-misc-2.6/include/linux/backing-dev.h
================================================== =================
--- scsi-misc-2.6.orig/include/linux/backing-dev.h
+++ scsi-misc-2.6/include/linux/backing-dev.h
@@ -26,6 +26,7 @@ enum bdi_state {
BDI_pdflush, /* A pdflush thread is working this device */
BDI_write_congested, /* The write queue is getting full */
BDI_read_congested, /* The read queue is getting full */
+ BDI_lld_congested, /* The device/host is busy */
BDI_unused, /* Available bits start here */
};

@@ -226,8 +227,15 @@ static inline int bdi_rw_congested(struc
(1 << BDI_write_congested));
}

+static inline int bdi_lld_congested(struct backing_dev_info *bdi)
+{
+ return bdi_congested(bdi, 1 << BDI_lld_congested);
+}
+
void clear_bdi_congested(struct backing_dev_info *bdi, int rw);
void set_bdi_congested(struct backing_dev_info *bdi, int rw);
+void clear_bdi_lld_congested(struct backing_dev_info *bdi);
+void set_bdi_lld_congested(struct backing_dev_info *bdi);
long congestion_wait(int rw, long timeout);


Index: scsi-misc-2.6/mm/backing-dev.c
================================================== =================
--- scsi-misc-2.6.orig/mm/backing-dev.c
+++ scsi-misc-2.6/mm/backing-dev.c
@@ -279,6 +279,18 @@ void set_bdi_congested(struct backing_de
}
EXPORT_SYMBOL(set_bdi_congested);

+void clear_bdi_lld_congested(struct backing_dev_info *bdi)
+{
+ clear_bit(BDI_lld_congested, &bdi->state);
+}
+EXPORT_SYMBOL_GPL(clear_bdi_lld_congested);
+
+void set_bdi_lld_congested(struct backing_dev_info *bdi)
+{
+ set_bit(BDI_lld_congested, &bdi->state);
+}
+EXPORT_SYMBOL_GPL(set_bdi_lld_congested);
+
/**
* congestion_wait - wait for a backing_dev to become uncongested
* @rw: READ or WRITE

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

On Fri, 19 Sep 2008 10:48:54 -0400 (EDT)
Kiyoshi Ueda <k-ueda@ct.jp.nec.com> wrote:

> This patch adds an interface to check lld's busy status
> from the block layer.
> This resolves a performance problem on request stacking devices below.
>
>
> Some drivers like scsi mid layer stop dispatching request when
> they detect busy state on its low-level device like host/bus/device.
> It allows other requests to stay in the I/O scheduler's queue
> for a chance of merging.
>
> Request stacking drivers like request-based dm should follow
> the same logic.
> However, there is no generic interface for the stacked device
> to check if the underlying device(s) are busy.
> If the request stacking driver dispatches and submits requests to
> the busy underlying device, the requests will stay in
> the underlying device's queue without a chance of merging.
> This causes performance problem on burst I/O load.
>
> With this patch, busy state of the underlying device is exported
> via the state flag of queue's backing_dev_info. So the request
> stacking driver can check it and stop dispatching requests if busy.
>
> The underlying device driver must set/clear the flag appropriately:
> ON: when the device driver can't process requests immediately.
> OFF: when the device driver can process requests immediately,
> including abnormal situations where the device driver needs
> to kill all requests.
>
>
> Signed-off-by: Kiyoshi Ueda <k-ueda@ct.jp.nec.com>
> Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
> include/linux/backing-dev.h | 8 ++++++++
> mm/backing-dev.c | 12 ++++++++++++
> 2 files changed, 20 insertions(+)
>
> Index: scsi-misc-2.6/include/linux/backing-dev.h
> ================================================== =================
> --- scsi-misc-2.6.orig/include/linux/backing-dev.h
> +++ scsi-misc-2.6/include/linux/backing-dev.h
> @@ -26,6 +26,7 @@ enum bdi_state {
> BDI_pdflush, /* A pdflush thread is working this device */
> BDI_write_congested, /* The write queue is getting full */
> BDI_read_congested, /* The read queue is getting full */
> + BDI_lld_congested, /* The device/host is busy */
> BDI_unused, /* Available bits start here */
> };
>
> @@ -226,8 +227,15 @@ static inline int bdi_rw_congested(struc
> (1 << BDI_write_congested));
> }
>
> +static inline int bdi_lld_congested(struct backing_dev_info *bdi)
> +{
> + return bdi_congested(bdi, 1 << BDI_lld_congested);
> +}
> +
> void clear_bdi_congested(struct backing_dev_info *bdi, int rw);
> void set_bdi_congested(struct backing_dev_info *bdi, int rw);
> +void clear_bdi_lld_congested(struct backing_dev_info *bdi);
> +void set_bdi_lld_congested(struct backing_dev_info *bdi);
> long congestion_wait(int rw, long timeout);
>
>
> Index: scsi-misc-2.6/mm/backing-dev.c
> ================================================== =================
> --- scsi-misc-2.6.orig/mm/backing-dev.c
> +++ scsi-misc-2.6/mm/backing-dev.c
> @@ -279,6 +279,18 @@ void set_bdi_congested(struct backing_de
> }
> EXPORT_SYMBOL(set_bdi_congested);
>
> +void clear_bdi_lld_congested(struct backing_dev_info *bdi)
> +{
> + clear_bit(BDI_lld_congested, &bdi->state);
> +}
> +EXPORT_SYMBOL_GPL(clear_bdi_lld_congested);
> +
> +void set_bdi_lld_congested(struct backing_dev_info *bdi)
> +{
> + set_bit(BDI_lld_congested, &bdi->state);
> +}
> +EXPORT_SYMBOL_GPL(set_bdi_lld_congested);
> +
> /**
> * congestion_wait - wait for a backing_dev to become uncongested
> * @rw: READ or WRITE

Is this really the right way to do it?

Back in the days when we first did the backing_dev_info.congested_fn()
logic it was decided that there basically was no single place in which
the congested state could be stored.

So we ended up deciding that whenever a caller wants to know a
backing_dev's congested status, it has to call in to the
->congested_fn() and that congested_fn would then call down into all
the constituent low-level drivers/queues/etc asking each one if it is
congested.

I mean, as a simple example which is probably wrong - what happens if a
single backing_dev is implemented via two different disks and
controllers, and they both become congested and then one of them comes
uncongested. Is there no way in which the above implemention can
incorrectly flag the backing_dev as being uncongested?

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Hi Andrew,

On Fri, 19 Sep 2008 14:33:44 -0700, Andrew Morton wrote:
> On Fri, 19 Sep 2008 10:48:54 -0400 (EDT)
> Kiyoshi Ueda <k-ueda@ct.jp.nec.com> wrote:
>
> > This patch adds an interface to check lld's busy status
> > from the block layer.
> > This resolves a performance problem on request stacking devices below.
> >
> >
> > Some drivers like scsi mid layer stop dispatching request when
> > they detect busy state on its low-level device like host/bus/device.
> > It allows other requests to stay in the I/O scheduler's queue
> > for a chance of merging.
> >
> > Request stacking drivers like request-based dm should follow
> > the same logic.
> > However, there is no generic interface for the stacked device
> > to check if the underlying device(s) are busy.
> > If the request stacking driver dispatches and submits requests to
> > the busy underlying device, the requests will stay in
> > the underlying device's queue without a chance of merging.
> > This causes performance problem on burst I/O load.
> >
> > With this patch, busy state of the underlying device is exported
> > via the state flag of queue's backing_dev_info. So the request
> > stacking driver can check it and stop dispatching requests if busy.
> >
> > The underlying device driver must set/clear the flag appropriately:
> > ON: when the device driver can't process requests immediately.
> > OFF: when the device driver can process requests immediately,
> > including abnormal situations where the device driver needs
> > to kill all requests.
> >
> >
> > Signed-off-by: Kiyoshi Ueda <k-ueda@ct.jp.nec.com>
> > Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
> > Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> > ---
> > include/linux/backing-dev.h | 8 ++++++++
> > mm/backing-dev.c | 12 ++++++++++++
> > 2 files changed, 20 insertions(+)
> >
> > Index: scsi-misc-2.6/include/linux/backing-dev.h
> > ================================================== =================
> > --- scsi-misc-2.6.orig/include/linux/backing-dev.h
> > +++ scsi-misc-2.6/include/linux/backing-dev.h
> > @@ -26,6 +26,7 @@ enum bdi_state {
> > BDI_pdflush, /* A pdflush thread is working this device */
> > BDI_write_congested, /* The write queue is getting full */
> > BDI_read_congested, /* The read queue is getting full */
> > + BDI_lld_congested, /* The device/host is busy */
> > BDI_unused, /* Available bits start here */
> > };
> >
> > @@ -226,8 +227,15 @@ static inline int bdi_rw_congested(struc
> > (1 << BDI_write_congested));
> > }
> >
> > +static inline int bdi_lld_congested(struct backing_dev_info *bdi)
> > +{
> > + return bdi_congested(bdi, 1 << BDI_lld_congested);
> > +}
> > +
> > void clear_bdi_congested(struct backing_dev_info *bdi, int rw);
> > void set_bdi_congested(struct backing_dev_info *bdi, int rw);
> > +void clear_bdi_lld_congested(struct backing_dev_info *bdi);
> > +void set_bdi_lld_congested(struct backing_dev_info *bdi);
> > long congestion_wait(int rw, long timeout);
> >
> >
> > Index: scsi-misc-2.6/mm/backing-dev.c
> > ================================================== =================
> > --- scsi-misc-2.6.orig/mm/backing-dev.c
> > +++ scsi-misc-2.6/mm/backing-dev.c
> > @@ -279,6 +279,18 @@ void set_bdi_congested(struct backing_de
> > }
> > EXPORT_SYMBOL(set_bdi_congested);
> >
> > +void clear_bdi_lld_congested(struct backing_dev_info *bdi)
> > +{
> > + clear_bit(BDI_lld_congested, &bdi->state);
> > +}
> > +EXPORT_SYMBOL_GPL(clear_bdi_lld_congested);
> > +
> > +void set_bdi_lld_congested(struct backing_dev_info *bdi)
> > +{
> > + set_bit(BDI_lld_congested, &bdi->state);
> > +}
> > +EXPORT_SYMBOL_GPL(set_bdi_lld_congested);
> > +
> > /**
> > * congestion_wait - wait for a backing_dev to become uncongested
> > * @rw: READ or WRITE
>
> Is this really the right way to do it?

I think so, but I may not understand what you mean correctly.
So please elaborate your concern if my explanation below doesn't
satisfy what you want to know.


> Back in the days when we first did the backing_dev_info.congested_fn()
> logic it was decided that there basically was no single place in which
> the congested state could be stored.
>
> So we ended up deciding that whenever a caller wants to know a
> backing_dev's congested status, it has to call in to the
> ->congested_fn() and that congested_fn would then call down into all
> the constituent low-level drivers/queues/etc asking each one if it is
> congested.

bdi_lld_congested() also does that using bdi_congested(), which calls
->congested_fn().
And only real device drivers (e.g. scsi, ide) set/clear the flag.
Stacking drivers like request-based dm don't.
So stacking drivers always check the BDI_lld_congested flag of
the bottom device of the device stack.

BDI_[write|read]_congested flags have been using for queue's
congestion, so that I/O queueing/merging can be proceeded even if
the lld is congested. So I added a new flag.


> I mean, as a simple example which is probably wrong - what happens if a
> single backing_dev is implemented via two different disks and
> controllers, and they both become congested and then one of them comes
> uncongested. Is there no way in which the above implemention can
> incorrectly flag the backing_dev as being uncongested?

Do you mean that "a single backing_dev via two disks/controllers" is
a dm device (e.g. a dm-multipath device having 2 paths, a dm-mirror
device having 2 disks)?

If so, dm doesn't set/clear the flag, and the decision, whether
the dm device itself is congested or not, depends on dm's target driver.

For example of dm-multipath,
o call bdi_lld_congested() for each path.
o if one of the paths are uncongested, dm-multipath will decide
the dm device is uncongested and dispatch incoming I/Os to
the uncongested path.

For example of dm-mirror,
o call bdi_lld_congested() for each disk.
o if the incoming I/O is READ, same logic as dm-multipath above.
if the incoming I/O is WRITE, dm-mirror will decide the dm device
is uncongested only when all disks are uncongested.

Thanks,
Kiyoshi Ueda

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

On Fri, 19 Sep 2008 19:11:22 -0400 (EDT)
Kiyoshi Ueda <k-ueda@ct.jp.nec.com> wrote:

> > Back in the days when we first did the backing_dev_info.congested_fn()
> > logic it was decided that there basically was no single place in which
> > the congested state could be stored.
> >
> > So we ended up deciding that whenever a caller wants to know a
> > backing_dev's congested status, it has to call in to the
> > ->congested_fn() and that congested_fn would then call down into all
> > the constituent low-level drivers/queues/etc asking each one if it is
> > congested.
>
> bdi_lld_congested() also does that using bdi_congested(), which calls
> ->congested_fn().
> And only real device drivers (e.g. scsi, ide) set/clear the flag.
> Stacking drivers like request-based dm don't.

umm, OK, that should work.

> So stacking drivers always check the BDI_lld_congested flag of
> the bottom device of the device stack.

How does a stacking driver know that the backing_device which it is
looking at is a "lowest level" device?

I don't think it does - only the code which implements that device
knows this, so the stacking driver has to call into that device's
congested_fn(), yes?

In which case one wonders why the state was stored in the
backing_dev_info at all. Why not store it in the device-private data
to avoid confusion and abuse?

> BDI_[write|read]_congested flags have been using for queue's
> congestion, so that I/O queueing/merging can be proceeded even if
> the lld is congested. So I added a new flag.

iirc, BDI_read/write_congested predated the introduction of the
congested_fn() and perhaps should have been removed once we went to the
congested_fn approach. But it's been a while since I spent a lot of
time looking in there.

>
> > I mean, as a simple example which is probably wrong - what happens if a
> > single backing_dev is implemented via two different disks and
> > controllers, and they both become congested and then one of them comes
> > uncongested. Is there no way in which the above implemention can
> > incorrectly flag the backing_dev as being uncongested?
>
> Do you mean that "a single backing_dev via two disks/controllers" is
> a dm device (e.g. a dm-multipath device having 2 paths, a dm-mirror
> device having 2 disks)?

Something along those lines, sure.

> If so, dm doesn't set/clear the flag, and the decision, whether
> the dm device itself is congested or not, depends on dm's target driver.
>
> For example of dm-multipath,
> o call bdi_lld_congested() for each path.
> o if one of the paths are uncongested, dm-multipath will decide
> the dm device is uncongested and dispatch incoming I/Os to
> the uncongested path.

hm, OK.

> For example of dm-mirror,
> o call bdi_lld_congested() for each disk.
> o if the incoming I/O is READ, same logic as dm-multipath above.
> if the incoming I/O is WRITE, dm-mirror will decide the dm device
> is uncongested only when all disks are uncongested.
>
> Thanks,
> Kiyoshi Ueda

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Hi Andrew,
(and James, Jens, please let us know your opinions on the possible
changes described below)

Andrew Morton wrote:
> On Fri, 19 Sep 2008 19:11:22 -0400 (EDT)
> Kiyoshi Ueda <k-ueda@ct.jp.nec.com> wrote:
>
>>> Back in the days when we first did the backing_dev_info.congested_fn()
>>> logic it was decided that there basically was no single place in which
>>> the congested state could be stored.
>>>
>>> So we ended up deciding that whenever a caller wants to know a
>>> backing_dev's congested status, it has to call in to the
>>> ->congested_fn() and that congested_fn would then call down into all
>>> the constituent low-level drivers/queues/etc asking each one if it is
>>> congested.
>> bdi_lld_congested() also does that using bdi_congested(), which calls
>> ->congested_fn().
>> And only real device drivers (e.g. scsi, ide) set/clear the flag.
>> Stacking drivers like request-based dm don't.
>
> umm, OK, that should work.
>
>> So stacking drivers always check the BDI_lld_congested flag of
>> the bottom device of the device stack.
>
> How does a stacking driver know that the backing_device which it is
> looking at is a "lowest level" device?
>
> I don't think it does - only the code which implements that device
> knows this, so the stacking driver has to call into that device's
> congested_fn(), yes?

Yes. So the stacking driver calls bdi_congested, which calls
the underlying device's congested_fn if exists, and eventually
checks the bottom device's congestion state.
Translation of multiple devices' congestion status is done by
the congested_fn of the stacking device.
E.g. dm-multipath returns 'not congested' if any of its paths are not
congested.

> In which case one wonders why the state was stored in the
> backing_dev_info at all. Why not store it in the device-private data
> to avoid confusion and abuse?

It should be possible.
We've just followed the existing scheme of BDI_{read,write}congested
because of their similarity.

I would like to know which part of the patch was your concern:
1) Exposing set/clear_bdi_lld_congested without explicit comments
that says they should be used only by bottom-level devices
2) A new bdi_state, BDI_lld_congested
3) Use of backing_dev_info for this purpose

If 1), either or both of the followings can be easily done:
[a] Add a comment in backing-dev.h that says set/clear_bdi_lld_congested
should be called only from the bottom device
[b] Move set/clear_bdi_lld_congested from mm/backing-dev.c
to block/blk-core.c, with renaming to blk_set/clear_lld_congested,
so that only a block device that knows what it does will set/clear
the flag

If 2) or 3), I think we need to rewrite the patch in either way of these:
[c] Add a new strategy function to request_queue and use it instead,
e.g. q->lld_busy_fn, which is NULL by default.
Set/clear QUEUE_FLAG_BUSY in request_queue by the bottom device,
and the block layer will check the flag if q->lld_busy_fn is NULL.
[d] Similar to [c], except that storing the busy flag in struct scsi_device
and q->lld_busy_fn() of the scsi device will check that.
If q->lld_busy_fn is NULL, the block layer will just return
'not congested'.

Which do you think is better?

>> BDI_[write|read]_congested flags have been using for queue's
>> congestion, so that I/O queueing/merging can be proceeded even if
>> the lld is congested. So I added a new flag.
>
> iirc, BDI_read/write_congested predated the introduction of the
> congested_fn() and perhaps should have been removed once we went to the
> congested_fn approach. But it's been a while since I spent a lot of
> time looking in there.

Thanks,
--
Jun'ichi Nomura, NEC Corporation

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel