scsi: Erroneous handling of sense status in SCSI EH commands

Consider a scenario where a SCSI EH command like TUR fails with a NOT READY status - MANUAL INTERVENTION REQUIRED (02/04/03). As evident in the ASC/ASCQ description, manual intervention is required in this case i.e. the target wants TUR to fail here so that the host administrator can take corrective action.

But this particular ASC/ASCQ is not handled in the scsi_check_sense, which ultimately causes scsi_eh_tur to erroneously report a SUCCESS (device ready) instead of the actual FAILURE state (device not ready).

This patch converts scsi_check_sense() to return SOFT_ERROR in cases where an error has been signalled via the sense code, but we should not wake the error handler. This error code will be reverted back to SUCCESS for normal command handling, but scsi_eh_completed_normally() will convert it to FAILED.

Reported-by Martin George<[email protected]>

Signed-off-by Hannes Reinecke<[email protected]>

drivers/scsi/scsi_error.c +2 -2

index 45c7564..e86e62e 100644
@@ -223,7 +223,7 @@ static inline void scsi_eh_prt_fail_stats(struct Scsi_Host *shost,
   * @scmd:	Cmd to have sense checked.
   *
   * Return value:
- * 	SUCCESS or FAILED or NEEDS_RETRY
+ * 	SUCCESS or FAILED or NEEDS_RETRY or SOFT_ERROR
   *
   * Notes:
   *	When a deferred error is detected the current command has

@@ -278,7 +278,7 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
 
  	case ABORTED_COMMAND:
  		if (sshdr.asc == 0x10) /* DIF */
-			return SUCCESS;
+			return SOFT_ERROR;
 
  		return NEEDS_RETRY;
  	case NOT_READY:

Quoted 45 lines · Hannes Reinecke

Consider a scenario where a SCSI EH command like TUR fails with a NOT READY
status - MANUAL INTERVENTION REQUIRED (02/04/03). As evident in the ASC/ASCQ
description, manual intervention is required in this case i.e. the target wants
TUR to fail here so that the host administrator can take corrective action.

But this particular ASC/ASCQ is not handled in the scsi_check_sense, which
ultimately causes scsi_eh_tur to erroneously report a SUCCESS (device ready)
instead of the actual FAILURE state (device not ready).

This patch converts scsi_check_sense() to return SOFT_ERROR in cases where an error has been signalled via the sense code, but we should not wake the error handler. This error code will be reverted back to SUCCESS for normal command handling, but scsi_eh_completed_normally() will convert it to FAILED.

Reported-by Martin George<[email protected]>

Signed-off-by Hannes Reinecke<[email protected]>

drivers/scsi/scsi_error.c +1 -1

index 45c7564..e86e62e 100644
@@ -223,7 +223,7 @@ static inline void scsi_eh_prt_fail_stats(struct
 Scsi_Host *shost,
   * @scmd:    Cmd to have sense checked.
   *
   * Return value:
- *     SUCCESS or FAILED or NEEDS_RETRY
+ *     SUCCESS or FAILED or NEEDS_RETRY or SOFT_ERROR
   *
   * Notes:

* When a deferred error is detected the current command has

drivers/scsi/scsi_error.c +1 -1

@@ -278,7 +278,7 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
 
      case ABORTED_COMMAND:
          if (sshdr.asc == 0x10) /* DIF */
-            return SUCCESS;
+            return SOFT_ERROR;
 

drivers/scsi/scsi_error.c static int scsi_check_sense(struct scsi_cmnd *scmd) · line 328

Quoted 9 lines

drivers/scsi/scsi_error.c +1 -1

          return NEEDS_RETRY;
      case NOT_READY:

@@ -324,19 +324,19 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
           * Pass the UA upwards for a determination in the completion
           * functions.
           */
-        return SUCCESS;
+        return SOFT_ERROR;
 

For normal IO execution the UA, not ready, and illegal request handling in scsi_io_completion would determine that some of these are retryable conditions, but with the patch in the scsi_error.cs path we will return soft error and fail. Also a lot of these that were retryable were returned as success in the past, but with the patch are being failed.

Martin made me the same bugzilla :) I was thinking we needed to make check_sense smarter or move some of the scsi_io_completion code to some lib helpers so scsi_error.c could call them.

And does scsi_eh_stu need to be covnerted to check for soft error and failed.

Also if we did this patch, then I think we also have to convert the device handlers.

Quoted 46 lines · Mike Christie

Quoted 45 lines · Hannes Reinecke

Consider a scenario where a SCSI EH command like TUR fails with a NOT READY
status - MANUAL INTERVENTION REQUIRED (02/04/03). As evident in the ASC/ASCQ
description, manual intervention is required in this case i.e. the target wants
TUR to fail here so that the host administrator can take corrective action.

But this particular ASC/ASCQ is not handled in the scsi_check_sense, which
ultimately causes scsi_eh_tur to erroneously report a SUCCESS (device ready)
instead of the actual FAILURE state (device not ready).

This patch converts scsi_check_sense() to return SOFT_ERROR in cases where an error has been signalled via the sense code, but we should not wake the error handler. This error code will be reverted back to SUCCESS for normal command handling, but scsi_eh_completed_normally() will convert it to FAILED.

Reported-by Martin George<[email protected]>

Signed-off-by Hannes Reinecke<[email protected]>

drivers/scsi/scsi_error.c +1 -1

index 45c7564..e86e62e 100644
@@ -223,7 +223,7 @@ static inline void scsi_eh_prt_fail_stats(struct
 Scsi_Host *shost,
    * @scmd:    Cmd to have sense checked.
    *
    * Return value:
- *     SUCCESS or FAILED or NEEDS_RETRY
+ *     SUCCESS or FAILED or NEEDS_RETRY or SOFT_ERROR
    *
    * Notes:

* When a deferred error is detected the current command has

drivers/scsi/scsi_error.c +1 -1

@@ -278,7 +278,7 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
 
       case ABORTED_COMMAND:
           if (sshdr.asc == 0x10) /* DIF */
-            return SUCCESS;
+            return SOFT_ERROR;
 

Quoted 17 lines

drivers/scsi/scsi_error.c static int scsi_check_sense(struct scsi_cmnd *scmd) · line 328

Quoted 9 lines

drivers/scsi/scsi_error.c +1 -1

           return NEEDS_RETRY;
       case NOT_READY:

@@ -324,19 +324,19 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
            * Pass the UA upwards for a determination in the completion
            * functions.
            */
-        return SUCCESS;
+        return SOFT_ERROR;
 

For normal IO execution the UA, not ready, and illegal request handling in scsi_io_completion would determine that some of these are retryable conditions, but with the patch in the scsi_error.cs path we will return soft error and fail. Also a lot of these that were retryable were returned as success in the past, but with the patch are being failed.

No. scsi_check_sense() is not called from scsi_io_completion; that

just calls scsi_normalize_sense().
scsi_check_sense() is only called from

- scsi_eh_completed_normally()
- scsi_eh_stu()
- scsi_decide_disposition()

All of which have been checked against this patch. scsi_io_completion() has it's own set of checks for sense codes which I don't modify here.

Quoted 5 lines

Martin made me the same bugzilla :) I was thinking we needed to make check_sense smarter or move some of the scsi_io_completion code to some lib helpers so scsi_error.c could call them.

Quoted 3 lines

And does scsi_eh_stu need to be covnerted to check for soft error and failed.

No. scsi_eh_stu() needs to check if a START STOP UNIT command needs to be send. The trigger for this is that scsi_check_sense() returns FAILED:

Quoted 3 lines

Also if we did this patch, then I think we also have to convert the device handlers.

Not necessarily. This patch just allows check_sense() to return an additional error code; the device handlers are not forced to use them. If they don't things will work like before.

For example, with your patch if for the scsi_send_eh_cmnd/scsi_eh_completed_normally/scsi_check_sense path we got 02/04/01 (not ready - becomming ready), scsi_send_eh_cmnd would see SOFT_ERROR and fail the scsi eh. And, I am saying don't we want retry this like we would if we were going through the normal IO path, so we do not offline devices on this type of failure?

Quoted 7 lines · Mike Christie

For example, with your patch if for the scsi_send_eh_cmnd/scsi_eh_completed_normally/scsi_check_sense path we got 02/04/01 (not ready - becomming ready), scsi_send_eh_cmnd would see SOFT_ERROR and fail the scsi eh. And, I am saying don't we want retry this like we would if we were going through the normal IO path, so we do not offline devices on this type of failure?

Just some corrections/clarifications. The device, target and bus reset handling are ok, but just host reset handling seems like the problem (at least the problem I was hitting I think). We do not call __scsi_report_device_reset for host reset handling, but some LLD eh_host_reset_handler implementations execute operations that can cause us to get something like a UA for the scsi eh TUR. For your patch do you also want to call __scsi_report_device_reset for the host reset handling for these drivers then?

Quoted 9 lines · Mike Christie

Quoted 7 lines · Mike Christie

For example, with your patch if for the scsi_send_eh_cmnd/scsi_eh_completed_normally/scsi_check_sense path we got 02/04/01 (not ready - becomming ready), scsi_send_eh_cmnd would see SOFT_ERROR and fail the scsi eh. And, I am saying don't we want retry this like we would if we were going through the normal IO path, so we do not offline devices on this type of failure?

Quoted 8 lines

Just some corrections/clarifications. The device, target and bus reset handling are ok, but just host reset handling seems like the problem (at least the problem I was hitting I think). We do not call __scsi_report_device_reset for host reset handling, but some LLD eh_host_reset_handler implementations execute operations that can cause us to get something like a UA for the scsi eh TUR. For your patch do you also want to call __scsi_report_device_reset for the host reset handling for these drivers then?

Ignore that. I see it is getting called for the host reset code. I am not sure what happened when I was testing it.

Quoted 2 lines

Ignore that. I see it is getting called for the host reset code. I am not sure what happened when I was testing it.

Just so I'm clear ... that's you withdrawing all objections to this patch, so I can put it in scsi-misc for testing?

Consider a scenario where a SCSI EH command like TUR fails with a NOT READY status - MANUAL INTERVENTION REQUIRED (02/04/03). As evident in the ASC/ASCQ description, manual intervention is required in this case i.e. the target wants TUR to fail here so that the host administrator can take corrective action.

But this particular ASC/ASCQ is not handled in the scsi_check_sense, which ultimately causes scsi_eh_tur to erroneously report a SUCCESS (device ready) instead of the actual FAILURE state (device not ready).

This patch converts scsi_check_sense() to return SOFT_ERROR in cases where an error has been signalled via the sense code, but we should not wake the error handler. This error code will be reverted back to SUCCESS for normal command handling, but scsi_eh_completed_normally() will convert it to FAILED.

Quoted 9 lines · Hannes Reinecke

Consider a scenario where a SCSI EH command like TUR fails with a NOT READY status - MANUAL INTERVENTION REQUIRED (02/04/03). As evident in the ASC/ASCQ description, manual intervention is required in this case i.e. the target wants TUR to fail here so that the host administrator can take corrective action.

But this particular ASC/ASCQ is not handled in the scsi_check_sense, which ultimately causes scsi_eh_tur to erroneously report a SUCCESS (device ready) instead of the actual FAILURE state (device not ready).

Quoted 5 lines

This patch converts scsi_check_sense() to return SOFT_ERROR in cases where an error has been signalled via the sense code, but we should not wake the error handler. This error code will be reverted back to SUCCESS for normal command handling, but scsi_eh_completed_normally() will convert it to FAILED.

I can't reconcile this patch with the detailed error handling ones: you change a lot of call sites to TARGET_ERROR in that patch and SOFT_ERROR in this one. Since the detailed error handling seems the more important, I'll drop this one ... can you work out a reconciliation, please?