SCSI EH cleanup

Subject: [PATCHv3 1/6] scsi_error: count medium access timeout only once per EH run

The current medium access timeout counter will be increased for each command, so if there are enough failed commands we'll hit

the medium access timeout for even a single failure. Fix this by making the timeout per EH run, ie the counter will only be increased once per device and EH run.

Cc Ewan Milne <[email protected]>

Cc Lawrence Obermann <[email protected]>

Cc Benjamin Block <[email protected]>

Cc Steffen Maier <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/scsi_error.c  | 16 +++++++++++++++-
 drivers/scsi/sd.c          | 21 +++++++++++++++++----
 drivers/scsi/sd.h          |  1 +
 include/scsi/scsi_driver.h |  2 +-
 4 files changed, 34 insertions(+), 6 deletions(-)

drivers/scsi/scsi_error.c +15 -1

index f2cafae..cec439c 100644
@@ -58,6 +58,7 @@
 static int scsi_eh_try_stu(struct scsi_cmnd *scmd);
 static int scsi_try_to_abort_cmd(struct scsi_host_template *,
 				 struct scsi_cmnd *);
+static int scsi_eh_reset(struct scsi_cmnd *scmd);
 
 /* called with shost->host_lock held */
 void scsi_eh_wakeup(struct Scsi_Host *shost)

@@ -249,6 +250,7 @@ int scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
 	if (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED)
 		eh_flag &= ~SCSI_EH_CANCEL_CMD;
 	scmd->eh_eflags |= eh_flag;
+	scsi_eh_reset(scmd);
 	list_add_tail(&scmd->eh_entry, &shost->eh_cmd_q);
 	shost->host_failed++;
 	scsi_eh_wakeup(shost);

@@ -1107,7 +1109,19 @@ static int scsi_eh_action(struct scsi_cmnd *scmd, int rtn)
 	if (!blk_rq_is_passthrough(scmd->request)) {
 		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
 		if (sdrv->eh_action)
-			rtn = sdrv->eh_action(scmd, rtn);
+			rtn = sdrv->eh_action(scmd, rtn, false);
+	}
+	return rtn;
+}
+
+static int scsi_eh_reset(struct scsi_cmnd *scmd)
+{
+	int rtn = SUCCESS;
+
+	if (!blk_rq_is_passthrough(scmd->request)) {
+		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
+		if (sdrv->eh_action)
+			rtn = sdrv->eh_action(scmd, rtn, true);
 	}
 	return rtn;
 }

drivers/scsi/sd.c +17 -4

index c7839f6..c794686 100644
@@ -115,7 +115,7 @@
 static int sd_init_command(struct scsi_cmnd *SCpnt);
 static void sd_uninit_command(struct scsi_cmnd *SCpnt);
 static int sd_done(struct scsi_cmnd *);
-static int sd_eh_action(struct scsi_cmnd *, int);
+static int sd_eh_action(struct scsi_cmnd *, int, bool);
 static void sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer);
 static void scsi_disk_release(struct device *cdev);
 static void sd_print_sense_hdr(struct scsi_disk *, struct scsi_sense_hdr *);

@@ -1689,18 +1689,28 @@ static int sd_pr_clear(struct block_device *bdev, u64 key)
  *	sd_eh_action - error handling callback
  *	@scmd:		sd-issued command that has failed
  *	@eh_disp:	The recovery disposition suggested by the midlayer
+ *	@reset:		Reset medium access counter
  *
  *	This function is called by the SCSI midlayer upon completion of an
  *	error test command (currently TEST UNIT READY). The result of sending
  *	the eh command is passed in eh_disp.  We're looking for devices that
  *	fail medium access commands but are OK with non access commands like
  *	test unit ready (so wrongly see the device as having a successful
- *	recovery)
+ *	recovery).
+ *	We have to be careful to count a medium access failure only once
+ *	per SCSI EH run; there might be several timed out commands which
+ *	will cause the 'max_medium_access_timeouts' counter to trigger
+ *	after the first SCSI EH run already and set the device to offline.
  **/
-static int sd_eh_action(struct scsi_cmnd *scmd, int eh_disp)
+static int sd_eh_action(struct scsi_cmnd *scmd, int eh_disp, bool reset)
 {
 	struct scsi_disk *sdkp = scsi_disk(scmd->request->rq_disk);
 
+	if (reset) {
+		/* New SCSI EH run, reset gate variable */
+		sdkp->medium_access_reset = 0;
+		return eh_disp;
+	}
 	if (!scsi_device_online(scmd->device) ||
 	    !scsi_medium_access_command(scmd) ||
 	    host_byte(scmd->result) != DID_TIME_OUT ||

@@ -1714,7 +1724,10 @@ static int sd_eh_action(struct scsi_cmnd *scmd, int eh_disp)
 	 * process of recovering or has it suffered an internal failure
 	 * that prevents access to the storage medium.
 	 */
-	sdkp->medium_access_timed_out++;
+	if (!sdkp->medium_access_reset) {
+		sdkp->medium_access_timed_out++;
+		sdkp->medium_access_reset = 1;
+	}
 
 	/*
 	 * If the device keeps failing read/write commands but TEST UNIT

drivers/scsi/sd.h +1 -0

index 4dac35e..6a4f75a 100644
@@ -106,6 +106,7 @@ struct scsi_disk {
 	unsigned	rc_basis: 2;
 	unsigned	zoned: 2;
 	unsigned	urswrz : 1;
+	unsigned	medium_access_reset : 1;
 };
 #define to_scsi_disk(obj) container_of(obj,struct scsi_disk,dev)
 

include/scsi/scsi_driver.h +1 -1

index 891a658..d5e0012 100644
@@ -15,7 +15,7 @@ struct scsi_driver {
 	int (*init_command)(struct scsi_cmnd *);
 	void (*uninit_command)(struct scsi_cmnd *);
 	int (*done)(struct scsi_cmnd *);
-	int (*eh_action)(struct scsi_cmnd *, int);
+	int (*eh_action)(struct scsi_cmnd *, int, bool);
 };
 #define to_scsi_driver(drv) \
 	container_of((drv), struct scsi_driver, gendrv)

The current medium access timeout counter will be increased for each command, so if there are enough failed commands we'll hit the medium access timeout for even a single failure.

drivers/scsi/scsi_error.c +8 -1

index f2cafae..cec439c 100644
@@ -58,6 +58,7 @@
 static int scsi_eh_try_stu(struct scsi_cmnd *scmd);
 static int scsi_try_to_abort_cmd(struct scsi_host_template *,
 				 struct scsi_cmnd *);
+static int scsi_eh_reset(struct scsi_cmnd *scmd);
 
 /* called with shost->host_lock held */
 void scsi_eh_wakeup(struct Scsi_Host *shost)

@@ -249,6 +250,7 @@ int scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
 	if (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED)
 		eh_flag &= ~SCSI_EH_CANCEL_CMD;
 	scmd->eh_eflags |= eh_flag;
+	scsi_eh_reset(scmd);
 	list_add_tail(&scmd->eh_entry, &shost->eh_cmd_q);
 	shost->host_failed++;
 	scsi_eh_wakeup(shost);

@@ -1107,7 +1109,19 @@ static int scsi_eh_action(struct scsi_cmnd *scmd, int rtn)
 	if (!blk_rq_is_passthrough(scmd->request)) {
 		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
 		if (sdrv->eh_action)
-			rtn = sdrv->eh_action(scmd, rtn);
+			rtn = sdrv->eh_action(scmd, rtn, false);
+	}
+	return rtn;
+}
+
+static int scsi_eh_reset(struct scsi_cmnd *scmd)

Quoted 3 lines · Hannes Reinecke

The current medium access timeout counter will be increased for each command, so if there are enough failed commands we'll hit the medium access timeout for even a single failure.

This sentence describes multiple failed commands as a single failure. That's confusing to me. Did you perhaps intend "for a single device failure"?

Quoted 35 lines

drivers/scsi/scsi_error.c +12 -1

index f2cafae..cec439c 100644
@@ -58,6 +58,7 @@
 static int scsi_eh_try_stu(struct scsi_cmnd *scmd);
 static int scsi_try_to_abort_cmd(struct scsi_host_template *,
 				 struct scsi_cmnd *);
+static int scsi_eh_reset(struct scsi_cmnd *scmd);
 
 /* called with shost->host_lock held */
 void scsi_eh_wakeup(struct Scsi_Host *shost)

@@ -249,6 +250,7 @@ int scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
 	if (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED)
 		eh_flag &= ~SCSI_EH_CANCEL_CMD;
 	scmd->eh_eflags |= eh_flag;
+	scsi_eh_reset(scmd);
 	list_add_tail(&scmd->eh_entry, &shost->eh_cmd_q);
 	shost->host_failed++;
 	scsi_eh_wakeup(shost);

@@ -1107,7 +1109,19 @@ static int scsi_eh_action(struct scsi_cmnd *scmd, int rtn)
 	if (!blk_rq_is_passthrough(scmd->request)) {
 		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
 		if (sdrv->eh_action)
-			rtn = sdrv->eh_action(scmd, rtn);
+			rtn = sdrv->eh_action(scmd, rtn, false);
+	}
+	return rtn;
+}
+
+static int scsi_eh_reset(struct scsi_cmnd *scmd)
+{
+	int rtn = SUCCESS;
+
+	if (!blk_rq_is_passthrough(scmd->request)) {

drivers/scsi/scsi_error.c static int scsi_eh_action(struct scsi_cmnd *scmd, int rtn) · line 1127

Quoted 6 lines

drivers/scsi/scsi_error.c +3 -0

+		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
+		if (sdrv->eh_action)
+			rtn = sdrv->eh_action(scmd, rtn, true);
 	}
 	return rtn;
 }

Can this function be moved up such that we don't need a new forward declaration?

drivers/scsi/scsi_error.c static int sd_pr_clear(struct block_device *bdev, u64 key) · line 1692

Quoted 5 lines

drivers/scsi/scsi_error.c +1 -0

@@ -1689,18 +1689,28 @@ static int sd_pr_clear(struct block_device *bdev, u64 key)
  *	sd_eh_action - error handling callback
  *	@scmd:		sd-issued command that has failed
  *	@eh_disp:	The recovery disposition suggested by the midlayer
+ *	@reset:		Reset medium access counter

It seems to me that @reset does not trigger a reset of the medium access counter but rather of the variable that prevents the medium access error counter to be incremented?

drivers/scsi/scsi_error.c static int sd_pr_clear(struct block_device *bdev, u64 key) · line 1695

Quoted 3 lines

drivers/scsi/scsi_error.c +3 -0

+ *	recovery).
+ *	We have to be careful to count a medium access failure only once
+ *	per SCSI EH run; there might be several timed out commands which

Did you perhaps intend "once per device per SCSI EH run"?

drivers/scsi/sd.h struct scsi_disk { · line 109

Quoted 7 lines

drivers/scsi/sd.h +1 -0

@@ -106,6 +106,7 @@ struct scsi_disk {
 	unsigned	rc_basis: 2;
 	unsigned	zoned: 2;
 	unsigned	urswrz : 1;
+	unsigned	medium_access_reset : 1;

The name of this new member is confusing to me. How about using the name "ignore_medium_access_errors" instead? And since this is a boolean, how about using true and false in assignments to this variable?

The current medium access timeout counter will be increased for each command, so if there are enough failed commands we'll hit the medium access timeout for even a single failure. Fix this by making the timeout per EH run, ie the counter will only be increased once per device and EH run.

Cc Ewan Milne <[email protected]>

Cc Lawrence Obermann <[email protected]>

Cc Benjamin Block <[email protected]>

Cc Steffen Maier <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/scsi_error.c  | 16 +++++++++++++++-
 drivers/scsi/sd.c          | 21 +++++++++++++++++----
 drivers/scsi/sd.h          |  1 +
 include/scsi/scsi_driver.h |  2 +-
 4 files changed, 34 insertions(+), 6 deletions(-)

drivers/scsi/scsi_error.c +15 -1

index f2cafae..cec439c 100644
@@ -58,6 +58,7 @@
 static int scsi_eh_try_stu(struct scsi_cmnd *scmd);
 static int scsi_try_to_abort_cmd(struct scsi_host_template *,
 				 struct scsi_cmnd *);
+static int scsi_eh_reset(struct scsi_cmnd *scmd);
 
 /* called with shost->host_lock held */
 void scsi_eh_wakeup(struct Scsi_Host *shost)

@@ -249,6 +250,7 @@ int scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
 	if (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED)
 		eh_flag &= ~SCSI_EH_CANCEL_CMD;
 	scmd->eh_eflags |= eh_flag;
+	scsi_eh_reset(scmd);
 	list_add_tail(&scmd->eh_entry, &shost->eh_cmd_q);
 	shost->host_failed++;
 	scsi_eh_wakeup(shost);

@@ -1107,7 +1109,19 @@ static int scsi_eh_action(struct scsi_cmnd *scmd, int rtn)
 	if (!blk_rq_is_passthrough(scmd->request)) {
 		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
 		if (sdrv->eh_action)
-			rtn = sdrv->eh_action(scmd, rtn);
+			rtn = sdrv->eh_action(scmd, rtn, false);
+	}
+	return rtn;
+}
+
+static int scsi_eh_reset(struct scsi_cmnd *scmd)
+{
+	int rtn = SUCCESS;
+
+	if (!blk_rq_is_passthrough(scmd->request)) {
+		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
+		if (sdrv->eh_action)
+			rtn = sdrv->eh_action(scmd, rtn, true);
 	}
 	return rtn;
 }

drivers/scsi/sd.c +17 -4

index c7839f6..c794686 100644
@@ -115,7 +115,7 @@
 static int sd_init_command(struct scsi_cmnd *SCpnt);
 static void sd_uninit_command(struct scsi_cmnd *SCpnt);
 static int sd_done(struct scsi_cmnd *);
-static int sd_eh_action(struct scsi_cmnd *, int);
+static int sd_eh_action(struct scsi_cmnd *, int, bool);
 static void sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer);
 static void scsi_disk_release(struct device *cdev);
 static void sd_print_sense_hdr(struct scsi_disk *, struct scsi_sense_hdr *);

