[RFC][PATCH] allow sleep inside EH hooks

[RFC][PATCH] allow sleep inside EH hooks

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 251 bytes --]


SCSI EH processing already serializes things during EH, so this spinlock 
isn't really needed.

Removing the spinlock outright would break drivers that surround logic 
with spin_unlock_irq()..spin_lock_irq(), so I introduced ->unlocked_eh 
option.



[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 4114 bytes --]

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -266,6 +266,7 @@ struct Scsi_Host *scsi_host_alloc(struct
 	shost->use_clustering = sht->use_clustering;
 	shost->ordered_flush = sht->ordered_flush;
 	shost->ordered_tag = sht->ordered_tag;
+	shost->unlocked_eh = sht->unlocked_eh;
 
 	/*
 	 * hosts/devices that do queueing must support ordered tags
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -528,10 +528,12 @@ static int scsi_send_eh_cmnd(struct scsi
 		 * abort a timed out command or not.  not sure how
 		 * we should treat them differently anyways.
 		 */
-		spin_lock_irqsave(shost->host_lock, flags);
+		if (!shost->unlocked_eh)
+			spin_lock_irqsave(shost->host_lock, flags);
 		if (shost->hostt->eh_abort_handler)
 			shost->hostt->eh_abort_handler(scmd);
-		spin_unlock_irqrestore(shost->host_lock, flags);
+		if (!shost->unlocked_eh)
+			spin_unlock_irqrestore(shost->host_lock, flags);
 			
 		scmd->request->rq_status = RQ_SCSI_DONE;
 		scmd->owner = SCSI_OWNER_ERROR_HANDLER;
@@ -752,9 +754,11 @@ static int scsi_try_to_abort_cmd(struct 
 
 	scmd->owner = SCSI_OWNER_LOWLEVEL;
 
-	spin_lock_irqsave(scmd->device->host->host_lock, flags);
+	if (!scmd->device->host->unlocked_eh)
+		spin_lock_irqsave(scmd->device->host->host_lock, flags);
 	rtn = scmd->device->host->hostt->eh_abort_handler(scmd);
-	spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
+	if (!scmd->device->host->unlocked_eh)
+		spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
 
 	return rtn;
 }
@@ -873,9 +877,11 @@ static int scsi_try_bus_device_reset(str
 
 	scmd->owner = SCSI_OWNER_LOWLEVEL;
 
-	spin_lock_irqsave(scmd->device->host->host_lock, flags);
+	if (!scmd->device->host->unlocked_eh)
+		spin_lock_irqsave(scmd->device->host->host_lock, flags);
 	rtn = scmd->device->host->hostt->eh_device_reset_handler(scmd);
-	spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
+	if (!scmd->device->host->unlocked_eh)
+		spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
 
 	if (rtn == SUCCESS) {
 		scmd->device->was_reset = 1;
@@ -1061,9 +1067,11 @@ static int scsi_try_bus_reset(struct scs
 	if (!scmd->device->host->hostt->eh_bus_reset_handler)
 		return FAILED;
 
-	spin_lock_irqsave(scmd->device->host->host_lock, flags);
+	if (!scmd->device->host->unlocked_eh)
+		spin_lock_irqsave(scmd->device->host->host_lock, flags);
 	rtn = scmd->device->host->hostt->eh_bus_reset_handler(scmd);
-	spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
+	if (!scmd->device->host->unlocked_eh)
+		spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
 
 	if (rtn == SUCCESS) {
 		if (!scmd->device->host->hostt->skip_settle_delay)
@@ -1092,9 +1100,11 @@ static int scsi_try_host_reset(struct sc
 	if (!scmd->device->host->hostt->eh_host_reset_handler)
 		return FAILED;
 
-	spin_lock_irqsave(scmd->device->host->host_lock, flags);
+	if (!scmd->device->host->unlocked_eh)
+		spin_lock_irqsave(scmd->device->host->host_lock, flags);
 	rtn = scmd->device->host->hostt->eh_host_reset_handler(scmd);
-	spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
+	if (!scmd->device->host->unlocked_eh)
+		spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
 
 	if (rtn == SUCCESS) {
 		if (!scmd->device->host->hostt->skip_settle_delay)
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -370,6 +370,11 @@ struct scsi_host_template {
 	unsigned ordered_tag:1;
 
 	/*
+	 * true if this LLD supports unlocked error handler hooks
+	 */
+	unsigned unlocked_eh:1;
+
+	/*
 	 * Countdown for host blocking with no commands outstanding
 	 */
 	unsigned int max_host_blocked;
@@ -527,6 +532,11 @@ struct Scsi_Host {
 	unsigned ordered_tag:1;
 
 	/*
+	 * true if this LLD supports unlocked error handler hooks
+	 */
+	unsigned unlocked_eh:1;
+
+	/*
 	 * Optional work queue to be utilized by the transport
 	 */
 	char work_q_name[KOBJ_NAME_LEN];

Re: [RFC][PATCH] allow sleep inside EH hooks

[Arjan van de Ven]

On Fri, 2005-05-27 at 00:32 -0400, Jeff Garzik wrote:
> SCSI EH processing already serializes things during EH, so this spinlock 
> isn't really needed.
> 
> Removing the spinlock outright would break drivers that surround logic 
> with spin_unlock_irq()..spin_lock_irq(), so I introduced ->unlocked_eh 
> option.

it's not THAT many drivers that do that, and relatively easy to grep
for... so why not just remove those unlock ... lock pairs instead?

Re: [RFC][PATCH] allow sleep inside EH hooks

[Christoph Hellwig]

On Fri, May 27, 2005 at 12:32:07AM -0400, Jeff Garzik wrote:
> 
> SCSI EH processing already serializes things during EH, so this spinlock 
> isn't really needed.
> 
> Removing the spinlock outright would break drivers that surround logic 
> with spin_unlock_irq()..spin_lock_irq(), so I introduced ->unlocked_eh 
> option.

Linus has vetoed such conditional locking in the past.  However if you do
it don't make it EH specific but introduce a ->concurrent flag that disables
taking host_lock for ->queuecommand aswell.

Re: [RFC][PATCH] allow sleep inside EH hooks

[Jeff Garzik]

Christoph Hellwig wrote:
> On Fri, May 27, 2005 at 12:32:07AM -0400, Jeff Garzik wrote:
> 
>>SCSI EH processing already serializes things during EH, so this spinlock 
>>isn't really needed.
>>
>>Removing the spinlock outright would break drivers that surround logic 
>>with spin_unlock_irq()..spin_lock_irq(), so I introduced ->unlocked_eh 
>>option.
> 
> 
> Linus has vetoed such conditional locking in the past.  However if you do
> it don't make it EH specific but introduce a ->concurrent flag that disables
> taking host_lock for ->queuecommand aswell.

Such a 'concurrent' flag violates Linus credo "do what you must, and no 
more."  It's also silly and much too invasive.

Removing the locking from the EH routines (only), and fixing up all 
necessary drivers, is much more appealing.

	Jeff

Re: [RFC][PATCH] allow sleep inside EH hooks

[Christoph Hellwig]

On Fri, May 27, 2005 at 03:48:40AM -0400, Jeff Garzik wrote:
> Christoph Hellwig wrote:
> >On Fri, May 27, 2005 at 12:32:07AM -0400, Jeff Garzik wrote:
> >
> >>SCSI EH processing already serializes things during EH, so this spinlock 
> >>isn't really needed.
> >>
> >>Removing the spinlock outright would break drivers that surround logic 
> >>with spin_unlock_irq()..spin_lock_irq(), so I introduced ->unlocked_eh 
> >>option.
> >
> >
> >Linus has vetoed such conditional locking in the past.  However if you do
> >it don't make it EH specific but introduce a ->concurrent flag that 
> >disables
> >taking host_lock for ->queuecommand aswell.
> 
> Such a 'concurrent' flag violates Linus credo "do what you must, and no 
> more."  It's also silly and much too invasive.
> 
> Removing the locking from the EH routines (only), and fixing up all 
> necessary drivers, is much more appealing.

No, hav ing the host_lock only held for ->queuecommand which doesn't
need that locking doesn't make any sense.  An API like the current one
where we take the lock over all run-time entry points makes some sense,
and one where we don't take the lock at all makes lots of sense.  One
where we take the lock where it hurts most but not in the other cases
doesn't.

Re: [RFC][PATCH] allow sleep inside EH hooks

[Jeff Garzik]

Christoph Hellwig wrote:
> On Fri, May 27, 2005 at 03:48:40AM -0400, Jeff Garzik wrote:
> 
>>Christoph Hellwig wrote:
>>
>>>On Fri, May 27, 2005 at 12:32:07AM -0400, Jeff Garzik wrote:
>>>
>>>
>>>>SCSI EH processing already serializes things during EH, so this spinlock 
>>>>isn't really needed.
>>>>
>>>>Removing the spinlock outright would break drivers that surround logic 
>>>>with spin_unlock_irq()..spin_lock_irq(), so I introduced ->unlocked_eh 
>>>>option.
>>>
>>>
>>>Linus has vetoed such conditional locking in the past.  However if you do
>>>it don't make it EH specific but introduce a ->concurrent flag that 
>>>disables
>>>taking host_lock for ->queuecommand aswell.
>>
>>Such a 'concurrent' flag violates Linus credo "do what you must, and no 
>>more."  It's also silly and much too invasive.
>>
>>Removing the locking from the EH routines (only), and fixing up all 
>>necessary drivers, is much more appealing.
> 
> 
> No, hav ing the host_lock only held for ->queuecommand which doesn't
> need that locking doesn't make any sense.  An API like the current one

It makes a lot of sense:  LLDs are written with the assumption that 
paths called from ->queuecommand will not be interrupted by their own 
interrupt handler, whereas error handling paths are typically written 
with precisely the -opposite- assumption.

Removing spin_lock_irq() from queuecommand in SCSI EH causes problems, 
and solves nothing.

	Jeff

Re: [RFC][PATCH] allow sleep inside EH hooks

[Luben Tuikov]

On 05/27/05 04:36, Jeff Garzik wrote:
> Christoph Hellwig wrote:
>>No, hav ing the host_lock only held for ->queuecommand which doesn't
>>need that locking doesn't make any sense.  An API like the current one
> 
> 
> It makes a lot of sense:  LLDs are written with the assumption that 
> paths called from ->queuecommand will not be interrupted by their own 
> interrupt handler, whereas error handling paths are typically written 
> with precisely the -opposite- assumption.
> 
> Removing spin_lock_irq() from queuecommand in SCSI EH causes problems, 
> and solves nothing.

scsi_done() itself needs no explicit locking, it is completely reentrant
and this is a good thing.

I'd like to see the same thing for queuecommand(), i.e. host_lock be gone.

Modern HA and their LLDD, implement completely reentrant queuecommand()
and can deliver commands back to SCSI Core, via scsi_done(), _simultaneously_.
I.e. no concurrency between queuecommand() and scsi_done() is necessary.

This will greatly "free up" design and logic in LLDD to achieve greater
throughput.

Sleeping in EH is good.

	Luben

Re: [RFC][PATCH] allow sleep inside EH hooks

[Jeff Garzik]

Luben Tuikov wrote:
> On 05/27/05 04:36, Jeff Garzik wrote:
> 
>>Christoph Hellwig wrote:
>>
>>>No, hav ing the host_lock only held for ->queuecommand which doesn't
>>>need that locking doesn't make any sense.  An API like the current one
>>
>>
>>It makes a lot of sense:  LLDs are written with the assumption that 
>>paths called from ->queuecommand will not be interrupted by their own 
>>interrupt handler, whereas error handling paths are typically written 
>>with precisely the -opposite- assumption.
>>
>>Removing spin_lock_irq() from queuecommand in SCSI EH causes problems, 
>>and solves nothing.
> 
> 
> scsi_done() itself needs no explicit locking, it is completely reentrant
> and this is a good thing.
> 
> I'd like to see the same thing for queuecommand(), i.e. host_lock be gone.

We can take up that topic once I'm done with sleeping-in-EH project :)

I don't disagree...   but changing the locking for ->queuecommand() is a 
-lot- more invasive, and requires much more care.

I'm also curious to see what others think about removing the host_lock 
acquisition from ->queuecommand() calls.

	Jeff

Re: [RFC][PATCH] allow sleep inside EH hooks

[James Bottomley]

On Fri, 2005-05-27 at 12:49 -0400, Jeff Garzik wrote:
> I'm also curious to see what others think about removing the host_lock 
> acquisition from ->queuecommand() calls.

Personally I don't see the need for it for two reasons:

1) There are certain atomic ops in the mid layer issue that necessitate
we acquire it anyway (serial number generation and state model checking)
2) All queuecommand routines should move to the model of either issuing
or rejecting the command ... if they do that, often there's no need for
the mucking with lock, they remain locked throughout.

On point 2), look at the changes to the aic7xxx driver.  Once its
internal issue queueing was pulled out, it no longer needs to muck with
the host lock in ahc_linux_queue.  If you don't enter this locked, it
would just have to take it at the top and release it before return, as
would most other well written drivers.

James

Re: [RFC][PATCH] allow sleep inside EH hooks

[Christoph Hellwig]

On Fri, May 27, 2005 at 01:04:49PM -0400, James Bottomley wrote:
> On Fri, 2005-05-27 at 12:49 -0400, Jeff Garzik wrote:
> > I'm also curious to see what others think about removing the host_lock 
> > acquisition from ->queuecommand() calls.
> 
> Personally I don't see the need for it for two reasons:
> 
> 1) There are certain atomic ops in the mid layer issue that necessitate
> we acquire it anyway (serial number generation and state model checking)
> 2) All queuecommand routines should move to the model of either issuing
> or rejecting the command ... if they do that, often there's no need for
> the mucking with lock, they remain locked throughout.
> 
> On point 2), look at the changes to the aic7xxx driver.  Once its
> internal issue queueing was pulled out, it no longer needs to muck with
> the host lock in ahc_linux_queue.  If you don't enter this locked, it
> would just have to take it at the top and release it before return, as
> would most other well written drivers.

The only think host_lock in ->queuecommand protects against is the
interrupt handler, which is completely in the driver.  At which point
it makes lots of sense to use a driver-private lock for it.  Even better
we can get the scsi subsystem out of the irq disabling business once
all drivers use their own locks in the interrupt handler.

Re: [RFC][PATCH] allow sleep inside EH hooks

[Jeff Garzik]

On Fri, May 27, 2005 at 06:22:32PM +0100, Christoph Hellwig wrote:
> On Fri, May 27, 2005 at 01:04:49PM -0400, James Bottomley wrote:
> > On Fri, 2005-05-27 at 12:49 -0400, Jeff Garzik wrote:
> > > I'm also curious to see what others think about removing the host_lock 
> > > acquisition from ->queuecommand() calls.
> > 
> > Personally I don't see the need for it for two reasons:
> > 
> > 1) There are certain atomic ops in the mid layer issue that necessitate
> > we acquire it anyway (serial number generation and state model checking)
> > 2) All queuecommand routines should move to the model of either issuing
> > or rejecting the command ... if they do that, often there's no need for
> > the mucking with lock, they remain locked throughout.
> > 
> > On point 2), look at the changes to the aic7xxx driver.  Once its
> > internal issue queueing was pulled out, it no longer needs to muck with
> > the host lock in ahc_linux_queue.  If you don't enter this locked, it
> > would just have to take it at the top and release it before return, as
> > would most other well written drivers.
> 
> The only think host_lock in ->queuecommand protects against is the
> interrupt handler, which is completely in the driver.  At which point
> it makes lots of sense to use a driver-private lock for it.  Even better
> we can get the scsi subsystem out of the irq disabling business once
> all drivers use their own locks in the interrupt handler.

Don't forget...

We still need to have some mechanism for handling the request_queue's
locking.

	Jeff

Re: [RFC][PATCH] allow sleep inside EH hooks

[Christoph Hellwig]

On Fri, May 27, 2005 at 12:49:25PM -0400, Jeff Garzik wrote:
> >I'd like to see the same thing for queuecommand(), i.e. host_lock be gone.
> 
> We can take up that topic once I'm done with sleeping-in-EH project :)
> 
> I don't disagree...   but changing the locking for ->queuecommand() is a 
> -lot- more invasive, and requires much more care.

I'll promise to do the ->queuecommand bits ASAP once you're done with
all error handlers :)