Re: [PATCH 4/5] scsi: make asynchronous aborts mandatory

[Johannes Thumshirn <jthumshirn@suse.de>]

On Thu, 2015-12-03 at 08:17 +0100, Hannes Reinecke wrote:
> 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 <hare@suse.de>
> ---
>  Documentation/scsi/scsi_eh.txt | 28 +++++++--------
>  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, 22 insertions(+), 97 deletions(-)
> 
> diff --git a/Documentation/scsi/scsi_eh.txt b/Documentation/scsi/scsi_eh.txt
> index 745eed5..6e07245fb 100644
> --- a/Documentation/scsi/scsi_eh.txt
> +++ b/Documentation/scsi/scsi_eh.txt
> @@ -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,13 +103,15 @@ 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. If that fails
> -    Step #3 is taken.
> + 2. scsi_abort_command() is invoked to schedule an asynchrous abort
> +    (Seee [1-3] for more information).
> +    Asynchronous abort are not invoked for commands which have
> +    SCSI_EH_ABORT_SCHEDULED 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) is invoked for the
> +    command.  See [1-4] for more information.
>  
>  [1-3] Asynchronous command aborts
>  
> @@ -124,16 +126,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
> @@ -249,7 +248,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++
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index eb0f19f..cf47b81 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -162,7 +162,7 @@ scmd_eh_abort_handler(struct work_struct *work)
>  		}
>  	}
>  
> -	scsi_eh_scmd_add(scmd, 0);
> +	scsi_eh_scmd_add(scmd);
>  }
>  
>  /**
> @@ -216,9 +216,8 @@ scsi_abort_command(struct scsi_cmnd *scmd)
>  /**
>   * 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;
> @@ -236,9 +235,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;
>  	list_add_tail(&scmd->eh_entry, &shost->eh_cmd_q);
>  	shost->host_failed++;
>  	scsi_eh_wakeup(shost);
> @@ -273,10 +269,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);
>  		}
>  	}
>  
> @@ -329,7 +324,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;
> @@ -1152,8 +1147,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;
>  
> @@ -1293,61 +1287,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
> @@ -1690,11 +1629,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;
> @@ -2138,8 +2072,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)
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 2dd7d0a..616e074 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1647,7 +1647,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;
>  	}
>  }
> diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
> index 8c26823..5534825 100644
> --- a/drivers/scsi/scsi_priv.h
> +++ b/drivers/scsi/scsi_priv.h
> @@ -18,7 +18,6 @@ struct scsi_nl_hdr;
>  /*
>   * 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) \
> @@ -71,7 +70,7 @@ extern enum blk_eh_timer_return scsi_times_out(struct
> request *req);
>  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);
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index ed52712..ef7fff6 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -444,11 +444,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;

Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html