2008-10-01 18:53:20

by Kiyoshi Ueda

[permalink] [raw]
Subject: [PATCH 1/1] scsi: export busy state via q->lld_busy_fn()

Hi James,

I hope the previous RFC patch(*) would be no problem, since I haven't
gotten any negative comment.
(*) http://lkml.org/lkml/2008/9/25/262

So could you take this patch for 2.6.28 additionally?
This patch implements a new interface of the block layer for
request stacking drivers.
There should be no effect on existing drivers' behavior.

This patch was created on top of the commit below of scsi-post-merge-2.6.
---------------------------------------------------------------------
commit e49f03f37104c0e7cae7c455480069bada00932f
Author: James Bottomley <[email protected]>
Date: Fri Sep 12 16:46:51 2008 -0500

[SCSI] scsi_error: fix target reset handling
---------------------------------------------------------------------

And this patch depends on the following block layer patch, which
is in Jens' for-2.6.28 of linux-2.6-block.
http://lkml.org/lkml/2008/9/29/142

Thanks,
Kiyoshi Ueda


Subject: [PATCH 1/1] scsi: export busy state via q->lld_busy_fn()

This patch implements q->lld_busy_fn() for scsi mid layer to export
its busy state.

Please note that it checks the cached information (sdev->lld_busy)
for efficiency, instead of checking the actual state of
sdev/starget/shost everytime.

So the care must be taken not to leave sdev->lld_busy = 1 for
the following cases:
- when there is no request in the sdev queue
- when scsi can't dispatch I/Os anymore and needs to kill I/Os
Otherwise, request stacking drivers may not submit any request,
and then, nobody sets back lld_busy = 0 and that causes deadlock.

OTOH, it has no major problem in setting sdev->lld_busy = 0 even when
the starget/shost is actually busy, because newly submitted request
will soon turn it to 1 in scsi_request_fn().


Signed-off-by: Kiyoshi Ueda <[email protected]>
Signed-off-by: Jun'ichi Nomura <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Mike Christie <[email protected]>
Cc: James Bottomley <[email protected]>
---
drivers/scsi/scsi.c | 4 ++--
drivers/scsi/scsi_lib.c | 20 +++++++++++++++++++-
include/scsi/scsi_device.h | 13 +++++++++++++
3 files changed, 34 insertions(+), 3 deletions(-)

Index: scsi-post-merge-2.6/drivers/scsi/scsi_lib.c
===================================================================
--- scsi-post-merge-2.6.orig/drivers/scsi/scsi_lib.c
+++ scsi-post-merge-2.6/drivers/scsi/scsi_lib.c
@@ -480,6 +480,8 @@ void scsi_device_unbusy(struct scsi_devi
spin_unlock(shost->host_lock);
spin_lock(sdev->request_queue->queue_lock);
sdev->device_busy--;
+ if (sdev->device_busy < sdev->queue_depth && !sdev->device_blocked)
+ sdev->lld_busy = 0;
spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
}

@@ -1535,6 +1537,13 @@ static void scsi_softirq_done(struct req
}
}

+static int scsi_lld_busy(struct request_queue *q)
+{
+ struct scsi_device *sdev = q->queuedata;
+
+ return sdev ? sdev->lld_busy : 0;
+}
+
/*
* Function: scsi_request_fn()
*
@@ -1577,9 +1586,14 @@ static void scsi_request_fn(struct reque
* accept it.
*/
req = elv_next_request(q);
- if (!req || !scsi_dev_queue_ready(q, sdev))
+ if (!req)
break;

