Make SCSI error handler code easier to understand

scsi_put_command() is either invoked before a command is queued or after a command has completed. scsi_cmnd.abort_work is scheduled after a command has timed out and before it is finished. The block layer guarantees that either the softirq_done_fn() or the rq_timed_out_fn() function is invoked but not both. This means that scsi_put_command() is never invoked while abort_work is scheduled. Hence remove the cancel_delayed_work() call from scsi_put_command().

Similarly, scsi_abort_command() is only invoked from the SCSI timeout handler. If scsi_abort_command() is invoked for a SCSI command with the SCSI_EH_ABORT_SCHEDULED flag set this means that scmd_eh_abort_handler() has already invoked scsi_queue_insert() and hence that scsi_cmnd.abort_work is no longer pending. Hence also remove the cancel_delayed_work() call from scsi_abort_command().

Signed-off-by Bart Van Assche <[email protected]>

Cc Hannes Reinecke <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc Christoph Hellwig <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Joe Lawrence <[email protected]>

---
 drivers/scsi/scsi.c       | 2 +-
 drivers/scsi/scsi_error.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

drivers/scsi/scsi.c +1 -1

index 88d46fe..c972eab 100644
@@ -334,7 +334,7 @@ void scsi_put_command(struct scsi_cmnd *cmd)
 	list_del_init(&cmd->list);
 	spin_unlock_irqrestore(&cmd->device->list_lock, flags);
 
-	cancel_delayed_work(&cmd->abort_work);
+	WARN_ON_ONCE(delayed_work_pending(&cmd->abort_work));
 
 	__scsi_put_command(cmd->device->host, cmd);
 }

drivers/scsi/scsi_error.c +0 -0

index f17aa7a..5232583 100644

Il 26/05/2014 17:14, Bart Van Assche ha scritto:

Quoted 47 lines

scsi_put_command() is either invoked before a command is queued or after a command has completed. scsi_cmnd.abort_work is scheduled after a command has timed out and before it is finished. The block layer guarantees that either the softirq_done_fn() or the rq_timed_out_fn() function is invoked but not both. This means that scsi_put_command() is never invoked while abort_work is scheduled. Hence remove the cancel_delayed_work() call from scsi_put_command().

Similarly, scsi_abort_command() is only invoked from the SCSI timeout handler. If scsi_abort_command() is invoked for a SCSI command with the SCSI_EH_ABORT_SCHEDULED flag set this means that scmd_eh_abort_handler() has already invoked scsi_queue_insert() and hence that scsi_cmnd.abort_work is no longer pending. Hence also remove the cancel_delayed_work() call from scsi_abort_command().

Signed-off-by Bart Van Assche <[email protected]>

Cc Hannes Reinecke <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc Christoph Hellwig <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Joe Lawrence <[email protected]>

---
 drivers/scsi/scsi.c       | 2 +-
 drivers/scsi/scsi_error.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

drivers/scsi/scsi.c +1 -1

index 88d46fe..c972eab 100644
@@ -334,7 +334,7 @@ void scsi_put_command(struct scsi_cmnd *cmd)
 	list_del_init(&cmd->list);
 	spin_unlock_irqrestore(&cmd->device->list_lock, flags);
 
-	cancel_delayed_work(&cmd->abort_work);
+	WARN_ON_ONCE(delayed_work_pending(&cmd->abort_work));
 
 	__scsi_put_command(cmd->device->host, cmd);
 }

drivers/scsi/scsi_error.c +0 -0

index f17aa7a..5232583 100644
@@ -193,7 +193,7 @@ scsi_abort_command(struct scsi_cmnd *scmd)
 		SCSI_LOG_ERROR_RECOVERY(3,
 			scmd_printk(KERN_INFO, scmd,
 				    "scmd %p previous abort failed\n", scmd));

drivers/scsi/scsi_error.c scsi_abort_command(struct scsi_cmnd *scmd) · line 199

Quoted 6 lines

drivers/scsi/scsi_error.c +1 -1

