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

[Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com>]

Hannes,

On 03/01/2017 06:15 AM, Hannes Reinecke wrote:
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index f2cafae..cec439c 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -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;
>  }

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

Plus,

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index f2cafae..cec439c 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -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, SUCCESS, true); // return value is ignored (early 
exit due to reset)
  	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);
-	}
-	return rtn;
+	return scsi_eh_reset(scmd, rtn, false);
  }


And the rest, without the 'reset' parameter.

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index c7839f6..c794686 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1689,X +1689,Y @@ 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
   *
   *	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.
   **/
@@ -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
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 4dac35e..6a4f75a 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -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)











-- 
Mauricio Faria de Oliveira
IBM Linux Technology Center