Re: [PATCH v3 3/4] sd: Make synchronize cache upon shutdown asynchronous

[Bart Van Assche <Bart.VanAssche@sandisk.com>]

On Tue, 2017-04-18 at 16:56 -0700, James Bottomley wrote:
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index e5a2d590a104..31171204cfd1 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2611,7 +2611,6 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
>  		case SDEV_QUIESCE:
>  		case SDEV_OFFLINE:
>  		case SDEV_TRANSPORT_OFFLINE:
> -		case SDEV_BLOCK:
>  			break;
>  		default:
>  			goto illegal;
> @@ -2625,6 +2624,7 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
>  		case SDEV_OFFLINE:
>  		case SDEV_TRANSPORT_OFFLINE:
>  		case SDEV_CANCEL:
> +		case SDEV_BLOCK:
>  		case SDEV_CREATED_BLOCK:
>  			break;
>  		default:
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index 82dfe07b1d47..e477f95bf169 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -1282,8 +1282,17 @@ void __scsi_remove_device(struct scsi_device *sdev)
>  		return;
>  
>  	if (sdev->is_visible) {
> -		if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
> -			return;
> +		/*
> +		 * If blocked, we go straight to DEL so any commands
> +		 * issued during the driver shutdown (like sync cache)
> +		 * are errored
> +		 */
> +		if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0) {
> +			if (scsi_device_set_state(sdev, SDEV_DEL) != 0)
> +				return;
> +			else
> +				scsi_start_queue(sdev);
> +		}
>  
>  		bsg_unregister_queue(sdev->request_queue);
>  		device_unregister(&sdev->sdev_dev);

Hello James,

This approach cannot work. A scsi_target_block() call by the transport
layer can happen concurrently with the __scsi_remove_device() call and hence
can occur at any time between the scsi_start_queue() call by
__scsi_remove_device() and the sd_shutdown() call, resulting in a deadlock.
I have been able to trigger this with my tests by simulating a cable pull
shortly before running "rmmod ib_srp".

That deadlock did not occur with the patch series that makes synchronize
cache upon shutdown asynchronous. I'm going to resubmit that patch series.

Bart.