-		cancel_delayed_work(&scmd->abort_work);
+		WARN_ON_ONCE(delayed_work_pending(&scmd->abort_work));
 		return FAILED;
 	}
 

I still prefer a BUG_ON in scsi_put_command, but anyway: series

Make it possible to test the REQ_ATOM_COMPLETE bit from outside the block layer core.

scsi_put_command() is either invoked before a command is queued or after a command has completed. scsi_cmnd.abort_work is scheduled after a command has timed out and before it is finished. The block layer guarantees that either the softirq_done_fn() or the rq_timed_out_fn() function is invoked but not both. This means that scsi_put_command() is never invoked while abort_work is scheduled. Hence remove the cancel_delayed_work() call from scsi_put_command().

Similarly, scsi_abort_command() is only invoked from the SCSI timeout handler. If scsi_abort_command() is invoked for a SCSI command with the SCSI_EH_ABORT_SCHEDULED flag set this means that scmd_eh_abort_handler() has already invoked scsi_queue_insert() and hence that scsi_cmnd.abort_work is no longer pending. Hence also remove the cancel_delayed_work() call from scsi_abort_command().

Signed-off-by Bart Van Assche <[email protected]>

Cc Hannes Reinecke <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc Christoph Hellwig <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Joe Lawrence <[email protected]>

---
  drivers/scsi/scsi.c       | 2 +-
  drivers/scsi/scsi_error.c | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)

drivers/scsi/scsi.c +1 -1

index 88d46fe..c972eab 100644
@@ -334,7 +334,7 @@ void scsi_put_command(struct scsi_cmnd *cmd)
  	list_del_init(&cmd->list);
  	spin_unlock_irqrestore(&cmd->device->list_lock, flags);
 
-	cancel_delayed_work(&cmd->abort_work);
+	WARN_ON_ONCE(delayed_work_pending(&cmd->abort_work));
 
  	__scsi_put_command(cmd->device->host, cmd);
  }

drivers/scsi/scsi_error.c +0 -0

index f17aa7a..5232583 100644

Make it possible to test the REQ_ATOM_COMPLETE bit from outside the block layer core.

Signed-off-by Bart Van Assche <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Hannes Reinecke <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc Christoph Hellwig <[email protected]>

Cc Joe Lawrence <[email protected]>

Every now and then someone asks how it is avoided that the SCSI error handler and the SCSI completion handler are invoked concurrently for the same SCSI command. Add a few WARN_ON_ONCE() statements that make it clear how this is avoided.

Signed-off-by Bart Van Assche <[email protected]>

Cc Hannes Reinecke <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc Christoph Hellwig <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Joe Lawrence <[email protected]>

---
  drivers/scsi/scsi_error.c | 6 ++++++
  1 file changed, 6 insertions(+)

drivers/scsi/scsi_error.c +4 -0

index 5232583..aee0dc0 100644
@@ -120,6 +120,8 @@ scmd_eh_abort_handler(struct work_struct *work)
  	struct scsi_device *sdev = scmd->device;
  	int rtn;
 
