| 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 |
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
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
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,
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(+)