Re: [PATCH] scsi: storvsc: Do not flag MAINTENANCE_IN return of SRB_STATUS_DATA_OVERRUN as an error

[Cathy Avery <cavery@redhat.com>]

On 11/22/24 13:11, Michael Kelley wrote:
> 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
>
It sounds like a reasonable idea. I'll try it out.

Cathy

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