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

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

[-- Attachment #1: Type: text/plain, Size: 2155 bytes --]

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Introduce-scsi_start_queue.patch --]
[-- Type: text/x-patch; name="0001-Introduce-scsi_start_queue.patch", Size: 2756 bytes --]

From c395ce2aaf6d8a644311f4c55dfa6aa560a93240 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@sandisk.com>
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 <bart.vanassche@sandisk.com>
Cc: Israel Rukshin <israelr@mellanox.com>
Cc: Max Gurtovoy <maxg@mellanox.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Benjamin Block <bblock@linux.vnet.ibm.com>
---
 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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Protect-SCSI-device-state-changes-with-a-mutex.patch --]
[-- Type: text/x-patch; name="0002-Protect-SCSI-device-state-changes-with-a-mutex.patch", Size: 6756 bytes --]

From f1a34dcc19fd624eca9fd98c36a74555f4746ff8 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@sandisk.com>
Date: Wed, 26 Apr 2017 09:24:08 -0700
Subject: [PATCH 2/3] Protect SCSI device state changes with a mutex

Enable this mechanism for all scsi_target_*block() callers but not
for the scsi_internal_device_unblock() calls from the mpt3sas driver
because that driver can call scsi_internal_device_unblock() from
atomic context.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c |  4 ++--
 drivers/scsi/scsi_lib.c              | 35 +++++++++++++++++++++++++++--------
 drivers/scsi/scsi_scan.c             |  1 +
 drivers/scsi/scsi_sysfs.c            | 11 ++++++++++-
 include/scsi/scsi_device.h           |  3 ++-
 5 files changed, 42 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 919ba2bb15f1..0edf5e7cd0ab 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -2883,7 +2883,7 @@ _scsih_internal_device_unblock(struct scsi_device *sdev,
 	sdev_printk(KERN_WARNING, sdev, "device_unblock and setting to running, "
 	    "handle(0x%04x)\n", sas_device_priv_data->sas_target->handle);
 	sas_device_priv_data->block = 0;
-	r = scsi_internal_device_unblock(sdev, SDEV_RUNNING);
+	r = scsi_internal_device_unblock(sdev, SDEV_RUNNING, false);
 	if (r == -EINVAL) {
 		/* The device has been set to SDEV_RUNNING by SD layer during
 		 * device addition but the request queue is still stopped by
@@ -2902,7 +2902,7 @@ _scsih_internal_device_unblock(struct scsi_device *sdev,
 			    r, sas_device_priv_data->sas_target->handle);
 
 		sas_device_priv_data->block = 0;
-		r = scsi_internal_device_unblock(sdev, SDEV_RUNNING);
+		r = scsi_internal_device_unblock(sdev, SDEV_RUNNING, false);
 		if (r)
 			sdev_printk(KERN_WARNING, sdev, "retried device_unblock"
 			    " failed with return(%d) for handle(0x%04x)\n",
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ffa6e61299a9..6ab7c63110b1 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2955,14 +2955,16 @@ scsi_internal_device_block(struct scsi_device *sdev, bool wait)
 {
 	struct request_queue *q = sdev->request_queue;
 	unsigned long flags;
-	int err = 0;
+	int err;
+
+	if (wait)
+		mutex_lock(&sdev->state_mutex);
 
 	err = scsi_device_set_state(sdev, SDEV_BLOCK);
 	if (err) {
 		err = scsi_device_set_state(sdev, SDEV_CREATED_BLOCK);
-
 		if (err)
-			return err;
+			goto unlock;
 	}
 
 	/* 
@@ -2983,7 +2985,11 @@ scsi_internal_device_block(struct scsi_device *sdev, bool wait)
 			scsi_wait_for_queuecommand(sdev);
 	}
 
-	return 0;
+unlock:
+	if (wait)
+		mutex_unlock(&sdev->state_mutex);
+
+	return err;
 }
 EXPORT_SYMBOL_GPL(scsi_internal_device_block);
  
@@ -3005,6 +3011,8 @@ void scsi_start_queue(struct scsi_device *sdev)
  * scsi_internal_device_unblock - resume a device after a block request
  * @sdev:	device to resume
  * @new_state:	state to set devices to after unblocking
+ * @wait:	whether or not to wait until concurrent state changes have
+ *		finished
  *
  * Called by scsi lld's or the midlayer to restart the device queue
  * for the previously suspended scsi device.  Called from interrupt or
@@ -3019,8 +3027,13 @@ void scsi_start_queue(struct scsi_device *sdev)
  */
 int
 scsi_internal_device_unblock(struct scsi_device *sdev,
-			     enum scsi_device_state new_state)
+			     enum scsi_device_state new_state, bool wait)
 {
+	int ret = -EINVAL;
+
+	if (wait)
+		mutex_lock(&sdev->state_mutex);
+
 	/*
 	 * Try to transition the scsi device to SDEV_RUNNING or one of the
 	 * offlined states and goose the device queue if successful.
@@ -3036,11 +3049,16 @@ scsi_internal_device_unblock(struct scsi_device *sdev,
 			sdev->sdev_state = SDEV_CREATED;
 	} else if (sdev->sdev_state != SDEV_CANCEL &&
 		 sdev->sdev_state != SDEV_OFFLINE)
-		return -EINVAL;
+		goto unlock;
 
+	ret = 0;
 	scsi_start_queue(sdev);
 
-	return 0;
+unlock:
+	if (wait)
+		mutex_unlock(&sdev->state_mutex);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(scsi_internal_device_unblock);
 
@@ -3073,7 +3091,8 @@ EXPORT_SYMBOL_GPL(scsi_target_block);
 static void
 device_unblock(struct scsi_device *sdev, void *data)
 {
-	scsi_internal_device_unblock(sdev, *(enum scsi_device_state *)data);
+	scsi_internal_device_unblock(sdev, *(enum scsi_device_state *)data,
+				     true);
 }
 
 static int
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 6f7128f49c30..6f4be6e2777d 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -231,6 +231,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
 	sdev->id = starget->id;
 	sdev->lun = lun;
 	sdev->channel = starget->channel;
+	mutex_init(&sdev->state_mutex);
 	sdev->sdev_state = SDEV_CREATED;
 	INIT_LIST_HEAD(&sdev->siblings);
 	INIT_LIST_HEAD(&sdev->same_target_siblings);
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 82dfe07b1d47..5892cd95c546 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1272,6 +1272,7 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 void __scsi_remove_device(struct scsi_device *sdev)
 {
 	struct device *dev = &sdev->sdev_gendev;
+	int res;
 
 	/*
 	 * This cleanup path is not reentrant and while it is impossible
@@ -1282,7 +1283,15 @@ void __scsi_remove_device(struct scsi_device *sdev)
 		return;
 
 	if (sdev->is_visible) {
-		if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
+		/*
+		 * Wait until a concurrent scsi_internal_target_block() call
+		 * has finished before changing the device state.
+		 */
+		mutex_lock(&sdev->state_mutex);
+		res = scsi_device_set_state(sdev, SDEV_CANCEL);
+		mutex_unlock(&sdev->state_mutex);
+
+		if (res != 0)
 			return;
 
 		bsg_unregister_queue(sdev->request_queue);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 080c7ce9bae8..25b649033615 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -207,6 +207,7 @@ struct scsi_device {
 	void			*handler_data;
 
 	unsigned char		access_state;
+	struct mutex		state_mutex;
 	enum scsi_device_state sdev_state;
 	unsigned long		sdev_data[0];
 } __attribute__((aligned(sizeof(unsigned long))));
@@ -474,7 +475,7 @@ static inline int scsi_device_created(struct scsi_device *sdev)
 
 int scsi_internal_device_block(struct scsi_device *sdev, bool wait);
 int scsi_internal_device_unblock(struct scsi_device *sdev,
-				 enum scsi_device_state new_state);
+				 enum scsi_device_state new_state, bool wait);
 
 /* accessor functions for the SCSI parameters */
 static inline int scsi_device_sync(struct scsi_device *sdev)
-- 
2.12.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Make-__scsi_remove_device-go-straight-from-BLOCKED-t.patch --]
[-- Type: text/x-patch; name="0003-Make-__scsi_remove_device-go-straight-from-BLOCKED-t.patch", Size: 2460 bytes --]