@@ -1689,18 +1689,28 @@ static int sd_pr_clear(struct block_device *bdev, u64 key)
  *	sd_eh_action - error handling callback
  *	@scmd:		sd-issued command that has failed
  *	@eh_disp:	The recovery disposition suggested by the midlayer
+ *	@reset:		Reset medium access counter
  *
  *	This function is called by the SCSI midlayer upon completion of an
  *	error test command (currently TEST UNIT READY). The result of sending
  *	the eh command is passed in eh_disp.  We're looking for devices that
  *	fail medium access commands but are OK with non access commands like
  *	test unit ready (so wrongly see the device as having a successful
- *	recovery)
+ *	recovery).
+ *	We have to be careful to count a medium access failure only once
+ *	per SCSI EH run; there might be several timed out commands which
+ *	will cause the 'max_medium_access_timeouts' counter to trigger
+ *	after the first SCSI EH run already and set the device to offline.
  **/
-static int sd_eh_action(struct scsi_cmnd *scmd, int eh_disp)
+static int sd_eh_action(struct scsi_cmnd *scmd, int eh_disp, bool reset)
 {
 	struct scsi_disk *sdkp = scsi_disk(scmd->request->rq_disk);
 
+	if (reset) {
+		/* New SCSI EH run, reset gate variable */
+		sdkp->medium_access_reset = 0;
+		return eh_disp;
+	}
 	if (!scsi_device_online(scmd->device) ||
 	    !scsi_medium_access_command(scmd) ||
 	    host_byte(scmd->result) != DID_TIME_OUT ||

@@ -1714,7 +1724,10 @@ static int sd_eh_action(struct scsi_cmnd *scmd, int eh_disp)
 	 * process of recovering or has it suffered an internal failure
 	 * that prevents access to the storage medium.
 	 */
-	sdkp->medium_access_timed_out++;
+	if (!sdkp->medium_access_reset) {
+		sdkp->medium_access_timed_out++;
+		sdkp->medium_access_reset = 1;
+	}
 
 	/*
 	 * If the device keeps failing read/write commands but TEST UNIT

drivers/scsi/sd.h +1 -0

index 4dac35e..6a4f75a 100644
@@ -106,6 +106,7 @@ struct scsi_disk {
 	unsigned	rc_basis: 2;
 	unsigned	zoned: 2;
 	unsigned	urswrz : 1;
+	unsigned	medium_access_reset : 1;
 };
 #define to_scsi_disk(obj) container_of(obj,struct scsi_disk,dev)
 

include/scsi/scsi_driver.h +1 -2

index 891a658..d5e0012 100644
@@ -15,7 +15,7 @@ struct scsi_driver {
 	int (*init_command)(struct scsi_cmnd *);
 	void (*uninit_command)(struct scsi_cmnd *);
 	int (*done)(struct scsi_cmnd *);
-	int (*eh_action)(struct scsi_cmnd *, int);
+	int (*eh_action)(struct scsi_cmnd *, int, bool);
 };
 #define to_scsi_driver(drv) \
 	container_of((drv), struct scsi_driver, gendrv)
--

1.8.5.6

Hej Hannes,

Quoted 11 lines · Hannes Reinecke

The current medium access timeout counter will be increased for each command, so if there are enough failed commands we'll hit the medium access timeout for even a single failure. Fix this by making the timeout per EH run, ie the counter will only be increased once per device and EH run.

Cc Ewan Milne <[email protected]>

Cc Lawrence Obermann <[email protected]>

Cc Benjamin Block <[email protected]>

Cc Steffen Maier <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

Steffen already suggested it, It would be nice to have a stable-tag here. This already caused multiple real-world false-positive outages, I think that qualifies for stable :)

Quoted 130 lines

---
 drivers/scsi/scsi_error.c  | 16 +++++++++++++++-
 drivers/scsi/sd.c          | 21 +++++++++++++++++----
 drivers/scsi/sd.h          |  1 +
 include/scsi/scsi_driver.h |  2 +-
 4 files changed, 34 insertions(+), 6 deletions(-)

drivers/scsi/scsi_error.c +15 -1

index f2cafae..cec439c 100644
@@ -58,6 +58,7 @@
 static int scsi_eh_try_stu(struct scsi_cmnd *scmd);
 static int scsi_try_to_abort_cmd(struct scsi_host_template *,
 				 struct scsi_cmnd *);
+static int scsi_eh_reset(struct scsi_cmnd *scmd);
 
 /* called with shost->host_lock held */
 void scsi_eh_wakeup(struct Scsi_Host *shost)

@@ -249,6 +250,7 @@ int scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
 	if (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED)
 		eh_flag &= ~SCSI_EH_CANCEL_CMD;
 	scmd->eh_eflags |= eh_flag;
+	scsi_eh_reset(scmd);
 	list_add_tail(&scmd->eh_entry, &shost->eh_cmd_q);
 	shost->host_failed++;
 	scsi_eh_wakeup(shost);

@@ -1107,7 +1109,19 @@ static int scsi_eh_action(struct scsi_cmnd *scmd, int rtn)
 	if (!blk_rq_is_passthrough(scmd->request)) {
 		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
 		if (sdrv->eh_action)
-			rtn = sdrv->eh_action(scmd, rtn);
+			rtn = sdrv->eh_action(scmd, rtn, false);
+	}
+	return rtn;
+}
+
+static int scsi_eh_reset(struct scsi_cmnd *scmd)
+{
+	int rtn = SUCCESS;
+
+	if (!blk_rq_is_passthrough(scmd->request)) {
+		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
+		if (sdrv->eh_action)
+			rtn = sdrv->eh_action(scmd, rtn, true);
 	}
 	return rtn;
 }

drivers/scsi/sd.c +17 -4

index c7839f6..c794686 100644
@@ -115,7 +115,7 @@
 static int sd_init_command(struct scsi_cmnd *SCpnt);
 static void sd_uninit_command(struct scsi_cmnd *SCpnt);
 static int sd_done(struct scsi_cmnd *);
-static int sd_eh_action(struct scsi_cmnd *, int);
+static int sd_eh_action(struct scsi_cmnd *, int, bool);
 static void sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer);
 static void scsi_disk_release(struct device *cdev);
 static void sd_print_sense_hdr(struct scsi_disk *, struct scsi_sense_hdr *);

@@ -1689,18 +1689,28 @@ static int sd_pr_clear(struct block_device *bdev, u64 key)
  *	sd_eh_action - error handling callback
  *	@scmd:		sd-issued command that has failed
  *	@eh_disp:	The recovery disposition suggested by the midlayer
+ *	@reset:		Reset medium access counter
  *
  *	This function is called by the SCSI midlayer upon completion of an
  *	error test command (currently TEST UNIT READY). The result of sending
  *	the eh command is passed in eh_disp.  We're looking for devices that
  *	fail medium access commands but are OK with non access commands like
  *	test unit ready (so wrongly see the device as having a successful
- *	recovery)
+ *	recovery).
+ *	We have to be careful to count a medium access failure only once
+ *	per SCSI EH run; there might be several timed out commands which
+ *	will cause the 'max_medium_access_timeouts' counter to trigger
+ *	after the first SCSI EH run already and set the device to offline.
  **/
-static int sd_eh_action(struct scsi_cmnd *scmd, int eh_disp)
+static int sd_eh_action(struct scsi_cmnd *scmd, int eh_disp, bool reset)
 {
 	struct scsi_disk *sdkp = scsi_disk(scmd->request->rq_disk);
 
+	if (reset) {
+		/* New SCSI EH run, reset gate variable */
+		sdkp->medium_access_reset = 0;
+		return eh_disp;
+	}
 	if (!scsi_device_online(scmd->device) ||
 	    !scsi_medium_access_command(scmd) ||
 	    host_byte(scmd->result) != DID_TIME_OUT ||

@@ -1714,7 +1724,10 @@ static int sd_eh_action(struct scsi_cmnd *scmd, int eh_disp)
 	 * process of recovering or has it suffered an internal failure
 	 * that prevents access to the storage medium.
 	 */
-	sdkp->medium_access_timed_out++;
+	if (!sdkp->medium_access_reset) {
+		sdkp->medium_access_timed_out++;
+		sdkp->medium_access_reset = 1;
+	}
 
 	/*
 	 * If the device keeps failing read/write commands but TEST UNIT

drivers/scsi/sd.h +1 -0

index 4dac35e..6a4f75a 100644
@@ -106,6 +106,7 @@ struct scsi_disk {
 	unsigned	rc_basis: 2;
 	unsigned	zoned: 2;
 	unsigned	urswrz : 1;
+	unsigned	medium_access_reset : 1;
 };
 #define to_scsi_disk(obj) container_of(obj,struct scsi_disk,dev)
 

include/scsi/scsi_driver.h +1 -2

index 891a658..d5e0012 100644
@@ -15,7 +15,7 @@ struct scsi_driver {
 	int (*init_command)(struct scsi_cmnd *);
 	void (*uninit_command)(struct scsi_cmnd *);
 	int (*done)(struct scsi_cmnd *);
-	int (*eh_action)(struct scsi_cmnd *, int);
+	int (*eh_action)(struct scsi_cmnd *, int, bool);
 };
 #define to_scsi_driver(drv) \
 	container_of((drv), struct scsi_driver, gendrv)
--

Quoted 2 lines

1.8.5.6

Bart already made some suggestions that I think are good, otherwise I have nothing big to add. Should work just fine.

The only thing that still bugs me about this is the corner-case we talked about the last time we talked about this. No show-stopper for me, but I though I might as well mention it.

When we do the different escalation-stages in scsi_unjam_host(), we always have the same basic pattern:

  for_all_in(work_q)) {
      cmd = get_next(eh_work_q);

      do_action(cmd); /* might be abort, lun_reset, target_reset, ... */

      if (was_ok()) {
          move_to(check_list, cmd);
	  move_same_scope_to(check_list, work_q);
      }

}

  for_all_in(check_list) {
      cmd  = get_next(eh_work_q);
      sdev = get_sdev(cmd);

      test_device(sdev); /* TUR and maybe STU */

      for_all_in_same_scope_in(check_list) {
          if (test_was_ok() && scsi_eh_action(scmd, SUCCESS) == SUCCESS)
              move_to(done_q, cmd)
          else
              move_to(work_q, cmd)
      }

}

return list_empty(eh_work_q)

(I hope I have this right, irritatingly this 'looks' different for no reason I can see for the different stages).

The corner-case being, if we have a cmd that failes in scsi_eh_action(), it will be put back into the work_q. But all other command for the same scope will be put into the done_q because for them, sd_eh_action() will early-return with SUCCESS (sdev is not online anymore).

Then, because the work_q is not empty, we will escalate to the next step. Worst case being, we escalate to host_reset, although EH is actually already done - we already decided that this sdev is offline in SD and even when we go through host-reset it will not become running anymore.. which makes the whole host-reset a big waste of time.

AFAIK the escalate to Host-Reset can only happen if the LLD has no pointer for STU, LUN-, Target- and Bus-Reset, so its actually not as bad, but I still think its something we should correct.

If you don't wanna change this here, I can probably send a part of the patch I made for this some time ago.

drivers/scsi/scsi_error.c +8 -1

index f2cafae..cec439c 100644
@@ -58,6 +58,7 @@
 static int scsi_eh_try_stu(struct scsi_cmnd *scmd);
 static int scsi_try_to_abort_cmd(struct scsi_host_template *,
 				 struct scsi_cmnd *);
+static int scsi_eh_reset(struct scsi_cmnd *scmd);
 
 /* called with shost->host_lock held */
 void scsi_eh_wakeup(struct Scsi_Host *shost)

@@ -249,6 +250,7 @@ int scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
 	if (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED)
 		eh_flag &= ~SCSI_EH_CANCEL_CMD;
 	scmd->eh_eflags |= eh_flag;
+	scsi_eh_reset(scmd);
 	list_add_tail(&scmd->eh_entry, &shost->eh_cmd_q);
 	shost->host_failed++;
 	scsi_eh_wakeup(shost);

@@ -1107,7 +1109,19 @@ static int scsi_eh_action(struct scsi_cmnd *scmd, int rtn)
 	if (!blk_rq_is_passthrough(scmd->request)) {
 		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
 		if (sdrv->eh_action)
-			rtn = sdrv->eh_action(scmd, rtn);
+			rtn = sdrv->eh_action(scmd, rtn, false);
+	}
+	return rtn;
+}
+
+static int scsi_eh_reset(struct scsi_cmnd *scmd)

Hannes,

Quoted 42 lines · Hannes Reinecke

drivers/scsi/scsi_error.c +2 -0

index f2cafae..cec439c 100644
@@ -58,6 +58,7 @@
 static int scsi_eh_try_stu(struct scsi_cmnd *scmd);
 static int scsi_try_to_abort_cmd(struct scsi_host_template *,
                  struct scsi_cmnd *);
+static int scsi_eh_reset(struct scsi_cmnd *scmd);
 
 /* called with shost->host_lock held */
 void scsi_eh_wakeup(struct Scsi_Host *shost)

@@ -249,6 +250,7 @@ int scsi_eh_scmd_add(struct scsi_cmnd *scmd, int
 eh_flag)
     if (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED)
         eh_flag &= ~SCSI_EH_CANCEL_CMD;
     scmd->eh_eflags |= eh_flag;
+    scsi_eh_reset(scmd);
     list_add_tail(&scmd->eh_entry, &shost->eh_cmd_q);
     shost->host_failed++;

     scsi_eh_wakeup(shost);

drivers/scsi/scsi_error.c +13 -1

