Re: [PATCH 3/4] Avoid re-enabling I/O after the transport became offline

[James Bottomley <jbottomley@parallels.com>]

On Tue, 2013-07-02 at 15:07 +0200, Bart Van Assche wrote:
> Disallow the SDEV_TRANSPORT_OFFLINE to SDEV_CANCEL transition such
> that no I/O is sent to devices for which the transport is offline.
> Notes:
> - Functions like sd_shutdown() use scsi_execute_req() and hence
>   set the REQ_PREEMPT flag. Such requests are passed to the LLD
>   queuecommand callback in the SDEV_CANCEL state.
> - This patch does not affect Fibre Channel LLD drivers since these
>   drivers invoke fc_remote_port_chkready() before submitting a SCSI
>   request to the HBA. That prevents a timeout to occur in state
>   SDEV_CANCEL if the transport is offline.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> Cc: Mike Christie <michaelc@cs.wisc.edu>
> Cc: James Bottomley <JBottomley@Parallels.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Tejun Heo <tj@kernel.org>
> ---
>  drivers/scsi/scsi_lib.c   |    1 -
>  drivers/scsi/scsi_sysfs.c |    9 ++++++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 124392f..a0fb56b 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2178,7 +2178,6 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
>  		case SDEV_RUNNING:
>  		case SDEV_QUIESCE:
>  		case SDEV_OFFLINE:
> -		case SDEV_TRANSPORT_OFFLINE:
>  		case SDEV_BLOCK:
>  			break;
>  		default:
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index 931a7d9..1711617 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -955,7 +955,14 @@ void __scsi_remove_device(struct scsi_device *sdev)
>  	struct device *dev = &sdev->sdev_gendev;
>  
>  	if (sdev->is_visible) {
> -		if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
> +		/*
> +		 * The transition from SDEV_TRANSPORT_OFFLINE into
> +		 * SDEV_CANCEL is not allowed since this transition would
> +		 * reenable I/O. However, if the device state was already
> +		 * SDEV_TRANSPORT_OFFLINE, proceed with device removal.
> +		 */
> +		if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0 &&
> +		    sdev->sdev_state != SDEV_TRANSPORT_OFFLINE)

This isn't the right way to do this, because it's adding uncharted state
to the state model.  What should happen is that this should be reflected
in the actual state model.  It sounds like we need a CANCEL_OFFLINE
state to which TRANSPORT_OFFLINE (and possibly OFFLINE) can transition.

The comment on the transition should state that CANCEL_OFFLINE won't
allow any I/O.

James