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

[James Bottomley]

On Wed, 2017-04-26 at 18:53 +, Bart Van Assche wrote:
> On Tue, 2017-04-18 at 08:56 -0700, James Bottomley wrote:
> > The priority has got to be the removal we've been ordered to do
> > rather
> > than waiting around to see if the transport comes back so we can
> > send a
> > final flush.
> > 
> > How about this approach.  It goes straight to DEL if the device is
> > blocked (skipping CANCEL).  This means that all the commands issued
> > in 
> > ->shutdown will error in the mid-layer, thus making the removal
> > proceed
> > without being stopped.
> > 
> > James
> > 
> > ---
> > 
> > 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..788309e307e9 100644
> > --- a/drivers/scsi/scsi_sysfs.c
> > +++ b/drivers/scsi/scsi_sysfs.c
> > @@ -1282,8 +1282,14 @@ void __scsi_remove_device(struct scsi_device
> > *sdev)
> > return;
> >  
> > if (sdev->is_visible) {
> > +   /*
> > +* 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)
> > -   return;
> > +   if (scsi_device_set_state(sdev, SDEV_DEL)
> > != 0)
> > +   return;
> >  
> > bsg_unregister_queue(sdev->request_queue);
> > device_unregister(>sdev_dev);
> > 
> > 
> 
> Hello James,
> 
> How about modifying the above patch by adding a mutex to avoid that 
> the transition to SDEV_DEL and blocking the request queue happen in 
> the wrong order, e.g. as in the attached three patches?

If you've hammered it with your usual testing and it survived, I think
that's enough to prove its a concurrency problem we have to solve, so
the critical section introduction looks good.  The only refinement I
think I'd ask for is rather than creating an entirely new mutex, what
about using the host->scan_mutex?  It simplifies your code in
__scsi_remove_device because the scan mutex is already held on entry.

I'm also not very happy with a conditional mutex: that's usually
something we try to avoid.   I'd prefer an underscore version of
scsi_internal_device_block that doesn't take the mutex and which
mpt3sas uses (and make the non underscore version take the mutex and
call the underscore version).

Thanks,

James

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

[Bart Van Assche]

On Tue, 2017-04-18 at 08:56 -0700, James Bottomley wrote:
> The priority has got to be the removal we've been ordered to do rather
> than waiting around to see if the transport comes back so we can send a
> final flush.
> 
> How about this approach.  It goes straight to DEL if the device is
> blocked (skipping CANCEL).  This means that all the commands issued in 
> ->shutdown will error in the mid-layer, thus making the removal proceed
> without being stopped.
> 
> James
> 
> ---
> 
> 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..788309e307e9 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -1282,8 +1282,14 @@ void __scsi_remove_device(struct scsi_device *sdev)
>   return;
>  
>   if (sdev->is_visible) {
> + /*
> +  * 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)
> - return;
> + if (scsi_device_set_state(sdev, SDEV_DEL) != 0)
> + return;
>  
>   bsg_unregister_queue(sdev->request_queue);
>   device_unregister(>sdev_dev);
> 
> 

Hello James,

How about modifying the above patch by adding a mutex to avoid that the 
transition
to SDEV_DEL and blocking the request queue happen in the wrong order, e.g. as in
the attached three patches?

Thanks,

Bart.From c395ce2aaf6d8a644311f4c55dfa6aa560a93240 Mon Sep 17 00:00:00 2001
From: Bart Van Assche 
Date: Tue, 28 Mar 2017 14:00:17 -0700
Subject: [PATCH 1/3] Introduce scsi_start_queue()

This patch does not change any functionality.

Signed-off-by: Bart Van Assche 
Cc: Israel Rukshin 
Cc: Max Gurtovoy 
Cc: Hannes Reinecke 
Cc: Benjamin Block 
---
 drivers/scsi/scsi_lib.c  | 25 +++--
 drivers/scsi/scsi_priv.h |  1 +
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index eecc005099b2..ffa6e61299a9 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2987,6 +2987,20 @@ scsi_internal_device_block(struct scsi_device *sdev, bool wait)
 }
 EXPORT_SYMBOL_GPL(scsi_internal_device_block);
  
+void scsi_start_queue(struct scsi_device *sdev)
+{
+	struct request_queue *q = sdev->request_queue;
+	unsigned long flags;
+
+	if (q->mq_ops) {
+		blk_mq_start_stopped_hw_queues(q, false);
+	} else {
+		spin_lock_irqsave(q->queue_lock, flags);
+		blk_start_queue(q);
+		spin_unlock_irqrestore(q->queue_lock, flags);
+	}
+}
+
 /**
  * scsi_internal_device_unblock - resume a device after a block request
  * @sdev:	device to resume
@@ -3007,9 +3021,6 @@ int
 scsi_internal_device_unblock(struct scsi_device *sdev,
 			 enum scsi_device_state new_state)
 {
-	struct request_queue *q = sdev->request_queue; 
-	unsigned long flags;
-
 	/*
 	 * Try to transition the scsi device to SDEV_RUNNING or one of the
 	 * offlined states and goose the device queue if successful.
@@ -3027,13 +3038,7 @@ scsi_internal_device_unblock(struct scsi_device *sdev,
 		 sdev->sdev_state != SDEV_OFFLINE)
 		return -EINVAL;
 
-	if (q->mq_ops) {
-		blk_mq_start_stopped_hw_queues(q, false);
-	} else {
-		spin_lock_irqsave(q->queue_lock, flags);
-		blk_start_queue(q);
-		spin_unlock_irqrestore(q->queue_lock, flags);
-	}
+	scsi_start_queue(sdev);
 
 	return 0;
 }
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index f11bd102d6d5..c7629e31a75b 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -89,6 +89,7 @@ extern void scsi_run_host_queues(struct Scsi_Host *shost);
 extern void scsi_requeue_run_queue(struct work_struct *work);
 extern struct request_queue *scsi_alloc_queue(struct scsi_device *sdev);
 extern struct request_queue *scsi_mq_alloc_queue(struct scsi_device *sdev);
+extern void 

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

[Bart Van Assche]

On Sun, 2017-04-23 at 12:28 -0500, James Bottomley wrote:
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1250,6 +1250,12 @@ scsi_prep_state_check(struct scsi_device *sdev, struct 
> request *req)
>   break;
>   case SDEV_BLOCK:
>   case SDEV_CREATED_BLOCK:
> + /* q lock is held only in the non-mq case */
> + if (req->q->mq_ops)
> + blk_mq_stop_hw_queues(req->q);
> + else
> + blk_stop_queue(req->q);
> +
>   ret = BLKPREP_DEFER;
>   break;
>   case SDEV_QUIESCE:

Hello James,

This change swaps the order of changing the device state and the block layer
state. Sorry but I don't like this. What will happen if e.g. the disk event
checker decides to check for events just before __scsi_remove_device()
changes the device state? I think that can that cause sd_shutdown() to be
called with the block layer queue stopped and hence that with this approach
it is still possible that sd_shutdown() hangs.

> @@ -2611,7 +2617,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;

A previous patch made two changes to scsi_device_set_state(). Are you sure
that we do no longer have to enable the SDEV_BLOCK to SDEV_DEL transition?

> @@ -2844,10 +2849,12 @@ static int scsi_request_fn_active(struct scsi_device 
> *sdev)
>   */
>  static void scsi_wait_for_queuecommand(struct scsi_device *sdev)
>  {
> - WARN_ON_ONCE(sdev->host->use_blk_mq);
> -
> - while (scsi_request_fn_active(sdev))
> - msleep(20);
> + if (sdev->request_queue->mq_ops) {
> + synchronize_rcu();
> + } else {
> + while (scsi_request_fn_active(sdev))
> + msleep(20);
> + }
>  }

The above code makes an assumption about the block layer internals, namely
that calling synchronize_rcu() is sufficient to wait for outstanding requests
to finish. Please do not embed any assumptions about block layer internals in
SCSI code but keep code that relies on block layer internals in the block
layer. If you have a look at blk_mq_quiesce_queue() then you will see that
calling synchronize_rcu() is not sufficient for hardware queues for which
BLK_MQ_F_BLOCKING has been set. I am aware that today the SCSI core does not
set that flag. However, the dependency of the dependency of the
synchronize_rcu() call on BLK_MQ_F_BLOCKING not being set is nontrivial.

Thanks,

Bart.

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

[Bart Van Assche]

On Thu, 2017-04-20 at 15:13 -0700, James Bottomley wrote:
> How is that possible?  Once the device goes into the CANCEL state, it
> no longer can be found by starget_for_each_device() because
> scsi_device_get() returns NULL ...

scsi_target_block() is not serialized against __scsi_remove_device(). I think
the following sequence of events can cause a queue to be stopped for a device
in the CANCEL state:
(a) scsi_target_block() triggers a call to scsi_get_device().
(b) __scsi_remove_device() is called from the context of another thread.
(c) __scsi_remove_device() changes the device state into SDEV_CANCEL.
(d) scsi_internal_device_block() calls blk_mq_stop_hw_queue().

Bart.

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

[Bart Van Assche]

On Thu, 2017-04-20 at 15:13 -0700, James Bottomley wrote:
> On Thu, 2017-04-20 at 21:59 +, Bart Van Assche wrote:
> > 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.
> 
> How is that possible?  Once the device goes into the CANCEL state, it
> no longer can be found by starget_for_each_device() because
> scsi_device_get() returns NULL ... unless you also have a patch
> altering that?

No changes were made in the SCSI core other than the attached two patches.
I'm not sure about the root cause but every time I simulated a transport
layer failure before I tried to remove the ib_srp kernel module I ran into
a deadlock (see also the call traces below). Inspection of the lsscsi
output and /sys/kernel/debug/block learned me that both queues involved in
the deadlock were stopped.

sysrq: SysRq : Show Blocked State
  taskPC stack   pid father
kworker/11:3D0  2910  2 0x
Workqueue: srp_remove srp_remove_work [ib_srp]
Call Trace:
 __schedule+0x3df/0xc10
 schedule+0x3d/0x90
 schedule_timeout+0x234/0x4b0
 io_schedule_timeout+0x1e/0x50
 wait_for_completion_io_timeout+0x118/0x180
 blk_execute_rq+0x8e/0xc0
 scsi_execute+0xe7/0x200
 sd_sync_cache+0x8a/0x170
 sd_shutdown+0x5f/0xe0
 sd_remove+0x63/0xc0
 device_release_driver_internal+0x13f/0x1e0
 device_release_driver+0x12/0x20
 bus_remove_device+0x114/0x190
 device_del+0x205/0x320
 __scsi_remove_device+0x132/0x140
 scsi_forget_host+0x60/0x70
 scsi_remove_host+0x71/0x110
 srp_remove_work+0x90/0x220 [ib_srp]
 process_one_work+0x20b/0x6a0
 worker_thread+0x4e/0x4a0
 kthread+0x113/0x150
 ret_from_fork+0x2e/0x40
kworker/4:3 D0  2913  2 0x
Workqueue: srp_remove srp_remove_work [ib_srp]
Call Trace:
 __schedule+0x3df/0xc10
 schedule+0x3d/0x90
 schedule_timeout+0x234/0x4b0
 io_schedule_timeout+0x1e/0x50
 wait_for_completion_io_timeout+0x118/0x180
 blk_execute_rq+0x8e/0xc0
 scsi_execute+0xe7/0x200
 sd_sync_cache+0x8a/0x170
 sd_shutdown+0x5f/0xe0
 sd_remove+0x63/0xc0
 device_release_driver_internal+0x13f/0x1e0
 device_release_driver+0x12/0x20
 bus_remove_device+0x114/0x190
 device_del+0x205/0x320
 __scsi_remove_device+0x132/0x140
 scsi_forget_host+0x60/0x70
 scsi_remove_host+0x71/0x110
 srp_remove_work+0x90/0x220 [ib_srp]
 process_one_work+0x20b/0x6a0
 worker_thread+0x4e/0x4a0
 kthread+0x113/0x150
 ret_from_fork+0x2e/0x40
modprobeD0  2916   2218 0x
Call Trace:
 __schedule+0x3df/0xc10
 schedule+0x3d/0x90
 schedule_timeout+0x273/0x4b0
 wait_for_completion+0x108/0x170
 flush_workqueue+0x207/0x720
 srp_remove_one+0xbe/0x110 [ib_srp]
 ib_unregister_client+0x18f/0x200 [ib_core]
 srp_cleanup_module+0x10/0x618 [ib_srp]
 SyS_delete_module+0x198/0x1f0
 entry_SYSCALL_64_fastpath+0x18/0xad

Bart.From c395ce2aaf6d8a644311f4c55dfa6aa560a93240 Mon Sep 17 00:00:00 2001
From: Bart Van Assche 
Date: Tue, 28 Mar 2017 14:00:17 -0700
Subject: [PATCH 1/2] Introduce scsi_start_queue()

This patch does not change any functionality.

Signed-off-by: Bart Van Assche 
Cc: Israel Rukshin 
Cc: Max Gurtovoy 
Cc: Hannes Reinecke 
Cc: Benjamin Block 
---
 drivers/scsi/scsi_lib.c  | 25 +++--
 drivers/scsi/scsi_priv.h |  1 +
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index eecc005099b2..ffa6e61299a9 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2987,6 +2987,20 @@ scsi_internal_device_block(struct scsi_device *sdev, bool wait)
 }
 EXPORT_SYMBOL_GPL(scsi_internal_device_block);
  