@@ -1107,7 +1109,19 @@ static int scsi_eh_action(struct scsi_cmnd
 *scmd, int rtn)
     if (!blk_rq_is_passthrough(scmd->request)) {
         struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
         if (sdrv->eh_action)
-            rtn = sdrv->eh_action(scmd, rtn);
+            rtn = sdrv->eh_action(scmd, rtn, false);
+    }
+    return rtn;
+}
+
+static int scsi_eh_reset(struct scsi_cmnd *scmd)
+{
+    int rtn = SUCCESS;
+
+    if (!blk_rq_is_passthrough(scmd->request)) {
+        struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
+        if (sdrv->eh_action)
+            rtn = sdrv->eh_action(scmd, rtn, true);
     }
     return rtn;

Quoted 1 line

}

Apparently we can de-duplicate code in scsi_eh_reset()/scsi_eh_action(), and change less of sd_eh_action() (i.e., no new parameter).

Something like this. Thoughts?

- Deduplicate code in scsi_eh_reset() and scsi_eh_action()
- A call to scsi_eh_reset() with reset == false calls into eh_action()

- A call to scsi_eh_reset() with reset == true returns early, (as happens with/if sd_eh_action() is called in your patch)

- A call to scsi_eh_reset() in scsi_eh_scmd_add() uses SUCCESS just for consistency with scsi_eh_reset() in your patch, as the return value is ignored in it.

- Less changes to sd_eh_action()
- Removed one chunk in sd_eh_action() ('reset gate variable')
- No more parameter changes in sd_eh_action() (no 'reset' parameter)

- Removed forward declaration of scsi_eh_reset(), as already suggested

static int scsi_eh_reset(struct scsi_cmnd *scmd, int eh_disp, bool reset)

{
    int rtn = eh_disp;

    if (!blk_rq_is_passthrough(scmd->request)) {
        struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
        if (sdrv->eh_action) {
            if (reset) {
                 struct scsi_disk *sdkp =

scsi_disk(scmd->request->rq_disk);

                /* New SCSI EH run, reset gate variable */
                sdkp->medium_access_reset = 0;
                return rtn;
            }
            rtn = sdrv->eh_action(scmd, rtn);
        }
     }
     return rtn;
}

This is assuming that we're always running on a scsi_disk, and that scsi_disk is the only one implementing 'eh_action'. Neither of which is necessarily true.

To detect if a failed command has been retried we must not clear scmd->eh_eflags when EH finishes.
The flag should be persistent throughout the lifetime of the command.

Reviewed-by Johannes Thumshirn <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

To detect if a failed command has been retried we must not clear scmd->eh_eflags when EH finishes.
The flag should be persistent throughout the lifetime of the command.

Reviewed-by Johannes Thumshirn <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

---
 Documentation/scsi/scsi_eh.txt      | 14 +++++++-------
 drivers/scsi/libsas/sas_scsi_host.c |  2 --
 drivers/scsi/scsi_error.c           |  4 ++--
 include/scsi/scsi_eh.h              |  1 +
 4 files changed, 10 insertions(+), 11 deletions(-)

Documentation/scsi/scsi_eh.txt +7 -7

index 37eca00..4edb9c1c 100644
@@ -105,11 +105,14 @@ function
 
  2. If the host supports asynchronous completion (as indicated by the
     no_async_abort setting in the host template) scsi_abort_command()
-    is invoked to schedule an asynchrous abort. If that fails
-    Step #3 is taken.
+    is invoked to schedule an asynchrous abort.
+    Asynchronous abort are not invoked for commands which the
+    SCSI_EH_ABORT_SCHEDULED flag is set (this indicates that the command
+    already had been aborted once, and this is a retry which failed),
+    or when the EH deadline is expired. In these case Step #3 is taken.
 
- 2. scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD) is invoked for the
-    command.  See [1-3] for more information.
+ 3. scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD) is invoked for the
+    command.  See [1-4] for more information.
 
 [1-3] Asynchronous command aborts
 

@@ -263,7 +266,6 @@ scmd->allowed.
 
  3. scmd recovered
     ACTION: scsi_eh_finish_cmd() is invoked to EH-finish scmd
-	- clear scmd->eh_eflags
 	- scsi_setup_cmd_retry()
 	- move from local eh_work_q to local eh_done_q
     LOCKING: none

@@ -456,8 +458,6 @@ except for #1 must be implemented by eh_strategy_handler().
 
  - shost->host_failed is zero.
 
- - Each scmd's eh_eflags field is cleared.
-
  - Each scmd is in such a state that scsi_setup_cmd_retry() on the
    scmd doesn't make any difference.
 

drivers/scsi/libsas/sas_scsi_host.c +0 -2

