diff mbox series

scsi: storvsc: Do not flag MAINTENANCE_IN return of SRB_STATUS_DATA_OVERRUN as an error

Message ID 20241122162355.2859481-1-cavery@redhat.com (mailing list archive)
State New
Headers show
Series scsi: storvsc: Do not flag MAINTENANCE_IN return of SRB_STATUS_DATA_OVERRUN as an error | expand

Commit Message

Cathy Avery Nov. 22, 2024, 4:23 p.m. UTC 
This patch partially reverts

	commit 812fe6420a6e789db68f18cdb25c5c89f4561334
	Author: Michael Kelley <mikelley@microsoft.com>
	Date:   Fri Aug 25 10:21:24 2023 -0700

	scsi: storvsc: Handle additional SRB status values

HyperV does not support MAINTENANCE_IN resulting in FC passthrough
returning the SRB_STATUS_DATA_OVERRUN value. Now that SRB_STATUS_DATA_OVERRUN
is treated as an error multipath ALUA paths go into a faulty state as multipath
ALUA submits RTPG commands via MAINTENANCE_IN.

[    3.215560] hv_storvsc 1d69d403-9692-4460-89f9-a8cbcc0f94f3:
tag#230 cmd 0xa3 status: scsi 0x0 srb 0x12 hv 0xc0000001
[    3.215572] scsi 1:0:0:32: alua: rtpg failed, result 458752

This patch essentially restores the previous handling of MAINTENANCE_IN
along with supressing any logging noise.

Signed-off-by: Cathy Avery <cavery@redhat.com>
---
 drivers/scsi/storvsc_drv.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Michael Kelley Nov. 22, 2024, 6:11 p.m. UTC | #1 
