| Message ID | 20231002155915.109359-8-hare@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: EH rework, main part | expand |
On 10/2/23 08:59, Hannes Reinecke wrote: > Streamline to use a similar code flow as the other reset functions. > > Signed-off-by: Hannes Reinecke <hare@suse.de> > --- > drivers/scsi/scsi_error.c | 26 +++++++++++--------------- > 1 file changed, 11 insertions(+), 15 deletions(-) > > diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c > index 21d84940c9cb..81b38f5da3b6 100644 > --- a/drivers/scsi/scsi_error.c > +++ b/drivers/scsi/scsi_error.c > @@ -1581,6 +1581,7 @@ static int scsi_eh_bus_device_reset(struct Scsi_Host *shost, > { > struct scsi_cmnd *scmd, *bdr_scmd, *next; > struct scsi_device *sdev; > + LIST_HEAD(check_list); > enum scsi_disposition rtn; > > shost_for_each_device(sdev, shost) { > @@ -1606,27 +1607,22 @@ static int scsi_eh_bus_device_reset(struct Scsi_Host *shost, > sdev_printk(KERN_INFO, sdev, > "%s: Sending BDR\n", current->comm)); > rtn = scsi_try_bus_device_reset(sdev); > - if (rtn == SUCCESS || rtn == FAST_IO_FAIL) { > - if (!scsi_device_online(sdev) || > - rtn == FAST_IO_FAIL || > - !scsi_eh_tur(bdr_scmd)) { > - list_for_each_entry_safe(scmd, next, > - work_q, eh_entry) { > - if (scmd->device == sdev && > - scsi_eh_action(scmd, rtn) != FAILED) > - __scsi_eh_finish_cmd(scmd, > - done_q, > - DID_RESET); > - } > - } > - } else { > + if (rtn != SUCCESS && rtn != FAST_IO_FAIL) > SCSI_LOG_ERROR_RECOVERY(3, > sdev_printk(KERN_INFO, sdev, > "%s: BDR failed\n", current->comm)); > + list_for_each_entry_safe(scmd, next, work_q, eh_entry) { > + if (scmd->device != sdev) > + continue; > + if (rtn == SUCCESS) > + list_move_tail(&scmd->eh_entry, &check_list); > + else if (rtn == FAST_IO_FAIL) > + __scsi_eh_finish_cmd(scmd, done_q, > + DID_TRANSPORT_DISRUPTED); > } > } > > - return list_empty(work_q); > + return scsi_eh_test_devices(&check_list, work_q, done_q, 0); > } I think the description of this patch is too brief. The following information is missing from the patch description: - Why the scsi_device_online() and scsi_eh_tur() checks have been left out. - Why DID_RESET has been changed into DID_TRANSPORT_DISRUPTED. - Why the list_move_tail() call has been introduced. - Why the scsi_eh_test_devices() call has been introduced. Thanks, Bart.
On 10/3/23 19:45, Bart Van Assche wrote: > On 10/2/23 08:59, Hannes Reinecke wrote: >> Streamline to use a similar code flow as the other reset functions. >> >> Signed-off-by: Hannes Reinecke <hare@suse.de> >> --- >> drivers/scsi/scsi_error.c | 26 +++++++++++--------------- >> 1 file changed, 11 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c >> index 21d84940c9cb..81b38f5da3b6 100644 >> --- a/drivers/scsi/scsi_error.c >> +++ b/drivers/scsi/scsi_error.c >> @@ -1581,6 +1581,7 @@ static int scsi_eh_bus_device_reset(struct >> Scsi_Host *shost, >> { >> struct scsi_cmnd *scmd, *bdr_scmd, *next; >> struct scsi_device *sdev; >> + LIST_HEAD(check_list); >> enum scsi_disposition rtn; >> shost_for_each_device(sdev, shost) { >> @@ -1606,27 +1607,22 @@ static int scsi_eh_bus_device_reset(struct >> Scsi_Host *shost, >> sdev_printk(KERN_INFO, sdev, >> "%s: Sending BDR\n", current->comm)); >> rtn = scsi_try_bus_device_reset(sdev); >> - if (rtn == SUCCESS || rtn == FAST_IO_FAIL) { >> - if (!scsi_device_online(sdev) || >> - rtn == FAST_IO_FAIL || >> - !scsi_eh_tur(bdr_scmd)) { >> - list_for_each_entry_safe(scmd, next, >> - work_q, eh_entry) { >> - if (scmd->device == sdev && >> - scsi_eh_action(scmd, rtn) != FAILED) >> - __scsi_eh_finish_cmd(scmd, >> - done_q, >> - DID_RESET); >> - } >> - } >> - } else { >> + if (rtn != SUCCESS && rtn != FAST_IO_FAIL) >> SCSI_LOG_ERROR_RECOVERY(3, >> sdev_printk(KERN_INFO, sdev, >> "%s: BDR failed\n", current->comm)); >> + list_for_each_entry_safe(scmd, next, work_q, eh_entry) { >> + if (scmd->device != sdev) >> + continue; >> + if (rtn == SUCCESS) >> + list_move_tail(&scmd->eh_entry, &check_list); >> + else if (rtn == FAST_IO_FAIL) >> + __scsi_eh_finish_cmd(scmd, done_q, >> + DID_TRANSPORT_DISRUPTED); >> } >> } >> - return list_empty(work_q); >> + return scsi_eh_test_devices(&check_list, work_q, done_q, 0); >> } > > I think the description of this patch is too brief. The following > information is missing from the patch description: > - Why the scsi_device_online() and scsi_eh_tur() checks have been left > out. > - Why DID_RESET has been changed into DID_TRANSPORT_DISRUPTED. > - Why the list_move_tail() call has been introduced. > - Why the scsi_eh_test_devices() call has been introduced. > Okay, will be doing so. Cheers, Hannes
On 2023/10/2 23:59, Hannes Reinecke wrote: > Streamline to use a similar code flow as the other reset functions. > > Signed-off-by: Hannes Reinecke <hare@suse.de> > --- > drivers/scsi/scsi_error.c | 26 +++++++++++--------------- > 1 file changed, 11 insertions(+), 15 deletions(-) > > diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c > index 21d84940c9cb..81b38f5da3b6 100644 > --- a/drivers/scsi/scsi_error.c > +++ b/drivers/scsi/scsi_error.c > @@ -1581,6 +1581,7 @@ static int scsi_eh_bus_device_reset(struct Scsi_Host *shost, > { > struct scsi_cmnd *scmd, *bdr_scmd, *next; > struct scsi_device *sdev; > + LIST_HEAD(check_list); > enum scsi_disposition rtn; > > shost_for_each_device(sdev, shost) { > @@ -1606,27 +1607,22 @@ static int scsi_eh_bus_device_reset(struct Scsi_Host *shost, > sdev_printk(KERN_INFO, sdev, > "%s: Sending BDR\n", current->comm)); > rtn = scsi_try_bus_device_reset(sdev); > - if (rtn == SUCCESS || rtn == FAST_IO_FAIL) { > - if (!scsi_device_online(sdev) || > - rtn == FAST_IO_FAIL || > - !scsi_eh_tur(bdr_scmd)) { > - list_for_each_entry_safe(scmd, next, > - work_q, eh_entry) { > - if (scmd->device == sdev && > - scsi_eh_action(scmd, rtn) != FAILED) > - __scsi_eh_finish_cmd(scmd, > - done_q, > - DID_RESET); > - } > - } > - } else { > + if (rtn != SUCCESS && rtn != FAST_IO_FAIL) > SCSI_LOG_ERROR_RECOVERY(3, > sdev_printk(KERN_INFO, sdev, > "%s: BDR failed\n", current->comm)); > + list_for_each_entry_safe(scmd, next, work_q, eh_entry) { > + if (scmd->device != sdev) > + continue; > + if (rtn == SUCCESS) > + list_move_tail(&scmd->eh_entry, &check_list); > + else if (rtn == FAST_IO_FAIL) > + __scsi_eh_finish_cmd(scmd, done_q, > + DID_TRANSPORT_DISRUPTED); > } > } > > - return list_empty(work_q); > + return scsi_eh_test_devices(&check_list, work_q, done_q, 0); > } > > /** I think the bdr_scmd related lines should also be removed like following: diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c index 81b38f5da3b6..e3b0ac270dd9 100644 --- a/drivers/scsi/scsi_error.c +++ b/drivers/scsi/scsi_error.c @@ -1579,7 +1579,7 @@ static int scsi_eh_bus_device_reset(struct Scsi_Host *shost, struct list_head *work_q, struct list_head *done_q) { - struct scsi_cmnd *scmd, *bdr_scmd, *next; + struct scsi_cmnd *scmd, *next; struct scsi_device *sdev; LIST_HEAD(check_list); enum scsi_disposition rtn; @@ -1593,15 +1593,6 @@ static int scsi_eh_bus_device_reset(struct Scsi_Host *shost, scsi_device_put(sdev); break; } - bdr_scmd = NULL; - list_for_each_entry(scmd, work_q, eh_entry) - if (scmd->device == sdev) { - bdr_scmd = scmd; - break; - } - - if (!bdr_scmd) - continue; SCSI_LOG_ERROR_RECOVERY(3, sdev_printk(KERN_INFO, sdev,
On 2023/10/12 15:48, Wenchao Hao wrote: > On 2023/10/2 23:59, Hannes Reinecke wrote: >> Streamline to use a similar code flow as the other reset functions. >> >> Signed-off-by: Hannes Reinecke <hare@suse.de> >> --- >> drivers/scsi/scsi_error.c | 26 +++++++++++--------------- >> 1 file changed, 11 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c >> index 21d84940c9cb..81b38f5da3b6 100644 >> --- a/drivers/scsi/scsi_error.c >> +++ b/drivers/scsi/scsi_error.c >> @@ -1581,6 +1581,7 @@ static int scsi_eh_bus_device_reset(struct Scsi_Host *shost, >> { >> struct scsi_cmnd *scmd, *bdr_scmd, *next; >> struct scsi_device *sdev; >> + LIST_HEAD(check_list); >> enum scsi_disposition rtn; >> shost_for_each_device(sdev, shost) { >> @@ -1606,27 +1607,22 @@ static int scsi_eh_bus_device_reset(struct Scsi_Host *shost, >> sdev_printk(KERN_INFO, sdev, >> "%s: Sending BDR\n", current->comm)); >> rtn = scsi_try_bus_device_reset(sdev); >> - if (rtn == SUCCESS || rtn == FAST_IO_FAIL) { >> - if (!scsi_device_online(sdev) || >> - rtn == FAST_IO_FAIL || >> - !scsi_eh_tur(bdr_scmd)) { >> - list_for_each_entry_safe(scmd, next, >> - work_q, eh_entry) { >> - if (scmd->device == sdev && >> - scsi_eh_action(scmd, rtn) != FAILED) >> - __scsi_eh_finish_cmd(scmd, >> - done_q, >> - DID_RESET); >> - } >> - } >> - } else { >> + if (rtn != SUCCESS && rtn != FAST_IO_FAIL) >> SCSI_LOG_ERROR_RECOVERY(3, >> sdev_printk(KERN_INFO, sdev, >> "%s: BDR failed\n", current->comm)); >> + list_for_each_entry_safe(scmd, next, work_q, eh_entry) { >> + if (scmd->device != sdev) >> + continue; >> + if (rtn == SUCCESS) >> + list_move_tail(&scmd->eh_entry, &check_list); >> + else if (rtn == FAST_IO_FAIL) >> + __scsi_eh_finish_cmd(scmd, done_q, >> + DID_TRANSPORT_DISRUPTED); >> } >> } >> - return list_empty(work_q); >> + return scsi_eh_test_devices(&check_list, work_q, done_q, 0); >> } >> /** > > I think the bdr_scmd related lines should also be removed like following: > > diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c > index 81b38f5da3b6..e3b0ac270dd9 100644 > --- a/drivers/scsi/scsi_error.c > +++ b/drivers/scsi/scsi_error.c > @@ -1579,7 +1579,7 @@ static int scsi_eh_bus_device_reset(struct Scsi_Host *shost, > struct list_head *work_q, > struct list_head *done_q) > { > - struct scsi_cmnd *scmd, *bdr_scmd, *next; > + struct scsi_cmnd *scmd, *next; > struct scsi_device *sdev; > LIST_HEAD(check_list); > enum scsi_disposition rtn; > @@ -1593,15 +1593,6 @@ static int scsi_eh_bus_device_reset(struct Scsi_Host *shost, > scsi_device_put(sdev); > break; > } > - bdr_scmd = NULL; > - list_for_each_entry(scmd, work_q, eh_entry) > - if (scmd->device == sdev) { > - bdr_scmd = scmd; > - break; > - } > - > - if (!bdr_scmd) > - continue; > > SCSI_LOG_ERROR_RECOVERY(3, > sdev_printk(KERN_INFO, sdev, > > > > > Sorry, please ignore this message. If remove these lines we would reset device with no failed command.
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c index 21d84940c9cb..81b38f5da3b6 100644 --- a/drivers/scsi/scsi_error.c +++ b/drivers/scsi/scsi_error.c @@ -1581,6 +1581,7 @@ static int scsi_eh_bus_device_reset(struct Scsi_Host *shost, { struct scsi_cmnd *scmd, *bdr_scmd, *next; struct scsi_device *sdev; + LIST_HEAD(check_list); enum scsi_disposition rtn; shost_for_each_device(sdev, shost) { @@ -1606,27 +1607,22 @@ static int scsi_eh_bus_device_reset(struct Scsi_Host *shost, sdev_printk(KERN_INFO, sdev, "%s: Sending BDR\n", current->comm)); rtn = scsi_try_bus_device_reset(sdev); - if (rtn == SUCCESS || rtn == FAST_IO_FAIL) { - if (!scsi_device_online(sdev) || - rtn == FAST_IO_FAIL || - !scsi_eh_tur(bdr_scmd)) { - list_for_each_entry_safe(scmd, next, - work_q, eh_entry) { - if (scmd->device == sdev && - scsi_eh_action(scmd, rtn) != FAILED) - __scsi_eh_finish_cmd(scmd, - done_q, - DID_RESET); - } - } - } else { + if (rtn != SUCCESS && rtn != FAST_IO_FAIL) SCSI_LOG_ERROR_RECOVERY(3, sdev_printk(KERN_INFO, sdev, "%s: BDR failed\n", current->comm)); + list_for_each_entry_safe(scmd, next, work_q, eh_entry) { + if (scmd->device != sdev) + continue; + if (rtn == SUCCESS) + list_move_tail(&scmd->eh_entry, &check_list); + else if (rtn == FAST_IO_FAIL) + __scsi_eh_finish_cmd(scmd, done_q, + DID_TRANSPORT_DISRUPTED); } } - return list_empty(work_q); + return scsi_eh_test_devices(&check_list, work_q, done_q, 0); } /**
Streamline to use a similar code flow as the other reset functions. Signed-off-by: Hannes Reinecke <hare@suse.de> --- drivers/scsi/scsi_error.c | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-)