index ee6b39a..87e5079 100644
@@ -613,8 +613,6 @@ static void sas_eh_handle_sas_errors(struct Scsi_Host *shost, struct list_head *
 		SAS_DPRINTK("trying to find task 0x%p\n", task);
 		res = sas_scsi_find_task(task);
 
-		cmd->eh_eflags = 0;
-
 		switch (res) {
 		case TASK_IS_DONE:
 			SAS_DPRINTK("%s: task 0x%p is done\n", __func__,

drivers/scsi/scsi_error.c +2 -2

index cec439c..e1ca3b8 100644
@@ -189,7 +189,6 @@ static int scsi_host_eh_past_deadline(struct Scsi_Host *shost)
 		/*
 		 * Retry after abort failed, escalate to next level.
 		 */
-		scmd->eh_eflags &= ~SCSI_EH_ABORT_SCHEDULED;
 		SCSI_LOG_ERROR_RECOVERY(3,
 			scmd_printk(KERN_INFO, scmd,
 				    "previous abort failed\n"));

@@ -933,6 +932,7 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct scsi_eh_save *ses,
 	ses->result = scmd->result;
 	ses->underflow = scmd->underflow;
 	ses->prot_op = scmd->prot_op;
+	ses->eh_eflags = scmd->eh_eflags;
 
 	scmd->prot_op = SCSI_PROT_NORMAL;
 	scmd->eh_eflags = 0;

@@ -996,6 +996,7 @@ void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd, struct scsi_eh_save *ses)
 	scmd->result = ses->result;
 	scmd->underflow = ses->underflow;
 	scmd->prot_op = ses->prot_op;
+	scmd->eh_eflags = ses->eh_eflags;
 }
 EXPORT_SYMBOL(scsi_eh_restore_cmnd);
 

@@ -1140,7 +1141,6 @@ static int scsi_eh_reset(struct scsi_cmnd *scmd)
  */
 void scsi_eh_finish_cmd(struct scsi_cmnd *scmd, struct list_head *done_q)
 {
-	scmd->eh_eflags = 0;
 	list_move_tail(&scmd->eh_entry, done_q);
 }
 EXPORT_SYMBOL(scsi_eh_finish_cmd);

include/scsi/scsi_eh.h +1 -1

index 98d366b..a25b328 100644
@@ -31,6 +31,7 @@ extern int scsi_get_sense_info_fld(const u8 * sense_buffer, int sb_len,
 struct scsi_eh_save {
 	/* saved state */
 	int result;
+	int eh_eflags;
 	enum dma_data_direction data_direction;
 	unsigned underflow;
 	unsigned char cmd_len;
--

1.8.5.6

When a command is sent as part of the error handling there is not point whatsoever to start EH escalation when that command fails; we are _already_ in the error handler, and the escalation is about to commence anyway.
So just call 'scsi_try_to_abort_cmd()' to abort outstanding commands and let the main EH routine handle the rest.

Signed-off-by Hannes Reinecke <[email protected]>

Reviewed-by Johannes Thumshirn <[email protected]>

Reviewed-by Bart Van Assche <[email protected]>

---
 drivers/scsi/scsi_error.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

drivers/scsi/scsi_error.c +0 -9

index e1ca3b8..4613aa1 100644
@@ -889,15 +889,6 @@ static int scsi_try_to_abort_cmd(struct scsi_host_template *hostt,
 	return hostt->eh_abort_handler(scmd);
 }
 
-static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd)
-{
-	if (scsi_try_to_abort_cmd(scmd->device->host->hostt, scmd) != SUCCESS)
-		if (scsi_try_bus_device_reset(scmd) != SUCCESS)
-			if (scsi_try_target_reset(scmd) != SUCCESS)
-				if (scsi_try_bus_reset(scmd) != SUCCESS)
-					scsi_try_host_reset(scmd);
-}
-
 /**
  * scsi_eh_prep_cmnd  - Save a scsi command info as part of error recovery
  * @scmd:       SCSI command structure to hijack

drivers/scsi/scsi_error.c +0 -1

--

1.8.5.6

Hello Hannes,

Quoted 35 lines · Hannes Reinecke

When a command is sent as part of the error handling there is not point whatsoever to start EH escalation when that command fails; we are _already_ in the error handler, and the escalation is about to commence anyway.
So just call 'scsi_try_to_abort_cmd()' to abort outstanding commands and let the main EH routine handle the rest.

Signed-off-by Hannes Reinecke <[email protected]>

Reviewed-by Johannes Thumshirn <[email protected]>

Reviewed-by Bart Van Assche <[email protected]>

---
 drivers/scsi/scsi_error.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

drivers/scsi/scsi_error.c +0 -9

index e1ca3b8..4613aa1 100644
@@ -889,15 +889,6 @@ static int scsi_try_to_abort_cmd(struct scsi_host_template *hostt,
 	return hostt->eh_abort_handler(scmd);
 }
 
-static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd)
-{
-	if (scsi_try_to_abort_cmd(scmd->device->host->hostt, scmd) != SUCCESS)
-		if (scsi_try_bus_device_reset(scmd) != SUCCESS)
-			if (scsi_try_target_reset(scmd) != SUCCESS)
-				if (scsi_try_bus_reset(scmd) != SUCCESS)
-					scsi_try_host_reset(scmd);
-}
-
 /**
  * scsi_eh_prep_cmnd  - Save a scsi command info as part of error recovery
  * @scmd:       SCSI command structure to hijack

drivers/scsi/scsi_error.c static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd, · line 1078

Quoted 7 lines

drivers/scsi/scsi_error.c +1 -1

@@ -1082,7 +1073,7 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 			break;
 		}
 	} else if (rtn != FAILED) {
-		scsi_abort_eh_cmnd(scmd);
+		scsi_try_to_abort_cmd(shost->hostt, scmd);
 		rtn = FAILED;
 	}

The idea is sound, but this implementation would cause "use-after-free"s.

I only know our own LLD well enough to judge, but with zFCP there will always be a chance that an abort fails - be it memory pressure, hardware/firmware behavior or internal EH in zFCP.

Calling queuecommand() will mean for us in the LLD, that we allocate a unique internal request struct for the scsi_cmnd (struct zfcp_fsf_request) and add that to our internal hash-table with outstanding commands. We assume this scsi_cmnd-pointer is ours till we complete it via scsi_done are yield it via successful EH-actions.

In case the abort fails, you fail to take back the ownership over the scsi command. Which in turn means possible "use-after-free"s when we still thinks the scsi command is ours, but EH has already overwritten the scsi-command with the original one. When we still get an answer or otherwise use the scsi_cmnd-pointer we would access an invalid one.

Quoted 58 lines · Benjamin Block

Hello Hannes,

Quoted 35 lines · Hannes Reinecke

When a command is sent as part of the error handling there is not point whatsoever to start EH escalation when that command fails; we are _already_ in the error handler, and the escalation is about to commence anyway.
So just call 'scsi_try_to_abort_cmd()' to abort outstanding commands and let the main EH routine handle the rest.

Signed-off-by Hannes Reinecke <[email protected]>

Reviewed-by Johannes Thumshirn <[email protected]>

Reviewed-by Bart Van Assche <[email protected]>

---
 drivers/scsi/scsi_error.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

drivers/scsi/scsi_error.c +0 -9

index e1ca3b8..4613aa1 100644
@@ -889,15 +889,6 @@ static int scsi_try_to_abort_cmd(struct scsi_host_template *hostt,
 	return hostt->eh_abort_handler(scmd);
 }
 
-static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd)
-{
-	if (scsi_try_to_abort_cmd(scmd->device->host->hostt, scmd) != SUCCESS)
-		if (scsi_try_bus_device_reset(scmd) != SUCCESS)
-			if (scsi_try_target_reset(scmd) != SUCCESS)
-				if (scsi_try_bus_reset(scmd) != SUCCESS)
-					scsi_try_host_reset(scmd);
-}
-
 /**
  * scsi_eh_prep_cmnd  - Save a scsi command info as part of error recovery
  * @scmd:       SCSI command structure to hijack

drivers/scsi/scsi_error.c static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd, · line 1078

Quoted 7 lines

drivers/scsi/scsi_error.c +1 -1

@@ -1082,7 +1073,7 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 			break;
 		}
 	} else if (rtn != FAILED) {
-		scsi_abort_eh_cmnd(scmd);
+		scsi_try_to_abort_cmd(shost->hostt, scmd);
 		rtn = FAILED;
 	}

The idea is sound, but this implementation would cause "use-after-free"s.

I only know our own LLD well enough to judge, but with zFCP there will always be a chance that an abort fails - be it memory pressure, hardware/firmware behavior or internal EH in zFCP.

Calling queuecommand() will mean for us in the LLD, that we allocate a unique internal request struct for the scsi_cmnd (struct zfcp_fsf_request) and add that to our internal hash-table with outstanding commands. We assume this scsi_cmnd-pointer is ours till we complete it via scsi_done are yield it via successful EH-actions.

Quoted 6 lines

In case the abort fails, you fail to take back the ownership over the scsi command. Which in turn means possible "use-after-free"s when we still thinks the scsi command is ours, but EH has already overwritten the scsi-command with the original one. When we still get an answer or otherwise use the scsi_cmnd-pointer we would access an invalid one.

That is actually not try.
As soon as we're calling 'scsi_try_to_abort_command()' ownership is assumed to reside in the SCSI midlayer;

also, the command used for
recovery here is actually using the same structure than the failed command, so if the command abort failed the command is already in the list of failed commands, and will be recovered after SCSI EH returned.

So no use-after-free here.

Quoted 65 lines · Hannes Reinecke

Quoted 58 lines · Benjamin Block

Hello Hannes,

Quoted 35 lines · Hannes Reinecke

When a command is sent as part of the error handling there is not point whatsoever to start EH escalation when that command fails; we are _already_ in the error handler, and the escalation is about to commence anyway.
So just call 'scsi_try_to_abort_cmd()' to abort outstanding commands and let the main EH routine handle the rest.

Signed-off-by Hannes Reinecke <[email protected]>

Reviewed-by Johannes Thumshirn <[email protected]>

Reviewed-by Bart Van Assche <[email protected]>

---
 drivers/scsi/scsi_error.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

drivers/scsi/scsi_error.c +0 -9

index e1ca3b8..4613aa1 100644
@@ -889,15 +889,6 @@ static int scsi_try_to_abort_cmd(struct scsi_host_template *hostt,
 	return hostt->eh_abort_handler(scmd);
 }
 
-static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd)
-{
-	if (scsi_try_to_abort_cmd(scmd->device->host->hostt, scmd) != SUCCESS)
-		if (scsi_try_bus_device_reset(scmd) != SUCCESS)
-			if (scsi_try_target_reset(scmd) != SUCCESS)
-				if (scsi_try_bus_reset(scmd) != SUCCESS)
-					scsi_try_host_reset(scmd);
-}
-
 /**
  * scsi_eh_prep_cmnd  - Save a scsi command info as part of error recovery
  * @scmd:       SCSI command structure to hijack

drivers/scsi/scsi_error.c static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd, · line 1078

Quoted 7 lines

drivers/scsi/scsi_error.c +1 -1

@@ -1082,7 +1073,7 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 			break;
 		}
 	} else if (rtn != FAILED) {
-		scsi_abort_eh_cmnd(scmd);
+		scsi_try_to_abort_cmd(shost->hostt, scmd);
 		rtn = FAILED;
 	}

The idea is sound, but this implementation would cause "use-after-free"s.

I only know our own LLD well enough to judge, but with zFCP there will always be a chance that an abort fails - be it memory pressure, hardware/firmware behavior or internal EH in zFCP.

Calling queuecommand() will mean for us in the LLD, that we allocate a unique internal request struct for the scsi_cmnd (struct zfcp_fsf_request) and add that to our internal hash-table with outstanding commands. We assume this scsi_cmnd-pointer is ours till we complete it via scsi_done are yield it via successful EH-actions.

Quoted 6 lines

In case the abort fails, you fail to take back the ownership over the scsi command. Which in turn means possible "use-after-free"s when we still thinks the scsi command is ours, but EH has already overwritten the scsi-command with the original one. When we still get an answer or otherwise use the scsi_cmnd-pointer we would access an invalid one.

Quoted 3 lines

That is actually not try.
As soon as we're calling 'scsi_try_to_abort_command()' ownership is assumed to reside in the SCSI midlayer;

That can not be true. First of all, look at the function itself (v4.10):

	static int scsi_try_to_abort_cmd...
	{
		if (!hostt->eh_abort_handler)
			return FAILED;

		return hostt->eh_abort_handler(scmd);
	}

If what you say is true, then this whole API of LLDs providing or choosing not to provide implementations for these function would be fundamentally broken.
The function itself returns FAILED when there is no such function.. how is a LLD that does not implement it ever to know that you took ownership by calling scsi_try_to_abort_cmd()?

Quoted 80 lines · Benjamin Block

Quoted 65 lines · Hannes Reinecke

Quoted 58 lines · Benjamin Block

Hello Hannes,

Quoted 35 lines · Hannes Reinecke

When a command is sent as part of the error handling there is not point whatsoever to start EH escalation when that command fails; we are _already_ in the error handler, and the escalation is about to commence anyway.
So just call 'scsi_try_to_abort_cmd()' to abort outstanding commands and let the main EH routine handle the rest.

Signed-off-by Hannes Reinecke <[email protected]>

Reviewed-by Johannes Thumshirn <[email protected]>

Reviewed-by Bart Van Assche <[email protected]>

---
 drivers/scsi/scsi_error.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

drivers/scsi/scsi_error.c +0 -9

index e1ca3b8..4613aa1 100644
@@ -889,15 +889,6 @@ static int scsi_try_to_abort_cmd(struct scsi_host_template *hostt,
 	return hostt->eh_abort_handler(scmd);
 }
 
-static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd)
-{
-	if (scsi_try_to_abort_cmd(scmd->device->host->hostt, scmd) != SUCCESS)
-		if (scsi_try_bus_device_reset(scmd) != SUCCESS)
-			if (scsi_try_target_reset(scmd) != SUCCESS)
-				if (scsi_try_bus_reset(scmd) != SUCCESS)
-					scsi_try_host_reset(scmd);
-}
-
 /**
  * scsi_eh_prep_cmnd  - Save a scsi command info as part of error recovery
  * @scmd:       SCSI command structure to hijack

drivers/scsi/scsi_error.c static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd, · line 1078

Quoted 7 lines

drivers/scsi/scsi_error.c +1 -1

@@ -1082,7 +1073,7 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 			break;
 		}
 	} else if (rtn != FAILED) {
-		scsi_abort_eh_cmnd(scmd);
+		scsi_try_to_abort_cmd(shost->hostt, scmd);
 		rtn = FAILED;
 	}

The idea is sound, but this implementation would cause "use-after-free"s.

I only know our own LLD well enough to judge, but with zFCP there will always be a chance that an abort fails - be it memory pressure, hardware/firmware behavior or internal EH in zFCP.

Calling queuecommand() will mean for us in the LLD, that we allocate a unique internal request struct for the scsi_cmnd (struct zfcp_fsf_request) and add that to our internal hash-table with outstanding commands. We assume this scsi_cmnd-pointer is ours till we complete it via scsi_done are yield it via successful EH-actions.

Quoted 6 lines

In case the abort fails, you fail to take back the ownership over the scsi command. Which in turn means possible "use-after-free"s when we still thinks the scsi command is ours, but EH has already overwritten the scsi-command with the original one. When we still get an answer or otherwise use the scsi_cmnd-pointer we would access an invalid one.

Quoted 3 lines

That is actually not try.
As soon as we're calling 'scsi_try_to_abort_command()' ownership is assumed to reside in the SCSI midlayer;

That can not be true. First of all, look at the function itself (v4.10):

	static int scsi_try_to_abort_cmd...
	{
		if (!hostt->eh_abort_handler)
			return FAILED;

		return hostt->eh_abort_handler(scmd);
	}

Quoted 7 lines

If what you say is true, then this whole API of LLDs providing or choosing not to provide implementations for these function would be fundamentally broken.
The function itself returns FAILED when there is no such function.. how is a LLD that does not implement it ever to know that you took ownership by calling scsi_try_to_abort_cmd()?

Well. Ok.

_Actually_, the whole concept of 'ownership' in SCSI EH is a bit flaky.

There are two ways of entering the error handler:
- SCSI command timeout
- Failing to evaluate the SCSI command status

For the latter case ownership already _is_ with the SCSI midlayer, as the LLDD called 'scsi_done' and with that moved ownership to the midlayer.

The interesting part is command timeout.
Once a command timeout triggers the block layer is calling 'blk_mark_rq_complete' to avoid any concurrent completions. IE any calls to scsi_done() will be short-circuited with that, effectively transferring ownership to SCSI midlayer.

Now the SCSI midlayer has to inform the LLDD that it has taken ownership; for that it calls the various eh_XXX callbacks into the LLDD. While it's quite clear that SUCCESS signals a transfer of ownership to SCSI ML, it's less clear what happens in the case of FAILED. Problem here is that the eh_XXX callbacks actually serve a dual purpose; one it to signal the transfer of ownership to SCSI ML and the other is to actually _do_ some action on that command.

But as FAILED is just _one_ value we have no idea in the midlayer if the change of ownership actually took place.

Which leads to the curious effect that _ultimatively_ control still resides with the LLDD when host_reset fails, so we actually should _never_ release the scsi command once host reset fails.

With scsi_try_to_abort() things are slightly different in the way that it's called _without_ SCSI EH being engaged.
However, as scsi_try_to_abort() is called from the timeout handler (which assumes that ownership does now reside with the midlayer) I don't see a problem with that.

Where you are right, in fact, is that we should not return FAILED when calling scsi_try_to_abort() when cleaning up EH commands; if the driver does not implement this function then no cleanup can be done, so calling scsi_try_to_abort() is just us being nice.

And I actually can see a problem with cleaning up EH commands if scsi_try_to_abort() returns FAILED; then the LLDD has potential _two_ stale references, one for the original command and one for the command send from SCSI EH.
The only way I would imagine this ever worked was by _reusing_ the references to the original command, effectively sending the TMF with the same credentials the original SCSI command had.
If a driver (for whatever reason) does _not_ do this things will fall apart indeed.

However, this was always a problem with SCSI EH; even with the original codepath we would have this problem, so I don't think it's a problem with this patchset.

Nevertheless, I'll be adding a fix for eh_try_to_abort() in the context of cleaning up EH commands.

Quoted 120 lines · Hannes Reinecke

Quoted 80 lines · Benjamin Block

Quoted 65 lines · Hannes Reinecke

Quoted 58 lines · Benjamin Block

Hello Hannes,

Quoted 35 lines · Hannes Reinecke

When a command is sent as part of the error handling there is not point whatsoever to start EH escalation when that command fails; we are _already_ in the error handler, and the escalation is about to commence anyway.
So just call 'scsi_try_to_abort_cmd()' to abort outstanding commands and let the main EH routine handle the rest.

Signed-off-by Hannes Reinecke <[email protected]>

Reviewed-by Johannes Thumshirn <[email protected]>

Reviewed-by Bart Van Assche <[email protected]>

---
 drivers/scsi/scsi_error.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

drivers/scsi/scsi_error.c +0 -9

index e1ca3b8..4613aa1 100644
@@ -889,15 +889,6 @@ static int scsi_try_to_abort_cmd(struct scsi_host_template *hostt,
 	return hostt->eh_abort_handler(scmd);
 }
 
-static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd)
-{
-	if (scsi_try_to_abort_cmd(scmd->device->host->hostt, scmd) != SUCCESS)
-		if (scsi_try_bus_device_reset(scmd) != SUCCESS)
-			if (scsi_try_target_reset(scmd) != SUCCESS)
-				if (scsi_try_bus_reset(scmd) != SUCCESS)
-					scsi_try_host_reset(scmd);
-}
-
 /**
  * scsi_eh_prep_cmnd  - Save a scsi command info as part of error recovery
  * @scmd:       SCSI command structure to hijack

drivers/scsi/scsi_error.c static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd, · line 1078

Quoted 7 lines

drivers/scsi/scsi_error.c +1 -1

@@ -1082,7 +1073,7 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 			break;
 		}
 	} else if (rtn != FAILED) {
-		scsi_abort_eh_cmnd(scmd);
+		scsi_try_to_abort_cmd(shost->hostt, scmd);
 		rtn = FAILED;
 	}

The idea is sound, but this implementation would cause "use-after-free"s.

I only know our own LLD well enough to judge, but with zFCP there will always be a chance that an abort fails - be it memory pressure, hardware/firmware behavior or internal EH in zFCP.

Calling queuecommand() will mean for us in the LLD, that we allocate a unique internal request struct for the scsi_cmnd (struct zfcp_fsf_request) and add that to our internal hash-table with outstanding commands. We assume this scsi_cmnd-pointer is ours till we complete it via scsi_done are yield it via successful EH-actions.

Quoted 6 lines

In case the abort fails, you fail to take back the ownership over the scsi command. Which in turn means possible "use-after-free"s when we still thinks the scsi command is ours, but EH has already overwritten the scsi-command with the original one. When we still get an answer or otherwise use the scsi_cmnd-pointer we would access an invalid one.

Quoted 3 lines

That is actually not try.
As soon as we're calling 'scsi_try_to_abort_command()' ownership is assumed to reside in the SCSI midlayer;

That can not be true. First of all, look at the function itself (v4.10):

	static int scsi_try_to_abort_cmd...
	{
		if (!hostt->eh_abort_handler)
			return FAILED;

		return hostt->eh_abort_handler(scmd);
	}

Quoted 7 lines

If what you say is true, then this whole API of LLDs providing or choosing not to provide implementations for these function would be fundamentally broken.
The function itself returns FAILED when there is no such function.. how is a LLD that does not implement it ever to know that you took ownership by calling scsi_try_to_abort_cmd()?

Well. Ok.

_Actually_, the whole concept of 'ownership' in SCSI EH is a bit flaky.

There are two ways of entering the error handler:
- SCSI command timeout
- Failing to evaluate the SCSI command status

For the latter case ownership already _is_ with the SCSI midlayer, as the LLDD called 'scsi_done' and with that moved ownership to the midlayer.

The interesting part is command timeout.
Once a command timeout triggers the block layer is calling 'blk_mark_rq_complete' to avoid any concurrent completions. IE any calls to scsi_done() will be short-circuited with that, effectively transferring ownership to SCSI midlayer.

Now the SCSI midlayer has to inform the LLDD that it has taken ownership; for that it calls the various eh_XXX callbacks into the LLDD. While it's quite clear that SUCCESS signals a transfer of ownership to SCSI ML, it's less clear what happens in the case of FAILED. Problem here is that the eh_XXX callbacks actually serve a dual purpose; one it to signal the transfer of ownership to SCSI ML and the other is to actually _do_ some action on that command.

But as FAILED is just _one_ value we have no idea in the midlayer if the change of ownership actually took place.

Which leads to the curious effect that _ultimatively_ control still resides with the LLDD when host_reset fails, so we actually should _never_ release the scsi command once host reset fails.

Quoted 6 lines

With scsi_try_to_abort() things are slightly different in the way that it's called _without_ SCSI EH being engaged.
However, as scsi_try_to_abort() is called from the timeout handler (which assumes that ownership does now reside with the midlayer) I don't see a problem with that.

Yeah, I was aware of these things, but I think we are talking about 2 different things/implications when we talk about 'ownership' in this context here. Which makes things seem to be worse than they are.

But we seem to agree on this: if eh_XXX returns FAILED, ownership ultimatively stays with the LLD. In which case the midlayer has to accommodate for the possibility that for example scsi_done is called.

Anyway, see below.

Quoted 16 lines

Where you are right, in fact, is that we should not return FAILED when calling scsi_try_to_abort() when cleaning up EH commands; if the driver does not implement this function then no cleanup can be done, so calling scsi_try_to_abort() is just us being nice.

And I actually can see a problem with cleaning up EH commands if scsi_try_to_abort() returns FAILED; then the LLDD has potential _two_ stale references, one for the original command and one for the command send from SCSI EH.
The only way I would imagine this ever worked was by _reusing_ the references to the original command, effectively sending the TMF with the same credentials the original SCSI command had.
If a driver (for whatever reason) does _not_ do this things will fall apart indeed.

Quoted 7 lines

However, this was always a problem with SCSI EH; even with the original codepath we would have this problem, so I don't think it's a problem with this patchset.

Nevertheless, I'll be adding a fix for eh_try_to_abort() in the context of cleaning up EH commands.

I slept over this a night or two and then I remembered that to get to this point in the first place, at least one eh_XXX callback must have returned SUCCESS for the command that is reused to send the EH command. So the original reference to it should be forgotten already, if not, then that is indeed a LLD bug.

That makes it much less troubling. But then again, sorry to say, but that still leaves me with one objection:

When you ignore the FAIL return for the abort, it opens up the possibility to have an unrelated EH-Command completion being triggered from a previous EH-Command reference.

Lets say we have a command A for LUN 1 behind Port α, command B for LUN 2 behind Port β; and lets assume both FAIL the abort, so they get to the stage where EH-Commands might be send. Both will be successfully 'freed' after a LUN-Reset is issued for both 1 and 2.

After each LUN-Reset we send a TUR, which is the EH-Command. If A_{TUR} times out and the abort fails (again), then the LLD might still have a reference to A_{TUR} and with that also to its overwritten scsi_done function-pointer.

When we later send B_{TUR}, A_{TUR} might concurrently complete, call the scsi_done function-pointer to scsi_eh_done and complete the host-wide EH-completion that was setup for B_{TUR} (shost->eh_action). This is possible because shost->eh_action has a value different from NULL for B_{TUR}.

And apart from this clear problem, I also find it still troubling that we call scsi_eh_restore_cmnd() before being sure that the reference to the EH-Command was forgotten. Because of that, we all of a sudden 'restore' a reference to a command that the LLD already has forgotten before, just via a different 'LLD-Object'. And fields like scsi_cmnd->result from A become writeable again, although the 'LLD-Object' should only point to A_{TUR}. And at the same time we 'forget' references to parts of A_{TUR} that we queued that command with.. such as cmnd, cmnd_len, data_direction, ... . That is kinda the original point I complained about.

All that was handled by the original code that escalated throughout the whole process, as ugly as it might be, before restoring the command and continuing with further possible EH-Commands. And yes, you are right, host-reset might also fail in the old code, leaving us essentially in the same mess. But then again, if host-reset fails, SCSI EH is pretty much lost anyway...

scsi_eh_scmd_add() currently only will fail if no error handler thread is started (which will never be the case) or if the state machine encounters an illegal transition.

But if we're encountering an invalid state transition chances is we cannot fixup things with the error handler. So better add a WARN_ON for illegal host states and make scsi_dh_scmd_add() a void function.

scsi_eh_scmd_add() currently only will fail if no error handler thread is started (which will never be the case) or if the state machine encounters an illegal transition.

But if we're encountering an invalid state transition chances is we cannot fixup things with the error handler. So better add a WARN_ON for illegal host states and make scsi_dh_scmd_add() a void function.

Signed-off-by Hannes Reinecke <[email protected]>

Reviewed-by Johannes Thumshirn <[email protected]>

---
 drivers/scsi/scsi_error.c | 41 +++++++++++++----------------------------
 drivers/scsi/scsi_lib.c   |  4 ++--
 drivers/scsi/scsi_priv.h  |  2 +-
 3 files changed, 16 insertions(+), 31 deletions(-)

drivers/scsi/scsi_error.c +2 -11

index 4613aa1..802a081 100644
@@ -163,13 +163,7 @@ static int scsi_host_eh_past_deadline(struct Scsi_Host *shost)
 		}
 	}
 
-	if (!scsi_eh_scmd_add(scmd, 0)) {
-		SCSI_LOG_ERROR_RECOVERY(3,
-			scmd_printk(KERN_WARNING, scmd,
-				    "terminate aborted command\n"));
-		set_host_byte(scmd, DID_TIME_OUT);
-		scsi_finish_command(scmd);
-	}
+	scsi_eh_scmd_add(scmd, 0);
 }
 
 /**

@@ -224,28 +218,23 @@ static int scsi_host_eh_past_deadline(struct Scsi_Host *shost)
  * scsi_eh_scmd_add - add scsi cmd to error handling.
  * @scmd:	scmd to run eh on.
  * @eh_flag:	optional SCSI_EH flag.
- *
- * Return value:
- *	0 on failure.
  */
-int scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
+void scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)

drivers/scsi/scsi_error.c +9 -14

 	spin_lock_irqsave(shost->host_lock, flags);
-	if (scsi_host_set_state(shost, SHOST_RECOVERY))
-		if (scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY))
-			goto out_unlock;
-
+	if (scsi_host_set_state(shost, SHOST_RECOVERY)) {
+		ret = scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY);
+		WARN_ON_ONCE(ret);
+	}
 	if (shost->eh_deadline != -1 && !shost->last_reset)
 		shost->last_reset = jiffies;
 