From f558b519922837eec5410de7aed059cb42c10b64 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@sandisk.com>
Date: Tue, 18 Apr 2017 10:11:02 -0700
Subject: [PATCH 3/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 <bart.vanassche@sandisk.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Cc: Israel Rukshin <israelr@mellanox.com>
Cc: Max Gurtovoy <maxg@mellanox.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Benjamin Block <bblock@linux.vnet.ibm.com>
---
 drivers/scsi/scsi_lib.c   |  2 +-
 drivers/scsi/scsi_sysfs.c | 13 +++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 6ab7c63110b1..2e84bb85d0e6 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 5892cd95c546..8b9b6aaf6a9e 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1288,7 +1288,20 @@ void __scsi_remove_device(struct scsi_device *sdev)
 		 * has finished before changing the device state.
 		 */
 		mutex_lock(&sdev->state_mutex);
+		/*
+		 * If blocked, we go straight to DEL and restart the queue so
+		 * any commands issued during driver shutdown (like sync
+		 * cache) are errored immediately.
+		 */
 		res = scsi_device_set_state(sdev, SDEV_CANCEL);
+		if (res != 0) {
+			res = scsi_device_set_state(sdev, SDEV_DEL);
+			if (res == 0) {
+				scsi_start_queue(sdev);
+				sdev_printk(KERN_DEBUG, sdev,
+				    "Changed state from BLOCKED to DEL\n");
+			}
+		}
 		mutex_unlock(&sdev->state_mutex);
 
 		if (res != 0)
-- 
2.12.2