+ if (!scsi_dev_queue_ready(q, sdev)) {
+ sdev->lld_busy = 1;
+ break;
+ }
+
if (unlikely(!scsi_device_online(sdev))) {
sdev_printk(KERN_ERR, sdev,
"rejecting I/O to offline device\n");
@@ -1649,6 +1663,8 @@ static void scsi_request_fn(struct reque
rtn = scsi_dispatch_cmd(cmd);
spin_lock_irq(q->queue_lock);
if(rtn) {
+ sdev->lld_busy = 1;
+
/* we're refusing the command; because of
* the way locks get dropped, we need to
* check here if plugging is required */
@@ -1673,6 +1689,7 @@ static void scsi_request_fn(struct reque
* later time.
*/
spin_lock_irq(q->queue_lock);
+ sdev->lld_busy = 1;
blk_requeue_request(q, req);
sdev->device_busy--;
if(sdev->device_busy == 0)
@@ -1756,6 +1773,7 @@ struct request_queue *scsi_alloc_queue(s
blk_queue_prep_rq(q, scsi_prep_fn);
blk_queue_softirq_done(q, scsi_softirq_done);
blk_queue_rq_timed_out(q, scsi_times_out);
+ blk_queue_lld_busy(q, scsi_lld_busy);
return q;
}

Index: scsi-post-merge-2.6/drivers/scsi/scsi.c
===================================================================
--- scsi-post-merge-2.6.orig/drivers/scsi/scsi.c
+++ scsi-post-merge-2.6/drivers/scsi/scsi.c
@@ -809,8 +809,6 @@ void scsi_finish_command(struct scsi_cmn
struct scsi_driver *drv;
unsigned int good_bytes;

- scsi_device_unbusy(sdev);
-
/*
* Clear the flags which say that the device/host is no longer
* capable of accepting new commands. These are set in scsi_queue.c
@@ -823,6 +821,8 @@ void scsi_finish_command(struct scsi_cmn
starget->target_blocked = 0;
sdev->device_blocked = 0;

+ scsi_device_unbusy(sdev);
+
/*
* If we have valid sense information, then some kind of recovery
* must have taken place. Make a note of this.
Index: scsi-post-merge-2.6/include/scsi/scsi_device.h
===================================================================
--- scsi-post-merge-2.6.orig/include/scsi/scsi_device.h
+++ scsi-post-merge-2.6/include/scsi/scsi_device.h
@@ -145,6 +145,19 @@ struct scsi_device {
unsigned retry_hwerror:1; /* Retry HARDWARE_ERROR */
unsigned last_sector_bug:1; /* do not use multisector accesses on
SD_LAST_BUGGY_SECTORS */
+ unsigned lld_busy:1; /* Exporting busy state for stacking
+ * drivers.
+ *
+ * 1 - There is one or more requests
+ * in the device's queue,
+ * and the device (or target/host)
+ * can not dispatch any request
+ * immediately.
+ * 0 - The device can dispatch
+ * (or kill) requests immediately,
+ * or there is no request in
+ * the device's queue even though
+ * target/host may busy. */

DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
struct list_head event_list; /* asserted events */


2008-10-03 18:15:40

by James Bottomley

[permalink] [raw]
Subject: Re: [PATCH 1/1] scsi: export busy state via q->lld_busy_fn()

On Wed, 2008-10-01 at 14:50 -0400, Kiyoshi Ueda wrote:
> Hi James,
>
> I hope the previous RFC patch(*) would be no problem, since I haven't
> gotten any negative comment.
> (*) http://lkml.org/lkml/2008/9/25/262
>
> So could you take this patch for 2.6.28 additionally?
> This patch implements a new interface of the block layer for
> request stacking drivers.
> There should be no effect on existing drivers' behavior.
>
> This patch was created on top of the commit below of scsi-post-merge-2.6.
> ---------------------------------------------------------------------
> commit e49f03f37104c0e7cae7c455480069bada00932f
> Author: James Bottomley <[email protected]>
> Date: Fri Sep 12 16:46:51 2008 -0500
>
> [SCSI] scsi_error: fix target reset handling
> ---------------------------------------------------------------------
>
> And this patch depends on the following block layer patch, which
> is in Jens' for-2.6.28 of linux-2.6-block.
> http://lkml.org/lkml/2008/9/29/142
>
> Thanks,
> Kiyoshi Ueda
>
>
> Subject: [PATCH 1/1] scsi: export busy state via q->lld_busy_fn()
>
> This patch implements q->lld_busy_fn() for scsi mid layer to export
> its busy state.
>
> Please note that it checks the cached information (sdev->lld_busy)
> for efficiency, instead of checking the actual state of
> sdev/starget/shost everytime.
>
> So the care must be taken not to leave sdev->lld_busy = 1 for
> the following cases:
> - when there is no request in the sdev queue
> - when scsi can't dispatch I/Os anymore and needs to kill I/Os
> Otherwise, request stacking drivers may not submit any request,
> and then, nobody sets back lld_busy = 0 and that causes deadlock.
>
> OTOH, it has no major problem in setting sdev->lld_busy = 0 even when
> the starget/shost is actually busy, because newly submitted request
> will soon turn it to 1 in scsi_request_fn().
>
>
> Signed-off-by: Kiyoshi Ueda <[email protected]>
> Signed-off-by: Jun'ichi Nomura <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Mike Christie <[email protected]>
> Cc: James Bottomley <[email protected]>
> ---
> drivers/scsi/scsi.c | 4 ++--
> drivers/scsi/scsi_lib.c | 20 +++++++++++++++++++-
> include/scsi/scsi_device.h | 13 +++++++++++++
> 3 files changed, 34 insertions(+), 3 deletions(-)
>
> Index: scsi-post-merge-2.6/drivers/scsi/scsi_lib.c
> ===================================================================
> --- scsi-post-merge-2.6.orig/drivers/scsi/scsi_lib.c
> +++ scsi-post-merge-2.6/drivers/scsi/scsi_lib.c
> @@ -480,6 +480,8 @@ void scsi_device_unbusy(struct scsi_devi
> spin_unlock(shost->host_lock);
> spin_lock(sdev->request_queue->queue_lock);
> sdev->device_busy--;
> + if (sdev->device_busy < sdev->queue_depth && !sdev->device_blocked)
> + sdev->lld_busy = 0;
> spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
> }
>
> @@ -1535,6 +1537,13 @@ static void scsi_softirq_done(struct req
> }
> }
>
> +static int scsi_lld_busy(struct request_queue *q)
> +{
> + struct scsi_device *sdev = q->queuedata;
> +
> + return sdev ? sdev->lld_busy : 0;
> +}

Since you've moved to a functional approach, why is this lld_busy flag
still necessary? Surely this function can just check the three blocked
conditions and the two overqueue ones at this point without ever having
to reach into any of the SCSI internals?

James

2008-10-03 20:09:29

by Kiyoshi Ueda

[permalink] [raw]
Subject: Re: [PATCH 1/1] scsi: export busy state via q->lld_busy_fn()

Hi James,

On Fri, 03 Oct 2008 13:14:56 -0500, James Bottomley wrote:
> On Wed, 2008-10-01 at 14:50 -0400, Kiyoshi Ueda wrote:
> > Hi James,
> >
> > I hope the previous RFC patch(*) would be no problem, since I haven't
> > gotten any negative comment.
> > (*) http://lkml.org/lkml/2008/9/25/262
> >
> > So could you take this patch for 2.6.28 additionally?
> > This patch implements a new interface of the block layer for
> > request stacking drivers.
> > There should be no effect on existing drivers' behavior.
> >
> > This patch was created on top of the commit below of scsi-post-merge-2.6.
> > ---------------------------------------------------------------------
> > commit e49f03f37104c0e7cae7c455480069bada00932f
> > Author: James Bottomley <[email protected]>
> > Date: Fri Sep 12 16:46:51 2008 -0500
> >
> > [SCSI] scsi_error: fix target reset handling
> > ---------------------------------------------------------------------
> >
> > And this patch depends on the following block layer patch, which
> > is in Jens' for-2.6.28 of linux-2.6-block.
> > http://lkml.org/lkml/2008/9/29/142
> >
> > Thanks,
> > Kiyoshi Ueda
> >
> >
> > Subject: [PATCH 1/1] scsi: export busy state via q->lld_busy_fn()
> >
> > This patch implements q->lld_busy_fn() for scsi mid layer to export
> > its busy state.
> >
> > Please note that it checks the cached information (sdev->lld_busy)
> > for efficiency, instead of checking the actual state of
> > sdev/starget/shost everytime.
> >
> > So the care must be taken not to leave sdev->lld_busy = 1 for
> > the following cases:
> > - when there is no request in the sdev queue
> > - when scsi can't dispatch I/Os anymore and needs to kill I/Os
> > Otherwise, request stacking drivers may not submit any request,
> > and then, nobody sets back lld_busy = 0 and that causes deadlock.
> >
> > OTOH, it has no major problem in setting sdev->lld_busy = 0 even when
> > the starget/shost is actually busy, because newly submitted request
> > will soon turn it to 1 in scsi_request_fn().
> >
> >
> > Signed-off-by: Kiyoshi Ueda <[email protected]>
> > Signed-off-by: Jun'ichi Nomura <[email protected]>
> > Cc: Andrew Morton <[email protected]>
> > Cc: Mike Christie <[email protected]>
> > Cc: James Bottomley <[email protected]>
> > ---
> > drivers/scsi/scsi.c | 4 ++--
> > drivers/scsi/scsi_lib.c | 20 +++++++++++++++++++-
> > include/scsi/scsi_device.h | 13 +++++++++++++
> > 3 files changed, 34 insertions(+), 3 deletions(-)
> >
> > Index: scsi-post-merge-2.6/drivers/scsi/scsi_lib.c
> > ===================================================================
> > --- scsi-post-merge-2.6.orig/drivers/scsi/scsi_lib.c
> > +++ scsi-post-merge-2.6/drivers/scsi/scsi_lib.c
> > @@ -480,6 +480,8 @@ void scsi_device_unbusy(struct scsi_devi
> > spin_unlock(shost->host_lock);
> > spin_lock(sdev->request_queue->queue_lock);
> > sdev->device_busy--;
> > + if (sdev->device_busy < sdev->queue_depth && !sdev->device_blocked)
> > + sdev->lld_busy = 0;
> > spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
> > }
> >
> > @@ -1535,6 +1537,13 @@ static void scsi_softirq_done(struct req
> > }
> > }
> >
> > +static int scsi_lld_busy(struct request_queue *q)
> > +{
> > + struct scsi_device *sdev = q->queuedata;
> > +
> > + return sdev ? sdev->lld_busy : 0;
> > +}
>
> Since you've moved to a functional approach, why is this lld_busy flag
> still necessary? Surely this function can just check the three blocked
> conditions and the two overqueue ones at this point without ever having
> to reach into any of the SCSI internals?

This flag is not necessary for the functionality, it's for efficiency.
I could take the "everytime checking" approach above, but I think
caching the busy state into the flag is more efficient, since:

- The check function will be called from request stacking drivers
frequently (e.g. request-based dm calls it everytime before
an request is dispatched from the dm device.)

- The scsi busy state checking needs queue lock and host lock
even while the scsi busy state doesn't changed from the previous
checking.

Actually, I don't get any performance problem by some simple testings
of the "everytime checking" approach.
Do you prefer the "everytime checking" approach to simplify the patch?

Thanks,
Kiyoshi Ueda

2008-10-03 21:52:31

by James Bottomley

[permalink] [raw]
Subject: Re: [dm-devel] Re: [PATCH 1/1] scsi: export busy state via q->lld_busy_fn()

On Fri, 2008-10-03 at 16:06 -0400, Kiyoshi Ueda wrote:
> Hi James,
>
> On Fri, 03 Oct 2008 13:14:56 -0500, James Bottomley wrote:
> > On Wed, 2008-10-01 at 14:50 -0400, Kiyoshi Ueda wrote:
> > > Hi James,
> > >
> > > I hope the previous RFC patch(*) would be no problem, since I haven't
> > > gotten any negative comment.
> > > (*) http://lkml.org/lkml/2008/9/25/262
> > >
> > > So could you take this patch for 2.6.28 additionally?
> > > This patch implements a new interface of the block layer for
> > > request stacking drivers.
> > > There should be no effect on existing drivers' behavior.
> > >
> > > This patch was created on top of the commit below of scsi-post-merge-2.6.
> > > ---------------------------------------------------------------------
> > > commit e49f03f37104c0e7cae7c455480069bada00932f
> > > Author: James Bottomley <[email protected]>
> > > Date: Fri Sep 12 16:46:51 2008 -0500
> > >
> > > [SCSI] scsi_error: fix target reset handling
> > > ---------------------------------------------------------------------
> > >
> > > And this patch depends on the following block layer patch, which
> > > is in Jens' for-2.6.28 of linux-2.6-block.
> > > http://lkml.org/lkml/2008/9/29/142
> > >
> > > Thanks,
> > > Kiyoshi Ueda
> > >
> > >
> > > Subject: [PATCH 1/1] scsi: export busy state via q->lld_busy_fn()
> > >
> > > This patch implements q->lld_busy_fn() for scsi mid layer to export
> > > its busy state.
> > >
> > > Please note that it checks the cached information (sdev->lld_busy)
> > > for efficiency, instead of checking the actual state of
> > > sdev/starget/shost everytime.
> > >
> > > So the care must be taken not to leave sdev->lld_busy = 1 for
> > > the following cases:
> > > - when there is no request in the sdev queue
> > > - when scsi can't dispatch I/Os anymore and needs to kill I/Os
> > > Otherwise, request stacking drivers may not submit any request,
> > > and then, nobody sets back lld_busy = 0 and that causes deadlock.
> > >
> > > OTOH, it has no major problem in setting sdev->lld_busy = 0 even when
> > > the starget/shost is actually busy, because newly submitted request
> > > will soon turn it to 1 in scsi_request_fn().
> > >
> > >
> > > Signed-off-by: Kiyoshi Ueda <[email protected]>
> > > Signed-off-by: Jun'ichi Nomura <[email protected]>
> > > Cc: Andrew Morton <[email protected]>
> > > Cc: Mike Christie <[email protected]>
> > > Cc: James Bottomley <[email protected]>
> > > ---
> > > drivers/scsi/scsi.c | 4 ++--
> > > drivers/scsi/scsi_lib.c | 20 +++++++++++++++++++-
> > > include/scsi/scsi_device.h | 13 +++++++++++++
> > > 3 files changed, 34 insertions(+), 3 deletions(-)
> > >
> > > Index: scsi-post-merge-2.6/drivers/scsi/scsi_lib.c
> > > ===================================================================
> > > --- scsi-post-merge-2.6.orig/drivers/scsi/scsi_lib.c
> > > +++ scsi-post-merge-2.6/drivers/scsi/scsi_lib.c
> > > @@ -480,6 +480,8 @@ void scsi_device_unbusy(struct scsi_devi
> > > spin_unlock(shost->host_lock);
> > > spin_lock(sdev->request_queue->queue_lock);
> > > sdev->device_busy--;
> > > + if (sdev->device_busy < sdev->queue_depth && !sdev->device_blocked)
> > > + sdev->lld_busy = 0;
> > > spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
> > > }
> > >
> > > @@ -1535,6 +1537,13 @@ static void scsi_softirq_done(struct req
> > > }
> > > }
> > >
> > > +static int scsi_lld_busy(struct request_queue *q)
> > > +{
> > > + struct scsi_device *sdev = q->queuedata;
> > > +
> > > + return sdev ? sdev->lld_busy : 0;
> > > +}
> >
> > Since you've moved to a functional approach, why is this lld_busy flag
> > still necessary? Surely this function can just check the three blocked
> > conditions and the two overqueue ones at this point without ever having
> > to reach into any of the SCSI internals?
>
> This flag is not necessary for the functionality, it's for efficiency.
> I could take the "everytime checking" approach above, but I think
> caching the busy state into the flag is more efficient, since:
>
> - The check function will be called from request stacking drivers
> frequently (e.g. request-based dm calls it everytime before
> an request is dispatched from the dm device.)

Hmm, well, so will the operations you've put into the stack, but for
every request, not just for stacked drivers.

> - The scsi busy state checking needs queue lock and host lock
> even while the scsi busy state doesn't changed from the previous
> checking.

You don't need the locks ... you don't care if the values change as you
read them because this is just heuristic. You do care that you get an
architectural guarantee that you read a consistent value (as in if the
variable is altering you read either the before or after) but this isn't
necessary as the algorithm is statistical: as long as it gets bogus
values infrequently the effects won't be noticeable different.

> Actually, I don't get any performance problem by some simple testings
> of the "everytime checking" approach.
> Do you prefer the "everytime checking" approach to simplify the patch?

I think so, thanks.

James

2008-10-04 18:16:42

by Kiyoshi Ueda

[permalink] [raw]
Subject: Re: [dm-devel] Re: [PATCH 1/1] scsi: export busy state via q->lld_busy_fn()

Hi James,

On Fri, 03 Oct 2008 16:51:46 -0500, James Bottomley wrote:
> On Fri, 2008-10-03 at 16:06 -0400, Kiyoshi Ueda wrote:
> > Hi James,
> >
> > On Fri, 03 Oct 2008 13:14:56 -0500, James Bottomley wrote:
> > > On Wed, 2008-10-01 at 14:50 -0400, Kiyoshi Ueda wrote:
> > > > Hi James,
> > > >
> > > > I hope the previous RFC patch(*) would be no problem, since I haven't
> > > > gotten any negative comment.
> > > > (*) http://lkml.org/lkml/2008/9/25/262
> > > >
> > > > So could you take this patch for 2.6.28 additionally?
> > > > This patch implements a new interface of the block layer for
> > > > request stacking drivers.
> > > > There should be no effect on existing drivers' behavior.
> > > >
> > > > This patch was created on top of the commit below of scsi-post-merge-2.6.
> > > > ---------------------------------------------------------------------
> > > > commit e49f03f37104c0e7cae7c455480069bada00932f
> > > > Author: James Bottomley <[email protected]>
> > > > Date: Fri Sep 12 16:46:51 2008 -0500
> > > >
> > > > [SCSI] scsi_error: fix target reset handling
> > > > ---------------------------------------------------------------------
> > > >
> > > > And this patch depends on the following block layer patch, which
> > > > is in Jens' for-2.6.28 of linux-2.6-block.
> > > > http://lkml.org/lkml/2008/9/29/142
> > > >
> > > > Thanks,
> > > > Kiyoshi Ueda
> > > >
> > > >
> > > > Subject: [PATCH 1/1] scsi: export busy state via q->lld_busy_fn()
> > > >
> > > > This patch implements q->lld_busy_fn() for scsi mid layer to export
> > > > its busy state.
> > > >
> > > > Please note that it checks the cached information (sdev->lld_busy)
> > > > for efficiency, instead of checking the actual state of
> > > > sdev/starget/shost everytime.
> > > >
> > > > So the care must be taken not to leave sdev->lld_busy = 1 for
> > > > the following cases:
> > > > - when there is no request in the sdev queue
> > > > - when scsi can't dispatch I/Os anymore and needs to kill I/Os
> > > > Otherwise, request stacking drivers may not submit any request,
> > > > and then, nobody sets back lld_busy = 0 and that causes deadlock.
> > > >
> > > > OTOH, it has no major problem in setting sdev->lld_busy = 0 even when
> > > > the starget/shost is actually busy, because newly submitted request
> > > > will soon turn it to 1 in scsi_request_fn().
> > > >
> > > >
> > > > Signed-off-by: Kiyoshi Ueda <[email protected]>
> > > > Signed-off-by: Jun'ichi Nomura <[email protected]>
> > > > Cc: Andrew Morton <[email protected]>
> > > > Cc: Mike Christie <[email protected]>
> > > > Cc: James Bottomley <[email protected]>
> > > > ---
> > > > drivers/scsi/scsi.c | 4 ++--
> > > > drivers/scsi/scsi_lib.c | 20 +++++++++++++++++++-
> > > > include/scsi/scsi_device.h | 13 +++++++++++++
> > > > 3 files changed, 34 insertions(+), 3 deletions(-)
> > > >
> > > > Index: scsi-post-merge-2.6/drivers/scsi/scsi_lib.c
> > > > ===================================================================
> > > > --- scsi-post-merge-2.6.orig/drivers/scsi/scsi_lib.c
> > > > +++ scsi-post-merge-2.6/drivers/scsi/scsi_lib.c
> > > > @@ -480,6 +480,8 @@ void scsi_device_unbusy(struct scsi_devi
> > > > spin_unlock(shost->host_lock);
> > > > spin_lock(sdev->request_queue->queue_lock);
> > > > sdev->device_busy--;
> > > > + if (sdev->device_busy < sdev->queue_depth && !sdev->device_blocked)
> > > > + sdev->lld_busy = 0;
> > > > spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
> > > > }
> > > >
> > > > @@ -1535,6 +1537,13 @@ static void scsi_softirq_done(struct req
> > > > }
> > > > }
> > > >
> > > > +static int scsi_lld_busy(struct request_queue *q)
> > > > +{
> > > > + struct scsi_device *sdev = q->queuedata;
> > > > +
> > > > + return sdev ? sdev->lld_busy : 0;
> > > > +}
> > >
> > > Since you've moved to a functional approach, why is this lld_busy flag
> > > still necessary? Surely this function can just check the three blocked
> > > conditions and the two overqueue ones at this point without ever having
> > > to reach into any of the SCSI internals?
> >
> > This flag is not necessary for the functionality, it's for efficiency.
> > I could take the "everytime checking" approach above, but I think
> > caching the busy state into the flag is more efficient, since:
> >
> > - The check function will be called from request stacking drivers
> > frequently (e.g. request-based dm calls it everytime before
> > an request is dispatched from the dm device.)
>
> Hmm, well, so will the operations you've put into the stack, but for
> every request, not just for stacked drivers.
>
> > - The scsi busy state checking needs queue lock and host lock
> > even while the scsi busy state doesn't changed from the previous
> > checking.
>
> You don't need the locks ... you don't care if the values change as you
> read them because this is just heuristic. You do care that you get an
> architectural guarantee that you read a consistent value (as in if the
> variable is altering you read either the before or after) but this isn't
> necessary as the algorithm is statistical: as long as it gets bogus
> values infrequently the effects won't be noticeable different.

Thank you for the good suggestion.
I sent another patch-set: http://lkml.org/lkml/2008/10/4/85

Thanks,
Kiyoshi Ueda