-	ret = 1;
 	if (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED)
 		eh_flag &= ~SCSI_EH_CANCEL_CMD;
 	scmd->eh_eflags |= eh_flag;

@@ -253,9 +242,7 @@ int scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
 	list_add_tail(&scmd->eh_entry, &shost->eh_cmd_q);
 	shost->host_failed++;
 	scsi_eh_wakeup(shost);
- out_unlock:
 	spin_unlock_irqrestore(shost->host_lock, flags);
-	return ret;
 }
 
 /**

@@ -284,13 +271,11 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
 		rtn = host->hostt->eh_timed_out(scmd);
 
 	if (rtn == BLK_EH_NOT_HANDLED) {
-		if (!host->hostt->no_async_abort &&
-		    scsi_abort_command(scmd) == SUCCESS)
-			return BLK_EH_NOT_HANDLED;
-
-		set_host_byte(scmd, DID_TIME_OUT);
-		if (!scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD))
-			rtn = BLK_EH_HANDLED;
+		if (host->hostt->no_async_abort ||
+		    scsi_abort_command(scmd) != SUCCESS) {
+			set_host_byte(scmd, DID_TIME_OUT);
+			scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD);
+		}
 	}
 
 	return rtn;

drivers/scsi/scsi_lib.c +2 -2

index f5e45a2..0735a46 100644
@@ -1593,8 +1593,8 @@ static void scsi_softirq_done(struct request *rq)
 			scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY);
 			break;
 		default:
-			if (!scsi_eh_scmd_add(cmd, 0))
-				scsi_finish_command(cmd);
+			scsi_eh_scmd_add(cmd, 0);
+			break;
 	}
 }
 

drivers/scsi/scsi_priv.h +1 -1

index 99bfc98..5be6cbf6 100644
@@ -72,7 +72,7 @@ extern int scsi_dev_info_list_add_keyed(int compatible, char *vendor,
 extern int scsi_error_handler(void *host);
 extern int scsi_decide_disposition(struct scsi_cmnd *cmd);
 extern void scsi_eh_wakeup(struct Scsi_Host *shost);
-extern int scsi_eh_scmd_add(struct scsi_cmnd *, int);
+extern void scsi_eh_scmd_add(struct scsi_cmnd *, int);
 void scsi_eh_ready_devs(struct Scsi_Host *shost,
 			struct list_head *work_q,
 			struct list_head *done_q);

Signature · 1 line

1.8.5.6

There hasn't been any reports for HBAs where asynchronous abort would not work, so we should make it mandatory and remove the fallback.

Signed-off-by Hannes Reinecke <[email protected]>

Reviewed-by Johannes Thumshirn <[email protected]>

Reviewed-by Bart Van Assche <[email protected]>

---
 Documentation/scsi/scsi_eh.txt | 18 ++++------
 drivers/scsi/scsi_error.c      | 81 ++++--------------------------------------
 drivers/scsi/scsi_lib.c        |  2 +-
 drivers/scsi/scsi_priv.h       |  3 +-
 include/scsi/scsi_host.h       |  5 ---
 5 files changed, 15 insertions(+), 94 deletions(-)

Documentation/scsi/scsi_eh.txt +6 -12

index 4edb9c1c..11e447b 100644
@@ -70,7 +70,7 @@ with the command.
 	scmd is requeued to blk queue.
 
  - otherwise
-	scsi_eh_scmd_add(scmd, 0) is invoked for the command.  See
+	scsi_eh_scmd_add(scmd) is invoked for the command.  See
 	[1-3] for details of this function.
 
 

@@ -103,9 +103,7 @@ function
         eh_timed_out() callback did not handle the command.
 	Step #2 is taken.
 
- 2. If the host supports asynchronous completion (as indicated by the
-    no_async_abort setting in the host template) scsi_abort_command()
-    is invoked to schedule an asynchrous abort.
+ 2. scsi_abort_command() is invoked to schedule an asynchrous abort.
     Asynchronous abort are not invoked for commands which the
     SCSI_EH_ABORT_SCHEDULED flag is set (this indicates that the command
     already had been aborted once, and this is a retry which failed),

@@ -127,16 +125,13 @@ function
 
  scmds enter EH via scsi_eh_scmd_add(), which does the following.
 
- 1. Turns on scmd->eh_eflags as requested.  It's 0 for error
-    completions and SCSI_EH_CANCEL_CMD for timeouts.
+ 1. Links scmd->eh_entry to shost->eh_cmd_q
 
- 2. Links scmd->eh_entry to shost->eh_cmd_q
+ 2. Sets SHOST_RECOVERY bit in shost->shost_state
 
- 3. Sets SHOST_RECOVERY bit in shost->shost_state
+ 3. Increments shost->host_failed
 
- 4. Increments shost->host_failed
-
- 5. Wakes up SCSI EH thread if shost->host_busy == shost->host_failed
+ 4. Wakes up SCSI EH thread if shost->host_busy == shost->host_failed
 
  As can be seen above, once any scmd is added to shost->eh_cmd_q,
 SHOST_RECOVERY shost_state bit is turned on.  This prevents any new

@@ -252,7 +247,6 @@ scmd->allowed.
 
  1. Error completion / time out
     ACTION: scsi_eh_scmd_add() is invoked for scmd
-	- set scmd->eh_eflags
 	- add scmd to shost->eh_cmd_q
 	- set SHOST_RECOVERY
 	- shost->host_failed++

drivers/scsi/scsi_error.c +7 -74

index 802a081..7b70ee9 100644
@@ -163,7 +163,7 @@ static int scsi_host_eh_past_deadline(struct Scsi_Host *shost)
 		}
 	}
 
-	scsi_eh_scmd_add(scmd, 0);
+	scsi_eh_scmd_add(scmd);
 }
 
 /**

@@ -217,9 +217,8 @@ static int scsi_host_eh_past_deadline(struct Scsi_Host *shost)
 /**
  * scsi_eh_scmd_add - add scsi cmd to error handling.
  * @scmd:	scmd to run eh on.
- * @eh_flag:	optional SCSI_EH flag.
  */
