On 8/17/21 11:14 AM, Hannes Reinecke wrote:
The target reset function should only depend on the scsi target,
not the scsi command.
Signed-off-by: Hannes Reinecke <hare@xxxxxxxx> > ---
Documentation/scsi/scsi_eh.rst | 10 ++++
Documentation/scsi/scsi_mid_low_api.rst | 18 +++++++
drivers/s390/scsi/zfcp_scsi.c | 7 ++-
drivers/scsi/scsi_error.c | 5 +-
include/scsi/scsi_host.h | 2 +-
34 files changed, 226 insertions(+), 192 deletions(-)
diff --git a/Documentation/scsi/scsi_eh.rst b/Documentation/scsi/scsi_eh.rst
index cf0649e0c3a9..e09c81a54702 100644
--- a/Documentation/scsi/scsi_eh.rst
+++ b/Documentation/scsi/scsi_eh.rst
@@ -215,6 +215,7 @@ considered to fail always.
int (* eh_abort_handler)(struct scsi_cmnd *);
int (* eh_device_reset_handler)(struct scsi_cmnd *);
+ int (* eh_target_reset_handler)(struct scsi_target *);
int (* eh_bus_reset_handler)(struct Scsi_Host *, int);
int (* eh_host_reset_handler)(struct Scsi_Host *);
@@ -410,6 +411,15 @@ scmd->allowed.
2. If !list_empty(&eh_work_q), invoke scsi_eh_bus_device_reset().
``scsi_eh_bus_device_reset``
This action is very similar to scsi_eh_stu() except that,
instead of issuing STU, hostt->eh_device_reset_handler()
is used. Also, as we're not issuing SCSI commands and
resetting clears all scmds on the sdev, there is no need
to choose error-completed scmds.
+ 2. If !list_empty(&eh_work_q), invoke scsi_eh_target_reset().
I think we have item 2 twice now, as this is preded by BDR as step 2.
Should target reset be the newly inserted step 3 and subsequent ones be
incremented by 1?
+
+ ``scsi_eh_target_reset``
+
+ hostt->eh_target_reset_handler() is invoked for each target
+ with failed scmds. If bus reset succeeds, all failed
bus reset => target reset
(I reckon you copy&pasted from scsi_eh_bus_reset ;-))
+ scmds on all ready or offline sdevs on the target are
+ EH-finished.
+
3. If !list_empty(&eh_work_q), invoke scsi_eh_bus_reset()
``scsi_eh_bus_reset``
hostt->eh_bus_reset_handler() is invoked for each channel
with failed scmds. If bus reset succeeds, all failed
scmds on all ready or offline sdevs on the channel are
EH-finished.
4. If !list_empty(&eh_work_q), invoke scsi_eh_host_reset()
``scsi_eh_host_reset``
This is the last resort. hostt->eh_host_reset_handler()
is invoked. If host reset succeeds, all failed scmds on
all ready or offline sdevs on the host are EH-finished.
5. If !list_empty(&eh_work_q), invoke scsi_eh_offline_sdevs()
``scsi_eh_offline_sdevs``
Take all sdevs which still have unrecovered scmds offline
and EH-finish the scmds.
diff --git a/Documentation/scsi/scsi_mid_low_api.rst b/Documentation/scsi/scsi_mid_low_api.rst
index 1b1c37445580..0afc1b4f89af 100644
--- a/Documentation/scsi/scsi_mid_low_api.rst
+++ b/Documentation/scsi/scsi_mid_low_api.rst
Missing an entry in the Summary section preceding the Details?:
Summary:
- bios_param - fetch head, sector, cylinder info for a disk
- eh_timed_out - notify the host that a command timer expired
- eh_abort_handler - abort given command
- eh_bus_reset_handler - issue SCSI bus reset
- eh_device_reset_handler - issue SCSI device reset
- eh_host_reset_handler - reset host (host bus adapter)
+ - eh_target_reset_handler - issue SCSI target reset
@@ -758,6 +758,24 @@ Details::
int eh_bus_reset_handler(struct Scsi_Host * host, int channel)
+ /**
+ * eh_target_reset_handler - issue SCSI target reset
+ * @starget: identifies SCSI target to be reset
+ *
+ * Returns SUCCESS if command aborted else FAILED
+ *
+ * Locks: None held
+ *
+ * Calling context: kernel thread
+ *
+ * Notes: Invoked from scsi_eh thread. No other commands will be
+ * queued on current host during eh.
+ *
+ * Optionally defined in: LLD
+ **/
+ int eh_target_reset_handler(struct scsi_target * starget)
+
+
nit-picky: It looks like this file had the reset_handlers sorted alphabetically
(rather than semantically in order of eh escalation) both in its sections
"Summary" and "Details". So the eh_target_reset_handler would go after
eh_host_reset_handler? (also to match order in Summary)
/**
* eh_device_reset_handler - issue SCSI device reset
* @scp: identifies SCSI device to be reset
In fact, the Documentation changes look like a different patch to me, namely
adding missing information about the target reset escalation step. That seems
independent of this patch which changes the callback signature.
diff --git a/drivers/s390/scsi/zfcp_scsi.c b/drivers/s390/scsi/zfcp_scsi.c
index 8bfa8ffd9ff6..6492c3b1b12f 100644
--- a/drivers/s390/scsi/zfcp_scsi.c
+++ b/drivers/s390/scsi/zfcp_scsi.c
@@ -340,9 +340,12 @@ static int zfcp_scsi_eh_device_reset_handler(struct scsi_cmnd *scpnt)
return zfcp_scsi_task_mgmt_function(sdev, FCP_TMF_LUN_RESET);
}
-static int zfcp_scsi_eh_target_reset_handler(struct scsi_cmnd *scpnt)
+/*
+ * Note: We need to select a LUN as the storage array doesn't
+ * necessarily supports LUN 0 and might refuse the target reset.
+ */
While the storage itself might be another condition, there is the following
necessary pre-condition about the FCP channel:
v4.18 commit 674595d8519f ("scsi: zfcp: decouple our scsi_eh callbacks from
scsi_cmnd")
zfcp_scsi_eh_target_reset_handler() is special: The FCP channel requires a
valid LUN handle so we try to find ourselves a stand-in scsi_device as
suggested by Hannes Reinecke. If it cannot find a stand-in scsi device,
trace a record
(cf. v4.10 commit 6f2ce1c6af37 ("scsi: zfcp: fix rport unblock race with LUN
recovery") I mentioned in the reply to patch 08/51)
If you think that commit description is insufficient, you could borrow text
from there for the code comment.
+static int zfcp_scsi_eh_target_reset_handler(struct scsi_target *starget)
{
- struct scsi_target *starget = scsi_target(scpnt->device);
struct fc_rport *rport = starget_to_rport(starget);
struct Scsi_Host *shost = rport_to_shost(rport);
struct scsi_device *sdev = NULL, *tmp_sdev;
The code change looks good.
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index d5e36488c87b..1d8e2f655833 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -876,14 +876,15 @@ static enum scsi_disposition scsi_try_target_reset(struct scsi_cmnd *scmd)
enum scsi_disposition rtn;
struct Scsi_Host *host = scmd->device->host;
struct scsi_host_template *hostt = host->hostt;
+ struct scsi_target *starget = scsi_target(scmd->device);
if (!hostt->eh_target_reset_handler)
return FAILED;
- rtn = hostt->eh_target_reset_handler(scmd);
+ rtn = hostt->eh_target_reset_handler(starget);
if (rtn == SUCCESS) {
spin_lock_irqsave(host->host_lock, flags);
- __starget_for_each_device(scsi_target(scmd->device), NULL,
+ __starget_for_each_device(starget, NULL,
__scsi_report_device_reset);
spin_unlock_irqrestore(host->host_lock, flags);
}
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index e0a102339317..8fcced0d98bd 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -140,7 +140,7 @@ struct scsi_host_template {
*/
int (* eh_abort_handler)(struct scsi_cmnd *);
int (* eh_device_reset_handler)(struct scsi_cmnd *);
- int (* eh_target_reset_handler)(struct scsi_cmnd *);
+ int (* eh_target_reset_handler)(struct scsi_target *);
int (* eh_bus_reset_handler)(struct Scsi_Host *, int);
int (* eh_host_reset_handler)(struct Scsi_Host *);
--
Mit freundlichen Gruessen / Kind regards
Steffen Maier
Linux on IBM Z Development
https://www.ibm.com/privacy/us/en/
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