+void scsi_start_queue(struct scsi_device *sdev)
+{
+	struct request_queue *q = sdev->request_queue;
+	unsigned long flags;
+
+	if (q->mq_ops) {
+		blk_mq_start_stopped_hw_queues(q, false);
+	} else {
+		spin_lock_irqsave(q->queue_lock, flags);
+		blk_start_queue(q);
+		spin_unlock_irqrestore(q->queue_lock, flags);
+	}
+}
+
 /**
  * scsi_internal_device_unblock - resume a device after a block request
  * @sdev:	device to resume
@@ -3007,9 +3021,6 @@ int
 scsi_internal_device_unblock(struct scsi_device *sdev,
 			 enum scsi_device_state new_state)
 {
-	struct request_queue *q = sdev->request_queue; 
-	unsigned long flags;
-
 	/*
 	 * Try to transition the scsi device to SDEV_RUNNING or one of the
 	 * offlined states and goose the device queue if successful.
@@ -3027,13 +3038,7 @@ scsi_internal_device_unblock(struct scsi_device *sdev,
 		 sdev->sdev_state != SDEV_OFFLINE)
 		return -EINVAL;
 
-	if (q->mq_ops) {
-		blk_mq_start_stopped_hw_queues(q, false);
-	} else {
-		spin_lock_irqsave(q->queue_lock, flags);
-		blk_start_queue(q);
-		

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

[James Bottomley]

On Thu, 2017-04-20 at 21:59 +, Bart Van Assche wrote:
> 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_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.

How is that possible?  Once the device goes into the CANCEL state, it
no longer can be found by starget_for_each_device() because
scsi_device_get() returns NULL ... unless you also have a patch
altering that?

James


> 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.

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

[Bart Van Assche]

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_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.

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

[Bart Van Assche]

On Tue, 2017-04-18 at 16:56 -0700, James Bottomley wrote:
> This means the combined 1/3 3/3 patch looks like this:
> [ ... ]

Hello James,

The two attached patches pass my tests. How would you like to proceed
with patch 1/2? Would you like to submit it yourself or is it OK for you
if I mention you as author and add your Signed-off-by?

Bart.From c395ce2aaf6d8a644311f4c55dfa6aa560a93240 Mon Sep 17 00:00:00 2001
From: Bart Van Assche 
Date: Tue, 28 Mar 2017 14:00:17 -0700
Subject: [PATCH 1/2] Introduce scsi_start_queue()

This patch does not change any functionality.

Signed-off-by: Bart Van Assche 
Cc: Israel Rukshin 
Cc: Max Gurtovoy 
Cc: Hannes Reinecke 
Cc: Benjamin Block 
---
 drivers/scsi/scsi_lib.c  | 25 +++--
 drivers/scsi/scsi_priv.h |  1 +
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index eecc005099b2..ffa6e61299a9 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2987,6 +2987,20 @@ scsi_internal_device_block(struct scsi_device *sdev, bool wait)
 }
 EXPORT_SYMBOL_GPL(scsi_internal_device_block);
  
+void scsi_start_queue(struct scsi_device *sdev)
+{
+	struct request_queue *q = sdev->request_queue;
+	unsigned long flags;
+
+	if (q->mq_ops) {
+		blk_mq_start_stopped_hw_queues(q, false);
+	} else {
+		spin_lock_irqsave(q->queue_lock, flags);
+		blk_start_queue(q);
+		spin_unlock_irqrestore(q->queue_lock, flags);
+	}
+}
+
 /**
  * scsi_internal_device_unblock - resume a device after a block request
  * @sdev:	device to resume
@@ -3007,9 +3021,6 @@ int
 scsi_internal_device_unblock(struct scsi_device *sdev,
 			 enum scsi_device_state new_state)
 {
-	struct request_queue *q = sdev->request_queue; 
-	unsigned long flags;
-
 	/*
 	 * Try to transition the scsi device to SDEV_RUNNING or one of the
 	 * offlined states and goose the device queue if successful.
@@ -3027,13 +3038,7 @@ scsi_internal_device_unblock(struct scsi_device *sdev,
 		 sdev->sdev_state != SDEV_OFFLINE)
 		return -EINVAL;
 
-	if (q->mq_ops) {
-		blk_mq_start_stopped_hw_queues(q, false);
-	} else {
-		spin_lock_irqsave(q->queue_lock, flags);
-		blk_start_queue(q);
-		spin_unlock_irqrestore(q->queue_lock, flags);
-	}
+	scsi_start_queue(sdev);
 
 	return 0;
 }
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index f11bd102d6d5..c7629e31a75b 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -89,6 +89,7 @@ extern void scsi_run_host_queues(struct Scsi_Host *shost);
 extern void scsi_requeue_run_queue(struct work_struct *work);
 extern struct request_queue *scsi_alloc_queue(struct scsi_device *sdev);
 extern struct request_queue *scsi_mq_alloc_queue(struct scsi_device *sdev);
+extern void scsi_start_queue(struct scsi_device *sdev);
 extern int scsi_mq_setup_tags(struct Scsi_Host *shost);
 extern void scsi_mq_destroy_tags(struct Scsi_Host *shost);
 extern int scsi_init_queue(void);
-- 
2.12.2

From 540cd36d79a46c634368365d3f0f5b8adf4fd687 Mon Sep 17 00:00:00 2001
From: Bart Van Assche 
Date: Tue, 18 Apr 2017 10:11:02 -0700
Subject: [PATCH 2/2] Make __scsi_remove_device go straight from BLOCKED to DEL

If a device is blocked, make __scsi_remove_device() cause it to
transition to the DEL state. This means that all the commands
issued in .shutdown() will error in the mid-layer, thus making
the removal proceed without being stopped.

This patch is a slightly modified version of a patch from James
Bottomley.

Signed-off-by: Bart Van Assche 
Cc: James Bottomley 
Cc: Israel Rukshin 
Cc: Max Gurtovoy 
Cc: Hannes Reinecke 
Cc: Benjamin Block 
---
 drivers/scsi/scsi_lib.c   |  2 +-
 drivers/scsi/scsi_sysfs.c | 15 +--
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ffa6e61299a9..376cd1da102c 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..732f1873f2fb 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1282,8 +1282,19 @@ void __scsi_remove_device(struct scsi_device *sdev)
 		return;
 
 	if (sdev->is_visible) 

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

[James Bottomley]

On Wed, 2017-04-19 at 00:02 +, Bart Van Assche wrote:
> On Tue, 2017-04-18 at 16:56 -0700, James Bottomley wrote:
> > Actually, I think 3/3 is wrong.  You need to start the queue as 
> > soon as we see it's blocked and the device has gone into DEL.  This 
> > is safe because DEL ensures nothing ever gets from the generic 
> > request function to queuecommand, so the device driver never sees
> > anything.
> 
> Hello James,
> 
> I agree that restarting the queue earlier is safe but I do not agree 
> that patch 3/3 is wrong. Can you explain why you think that patch 3/3 
> is wrong?

The shutdown occurs in driver removal, which precedes the queue start
in 3/3.  To avoid hangs and timeouts, we need the queue to be started
before the driver shutdown tries to inject commands.  They're not
fatal, but they will likely show up as annoying messages (and time
delays) which we can avoid by starting the queue the moment we go into
DEL.

James

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

[Bart Van Assche]

On Tue, 2017-04-18 at 16:56 -0700, James Bottomley wrote:
> Actually, I think 3/3 is wrong.  You need to start the queue as soon as
> we see it's blocked and the device has gone into DEL.  This is safe
> because DEL ensures nothing ever gets from the generic request function
> to queuecommand, so the device driver never sees anything.

Hello James,

I agree that restarting the queue earlier is safe but I do not agree that
patch 3/3 is wrong. Can you explain why you think that patch 3/3 is wrong?

Thanks,

Bart.

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

[James Bottomley]

On Tue, 2017-04-18 at 23:47 +, Bart Van Assche wrote:
> On Tue, 2017-04-18 at 08:56 -0700, James Bottomley wrote:
> > How about this approach.  It goes straight to DEL if the device is
> > blocked (skipping CANCEL).  This means that all the commands issued
> > in 
> > ->shutdown will error in the mid-layer, thus making the removal
> > proceed
> > without being stopped.
> 
> Hello James,
> 
> The three attached patches pass my tests. Please let me know how you 
> would like to proceed with patch 1/3. Would you like to submit it 
> yourself or is it OK for you if I mention you as author and add your
> Signed-off-by?

Actually, I think 3/3 is wrong.  You need to start the queue as soon as
we see it's blocked and the device has gone into DEL.  This is safe
because DEL ensures nothing ever gets from the generic request function
to queuecommand, so the device driver never sees anything.

This means the combined 1/3 3/3 patch looks like this:

James

---

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_dev);

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

[Bart Van Assche]

On Tue, 2017-04-18 at 08:56 -0700, James Bottomley wrote:
> How about this approach.  It goes straight to DEL if the device is
> blocked (skipping CANCEL).  This means that all the commands issued in 
> ->shutdown will error in the mid-layer, thus making the removal proceed
> without being stopped.

Hello James,

The three attached patches pass my tests. Please let me know how you would
like to proceed with patch 1/3. Would you like to submit it yourself or is
it OK for you if I mention you as author and add your Signed-off-by?

Thanks,

Bart.From 9482fdc8b322f15ced6f64d57f45026367604a23 Mon Sep 17 00:00:00 2001
From: Bart Van Assche 
Date: Tue, 18 Apr 2017 10:11:02 -0700
Subject: [PATCH 1/3] Make __scsi_remove_device go straight from BLOCKED to DEL

If a device is blocked, make __scsi_remove_device() cause it to
transition to the DEL state. This means that all the commands
issued in .shutdown() will error in the mid-layer, thus making
the removal proceed without being stopped.

This patch is a slightly modified version of a patch from James
Bottomley.

Signed-off-by: Bart Van Assche 
Cc: James Bottomley 
Cc: Israel Rukshin 
Cc: Max Gurtovoy 
Cc: Hannes Reinecke 
Cc: Benjamin Block 
---
 drivers/scsi/scsi_lib.c   |  2 +-
 drivers/scsi/scsi_sysfs.c | 12 +++-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index eecc005099b2..277c8b3ae7b0 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..f95d191ec809 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1282,9 +1282,19 @@ void __scsi_remove_device(struct scsi_device *sdev)
 		return;
 
 	if (sdev->is_visible) {
-		if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
+		/*
+		 * 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 &&
+		scsi_device_set_state(sdev, SDEV_DEL) != 0)
 			return;
 
+		if (sdev->sdev_state == SDEV_DEL)
+			sdev_printk(KERN_DEBUG, sdev,
+"Changed state from BLOCKED to DEL\n");
+
 		bsg_unregister_queue(sdev->request_queue);
 		device_unregister(>sdev_dev);
 		transport_remove_device(dev);
-- 
2.12.2

From c3f85b714fcfb12d43669b7f295a09f4718c2704 Mon Sep 17 00:00:00 2001
From: Bart Van Assche 
Date: Tue, 28 Mar 2017 14:00:17 -0700
Subject: [PATCH 2/3] Introduce scsi_start_queue()

This patch does not change any functionality.

Signed-off-by: Bart Van Assche 
Cc: Israel Rukshin 
Cc: Max Gurtovoy 
Cc: Hannes Reinecke 
Cc: Benjamin Block 
---
 drivers/scsi/scsi_lib.c  | 25 +++--
 drivers/scsi/scsi_priv.h |  1 +
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 277c8b3ae7b0..376cd1da102c 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2987,6 +2987,20 @@ scsi_internal_device_block(struct scsi_device *sdev, bool wait)
 }
 EXPORT_SYMBOL_GPL(scsi_internal_device_block);
  
+void scsi_start_queue(struct scsi_device *sdev)
+{
+	struct request_queue *q = sdev->request_queue;
+	unsigned long flags;
+
+	if (q->mq_ops) {
+		blk_mq_start_stopped_hw_queues(q, false);
+	} else {
+		spin_lock_irqsave(q->queue_lock, flags);
+		blk_start_queue(q);
+		spin_unlock_irqrestore(q->queue_lock, flags);
+	}
+}
+
 /**
  * scsi_internal_device_unblock - resume a device after a block request
  * @sdev:	device to resume
@@ -3007,9 +3021,6 @@ int
 scsi_internal_device_unblock(struct scsi_device *sdev,
 			 enum scsi_device_state new_state)
 {
-	struct request_queue *q = sdev->request_queue; 
-	unsigned long flags;
-
 	/*
 	 * Try to transition the scsi device to SDEV_RUNNING or one of the
 	 * offlined states and goose the device queue if successful.
@@ -3027,13 +3038,7 @@ scsi_internal_device_unblock(struct scsi_device *sdev,
 		 sdev->sdev_state != SDEV_OFFLINE)
 		return -EINVAL;
 
-	if (q->mq_ops) {
-		blk_mq_start_stopped_hw_queues(q, false);
-	} else {
-		spin_lock_irqsave(q->queue_lock, flags);
-		blk_start_queue(q);
-		

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

[Bart Van Assche]

On Tue, 2017-04-18 at 08:56 -0700, James Bottomley wrote:
> How about this approach.  It goes straight to DEL if the device is
> blocked (skipping CANCEL).  This means that all the commands issued in 
> ->shutdown will error in the mid-layer, thus making the removal proceed
> without being stopped.

Hello James,

I think that's a good start but not a full solution. The block layer queue
still needs to be restarted to make sure that any requests that got queued
after the transition to the SDEV_BLOCK state and before the transition to
SDEV_DEL get processed. Anyway, I will see whether I can come up with a
patch based on this approach.

Bart.

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

[James Bottomley]

On Tue, 2017-04-18 at 16:44 +0200, Benjamin Block wrote:
> Hej Bart,
> 
> sry for the late'ish reply, had a long weekend.
> 
> On Thu, Apr 13, 2017 at 12:28:54AM +, Bart Van Assche wrote:
> > On Wed, 2017-04-12 at 16:41 +0200, Benjamin Block wrote:
> > > On Mon, Apr 10, 2017 at 10:54:01AM -0700, Bart Van Assche wrote:
> > > > [ ... ]
> > > OK, so I take it the problem is when the queue is stopped, then 
> > > the completion in blk_execute_rq() will never be triggered and 
> > > then we wait for a timeout there, or potentially forever?
> > 
> > Hello Benjamin,
> > 
> > Thanks for the review.
> > 
> > If a request is queued after a queue has been stopped then that 
> > request will never be started and hence even the timeout timer 
> > won't be started. blk_cleanup_queue() hangs if invoked for a 
> > stopped queue and one or more requests have not yet been started.
> > 
> > > But then what is the point in trying to do it async here anyway? 
> > > Won't that just be doomed in the same way, just that we don't see 
> > > the effect?
> > 
> > Have you noticed that patch 4/4 in this series restarts the queue 
> > just before calling blk_cleanup_queue()?
> > 
> > Anyway, can you have a look at the patch below and see whether this 
> > new version addresses all the concerns you had reported in your
> > previous e-mail?
> > 
> 
> Yes, the code- and comment-changes in sd_shutdown() are good. 
> Apparently there is something new with the done-function now, but you 
> got that from Israel.
> 
> I still wonder why we try 'so hard' scheduling a command for a dead
> device, but as that seems to be the status quo, and only lacks in the
> case where the LLD is already half-way gone, its ok for me too. I 
> mean, the order is a bit screwed.. we apparently first remove the 
> driver and post-factum try to drain the queue.. that is strange.

Yes, I've said all along that adding asynchronicity is only going to
add more problems.

The priority has got to be the removal we've been ordered to do rather
than waiting around to see if the transport comes back so we can send a
final flush.

How about this approach.  It goes straight to DEL if the device is
blocked (skipping CANCEL).  This means that all the commands issued in 
->shutdown will error in the mid-layer, thus making the removal proceed
without being stopped.

James

---

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..788309e307e9 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1282,8 +1282,14 @@ void __scsi_remove_device(struct scsi_device *sdev)
return;
 
if (sdev->is_visible) {
+   /*
+* 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)
-   return;
+   if (scsi_device_set_state(sdev, SDEV_DEL) != 0)
+   return;
 
bsg_unregister_queue(sdev->request_queue);
device_unregister(>sdev_dev);

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

[Bart Van Assche]

On Tue, 2017-04-18 at 16:44 +0200, Benjamin Block wrote:
> I still wonder why we try 'so hard' scheduling a command for a dead
> device, but as that seems to be the status quo, and only lacks in the
> case where the LLD is already half-way gone, its ok for me too. I mean,
> the order is a bit screwed.. we apparently first remove the driver and
> post-factum try to drain the queue.. that is strange.

Hello Benjamin,

That's indeed strange. But I'm not sure whether it is possible to address
this without changing all SCSI LLDs.

Bart.

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

[Benjamin Block]

Hej Bart,

sry for the late'ish reply, had a long weekend.

On Thu, Apr 13, 2017 at 12:28:54AM +, Bart Van Assche wrote:
> On Wed, 2017-04-12 at 16:41 +0200, Benjamin Block wrote:
> > On Mon, Apr 10, 2017 at 10:54:01AM -0700, Bart Van Assche wrote:
> > > [ ... ]
> > OK, so I take it the problem is when the queue is stopped, then the
> > completion in blk_execute_rq() will never be triggered and then we wait
> > for a timeout there, or potentially forever?
>
> Hello Benjamin,
>
> Thanks for the review.
>
> If a request is queued after a queue has been stopped then that request
> will never be started and hence even the timeout timer won't be started.
> blk_cleanup_queue() hangs if invoked for a stopped queue and one or more
> requests have not yet been started.
>
> > But then what is the point in trying to do it async here anyway? Won't
> > that just be doomed in the same way, just that we don't see the effect?
>
> Have you noticed that patch 4/4 in this series restarts the queue just
> before calling blk_cleanup_queue()?
>
> Anyway, can you have a look at the patch below and see whether this new
> version addresses all the concerns you had reported in your previous
> e-mail?
>

Yes, the code- and comment-changes in sd_shutdown() are good. Apparently
there is something new with the done-function now, but you got that from
Israel.

I still wonder why we try 'so hard' scheduling a command for a dead
device, but as that seems to be the status quo, and only lacks in the
case where the LLD is already half-way gone, its ok for me too. I mean,
the order is a bit screwed.. we apparently first remove the driver and
post-factum try to drain the queue.. that is strange.


- Benjamin

On Mon, Apr 17, 2017 at 10:34:35AM -0700, Bart Van Assche wrote:
> This patch avoids that sd_shutdown() hangs on the SYNCHRONIZE CACHE
> command if the block layer queue has been stopped by
> scsi_target_block().
> 
> Signed-off-by: Bart Van Assche 
> Cc: Israel Rukshin 
> Cc: Max Gurtovoy 
> Cc: Hannes Reinecke 
> Cc: Benjamin Block 
> ---
>  drivers/scsi/sd.c | 45 -
>  1 file changed, 40 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index fe0f7997074e..deff564fe649 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1489,6 +1489,33 @@ static unsigned int sd_check_events(struct gendisk 
> *disk, unsigned int clearing)
>   return retval;
>  }
> 
> +static void sd_sync_cache_done(struct request *rq, int e)
> +{
> + struct scsi_disk *sdkp = scsi_disk(rq->rq_disk);
> +
> + sd_printk(KERN_DEBUG, sdkp, "%s\n", __func__);
> +
> + blk_put_request(rq);
> +}
> +
> +/*
> + * Issue a SYNCHRONIZE CACHE command asynchronously. Since 
> blk_cleanup_queue()
> + * waits for all commands to finish, __scsi_remove_device() will wait for the
> + * SYNCHRONIZE CACHE command to finish.
> + */
> +static int sd_sync_cache_async(struct scsi_disk *sdkp)
> +{
> + const struct scsi_device *sdp = sdkp->device;
> + const int timeout = sdp->request_queue->rq_timeout *
> + SD_FLUSH_TIMEOUT_MULTIPLIER;
> + const unsigned char cmd[10] = { SYNCHRONIZE_CACHE };
> +
> + sd_printk(KERN_DEBUG, sdkp, "%s\n", __func__);
> + return scsi_execute_async(sdp, sdkp->disk, cmd, DMA_NONE, NULL, 0,
> +   timeout, SD_MAX_RETRIES, 0, 0,
> +   sd_sync_cache_done);
> +}
> +
>  static int sd_sync_cache(struct scsi_disk *sdkp)
>  {
>   int retries, res;
> @@ -3349,13 +3376,15 @@ static int sd_start_stop_device(struct scsi_disk 
> *sdkp, int start)
>  }
> 
>  /*
> - * Send a SYNCHRONIZE CACHE instruction down to the device through
> - * the normal SCSI command structure.  Wait for the command to
> - * complete.
> + * Send a SYNCHRONIZE CACHE instruction down to the device through the normal
> + * SCSI command structure. When stopping the disk, wait for the command to
> + * complete. When not stopping the disk, the blk_cleanup_queue() call in
> + * __scsi_remove_device() will wait for this command to complete.
>   */
>  static void sd_shutdown(struct device *dev)
>  {
>   struct scsi_disk *sdkp = dev_get_drvdata(dev);
> + bool stop_disk;
> 
>   if (!sdkp)
>   return; /* this can happen */
> @@ -3363,12 +3392,18 @@ static void sd_shutdown(struct device *dev)
>   if (pm_runtime_suspended(dev))
>   return;
> 
> + stop_disk = system_state != SYSTEM_RESTART &&
> + sdkp->device->manage_start_stop;
> +
>   if (sdkp->WCE && sdkp->media_present) {
>   sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
> - sd_sync_cache(sdkp);
> + if (stop_disk)
> + sd_sync_cache(sdkp);
> + else
> + sd_sync_cache_async(sdkp);
>