-void scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
+void scsi_eh_scmd_add(struct scsi_cmnd *scmd)
 {
 	struct Scsi_Host *shost = scmd->device->host;
 	unsigned long flags;

@@ -235,9 +234,6 @@ void scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
 	if (shost->eh_deadline != -1 && !shost->last_reset)
 		shost->last_reset = jiffies;
 
-	if (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED)
-		eh_flag &= ~SCSI_EH_CANCEL_CMD;
-	scmd->eh_eflags |= eh_flag;
 	scsi_eh_reset(scmd);
 	list_add_tail(&scmd->eh_entry, &shost->eh_cmd_q);
 	shost->host_failed++;

@@ -271,10 +267,9 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
 		rtn = host->hostt->eh_timed_out(scmd);
 
 	if (rtn == BLK_EH_NOT_HANDLED) {
-		if (host->hostt->no_async_abort ||
-		    scsi_abort_command(scmd) != SUCCESS) {
+		if (scsi_abort_command(scmd) != SUCCESS) {
 			set_host_byte(scmd, DID_TIME_OUT);
-			scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD);
+			scsi_eh_scmd_add(scmd);
 		}
 	}
 

@@ -327,7 +322,7 @@ static inline void scsi_eh_prt_fail_stats(struct Scsi_Host *shost,
 		list_for_each_entry(scmd, work_q, eh_entry) {
 			if (scmd->device == sdev) {
 				++total_failures;
-				if (scmd->eh_eflags & SCSI_EH_CANCEL_CMD)
+				if (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED)
 					++cmd_cancel;
 				else
 					++cmd_failed;

@@ -1153,8 +1148,7 @@ int scsi_eh_get_sense(struct list_head *work_q,
 	 * should not get sense.
 	 */
 	list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
-		if ((scmd->eh_eflags & SCSI_EH_CANCEL_CMD) ||
-		    (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED) ||
+		if ((scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED) ||
 		    SCSI_SENSE_VALID(scmd))
 			continue;
 

@@ -1294,61 +1288,6 @@ static int scsi_eh_test_devices(struct list_head *cmd_list,
 	return list_empty(work_q);
 }
 
-
-/**
- * scsi_eh_abort_cmds - abort pending commands.
- * @work_q:	&list_head for pending commands.
- * @done_q:	&list_head for processed commands.
- *
- * Decription:
- *    Try and see whether or not it makes sense to try and abort the
- *    running command.  This only works out to be the case if we have one
- *    command that has timed out.  If the command simply failed, it makes
- *    no sense to try and abort the command, since as far as the shost
- *    adapter is concerned, it isn't running.
- */
-static int scsi_eh_abort_cmds(struct list_head *work_q,
-			      struct list_head *done_q)
-{
-	struct scsi_cmnd *scmd, *next;
-	LIST_HEAD(check_list);
-	int rtn;
-	struct Scsi_Host *shost;
-
-	list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
-		if (!(scmd->eh_eflags & SCSI_EH_CANCEL_CMD))
-			continue;
-		shost = scmd->device->host;
-		if (scsi_host_eh_past_deadline(shost)) {
-			list_splice_init(&check_list, work_q);
-			SCSI_LOG_ERROR_RECOVERY(3,
-				scmd_printk(KERN_INFO, scmd,
-					    "%s: skip aborting cmd, past eh deadline\n",
-					    current->comm));
-			return list_empty(work_q);
-		}
-		SCSI_LOG_ERROR_RECOVERY(3,
-			scmd_printk(KERN_INFO, scmd,
-				     "%s: aborting cmd\n", current->comm));
-		rtn = scsi_try_to_abort_cmd(shost->hostt, scmd);
-		if (rtn == FAILED) {
-			SCSI_LOG_ERROR_RECOVERY(3,
-				scmd_printk(KERN_INFO, scmd,
-					    "%s: aborting cmd failed\n",
-					     current->comm));
-			list_splice_init(&check_list, work_q);
-			return list_empty(work_q);
-		}
-		scmd->eh_eflags &= ~SCSI_EH_CANCEL_CMD;
-		if (rtn == FAST_IO_FAIL)
-			scsi_eh_finish_cmd(scmd, done_q);
-		else
-			list_move_tail(&scmd->eh_entry, &check_list);
-	}
-
-	return scsi_eh_test_devices(&check_list, work_q, done_q, 0);
-}
-
 /**
  * scsi_eh_try_stu - Send START_UNIT to device.
  * @scmd:	&scsi_cmnd to send START_UNIT

@@ -1691,11 +1630,6 @@ static void scsi_eh_offline_sdevs(struct list_head *work_q,
 		sdev_printk(KERN_INFO, scmd->device, "Device offlined - "
 			    "not ready after error recovery\n");
 		scsi_device_set_state(scmd->device, SDEV_OFFLINE);
-		if (scmd->eh_eflags & SCSI_EH_CANCEL_CMD) {
-			/*
-			 * FIXME: Handle lost cmds.
-			 */
-		}
 		scsi_eh_finish_cmd(scmd, done_q);
 	}
 	return;

@@ -2139,8 +2073,7 @@ static void scsi_unjam_host(struct Scsi_Host *shost)
 	SCSI_LOG_ERROR_RECOVERY(1, scsi_eh_prt_fail_stats(shost, &eh_work_q));
 
 	if (!scsi_eh_get_sense(&eh_work_q, &eh_done_q))
-		if (!scsi_eh_abort_cmds(&eh_work_q, &eh_done_q))
-			scsi_eh_ready_devs(shost, &eh_work_q, &eh_done_q);
+		scsi_eh_ready_devs(shost, &eh_work_q, &eh_done_q);
 
 	spin_lock_irqsave(shost->host_lock, flags);
 	if (shost->eh_deadline != -1)

drivers/scsi/scsi_lib.c +1 -1

index 0735a46..98b2df8 100644
@@ -1593,7 +1593,7 @@ static void scsi_softirq_done(struct request *rq)
 			scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY);
 			break;
 		default:
-			scsi_eh_scmd_add(cmd, 0);
+			scsi_eh_scmd_add(cmd);
 			break;
 	}
 }

drivers/scsi/scsi_priv.h +1 -2

index 5be6cbf6..e20ab10 100644
@@ -18,7 +18,6 @@
 /*
  * Scsi Error Handler Flags
  */
-#define SCSI_EH_CANCEL_CMD	0x0001	/* Cancel this cmd */
 #define SCSI_EH_ABORT_SCHEDULED	0x0002	/* Abort has been scheduled */
 
 #define SCSI_SENSE_VALID(scmd) \

@@ -72,7 +71,7 @@ extern int scsi_dev_info_list_add_keyed(int compatible, char *vendor,
 extern int scsi_error_handler(void *host);
 extern int scsi_decide_disposition(struct scsi_cmnd *cmd);
 extern void scsi_eh_wakeup(struct Scsi_Host *shost);
-extern void scsi_eh_scmd_add(struct scsi_cmnd *, int);
+extern void scsi_eh_scmd_add(struct scsi_cmnd *);
 void scsi_eh_ready_devs(struct Scsi_Host *shost,
 			struct list_head *work_q,
 			struct list_head *done_q);

include/scsi/scsi_host.h +0 -5

index 3cd8c3b..afb0481 100644
@@ -452,11 +452,6 @@ struct scsi_host_template {
 	unsigned no_write_same:1;
 
 	/*
-	 * True if asynchronous aborts are not supported
-	 */
-	unsigned no_async_abort:1;
-
-	/*
 	 * Countdown for host blocking with no commands outstanding.
 	 */
 	unsigned int max_host_blocked;

Signature · 1 line

1.8.5.6

Hello Hannes,

Quoted 271 lines · Hannes Reinecke

There hasn't been any reports for HBAs where asynchronous abort would not work, so we should make it mandatory and remove the fallback.

Signed-off-by Hannes Reinecke <[email protected]>

Reviewed-by Johannes Thumshirn <[email protected]>

Reviewed-by Bart Van Assche <[email protected]>

---
 Documentation/scsi/scsi_eh.txt | 18 ++++------
 drivers/scsi/scsi_error.c      | 81 ++++--------------------------------------
 drivers/scsi/scsi_lib.c        |  2 +-
 drivers/scsi/scsi_priv.h       |  3 +-
 include/scsi/scsi_host.h       |  5 ---
 5 files changed, 15 insertions(+), 94 deletions(-)

Documentation/scsi/scsi_eh.txt +6 -12

index 4edb9c1c..11e447b 100644
@@ -70,7 +70,7 @@ with the command.
 	scmd is requeued to blk queue.
 
  - otherwise
-	scsi_eh_scmd_add(scmd, 0) is invoked for the command.  See
+	scsi_eh_scmd_add(scmd) is invoked for the command.  See
 	[1-3] for details of this function.
 
 

@@ -103,9 +103,7 @@ function
         eh_timed_out() callback did not handle the command.
 	Step #2 is taken.
 
- 2. If the host supports asynchronous completion (as indicated by the
-    no_async_abort setting in the host template) scsi_abort_command()
-    is invoked to schedule an asynchrous abort.
+ 2. scsi_abort_command() is invoked to schedule an asynchrous abort.
     Asynchronous abort are not invoked for commands which the
     SCSI_EH_ABORT_SCHEDULED flag is set (this indicates that the command
     already had been aborted once, and this is a retry which failed),

@@ -127,16 +125,13 @@ function
 
  scmds enter EH via scsi_eh_scmd_add(), which does the following.
 
- 1. Turns on scmd->eh_eflags as requested.  It's 0 for error
-    completions and SCSI_EH_CANCEL_CMD for timeouts.
+ 1. Links scmd->eh_entry to shost->eh_cmd_q
 
- 2. Links scmd->eh_entry to shost->eh_cmd_q
+ 2. Sets SHOST_RECOVERY bit in shost->shost_state
 
- 3. Sets SHOST_RECOVERY bit in shost->shost_state
+ 3. Increments shost->host_failed
 
- 4. Increments shost->host_failed
-
- 5. Wakes up SCSI EH thread if shost->host_busy == shost->host_failed
+ 4. Wakes up SCSI EH thread if shost->host_busy == shost->host_failed
 
  As can be seen above, once any scmd is added to shost->eh_cmd_q,
 SHOST_RECOVERY shost_state bit is turned on.  This prevents any new

@@ -252,7 +247,6 @@ scmd->allowed.
 
  1. Error completion / time out
     ACTION: scsi_eh_scmd_add() is invoked for scmd
-	- set scmd->eh_eflags
 	- add scmd to shost->eh_cmd_q
 	- set SHOST_RECOVERY
 	- shost->host_failed++

drivers/scsi/scsi_error.c +7 -74

index 802a081..7b70ee9 100644
@@ -163,7 +163,7 @@ static int scsi_host_eh_past_deadline(struct Scsi_Host *shost)
 		}
 	}
 
-	scsi_eh_scmd_add(scmd, 0);
+	scsi_eh_scmd_add(scmd);
 }
 
 /**

@@ -217,9 +217,8 @@ static int scsi_host_eh_past_deadline(struct Scsi_Host *shost)
 /**
  * scsi_eh_scmd_add - add scsi cmd to error handling.
  * @scmd:	scmd to run eh on.
- * @eh_flag:	optional SCSI_EH flag.
  */
-void scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
+void scsi_eh_scmd_add(struct scsi_cmnd *scmd)
 {
 	struct Scsi_Host *shost = scmd->device->host;
 	unsigned long flags;

@@ -235,9 +234,6 @@ void scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
 	if (shost->eh_deadline != -1 && !shost->last_reset)
 		shost->last_reset = jiffies;
 
-	if (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED)
-		eh_flag &= ~SCSI_EH_CANCEL_CMD;
-	scmd->eh_eflags |= eh_flag;
 	scsi_eh_reset(scmd);
 	list_add_tail(&scmd->eh_entry, &shost->eh_cmd_q);
 	shost->host_failed++;

@@ -271,10 +267,9 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
 		rtn = host->hostt->eh_timed_out(scmd);
 
 	if (rtn == BLK_EH_NOT_HANDLED) {
-		if (host->hostt->no_async_abort ||
-		    scsi_abort_command(scmd) != SUCCESS) {
+		if (scsi_abort_command(scmd) != SUCCESS) {
 			set_host_byte(scmd, DID_TIME_OUT);
-			scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD);
+			scsi_eh_scmd_add(scmd);
 		}
 	}
 

@@ -327,7 +322,7 @@ static inline void scsi_eh_prt_fail_stats(struct Scsi_Host *shost,
 		list_for_each_entry(scmd, work_q, eh_entry) {
 			if (scmd->device == sdev) {
 				++total_failures;
-				if (scmd->eh_eflags & SCSI_EH_CANCEL_CMD)
+				if (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED)
 					++cmd_cancel;
 				else
 					++cmd_failed;

@@ -1153,8 +1148,7 @@ int scsi_eh_get_sense(struct list_head *work_q,
 	 * should not get sense.
 	 */
 	list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
-		if ((scmd->eh_eflags & SCSI_EH_CANCEL_CMD) ||
-		    (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED) ||
+		if ((scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED) ||
 		    SCSI_SENSE_VALID(scmd))
 			continue;
 

@@ -1294,61 +1288,6 @@ static int scsi_eh_test_devices(struct list_head *cmd_list,
 	return list_empty(work_q);
 }
 
-
-/**
- * scsi_eh_abort_cmds - abort pending commands.
- * @work_q:	&list_head for pending commands.
- * @done_q:	&list_head for processed commands.
- *
- * Decription:
- *    Try and see whether or not it makes sense to try and abort the
- *    running command.  This only works out to be the case if we have one
- *    command that has timed out.  If the command simply failed, it makes
- *    no sense to try and abort the command, since as far as the shost
- *    adapter is concerned, it isn't running.
- */
-static int scsi_eh_abort_cmds(struct list_head *work_q,
-			      struct list_head *done_q)
-{
-	struct scsi_cmnd *scmd, *next;
-	LIST_HEAD(check_list);
-	int rtn;
-	struct Scsi_Host *shost;
-
-	list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
-		if (!(scmd->eh_eflags & SCSI_EH_CANCEL_CMD))
-			continue;
-		shost = scmd->device->host;
-		if (scsi_host_eh_past_deadline(shost)) {
-			list_splice_init(&check_list, work_q);
-			SCSI_LOG_ERROR_RECOVERY(3,
-				scmd_printk(KERN_INFO, scmd,
-					    "%s: skip aborting cmd, past eh deadline\n",
-					    current->comm));
-			return list_empty(work_q);
-		}
-		SCSI_LOG_ERROR_RECOVERY(3,
-			scmd_printk(KERN_INFO, scmd,
-				     "%s: aborting cmd\n", current->comm));
-		rtn = scsi_try_to_abort_cmd(shost->hostt, scmd);
-		if (rtn == FAILED) {
-			SCSI_LOG_ERROR_RECOVERY(3,
-				scmd_printk(KERN_INFO, scmd,
-					    "%s: aborting cmd failed\n",
-					     current->comm));
-			list_splice_init(&check_list, work_q);
-			return list_empty(work_q);
-		}
-		scmd->eh_eflags &= ~SCSI_EH_CANCEL_CMD;
-		if (rtn == FAST_IO_FAIL)
-			scsi_eh_finish_cmd(scmd, done_q);
-		else
-			list_move_tail(&scmd->eh_entry, &check_list);
-	}
-
-	return scsi_eh_test_devices(&check_list, work_q, done_q, 0);
-}
-
 /**
  * scsi_eh_try_stu - Send START_UNIT to device.
  * @scmd:	&scsi_cmnd to send START_UNIT

@@ -1691,11 +1630,6 @@ static void scsi_eh_offline_sdevs(struct list_head *work_q,
 		sdev_printk(KERN_INFO, scmd->device, "Device offlined - "
 			    "not ready after error recovery\n");
 		scsi_device_set_state(scmd->device, SDEV_OFFLINE);
-		if (scmd->eh_eflags & SCSI_EH_CANCEL_CMD) {
-			/*
-			 * FIXME: Handle lost cmds.
-			 */
-		}
 		scsi_eh_finish_cmd(scmd, done_q);
 	}
 	return;

@@ -2139,8 +2073,7 @@ static void scsi_unjam_host(struct Scsi_Host *shost)
 	SCSI_LOG_ERROR_RECOVERY(1, scsi_eh_prt_fail_stats(shost, &eh_work_q));
 
 	if (!scsi_eh_get_sense(&eh_work_q, &eh_done_q))
-		if (!scsi_eh_abort_cmds(&eh_work_q, &eh_done_q))
-			scsi_eh_ready_devs(shost, &eh_work_q, &eh_done_q);
+		scsi_eh_ready_devs(shost, &eh_work_q, &eh_done_q);
 
 	spin_lock_irqsave(shost->host_lock, flags);
 	if (shost->eh_deadline != -1)

drivers/scsi/scsi_lib.c +1 -1

index 0735a46..98b2df8 100644
@@ -1593,7 +1593,7 @@ static void scsi_softirq_done(struct request *rq)
 			scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY);
 			break;
 		default:
-			scsi_eh_scmd_add(cmd, 0);
+			scsi_eh_scmd_add(cmd);
 			break;
 	}
 }

