Skip to contentSkip to conversation

allow sleep inside EH hooks

RFC PATCH In review Jeff Garzik 5 participants 12 messages May 27, 2005 SCSI

lore
Share & tools
lore

Jeff Garzik

author
#0 lore raw

RFC PATCH 1/1 allow sleep inside EH hooks

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.

drivers/scsi/hosts.c +1 -0
@@ -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
drivers/scsi/scsi_error.c +20 -10
@@ -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)
include/scsi/scsi_host.h +10 -0
@@ -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];
#1 lore raw
Quoted 6 lines · Jeff Garzik

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?

#2 lore raw
In reply to Jeff Garzik
Quoted 7 lines · Jeff Garzik

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.

author
#3 lore raw
Quoted 10 lines · Christoph Hellwig
Quoted 3 lines · Jeff Garzik

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

#4 lore raw
Quoted 16 lines · Jeff Garzik
Quoted 10 lines · Christoph Hellwig
Quoted 6 lines · Jeff Garzik

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.

Quoted 4 lines

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.

Quoted 5 lines

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.

author
#5 lore raw
Quoted 16 lines · Christoph Hellwig
Quoted 14 lines · Jeff Garzik
Quoted 11 lines · Christoph Hellwig
Quoted 3 lines · Jeff Garzik

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.

Quoted 7 lines
Quoted 3 lines

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

#6 lore raw
Quoted 5 lines · Jeff Garzik
Quoted 2 lines · Christoph Hellwig

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

Quoted 7 lines

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

author
#7 lore raw
Quoted 10 lines · Luben Tuikov
Quoted 8 lines · Jeff Garzik
Quoted 2 lines · Christoph Hellwig

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.

Quoted 7 lines
Quoted 1 line

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

#8 lore raw
Quoted 2 lines · Jeff Garzik

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

#9 lore raw
Quoted 12 lines · James Bottomley
Quoted 2 lines · Jeff Garzik

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.

Quoted 5 lines

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.

author
#10 lore raw
Quoted 19 lines · Christoph Hellwig
Quoted 12 lines · James Bottomley
Quoted 2 lines · Jeff Garzik

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.

Quoted 5 lines

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.

Quoted 5 lines

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

#11 lore raw
In reply to Jeff Garzik
Quoted 6 lines · Jeff Garzik
Quoted 1 line

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 :)