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.From c395ce2aaf6d8a644311f4c55dfa6aa560a93240 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@xxxxxxxxxxx>
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@xxxxxxxxxxx>
Cc: Israel Rukshin <israelr@xxxxxxxxxxxx>
Cc: Max Gurtovoy <maxg@xxxxxxxxxxxx>
Cc: Hannes Reinecke <hare@xxxxxxx>
Cc: Benjamin Block <bblock@xxxxxxxxxxxxxxxxxx>
---
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 f1a34dcc19fd624eca9fd98c36a74555f4746ff8 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@xxxxxxxxxxx>
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@xxxxxxxxxxx>
---
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
From f558b519922837eec5410de7aed059cb42c10b64 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@xxxxxxxxxxx>
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@xxxxxxxxxxx>
Cc: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>
Cc: Israel Rukshin <israelr@xxxxxxxxxxxx>
Cc: Max Gurtovoy <maxg@xxxxxxxxxxxx>
Cc: Hannes Reinecke <hare@xxxxxxx>
Cc: Benjamin Block <bblock@xxxxxxxxxxxxxxxxxx>
---
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