drivers/scsi/scsi_priv.h +1 -2

index 5be6cbf6..e20ab10 100644
@@ -18,7 +18,6 @@
 /*
  * Scsi Error Handler Flags
  */
-#define SCSI_EH_CANCEL_CMD	0x0001	/* Cancel this cmd */
 #define SCSI_EH_ABORT_SCHEDULED	0x0002	/* Abort has been scheduled */
 
 #define SCSI_SENSE_VALID(scmd) \

@@ -72,7 +71,7 @@ extern int scsi_dev_info_list_add_keyed(int compatible, char *vendor,
 extern int scsi_error_handler(void *host);
 extern int scsi_decide_disposition(struct scsi_cmnd *cmd);
 extern void scsi_eh_wakeup(struct Scsi_Host *shost);
-extern void scsi_eh_scmd_add(struct scsi_cmnd *, int);
+extern void scsi_eh_scmd_add(struct scsi_cmnd *);
 void scsi_eh_ready_devs(struct Scsi_Host *shost,
 			struct list_head *work_q,
 			struct list_head *done_q);

include/scsi/scsi_host.h +0 -5

index 3cd8c3b..afb0481 100644
@@ -452,11 +452,6 @@ struct scsi_host_template {
 	unsigned no_write_same:1;
 
 	/*
-	 * True if asynchronous aborts are not supported
-	 */
-	unsigned no_async_abort:1;
-
-	/*
 	 * Countdown for host blocking with no commands outstanding.
 	 */
 	unsigned int max_host_blocked;

Signature · 1 line

1.8.5.6

Hmm so, I guess we compromise in terms of how granular we want to recover?

When say an abort for command A on LUN 1 behind Port α fails for some reason, then we also skip all possible aborts for command B on LUN 2 behind Port α and command C on LUN 1 behind Port β? (The host might already be in recovery by the time command B and C fail)

I mean, in the end, all traffic holds on that host anyway, as long as we get into EH - which is true as soon as one abort fails. Might as well skip the wait time till we get there.

I would like it, if we could document that fact a bit more direct. Otherwise I think that's good. The code also looks good.

Reviewed-by Benjamin Block <[email protected]>

Quoted 278 lines · Benjamin Block

Hello Hannes,

Quoted 269 lines · Hannes Reinecke

There hasn't been any reports for HBAs where asynchronous abort would not work, so we should make it mandatory and remove the fallback.

Signed-off-by Hannes Reinecke <[email protected]>

Reviewed-by Johannes Thumshirn <[email protected]>

Reviewed-by Bart Van Assche <[email protected]>

---
 Documentation/scsi/scsi_eh.txt | 18 ++++------
 drivers/scsi/scsi_error.c      | 81 ++++--------------------------------------
 drivers/scsi/scsi_lib.c        |  2 +-
 drivers/scsi/scsi_priv.h       |  3 +-
 include/scsi/scsi_host.h       |  5 ---
 5 files changed, 15 insertions(+), 94 deletions(-)

Documentation/scsi/scsi_eh.txt +6 -12

index 4edb9c1c..11e447b 100644
@@ -70,7 +70,7 @@ with the command.
 	scmd is requeued to blk queue.
 
  - otherwise
-	scsi_eh_scmd_add(scmd, 0) is invoked for the command.  See
+	scsi_eh_scmd_add(scmd) is invoked for the command.  See
 	[1-3] for details of this function.
 
 

@@ -103,9 +103,7 @@ function
         eh_timed_out() callback did not handle the command.
 	Step #2 is taken.
 
- 2. If the host supports asynchronous completion (as indicated by the
-    no_async_abort setting in the host template) scsi_abort_command()
-    is invoked to schedule an asynchrous abort.
+ 2. scsi_abort_command() is invoked to schedule an asynchrous abort.
     Asynchronous abort are not invoked for commands which the
     SCSI_EH_ABORT_SCHEDULED flag is set (this indicates that the command
     already had been aborted once, and this is a retry which failed),

@@ -127,16 +125,13 @@ function
 
  scmds enter EH via scsi_eh_scmd_add(), which does the following.
 
- 1. Turns on scmd->eh_eflags as requested.  It's 0 for error
-    completions and SCSI_EH_CANCEL_CMD for timeouts.
+ 1. Links scmd->eh_entry to shost->eh_cmd_q
 
- 2. Links scmd->eh_entry to shost->eh_cmd_q
+ 2. Sets SHOST_RECOVERY bit in shost->shost_state
 
- 3. Sets SHOST_RECOVERY bit in shost->shost_state
+ 3. Increments shost->host_failed
 
- 4. Increments shost->host_failed
-
- 5. Wakes up SCSI EH thread if shost->host_busy == shost->host_failed
+ 4. Wakes up SCSI EH thread if shost->host_busy == shost->host_failed
 
  As can be seen above, once any scmd is added to shost->eh_cmd_q,
 SHOST_RECOVERY shost_state bit is turned on.  This prevents any new

@@ -252,7 +247,6 @@ scmd->allowed.
 
  1. Error completion / time out
     ACTION: scsi_eh_scmd_add() is invoked for scmd
-	- set scmd->eh_eflags
 	- add scmd to shost->eh_cmd_q
 	- set SHOST_RECOVERY
 	- shost->host_failed++

drivers/scsi/scsi_error.c +7 -74

index 802a081..7b70ee9 100644
@@ -163,7 +163,7 @@ static int scsi_host_eh_past_deadline(struct Scsi_Host *shost)
 		}
 	}
 
-	scsi_eh_scmd_add(scmd, 0);
+	scsi_eh_scmd_add(scmd);
 }
 
 /**

@@ -217,9 +217,8 @@ static int scsi_host_eh_past_deadline(struct Scsi_Host *shost)
 /**
  * scsi_eh_scmd_add - add scsi cmd to error handling.
  * @scmd:	scmd to run eh on.
- * @eh_flag:	optional SCSI_EH flag.
  */
-void scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
+void scsi_eh_scmd_add(struct scsi_cmnd *scmd)
 {
 	struct Scsi_Host *shost = scmd->device->host;
 	unsigned long flags;

@@ -235,9 +234,6 @@ void scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
 	if (shost->eh_deadline != -1 && !shost->last_reset)
 		shost->last_reset = jiffies;
 
-	if (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED)
-		eh_flag &= ~SCSI_EH_CANCEL_CMD;
-	scmd->eh_eflags |= eh_flag;
 	scsi_eh_reset(scmd);
 	list_add_tail(&scmd->eh_entry, &shost->eh_cmd_q);
 	shost->host_failed++;

@@ -271,10 +267,9 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
 		rtn = host->hostt->eh_timed_out(scmd);
 
 	if (rtn == BLK_EH_NOT_HANDLED) {
-		if (host->hostt->no_async_abort ||
-		    scsi_abort_command(scmd) != SUCCESS) {
+		if (scsi_abort_command(scmd) != SUCCESS) {
 			set_host_byte(scmd, DID_TIME_OUT);
-			scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD);
+			scsi_eh_scmd_add(scmd);
 		}
 	}
 

@@ -327,7 +322,7 @@ static inline void scsi_eh_prt_fail_stats(struct Scsi_Host *shost,
 		list_for_each_entry(scmd, work_q, eh_entry) {
 			if (scmd->device == sdev) {
 				++total_failures;
-				if (scmd->eh_eflags & SCSI_EH_CANCEL_CMD)
+				if (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED)
 					++cmd_cancel;
 				else
 					++cmd_failed;

@@ -1153,8 +1148,7 @@ int scsi_eh_get_sense(struct list_head *work_q,
 	 * should not get sense.
 	 */
 	list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
-		if ((scmd->eh_eflags & SCSI_EH_CANCEL_CMD) ||
-		    (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED) ||
+		if ((scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED) ||
 		    SCSI_SENSE_VALID(scmd))
 			continue;
 

@@ -1294,61 +1288,6 @@ static int scsi_eh_test_devices(struct list_head *cmd_list,
 	return list_empty(work_q);
 }
 
-
-/**
- * scsi_eh_abort_cmds - abort pending commands.
- * @work_q:	&list_head for pending commands.
- * @done_q:	&list_head for processed commands.
- *
- * Decription:
- *    Try and see whether or not it makes sense to try and abort the
- *    running command.  This only works out to be the case if we have one
- *    command that has timed out.  If the command simply failed, it makes
- *    no sense to try and abort the command, since as far as the shost
- *    adapter is concerned, it isn't running.
- */
-static int scsi_eh_abort_cmds(struct list_head *work_q,
-			      struct list_head *done_q)
-{
-	struct scsi_cmnd *scmd, *next;
-	LIST_HEAD(check_list);
-	int rtn;
-	struct Scsi_Host *shost;
-
-	list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
-		if (!(scmd->eh_eflags & SCSI_EH_CANCEL_CMD))
-			continue;
-		shost = scmd->device->host;
-		if (scsi_host_eh_past_deadline(shost)) {
-			list_splice_init(&check_list, work_q);
-			SCSI_LOG_ERROR_RECOVERY(3,
-				scmd_printk(KERN_INFO, scmd,
-					    "%s: skip aborting cmd, past eh deadline\n",
-					    current->comm));
-			return list_empty(work_q);
-		}
-		SCSI_LOG_ERROR_RECOVERY(3,
-			scmd_printk(KERN_INFO, scmd,
-				     "%s: aborting cmd\n", current->comm));
-		rtn = scsi_try_to_abort_cmd(shost->hostt, scmd);
-		if (rtn == FAILED) {
-			SCSI_LOG_ERROR_RECOVERY(3,
-				scmd_printk(KERN_INFO, scmd,
-					    "%s: aborting cmd failed\n",
-					     current->comm));
-			list_splice_init(&check_list, work_q);
-			return list_empty(work_q);
-		}
-		scmd->eh_eflags &= ~SCSI_EH_CANCEL_CMD;
-		if (rtn == FAST_IO_FAIL)
-			scsi_eh_finish_cmd(scmd, done_q);
-		else
-			list_move_tail(&scmd->eh_entry, &check_list);
-	}
-
-	return scsi_eh_test_devices(&check_list, work_q, done_q, 0);
-}
-
 /**
  * scsi_eh_try_stu - Send START_UNIT to device.
  * @scmd:	&scsi_cmnd to send START_UNIT

@@ -1691,11 +1630,6 @@ static void scsi_eh_offline_sdevs(struct list_head *work_q,
 		sdev_printk(KERN_INFO, scmd->device, "Device offlined - "
 			    "not ready after error recovery\n");
 		scsi_device_set_state(scmd->device, SDEV_OFFLINE);
-		if (scmd->eh_eflags & SCSI_EH_CANCEL_CMD) {
-			/*
-			 * FIXME: Handle lost cmds.
-			 */
-		}
 		scsi_eh_finish_cmd(scmd, done_q);
 	}
 	return;

@@ -2139,8 +2073,7 @@ static void scsi_unjam_host(struct Scsi_Host *shost)
 	SCSI_LOG_ERROR_RECOVERY(1, scsi_eh_prt_fail_stats(shost, &eh_work_q));
 
 	if (!scsi_eh_get_sense(&eh_work_q, &eh_done_q))
-		if (!scsi_eh_abort_cmds(&eh_work_q, &eh_done_q))
-			scsi_eh_ready_devs(shost, &eh_work_q, &eh_done_q);
+		scsi_eh_ready_devs(shost, &eh_work_q, &eh_done_q);
 
 	spin_lock_irqsave(shost->host_lock, flags);
 	if (shost->eh_deadline != -1)

drivers/scsi/scsi_lib.c +1 -1

index 0735a46..98b2df8 100644
@@ -1593,7 +1593,7 @@ static void scsi_softirq_done(struct request *rq)
 			scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY);
 			break;
 		default:
-			scsi_eh_scmd_add(cmd, 0);
+			scsi_eh_scmd_add(cmd);
 			break;
 	}
 }

drivers/scsi/scsi_priv.h +1 -2

index 5be6cbf6..e20ab10 100644
@@ -18,7 +18,6 @@
 /*
  * Scsi Error Handler Flags
  */
-#define SCSI_EH_CANCEL_CMD	0x0001	/* Cancel this cmd */
 #define SCSI_EH_ABORT_SCHEDULED	0x0002	/* Abort has been scheduled */
 
 #define SCSI_SENSE_VALID(scmd) \