+	WARN_ON_ONCE(!blk_rq_completed(scmd->request));
+
  	if (scsi_host_eh_past_deadline(sdev->host)) {
  		SCSI_LOG_ERROR_RECOVERY(3,
  			scmd_printk(KERN_INFO, scmd,

@@ -185,6 +187,8 @@ scsi_abort_command(struct scsi_cmnd *scmd)
  	struct Scsi_Host *shost = sdev->host;
  	unsigned long flags;
 
+	WARN_ON_ONCE(!blk_rq_completed(scmd->request));
+
  	if (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED) {
  		/*
  		 * Retry after abort failed, escalate to next level.

Quoted 6 lines · Bart Van Assche

drivers/scsi/scsi_error.c +0 -0

index f17aa7a..5232583 100644

drivers/scsi/scsi_error.c scsi_abort_command(struct scsi_cmnd *scmd) · line 199

Quoted 8 lines

drivers/scsi/scsi_error.c +1 -1

@@ -193,7 +193,7 @@ scsi_abort_command(struct scsi_cmnd *scmd)
          SCSI_LOG_ERROR_RECOVERY(3,
              scmd_printk(KERN_INFO, scmd,
                      "scmd %p previous abort failed\n", scmd));
-        cancel_delayed_work(&scmd->abort_work);
+        WARN_ON_ONCE(delayed_work_pending(&scmd->abort_work));
          return FAILED;
      }
 

The first bit is okay, the second isn't.

The second bit is for these cases where the abort got scheduled (in scsi_abort_command()), but the workqueue didn't get executed by the time the next timeout occured.
I know, highly unlikely, but there is no safeguarding that it _cannot_ happen.
So the second cancel_delayed_work() has to stay.

Quoted 17 lines · Hannes Reinecke

Quoted 6 lines · Bart Van Assche

drivers/scsi/scsi_error.c +0 -0

index f17aa7a..5232583 100644

drivers/scsi/scsi_error.c scsi_abort_command(struct scsi_cmnd *scmd) · line 199

Quoted 8 lines

drivers/scsi/scsi_error.c +1 -1

@@ -193,7 +193,7 @@ scsi_abort_command(struct scsi_cmnd *scmd)
           SCSI_LOG_ERROR_RECOVERY(3,
               scmd_printk(KERN_INFO, scmd,
                       "scmd %p previous abort failed\n", scmd));
-        cancel_delayed_work(&scmd->abort_work);
+        WARN_ON_ONCE(delayed_work_pending(&scmd->abort_work));
           return FAILED;
       }
 

The first bit is okay, the second isn't.

Quoted 6 lines

The second bit is for these cases where the abort got scheduled (in scsi_abort_command()), but the workqueue didn't get executed by the time the next timeout occured.
I know, highly unlikely, but there is no safeguarding that it _cannot_ happen.
So the second cancel_delayed_work() has to stay.

But how could that next timeout occur while abort_work is still pending ? The block layer removes a request from the timeout list before invoking the timeout handler (see also blk_rq_check_expired()). This means that no block layer timers are active after abort_work has been scheduled and before scmd_eh_abort_handler() is called. This also means that a second timeout can only occur after a SCSI command has been reinserted to a SCSI device queue. And such a reinsertion can only occur after scmd_eh_abort_handler() has started. The pending bit is cleared from a work struct before the associated handler is invoked. This is why I think the above cancel_delayed_work() statement is not necessary. Or did I perhaps overlook something ?

Quoted 2 lines · Bart Van Assche

Make it possible to test the REQ_ATOM_COMPLETE bit from outside the block layer core.

I don't see the value of patches 2,3 they're checking for an impossible condition ... why might it be possible?

Quoted 2 lines

I don't see the value of patches 2,3 they're checking for an impossible condition ... why might it be possible?

When reading the source code in scsi_error.c it's easy to overlook that scmd_eh_abort_handler(), scsi_abort_command() and scsi_eh_scmd_add() are all invoked for requests in which the REQ_ATOM_COMPLETE bit has been set. Although it is possible to mention this as a comment above these functions, such comments are not checked at runtime. It would require additional work from the reader to verify whether or not such source code comments are up to date. However, the condition inside a WARN_ON_ONCE() statement is checked every time the code is executed. Hence my preference for a WARN_ON_ONCE() statement instead of writing down somewhere that these three functions operate on requests in which the REQ_ATOM_COMPLETE bit has been set.

Quoted 14 lines · Bart Van Assche

Quoted 2 lines

I don't see the value of patches 2,3 they're checking for an impossible condition ... why might it be possible?

When reading the source code in scsi_error.c it's easy to overlook that scmd_eh_abort_handler(), scsi_abort_command() and scsi_eh_scmd_add() are all invoked for requests in which the REQ_ATOM_COMPLETE bit has been set. Although it is possible to mention this as a comment above these functions, such comments are not checked at runtime. It would require additional work from the reader to verify whether or not such source code comments are up to date. However, the condition inside a WARN_ON_ONCE() statement is checked every time the code is executed. Hence my preference for a WARN_ON_ONCE() statement instead of writing down somewhere that these three functions operate on requests in which the REQ_ATOM_COMPLETE bit has been set.

I really do like the REQ_ATOM_COMPLETE asserts - as experience tells these kinds of assumptions are best checked, otherwise they will unintentionally be violated.

I'm less excited about the list walk I have to say, as the overhead is getting fairly large for a simple assertation.

Il 26/05/2014 17:14, Bart Van Assche ha scritto:

Quoted 19 lines

drivers/scsi/scsi.c +1 -1

index 88d46fe..c972eab 100644
@@ -334,7 +334,7 @@ void scsi_put_command(struct scsi_cmnd *cmd)
     list_del_init(&cmd->list);
     spin_unlock_irqrestore(&cmd->device->list_lock, flags);
 
-    cancel_delayed_work(&cmd->abort_work);
+    WARN_ON_ONCE(delayed_work_pending(&cmd->abort_work));
 
     __scsi_put_command(cmd->device->host, cmd);
 }

drivers/scsi/scsi_error.c +0 -0

index f17aa7a..5232583 100644

drivers/scsi/scsi_error.c scsi_abort_command(struct scsi_cmnd *scmd) · line 199

Quoted 8 lines

drivers/scsi/scsi_error.c +1 -1

@@ -193,7 +193,7 @@ scsi_abort_command(struct scsi_cmnd *scmd)
         SCSI_LOG_ERROR_RECOVERY(3,
             scmd_printk(KERN_INFO, scmd,
                     "scmd %p previous abort failed\n", scmd));
-        cancel_delayed_work(&scmd->abort_work);
+        WARN_ON_ONCE(delayed_work_pending(&scmd->abort_work));
         return FAILED;
     }
 

I still prefer a BUG_ON in scsi_put_command, but anyway: series

Reviewed-by Paolo Bonzini <[email protected]>

Quoted 31 lines · Paolo Bonzini

Il 26/05/2014 17:14, Bart Van Assche ha scritto:

Quoted 19 lines

drivers/scsi/scsi.c +1 -1

index 88d46fe..c972eab 100644
@@ -334,7 +334,7 @@ void scsi_put_command(struct scsi_cmnd *cmd)
     list_del_init(&cmd->list);
     spin_unlock_irqrestore(&cmd->device->list_lock, flags);
 
-    cancel_delayed_work(&cmd->abort_work);
+    WARN_ON_ONCE(delayed_work_pending(&cmd->abort_work));
 
     __scsi_put_command(cmd->device->host, cmd);
 }

drivers/scsi/scsi_error.c +0 -0

index f17aa7a..5232583 100644

drivers/scsi/scsi_error.c scsi_abort_command(struct scsi_cmnd *scmd) · line 199

Quoted 8 lines

drivers/scsi/scsi_error.c +1 -1

@@ -193,7 +193,7 @@ scsi_abort_command(struct scsi_cmnd *scmd)
         SCSI_LOG_ERROR_RECOVERY(3,
             scmd_printk(KERN_INFO, scmd,
                     "scmd %p previous abort failed\n", scmd));
-        cancel_delayed_work(&scmd->abort_work);
+        WARN_ON_ONCE(delayed_work_pending(&scmd->abort_work));
         return FAILED;
     }
 

I still prefer a BUG_ON in scsi_put_command, but anyway: series

Quoted 1 line

Reviewed-by Paolo Bonzini <[email protected]>

Hello Paolo,

As you probably know scsi_put_command() can get called from softirq context. A BUG_ON() in that context might make it unnecessary hard for a user to collect call traces.

Quoted 4 lines · James Bottomley

Quoted 2 lines · Bart Van Assche

Make it possible to test the REQ_ATOM_COMPLETE bit from outside the block layer core.

Quoted 2 lines

I don't see the value of patches 2,3 they're checking for an impossible condition ... why might it be possible?

When reading the source code in scsi_error.c it's easy to overlook that scmd_eh_abort_handler(), scsi_abort_command() and scsi_eh_scmd_add() are all invoked for requests in which the REQ_ATOM_COMPLETE bit has been set.

Although it is possible to mention this as a comment above these
functions, such comments are not checked at runtime. It would require additional work from the reader to verify whether or not such source code comments are up to date. However, the condition inside a WARN_ON_ONCE() statement is checked every time the code is executed. Hence my preference for a WARN_ON_ONCE() statement instead of writing down somewhere that these three functions operate on requests in which the REQ_ATOM_COMPLETE bit has been set.

Quoted 3 lines · Bart Van Assche

As you probably know scsi_put_command() can get called from softirq context. A BUG_ON() in that context might make it unnecessary hard for a user to collect call traces.

Why? The messages dumped are the same, the trace just starts from the IRQ context ... I don't see what the problem is.

The question isn't ease of gathering the data, it's correctness. The point is that if the assert fails we have a free of an in-use command leading to a nasty use after free ... the machine state is hosed at that point.

Quoted 5 lines · James Bottomley

Quoted 3 lines · Bart Van Assche

As you probably know scsi_put_command() can get called from softirq context. A BUG_ON() in that context might make it unnecessary hard for a user to collect call traces.

Quoted 7 lines

Why? The messages dumped are the same, the trace just starts from the IRQ context ... I don't see what the problem is.

The question isn't ease of gathering the data, it's correctness. The point is that if the assert fails we have a free of an in-use command leading to a nasty use after free ... the machine state is hosed at that point.

Please keep in mind that even if the SCSI mid-layer functions correctly it is still possible that another driver in the system could cause these tests to fail if it triggers e.g. a use-after-free.

If BUG_ON() is invoked the dumped message will be displayed on the screen but will not be saved in the system log. This is inconvenient because it means that if someone wants to capture the dumped message a camera is necessary and one has to step to the physical console to capture this message. Using WARN_ON() or WARN_ON_ONCE() makes it a lot easier for users to capture any message that is displayed.

Quoted 4 lines · Bart Van Assche

When reading the source code in scsi_error.c it's easy to overlook that scmd_eh_abort_handler(), scsi_abort_command() and scsi_eh_scmd_add() are all invoked for requests in which the REQ_ATOM_COMPLETE bit has been set.

I really don't like this entanglement of state of block and SCSI. "complete" in block terms isn't the same as in SCSI terms. The REQ_ATOM_COMPLETE flag is fully internal to block and indicates that we've taken over processing the command and any completions into block get ignored. This is for the possible race between timeout and inbound command completion. If you start coding SCSI assertions in terms of it, you're entangling layers that should be separate.

The assertion in SCSI terms is that abort and ->done cannot race.

The whole reason BUG_ON doesn't leave a log trace is to try to prevent corruption propagating to the data storage devices. What you propose would be inviting that corruption in the name of getting a log entry.

Quoted 14 lines · James Bottomley

Quoted 4 lines · Bart Van Assche

When reading the source code in scsi_error.c it's easy to overlook that scmd_eh_abort_handler(), scsi_abort_command() and scsi_eh_scmd_add() are all invoked for requests in which the REQ_ATOM_COMPLETE bit has been set.

I really don't like this entanglement of state of block and SCSI. "complete" in block terms isn't the same as in SCSI terms. The REQ_ATOM_COMPLETE flag is fully internal to block and indicates that we've taken over processing the command and any completions into block get ignored. This is for the possible race between timeout and inbound command completion. If you start coding SCSI assertions in terms of it, you're entangling layers that should be separate.

Quoted 1 line

The assertion in SCSI terms is that abort and ->done cannot race.

Recent e-mail threads on the linux-scsi mailing list have shown that it is easily overlooked which error handler functions are called only for requests marked by the block layer as "complete" and hence cannot race with scsi_softirq_done(). So far my proposals for how to improve the documentation of how this race is avoided were rejected. Do you perhaps have a proposal of how this should be documented properly ?

I could also see us one day extending the TMF capability to abort any running command, which would make even an assertion of block timed out or completed invalid.

Il 27/05/2014 12:21, James Bottomley ha scritto:

Quoted 3 lines

I could also see us one day extending the TMF capability to abort any running command, which would make even an assertion of block timed out or completed invalid.

Actually the assertion would remain valid, and that's exactly what Bart wants to document with this assertion.

Quoted 7 lines

Il 27/05/2014 12:21, James Bottomley ha scritto:

Quoted 3 lines

I could also see us one day extending the TMF capability to abort any running command, which would make even an assertion of block timed out or completed invalid.

Actually the assertion would remain valid, and that's exactly what Bart wants to document with this assertion.

No, it wouldn't: if we abort a running command by definition the command hadn't timed out and might not be completed. This is required by TMF handling because now you have an abort racing with a completion. Either the command completes normally because it misses the abort or the abort gets to it and its returned status is set to TASK_ABORTED. That's the only way you can tell if the abort was successful or not.

If you're thinking we would tell block to ignore returning commands before issuing the abort, we'd never be able to tell if the abort were successful, so we have to allow the race to collect the status.

Il 27/05/2014 12:59, James Bottomley ha scritto:

Quoted 16 lines · Paolo Bonzini

Quoted 7 lines

Il 27/05/2014 12:21, James Bottomley ha scritto:

Quoted 3 lines

I could also see us one day extending the TMF capability to abort any running command, which would make even an assertion of block timed out or completed invalid.

Actually the assertion would remain valid, and that's exactly what Bart wants to document with this assertion.

No, it wouldn't: if we abort a running command by definition the command hadn't timed out and might not be completed. This is required by TMF handling because now you have an abort racing with a completion. Either the command completes normally because it misses the abort or the abort gets to it and its returned status is set to TASK_ABORTED. That's the only way you can tell if the abort was successful or not.

Quoted 3 lines

If you're thinking we would tell block to ignore returning commands before issuing the abort, we'd never be able to tell if the abort were successful, so we have to allow the race to collect the status.

You could use a different mechanism than a softirq to tell the abort were successful, for example by overriding scsi_done. But with respect to the block layer, the mechanics of avoiding the race and double-free would probably be the same.

Quoted 4 lines

You could use a different mechanism than a softirq to tell the abort were successful, for example by overriding scsi_done. But with respect to the block layer, the mechanics of avoiding the race and double-free would probably be the same.

I think there's some confusion about what the race and double free is:
It only occurs with timeouts. In a timeout situation, the host had decided it's not waiting any longer for the target to respond and proceeds to error recovery. At any time between the host making this decision up to the point it kicks the target hard enough to clear all in-flight commands, the target may return the command. If we didn't have some ignore function on command completions while we're handling errors, this would lead to double completion.

If we decided to allow arbitrary aborts of running commands, we would send a TMF in during the normal (i.e. un timed out) command period. Because there's no timeout involved, there's no double free problem. The race in this case is whether the abort catches the command or not and to mediate that race we need the normal status return.

Il 27/05/2014 13:26, James Bottomley ha scritto:

Quoted 14 lines

Quoted 4 lines

You could use a different mechanism than a softirq to tell the abort were successful, for example by overriding scsi_done. But with respect to the block layer, the mechanics of avoiding the race and double-free would probably be the same.

I think there's some confusion about what the race and double free is:
It only occurs with timeouts. In a timeout situation, the host had decided it's not waiting any longer for the target to respond and proceeds to error recovery. At any time between the host making this decision up to the point it kicks the target hard enough to clear all in-flight commands, the target may return the command. If we didn't have some ignore function on command completions while we're handling errors, this would lead to double completion.

Quoted 5 lines

If we decided to allow arbitrary aborts of running commands, we would send a TMF in during the normal (i.e. un timed out) command period. Because there's no timeout involved, there's no double free problem. The race in this case is whether the abort catches the command or not and to mediate that race we need the normal status return.

I'm not sure why "no timeout" implies "no double free". There would still be a race between the interrupt handler and softirq on one side, and the abort handler on the other.

Every now and then someone asks how it is avoided that the SCSI error handler and the SCSI completion handler are invoked concurrently for the same SCSI command. Hence this patch series that should make the SCSI error handler code a little easier to understand.

Quoted 4 lines · On Mon, 26 May 2014 17:12:27 +0200 Bart Van Assche <[email protected]> wrote:

Every now and then someone asks how it is avoided that the SCSI error handler and the SCSI completion handler are invoked concurrently for the same SCSI command. Hence this patch series that should make the SCSI error handler code a little easier to understand.

Hi Bart,

We talked about REQ_ATOM_COMPLETE a while back (so I may be one of those people who periodically ask about SCSI error handling / completion), and I just thought I'd chime in here to clarify the scenario we were worried about.

Before d555a2ab "[SCSI] Fix spurious request sense in error handling" we saw the following sequence of events:

  [ 1394.345991] sd 2:0:1:0: [sdw] Done:
  [ 1394.361790] TIMEOUT
  [ 1394.374148] sd 2:0:1:0: [sdw]
  [ 1394.388775] Result: hostbyte=DID_ERROR driverbyte=DRIVER_OK
  [ 1394.409068] sd 2:0:1:0: [sdw] CDB:
  [ 1394.424782] Read(10): 28 00 00 00 00 18 00 00 08 00
  [ 1394.443772] sd 2:0:1:0: [sdw] scmd ffff88104dce02c0 abort scheduled
  [ 1394.474026] sd 2:0:1:0: [sdw] aborting command ffff88104dce02c0
  [ 1394.573338] qla2xxx [0000:46:00.0]-801c:2: Abort command issued nexus=2:1:0 --  1 2002.
  [ 1394.609313] sd 2:0:1:0: [sdw] scmd ffff88104dce02c0 retry aborted command

  [ 1425.397434] sd 2:0:1:0: [sdw] Done:
  [ 1425.417802] TIMEOUT
  [ 1425.431914] sd 2:0:1:0: [sdw]
  [ 1425.448247] Result: hostbyte=DID_TIME_OUT driverbyte=DRIVER_OK
  [ 1425.470736] sd 2:0:1:0: [sdw] CDB:
  [ 1425.488005] Read(10): 28 00 00 00 00 18 00 00 08 00
  [ 1425.508472] sd 2:0:1:0: [sdw] scmd ffff88104dce02c0 previous abort failed
  [ 1425.533299] scsi_eh_2: waking up 0/1/1
  [ 1425.551189] sd 2:0:1:0: scsi_eh_prt_fail_stats: cmds failed: 1, cancel: 0
  [ 1425.575897] Total of 1 commands on 1 devices require eh work
  [ 1425.597970] sd 2:0:1:0: [sdw] scsi_eh_2: requesting sense

ie, something like:

CMD -> .queuecommand ... timeout
ABORT -> .eh_abort_handler
CMD (retry) -> .queuecommand ... timeout ABORT -> scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD) ... into scsi_error_handler ...
REQUEST SENSE -> .queuecommand

We eventually found our way into scsi_unjam_host, which "*knows* that all commands on the bus have either completed, failed or timed out.", and then scsi_send_eh_cmnd, which "hijacks" a SCSI command structure. Presumably the second comment follows from the first.

But in the case of timeout, when was the LLDD informed to abort or forget about that scsi command structure?

In our testing, it appeared that the LLDD's .queuecommand was called back-to-back with the same scsi_cmnd pointer. The driver in question was qla2xxx, who keeps a driver-private structure to scsi_cmnd mapping (assumed to be one-to-one) that got confused by this sequence of events.

After d555a2ab, in this scenario scsi_unjam_host skips the request sense and escalates to the next level (scsi_eh_abort_cmds), skipping straight to the abort as we would have expected... This avoids the scsi_cmnd "hijack"... but was it ever safe to do so in the first place?

Or should LLDD expect to gracefully handle the same scsi_cmnd pointer coming into .queuecommand w/o an intermediate abort or other scsi_host_template callback?