From: Cathy Avery <cavery@redhat.com> Sent: Friday, November 22, 2024 8:24 AM
> 
> This patch partially reverts
> 
> 	commit 812fe6420a6e789db68f18cdb25c5c89f4561334
> 	Author: Michael Kelley <mikelley@microsoft.com>
> 	Date:   Fri Aug 25 10:21:24 2023 -0700
> 
> 	scsi: storvsc: Handle additional SRB status values
> 
> HyperV does not support MAINTENANCE_IN resulting in FC passthrough
> returning the SRB_STATUS_DATA_OVERRUN value. Now that
> SRB_STATUS_DATA_OVERRUN
> is treated as an error multipath ALUA paths go into a faulty state as multipath
> ALUA submits RTPG commands via MAINTENANCE_IN.
> 
> [    3.215560] hv_storvsc 1d69d403-9692-4460-89f9-a8cbcc0f94f3:
> tag#230 cmd 0xa3 status: scsi 0x0 srb 0x12 hv 0xc0000001
> [    3.215572] scsi 1:0:0:32: alua: rtpg failed, result 458752
> 
> This patch essentially restores the previous handling of MAINTENANCE_IN
> along with supressing any logging noise.
> 
> Signed-off-by: Cathy Avery <cavery@redhat.com>
> ---
>  drivers/scsi/storvsc_drv.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 7ceb982040a5..088fefe40999 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -149,6 +149,8 @@ struct hv_fc_wwn_packet {
>  */
>  static int vmstor_proto_version;
> 
> +static bool hv_dev_is_fc(struct hv_device *hv_dev);
> +
>  #define STORVSC_LOGGING_NONE	0
>  #define STORVSC_LOGGING_ERROR	1
>  #define STORVSC_LOGGING_WARN	2
> @@ -980,6 +982,13 @@ static void storvsc_handle_error(struct vmscsi_request
> *vm_srb,
>  	void (*process_err_fn)(struct work_struct *work);
>  	struct hv_host_device *host_dev = shost_priv(host);
> 
> +	// HyperV FC does not support MAINTENANCE_IN ignore
> +	if ((scmnd->cmnd[0] == MAINTENANCE_IN) &&
> +		(SRB_STATUS(vm_srb->srb_status) == SRB_STATUS_DATA_OVERRUN) &&
> +		hv_dev_is_fc(host_dev->dev)) {
> +		return;
> +	}
> +

Could use the following coding instead to avoid the explicit check of srb_status:

@@ -981,6 +981,16 @@ static void storvsc_handle_error(struct vmscsi_request *vm_srb,
        struct hv_host_device *host_dev = shost_priv(host);

        switch (SRB_STATUS(vm_srb->srb_status)) {
+       case SRB_STATUS_DATA_OVERRUN:
+               /*
+                * Hyper-V does not support MAINTENANCE_IN, resulting in FC
+                * passthrough returning DATA_OVERRUN. But treating as an
+                * error incorrectly puts ALUA paths into a fault state.
+                */
+               if ((scmnd->cmnd[0] == MAINTENANCE_IN) &&
+                               hv_dev_is_fc(host_dev->dev))
+                       return;
+               fallthrough;
        case SRB_STATUS_ERROR:
        case SRB_STATUS_ABORTED:
        case SRB_STATUS_INVALID_REQUEST:
@@ -988,7 +998,6 @@ static void storvsc_handle_error(struct vmscsi_request *vm_srb,
        case SRB_STATUS_TIMEOUT:
        case SRB_STATUS_SELECTION_TIMEOUT:
        case SRB_STATUS_BUS_RESET:
-       case SRB_STATUS_DATA_OVERRUN:
                if (vm_srb->srb_status & SRB_STATUS_AUTOSENSE_VALID) {
                        /* Check for capacity change */

>  	switch (SRB_STATUS(vm_srb->srb_status)) {
>  	case SRB_STATUS_ERROR:
>  	case SRB_STATUS_ABORTED:
> @@ -1161,6 +1170,12 @@ static void storvsc_on_io_completion(struct storvsc_device *stor_device,
>  	stor_pkt->vm_srb.sense_info_length = min_t(u8, STORVSC_SENSE_BUFFER_SIZE,
>  		vstor_packet->vm_srb.sense_info_length);
> 
> +	// HyperV FC does not support MAINTENANCE_IN ignore

For consistency, prefer to not use C++ style comments.

> +	if ((SRB_STATUS(vstor_packet->vm_srb.srb_status) == SRB_STATUS_DATA_OVERRUN) &&
> +		(stor_pkt->vm_srb.cdb[0] == MAINTENANCE_IN) &&
> +		hv_dev_is_fc(device))
> +		goto skip_logging;
> +

Just wondering:  There's already a hack earlier in this function for
INQUIRY and MODE_SENSE commands that sets scsi_status and
srb_status to indicate success. What if this case did the same? Then
nothing would be logged, and a special case would not be needed in
storvsc_handle_error().  Or do you still need the error and sense info to
propagate to higher levels?  (in which case what you've done here is OK)

Michael

>  	if (vstor_packet->vm_srb.scsi_status != 0 ||
>  	    vstor_packet->vm_srb.srb_status != SRB_STATUS_SUCCESS) {
> 
> @@ -1181,6 +1196,7 @@ static void storvsc_on_io_completion(struct storvsc_device *stor_device,
>  			vstor_packet->status);
>  	}
> 
> +skip_logging:
>  	if (vstor_packet->vm_srb.scsi_status == SAM_STAT_CHECK_CONDITION &&
>  	    (vstor_packet->vm_srb.srb_status & SRB_STATUS_AUTOSENSE_VALID))
>  		memcpy(request->cmd->sense_buffer,
> --
> 2.42.0
diff mbox series

Patch

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 7ceb982040a5..088fefe40999 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -149,6 +149,8 @@  struct hv_fc_wwn_packet {
 */
 static int vmstor_proto_version;
 
+static bool hv_dev_is_fc(struct hv_device *hv_dev);
+
 #define STORVSC_LOGGING_NONE	0
 #define STORVSC_LOGGING_ERROR	1
 #define STORVSC_LOGGING_WARN	2
@@ -980,6 +982,13 @@  static void storvsc_handle_error(struct vmscsi_request *vm_srb,
 	void (*process_err_fn)(struct work_struct *work);
 	struct hv_host_device *host_dev = shost_priv(host);
 
+	// HyperV FC does not support MAINTENANCE_IN ignore
+	if ((scmnd->cmnd[0] == MAINTENANCE_IN) &&
+		(SRB_STATUS(vm_srb->srb_status) == SRB_STATUS_DATA_OVERRUN) &&
+		hv_dev_is_fc(host_dev->dev)) {
+		return;
+	}
+
 	switch (SRB_STATUS(vm_srb->srb_status)) {
 	case SRB_STATUS_ERROR:
 	case SRB_STATUS_ABORTED:
@@ -1161,6 +1170,12 @@  static void storvsc_on_io_completion(struct storvsc_device *stor_device,
 	stor_pkt->vm_srb.sense_info_length = min_t(u8, STORVSC_SENSE_BUFFER_SIZE,
 		vstor_packet->vm_srb.sense_info_length);
 
+	// HyperV FC does not support MAINTENANCE_IN ignore
+	if ((SRB_STATUS(vstor_packet->vm_srb.srb_status) == SRB_STATUS_DATA_OVERRUN) &&
+		(stor_pkt->vm_srb.cdb[0] == MAINTENANCE_IN) &&
+		hv_dev_is_fc(device))
+		goto skip_logging;
+
 	if (vstor_packet->vm_srb.scsi_status != 0 ||
 	    vstor_packet->vm_srb.srb_status != SRB_STATUS_SUCCESS) {
 
@@ -1181,6 +1196,7 @@  static void storvsc_on_io_completion(struct storvsc_device *stor_device,
 			vstor_packet->status);
 	}
 
+skip_logging:
 	if (vstor_packet->vm_srb.scsi_status == SAM_STAT_CHECK_CONDITION &&
 	    (vstor_packet->vm_srb.srb_status & SRB_STATUS_AUTOSENSE_VALID))
 		memcpy(request->cmd->sense_buffer,