@@ -72,7 +71,7 @@ extern int scsi_dev_info_list_add_keyed(int compatible, char *vendor,
 extern int scsi_error_handler(void *host);
 extern int scsi_decide_disposition(struct scsi_cmnd *cmd);
 extern void scsi_eh_wakeup(struct Scsi_Host *shost);
-extern void scsi_eh_scmd_add(struct scsi_cmnd *, int);
+extern void scsi_eh_scmd_add(struct scsi_cmnd *);
 void scsi_eh_ready_devs(struct Scsi_Host *shost,
 			struct list_head *work_q,
 			struct list_head *done_q);

include/scsi/scsi_host.h +0 -6

index 3cd8c3b..afb0481 100644
@@ -452,11 +452,6 @@ struct scsi_host_template {
 	unsigned no_write_same:1;
 
 	/*
-	 * True if asynchronous aborts are not supported
-	 */
-	unsigned no_async_abort:1;
-
-	/*
 	 * Countdown for host blocking with no commands outstanding.
 	 */
 	unsigned int max_host_blocked;
--

Quoted 2 lines

1.8.5.6

Hmm so, I guess we compromise in terms of how granular we want to recover?

Quoted 5 lines

When say an abort for command A on LUN 1 behind Port α fails for some reason, then we also skip all possible aborts for command B on LUN 2 behind Port α and command C on LUN 1 behind Port β? (The host might already be in recovery by the time command B and C fail)

No.
Recovery will only be started once all commands have been completed or aborted.
Hence will be be aborting all commands before entering SCSI EH.

Quoted 5 lines

I mean, in the end, all traffic holds on that host anyway, as long as we get into EH - which is true as soon as one abort fails. Might as well skip the wait time till we get there.

There still is a benefit to be had from sending individual aborts; after all, the commands are supposed to be independently, and sending an abort serves as a pointer to the LLDD to remove any reference to it.

Quoted 6 lines

I would like it, if we could document that fact a bit more direct. Otherwise I think that's good. The code also looks good.

Reviewed-by Benjamin Block <[email protected]>

Quoted 284 lines · Hannes Reinecke

Quoted 278 lines · Benjamin Block

Hello Hannes,

Quoted 269 lines · Hannes Reinecke

There hasn't been any reports for HBAs where asynchronous abort would not work, so we should make it mandatory and remove the fallback.

Signed-off-by Hannes Reinecke <[email protected]>

Reviewed-by Johannes Thumshirn <[email protected]>

Reviewed-by Bart Van Assche <[email protected]>

---
 Documentation/scsi/scsi_eh.txt | 18 ++++------
 drivers/scsi/scsi_error.c      | 81 ++++--------------------------------------
 drivers/scsi/scsi_lib.c        |  2 +-
 drivers/scsi/scsi_priv.h       |  3 +-
 include/scsi/scsi_host.h       |  5 ---
 5 files changed, 15 insertions(+), 94 deletions(-)

Documentation/scsi/scsi_eh.txt +6 -12

index 4edb9c1c..11e447b 100644
@@ -70,7 +70,7 @@ with the command.
 	scmd is requeued to blk queue.
 
  - otherwise
-	scsi_eh_scmd_add(scmd, 0) is invoked for the command.  See
+	scsi_eh_scmd_add(scmd) is invoked for the command.  See
 	[1-3] for details of this function.
 
 

@@ -103,9 +103,7 @@ function
         eh_timed_out() callback did not handle the command.
 	Step #2 is taken.
 
- 2. If the host supports asynchronous completion (as indicated by the
-    no_async_abort setting in the host template) scsi_abort_command()
-    is invoked to schedule an asynchrous abort.
+ 2. scsi_abort_command() is invoked to schedule an asynchrous abort.
     Asynchronous abort are not invoked for commands which the
     SCSI_EH_ABORT_SCHEDULED flag is set (this indicates that the command
     already had been aborted once, and this is a retry which failed),

@@ -127,16 +125,13 @@ function
 
  scmds enter EH via scsi_eh_scmd_add(), which does the following.
 
- 1. Turns on scmd->eh_eflags as requested.  It's 0 for error
-    completions and SCSI_EH_CANCEL_CMD for timeouts.
+ 1. Links scmd->eh_entry to shost->eh_cmd_q
 
- 2. Links scmd->eh_entry to shost->eh_cmd_q
+ 2. Sets SHOST_RECOVERY bit in shost->shost_state
 
- 3. Sets SHOST_RECOVERY bit in shost->shost_state
+ 3. Increments shost->host_failed
 
- 4. Increments shost->host_failed
-
- 5. Wakes up SCSI EH thread if shost->host_busy == shost->host_failed
+ 4. Wakes up SCSI EH thread if shost->host_busy == shost->host_failed
 
  As can be seen above, once any scmd is added to shost->eh_cmd_q,
 SHOST_RECOVERY shost_state bit is turned on.  This prevents any new

@@ -252,7 +247,6 @@ scmd->allowed.
 
  1. Error completion / time out
     ACTION: scsi_eh_scmd_add() is invoked for scmd
-	- set scmd->eh_eflags
 	- add scmd to shost->eh_cmd_q
 	- set SHOST_RECOVERY
 	- shost->host_failed++

drivers/scsi/scsi_error.c +7 -74

index 802a081..7b70ee9 100644
@@ -163,7 +163,7 @@ static int scsi_host_eh_past_deadline(struct Scsi_Host *shost)
 		}
 	}
 
-	scsi_eh_scmd_add(scmd, 0);
+	scsi_eh_scmd_add(scmd);
 }
 
 /**

@@ -217,9 +217,8 @@ static int scsi_host_eh_past_deadline(struct Scsi_Host *shost)
 /**
  * scsi_eh_scmd_add - add scsi cmd to error handling.
  * @scmd:	scmd to run eh on.
- * @eh_flag:	optional SCSI_EH flag.
  */
-void scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
+void scsi_eh_scmd_add(struct scsi_cmnd *scmd)
 {
 	struct Scsi_Host *shost = scmd->device->host;
 	unsigned long flags;

@@ -235,9 +234,6 @@ void scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
 	if (shost->eh_deadline != -1 && !shost->last_reset)
 		shost->last_reset = jiffies;
 
-	if (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED)
-		eh_flag &= ~SCSI_EH_CANCEL_CMD;
-	scmd->eh_eflags |= eh_flag;
 	scsi_eh_reset(scmd);
 	list_add_tail(&scmd->eh_entry, &shost->eh_cmd_q);
 	shost->host_failed++;

@@ -271,10 +267,9 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
 		rtn = host->hostt->eh_timed_out(scmd);
 
 	if (rtn == BLK_EH_NOT_HANDLED) {
-		if (host->hostt->no_async_abort ||
-		    scsi_abort_command(scmd) != SUCCESS) {
+		if (scsi_abort_command(scmd) != SUCCESS) {
 			set_host_byte(scmd, DID_TIME_OUT);
-			scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD);
+			scsi_eh_scmd_add(scmd);
 		}
 	}
 

@@ -327,7 +322,7 @@ static inline void scsi_eh_prt_fail_stats(struct Scsi_Host *shost,
 		list_for_each_entry(scmd, work_q, eh_entry) {
 			if (scmd->device == sdev) {
 				++total_failures;
-				if (scmd->eh_eflags & SCSI_EH_CANCEL_CMD)
+				if (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED)
 					++cmd_cancel;
 				else
 					++cmd_failed;

@@ -1153,8 +1148,7 @@ int scsi_eh_get_sense(struct list_head *work_q,
 	 * should not get sense.
 	 */
 	list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
-		if ((scmd->eh_eflags & SCSI_EH_CANCEL_CMD) ||
-		    (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED) ||
+		if ((scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED) ||
 		    SCSI_SENSE_VALID(scmd))
 			continue;
 

@@ -1294,61 +1288,6 @@ static int scsi_eh_test_devices(struct list_head *cmd_list,
 	return list_empty(work_q);
 }
 
-
-/**
- * scsi_eh_abort_cmds - abort pending commands.
- * @work_q:	&list_head for pending commands.
- * @done_q:	&list_head for processed commands.
- *
- * Decription:
- *    Try and see whether or not it makes sense to try and abort the
- *    running command.  This only works out to be the case if we have one
- *    command that has timed out.  If the command simply failed, it makes
- *    no sense to try and abort the command, since as far as the shost
- *    adapter is concerned, it isn't running.
- */
-static int scsi_eh_abort_cmds(struct list_head *work_q,
-			      struct list_head *done_q)
-{
-	struct scsi_cmnd *scmd, *next;
-	LIST_HEAD(check_list);
-	int rtn;
-	struct Scsi_Host *shost;
-
-	list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
-		if (!(scmd->eh_eflags & SCSI_EH_CANCEL_CMD))
-			continue;
-		shost = scmd->device->host;
-		if (scsi_host_eh_past_deadline(shost)) {
-			list_splice_init(&check_list, work_q);
-			SCSI_LOG_ERROR_RECOVERY(3,
-				scmd_printk(KERN_INFO, scmd,
-					    "%s: skip aborting cmd, past eh deadline\n",
-					    current->comm));
-			return list_empty(work_q);
-		}
-		SCSI_LOG_ERROR_RECOVERY(3,
-			scmd_printk(KERN_INFO, scmd,
-				     "%s: aborting cmd\n", current->comm));
-		rtn = scsi_try_to_abort_cmd(shost->hostt, scmd);
-		if (rtn == FAILED) {
-			SCSI_LOG_ERROR_RECOVERY(3,
-				scmd_printk(KERN_INFO, scmd,
-					    "%s: aborting cmd failed\n",
-					     current->comm));
-			list_splice_init(&check_list, work_q);
-			return list_empty(work_q);
-		}
-		scmd->eh_eflags &= ~SCSI_EH_CANCEL_CMD;
-		if (rtn == FAST_IO_FAIL)
-			scsi_eh_finish_cmd(scmd, done_q);
-		else
-			list_move_tail(&scmd->eh_entry, &check_list);
-	}
-
-	return scsi_eh_test_devices(&check_list, work_q, done_q, 0);
-}
-
 /**
  * scsi_eh_try_stu - Send START_UNIT to device.
  * @scmd:	&scsi_cmnd to send START_UNIT

@@ -1691,11 +1630,6 @@ static void scsi_eh_offline_sdevs(struct list_head *work_q,
 		sdev_printk(KERN_INFO, scmd->device, "Device offlined - "
 			    "not ready after error recovery\n");
 		scsi_device_set_state(scmd->device, SDEV_OFFLINE);
-		if (scmd->eh_eflags & SCSI_EH_CANCEL_CMD) {
-			/*
-			 * FIXME: Handle lost cmds.
-			 */
-		}
 		scsi_eh_finish_cmd(scmd, done_q);
 	}
 	return;

@@ -2139,8 +2073,7 @@ static void scsi_unjam_host(struct Scsi_Host *shost)
 	SCSI_LOG_ERROR_RECOVERY(1, scsi_eh_prt_fail_stats(shost, &eh_work_q));
 
 	if (!scsi_eh_get_sense(&eh_work_q, &eh_done_q))
-		if (!scsi_eh_abort_cmds(&eh_work_q, &eh_done_q))
-			scsi_eh_ready_devs(shost, &eh_work_q, &eh_done_q);
+		scsi_eh_ready_devs(shost, &eh_work_q, &eh_done_q);
 
 	spin_lock_irqsave(shost->host_lock, flags);
 	if (shost->eh_deadline != -1)

drivers/scsi/scsi_lib.c +1 -1

index 0735a46..98b2df8 100644
@@ -1593,7 +1593,7 @@ static void scsi_softirq_done(struct request *rq)
 			scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY);
 			break;
 		default:
-			scsi_eh_scmd_add(cmd, 0);
+			scsi_eh_scmd_add(cmd);
 			break;
 	}
 }

drivers/scsi/scsi_priv.h +1 -2

index 5be6cbf6..e20ab10 100644
@@ -18,7 +18,6 @@
 /*
  * Scsi Error Handler Flags
  */
-#define SCSI_EH_CANCEL_CMD	0x0001	/* Cancel this cmd */
 #define SCSI_EH_ABORT_SCHEDULED	0x0002	/* Abort has been scheduled */
 
 #define SCSI_SENSE_VALID(scmd) \

@@ -72,7 +71,7 @@ extern int scsi_dev_info_list_add_keyed(int compatible, char *vendor,
 extern int scsi_error_handler(void *host);
 extern int scsi_decide_disposition(struct scsi_cmnd *cmd);
 extern void scsi_eh_wakeup(struct Scsi_Host *shost);
-extern void scsi_eh_scmd_add(struct scsi_cmnd *, int);
+extern void scsi_eh_scmd_add(struct scsi_cmnd *);
 void scsi_eh_ready_devs(struct Scsi_Host *shost,
 			struct list_head *work_q,
 			struct list_head *done_q);

include/scsi/scsi_host.h +0 -6

index 3cd8c3b..afb0481 100644
@@ -452,11 +452,6 @@ struct scsi_host_template {
 	unsigned no_write_same:1;
 
 	/*
-	 * True if asynchronous aborts are not supported
-	 */
-	unsigned no_async_abort:1;
-
-	/*
 	 * Countdown for host blocking with no commands outstanding.
 	 */
 	unsigned int max_host_blocked;
--

Quoted 2 lines

1.8.5.6

Hmm so, I guess we compromise in terms of how granular we want to recover?

Quoted 5 lines

When say an abort for command A on LUN 1 behind Port α fails for some reason, then we also skip all possible aborts for command B on LUN 2 behind Port α and command C on LUN 1 behind Port β? (The host might already be in recovery by the time command B and C fail)

Quoted 5 lines

No.
Recovery will only be started once all commands have been completed or aborted.
Hence will be be aborting all commands before entering SCSI EH.

When you call scsi_abort_command() after a timeout happend, it will check with scsi_host_in_recovery() whether the host has recovery already set or not. And only if not it will schedule the abort.

When the first command A times out, it will schedule a async abort, and if that fails it will set the state of the host in scsi_eh_scmd_add() to SHOST_RECOVERY before even the EH thread is kicked. So if command B and C timeout later than that, there won't be any abort scheduled.

That is with state tag v4.10, and your patchset. And I have actually observed this behavior in real life already (granted, the timeouts happened after injects), with the difference that previously the abort would still happen in the EH handling.