Block timeouts seem not to be working
James Bottomley
maintainerI just noticed this with a rather finickey SAS system I have. It's got a SATA DVD attached over an expander. Periodically the DVD just hangs up, so we wait for the timeout and then send a phy reset which clears it.
What I'm seeing with the new block timer code is that the timer never expires. I can dig some more into this, but if you wanted to test it as well, the timer code is easy to excite. Just throw away one command in every 128 or so in the queuecommand routine of your favourite HBA driver.
James
Quoted 10 lines · James Bottomley
I just noticed this with a rather finickey SAS system I have. It's got a SATA DVD attached over an expander. Periodically the DVD just hangs up, so we wait for the timeout and then send a phy reset which clears it.
What I'm seeing with the new block timer code is that the timer never expires. I can dig some more into this, but if you wanted to test it as well, the timer code is easy to excite. Just throw away one command in every 128 or so in the queuecommand routine of your favourite HBA driver.
I have not seen the case of the timer never expiring, but will look into other test cases.
Mike C and I where seeing timeout issues (host staying in recovery state or list debug bugon's) when running tests with timeouts set to 1 or 2 seconds. We are working with Jens to address this. The issue we where hitting appears related to the scsi_eh / scsi_done completion synchronization.
-andmike
--
Michael Anderson
[email protected]
Quoted 10 lines · James Bottomley
I just noticed this with a rather finickey SAS system I have. It's got a SATA DVD attached over an expander. Periodically the DVD just hangs up, so we wait for the timeout and then send a phy reset which clears it.
What I'm seeing with the new block timer code is that the timer never expires. I can dig some more into this, but if you wanted to test it as well, the timer code is easy to excite. Just throw away one command in every 128 or so in the queuecommand routine of your favourite HBA driver.
James, I've seen a few oddities as well, I'll be beating on it tomorrow again to shake out the last bug(s).
Signature · 1 line
Jens Axboe
James Bottomley
maintainerQuoted 6 lines · Jens Axboe
Quoted 5 lines · James Bottomley
I just noticed this with a rather finickey SAS system I have. It's got a SATA DVD attached over an expander. Periodically the DVD just hangs up, so we wait for the timeout and then send a phy reset which clears it.
Quoted 8 lines
Quoted 5 lines
What I'm seeing with the new block timer code is that the timer never expires. I can dig some more into this, but if you wanted to test it as well, the timer code is easy to excite. Just throw away one command in every 128 or so in the queuecommand routine of your favourite HBA driver.
James, I've seen a few oddities as well, I'll be beating on it tomorrow again to shake out the last bug(s).
Actually, turns out it's nothing to do with block timeouts, it's a target reset bug.
This loop:
for (id = 0; id <= shost->max_id; id++) {Never terminates if shost->max_id is set to ~0, like aic94xx does.
It's also pretty inefficient since you mostly have compact target numbers, but the max_id can be very high. The best way would be to sort the recovery list by target id and skip them if they're equal, but even a worst case O(N^2) traversal is probably OK here.
James
---
drivers/scsi/scsi_error.c +15 -4
@@ -1065,10 +1065,10 @@ static int scsi_eh_target_reset(struct Scsi_Host *shost, struct list_head *done_q) {struct scsi_cmnd *scmd, *tgtr_scmd, *next; -unsigned int id; +unsigned int id =0 ;int rtn; -for (id =0 ; id <= shost->max_id; id++) { + do { tgtr_scmd = NULL; list_for_each_entry(scmd, work_q, eh_entry) { if (id == scmd_id(scmd)) {
@@ -1076,8 +1076,18 @@ static int scsi_eh_target_reset(struct Scsi_Host *shost,break ; } } +if (!tgtr_scmd) { +/* not one exactly equal; find the next highest */ +list_for_each_entry (scmd, work_q, eh_entry) { +if (scmd_id (scmd) > id && + (!tgtr_scmd || +scmd_id (tgtr_scmd) >scmd_id (scmd))) + tgtr_scmd = scmd; + } + }if (!tgtr_scmd) -continue ; +/* no more commands, that's it */ +break ; SCSI_LOG_ERROR_RECOVERY(3, printk("%s: Sending target reset " "to target %d\n",
@@ -1096,7 +1106,8 @@ static int scsi_eh_target_reset(struct Scsi_Host *shost," failed target: " "%d\n" , current->comm, id)); - } + id++; + }while (id !=0 );return list_empty (work_q); }
Quoted 20 lines · James Bottomley
Quoted 6 lines · Jens Axboe
Quoted 5 lines · James Bottomley
I just noticed this with a rather finickey SAS system I have. It's got a SATA DVD attached over an expander. Periodically the DVD just hangs up, so we wait for the timeout and then send a phy reset which clears it.
Quoted 7 lines
Quoted 5 lines
What I'm seeing with the new block timer code is that the timer never expires. I can dig some more into this, but if you wanted to test it as well, the timer code is easy to excite. Just throw away one command in every 128 or so in the queuecommand routine of your favourite HBA driver.
James, I've seen a few oddities as well, I'll be beating on it tomorrow again to shake out the last bug(s).
Actually, turns out it's nothing to do with block timeouts, it's a target reset bug.
This loop:
Quoted 9 lines
for (id = 0; id <= shost->max_id; id++) {Never terminates if
shost->max_idis set to ~0, like aic94xx does.It's also pretty inefficient since you mostly have compact target numbers, but the
max_idcan be very high. The best way would be to sort the recovery list by target id and skip them if they're equal, but even a worst case O(N^2) traversal is probably OK here.
Sorry about that. I really screwed up on multiple counts there. I tested your patch with Linus's tree here on some drivers by setting the command timeout to 1 second and letting IO run against slow targets. I saw the eh run multiple times and it ran fine with your patch. Thanks for finding that.