* [PATCH v14 1/6] Fix race between starved list and device removal
2013-07-05 13:22 [PATCH v14 0/6] SCSI device removal fixes Bart Van Assche
@ 2013-07-05 13:23 ` Bart Van Assche
2013-07-05 13:24 ` [PATCH v14 2/6] Avoid calling __scsi_remove_device() twice Bart Van Assche
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Bart Van Assche @ 2013-07-05 13:23 UTC (permalink / raw)
To: James Bottomley
Cc: linux-scsi, Mike Christie, Hannes Reinecke, Chanho Min,
Joe Lawrence, David Milburn
scsi_run_queue() examines all SCSI devices that are present on
the starved list. Since scsi_run_queue() unlocks the SCSI host
lock a SCSI device can get removed after it has been removed
from the starved list and before its queue is run. Protect
against that race condition by holding a reference on the
queue while running it.
Signed-off-by: James Bottomley <JBottomley@Parallels.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Reported-by: Chanho Min <chanho.min@lge.com>
Reference: http://lkml.org/lkml/2012/8/2/96
Cc: Mike Christie <michaelc@cs.wisc.edu>
Cc: Hannes Reinecke <hare@suse.de>
Cc: <stable@vger.kernel.org>
---
drivers/scsi/scsi_lib.c | 26 +++++++++++++++++++++-----
1 file changed, 21 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 86d5220..df8bd5a 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -434,6 +434,8 @@ static void scsi_run_queue(struct request_queue *q)
list_splice_init(&shost->starved_list, &starved_list);
while (!list_empty(&starved_list)) {
+ struct request_queue *slq;
+
/*
* As long as shost is accepting commands and we have
* starved queues, call blk_run_queue. scsi_request_fn
@@ -456,11 +458,25 @@ static void scsi_run_queue(struct request_queue *q)
continue;
}
- spin_unlock(shost->host_lock);
- spin_lock(sdev->request_queue->queue_lock);
- __blk_run_queue(sdev->request_queue);
- spin_unlock(sdev->request_queue->queue_lock);
- spin_lock(shost->host_lock);
+ /*
+ * Once we drop the host lock, a racing scsi_remove_device()
+ * call may remove the sdev from the starved list and destroy
+ * it and the queue. Mitigate by taking a reference to the
+ * queue and never touching the sdev again after we drop the
+ * host lock. Note: if __scsi_remove_device() invokes
+ * blk_cleanup_queue() before the queue is run from this
+ * function then blk_run_queue() will return immediately since
+ * blk_cleanup_queue() marks the queue with QUEUE_FLAG_DYING.
+ */
+ slq = sdev->request_queue;
+ if (!blk_get_queue(slq))
+ continue;
+ spin_unlock_irqrestore(shost->host_lock, flags);
+
+ blk_run_queue(slq);
+ blk_put_queue(slq);
+
+ spin_lock_irqsave(shost->host_lock, flags);
}
/* put any unprocessed entries back */
list_splice(&starved_list, &shost->starved_list);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v14 2/6] Avoid calling __scsi_remove_device() twice
2013-07-05 13:22 [PATCH v14 0/6] SCSI device removal fixes Bart Van Assche
2013-07-05 13:23 ` [PATCH v14 1/6] Fix race between starved list and device removal Bart Van Assche
@ 2013-07-05 13:24 ` Bart Van Assche
2013-07-05 13:25 ` [PATCH v14 3/6] Introduce scsi_device_being_removed() Bart Van Assche
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Bart Van Assche @ 2013-07-05 13:24 UTC (permalink / raw)
To: James Bottomley
Cc: linux-scsi, Mike Christie, Hannes Reinecke, Chanho Min,
Joe Lawrence, David Milburn
If something goes wrong during LUN scanning, e.g. a transport layer
failure occurs, then __scsi_remove_device() can get invoked by the
LUN scanning code for a SCSI device in state SDEV_CREATED_BLOCK and
before the SCSI device has been added to sysfs (is_visible == 0).
Make sure that even in this case the transition into state SDEV_DEL
occurs. This avoids that __scsi_remove_device() can get invoked a
second time by scsi_forget_host() if this last function is invoked
from another thread than the thread that performs LUN scanning.
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: James Bottomley <JBottomley@Parallels.com>
Cc: Mike Christie <michaelc@cs.wisc.edu>
Cc: Hannes Reinecke <hare@suse.de>
---
drivers/scsi/scsi_lib.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index df8bd5a..124392f 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2193,6 +2193,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_CREATED_BLOCK:
break;
default:
goto illegal;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v14 3/6] Introduce scsi_device_being_removed()
2013-07-05 13:22 [PATCH v14 0/6] SCSI device removal fixes Bart Van Assche
2013-07-05 13:23 ` [PATCH v14 1/6] Fix race between starved list and device removal Bart Van Assche
2013-07-05 13:24 ` [PATCH v14 2/6] Avoid calling __scsi_remove_device() twice Bart Van Assche
@ 2013-07-05 13:25 ` Bart Van Assche
2013-07-05 13:26 ` [PATCH v14 4/6] Rework scsi_internal_device_unblock() Bart Van Assche
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Bart Van Assche @ 2013-07-05 13:25 UTC (permalink / raw)
To: James Bottomley
Cc: linux-scsi, Mike Christie, Hannes Reinecke, Chanho Min,
Joe Lawrence, David Milburn
Introduce the helper function scsi_device_being_removed(). This patch
does not change any functionality.
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Acked-by: Hannes Reinecke <hare@suse.de>
Cc: James Bottomley <JBottomley@Parallels.com>
Cc: Mike Christie <michaelc@cs.wisc.edu>
---
drivers/scsi/device_handler/scsi_dh.c | 7 ++-----
include/scsi/scsi_device.h | 5 +++++
2 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
index 33e422e..78b3ddb 100644
--- a/drivers/scsi/device_handler/scsi_dh.c
+++ b/drivers/scsi/device_handler/scsi_dh.c
@@ -156,8 +156,7 @@ store_dh_state(struct device *dev, struct device_attribute *attr,
struct scsi_device_handler *scsi_dh;
int err = -EINVAL;
- if (sdev->sdev_state == SDEV_CANCEL ||
- sdev->sdev_state == SDEV_DEL)
+ if (scsi_device_being_removed(sdev))
return -ENODEV;
if (!sdev->scsi_dh_data) {
@@ -400,9 +399,7 @@ int scsi_dh_activate(struct request_queue *q, activate_complete fn, void *data)
if (sdev->scsi_dh_data)
scsi_dh = sdev->scsi_dh_data->scsi_dh;
dev = get_device(&sdev->sdev_gendev);
- if (!scsi_dh || !dev ||
- sdev->sdev_state == SDEV_CANCEL ||
- sdev->sdev_state == SDEV_DEL)
+ if (!scsi_dh || !dev || scsi_device_being_removed(sdev))
err = SCSI_DH_NOSYS;
if (sdev->sdev_state == SDEV_OFFLINE)
err = SCSI_DH_DEV_OFFLINED;
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index cc64587..fead252 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -454,6 +454,11 @@ static inline int scsi_device_created(struct scsi_device *sdev)
return sdev->sdev_state == SDEV_CREATED ||
sdev->sdev_state == SDEV_CREATED_BLOCK;
}
+static inline int scsi_device_being_removed(struct scsi_device *sdev)
+{
+ return sdev->sdev_state == SDEV_CANCEL ||
+ sdev->sdev_state == SDEV_DEL;
+}
/* accessor functions for the SCSI parameters */
static inline int scsi_device_sync(struct scsi_device *sdev)
--
1.7.10.4
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v14 4/6] Rework scsi_internal_device_unblock()
2013-07-05 13:22 [PATCH v14 0/6] SCSI device removal fixes Bart Van Assche
` (2 preceding siblings ...)
2013-07-05 13:25 ` [PATCH v14 3/6] Introduce scsi_device_being_removed() Bart Van Assche
@ 2013-07-05 13:26 ` Bart Van Assche
2013-07-05 13:27 ` [PATCH v14 5/6] Avoid re-enabling I/O after the transport became offline Bart Van Assche
2013-07-05 13:28 ` [PATCH v14 6/6] Disallow changing the device state via sysfs into "deleted" Bart Van Assche
5 siblings, 0 replies; 7+ messages in thread
From: Bart Van Assche @ 2013-07-05 13:26 UTC (permalink / raw)
To: James Bottomley
Cc: linux-scsi, Mike Christie, Hannes Reinecke, Chanho Min,
Joe Lawrence, David Milburn
Modify scsi_internal_device_unblock() such that it uses
scsi_device_set_state() to change the device state. This is only
possible by changing scsi_device_set_state() such that it allows
the transition from SDEV_CREATED_BLOCK to the SDEV_OFFLINE and
SDEV_TRANSPORT_OFFLINE states. Notes:
- All callers of scsi_internal_device_unblock() ignore the return
value of this function.
- Since the SDEV_CREATED_BLOCK to SDEV_{TRANSPORT_,}OFFLINE transition
is now allowed, direct scsi_device_set_state() calls that change the
device state from SDEV_CREATED_BLOCK into SDEV_*OFFLINE will now
proceed instead of being rejected.
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: James Bottomley <JBottomley@Parallels.com>
Cc: Mike Christie <michaelc@cs.wisc.edu>
Cc: Hannes Reinecke <hare@suse.de>
---
drivers/scsi/scsi_lib.c | 29 +++++++++++------------------
1 file changed, 11 insertions(+), 18 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 124392f..9eb05a7 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2147,6 +2147,7 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
case SDEV_RUNNING:
case SDEV_QUIESCE:
case SDEV_BLOCK:
+ case SDEV_CREATED_BLOCK:
break;
default:
goto illegal;
@@ -2501,29 +2502,21 @@ scsi_internal_device_unblock(struct scsi_device *sdev,
{
struct request_queue *q = sdev->request_queue;
unsigned long flags;
+ int res;
/*
* Try to transition the scsi device to SDEV_RUNNING or one of the
* offlined states and goose the device queue if successful.
*/
- if ((sdev->sdev_state == SDEV_BLOCK) ||
- (sdev->sdev_state == SDEV_TRANSPORT_OFFLINE))
- sdev->sdev_state = new_state;
- else if (sdev->sdev_state == SDEV_CREATED_BLOCK) {
- if (new_state == SDEV_TRANSPORT_OFFLINE ||
- new_state == SDEV_OFFLINE)
- sdev->sdev_state = new_state;
- else
- sdev->sdev_state = SDEV_CREATED;
- } else if (sdev->sdev_state != SDEV_CANCEL &&
- sdev->sdev_state != SDEV_OFFLINE)
- return -EINVAL;
-
- spin_lock_irqsave(q->queue_lock, flags);
- blk_start_queue(q);
- spin_unlock_irqrestore(q->queue_lock, flags);
-
- return 0;
+ if (sdev->sdev_state == SDEV_CREATED_BLOCK && new_state == SDEV_RUNNING)
+ new_state = SDEV_CREATED;
+ res = scsi_device_set_state(sdev, new_state);
+ if (!scsi_device_blocked(sdev)) {
+ spin_lock_irqsave(q->queue_lock, flags);
+ blk_start_queue(q);
+ spin_unlock_irqrestore(q->queue_lock, flags);
+ }
+ return res;
}
EXPORT_SYMBOL_GPL(scsi_internal_device_unblock);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v14 5/6] Avoid re-enabling I/O after the transport became offline
2013-07-05 13:22 [PATCH v14 0/6] SCSI device removal fixes Bart Van Assche
` (3 preceding siblings ...)
2013-07-05 13:26 ` [PATCH v14 4/6] Rework scsi_internal_device_unblock() Bart Van Assche
@ 2013-07-05 13:27 ` Bart Van Assche
2013-07-05 13:28 ` [PATCH v14 6/6] Disallow changing the device state via sysfs into "deleted" Bart Van Assche
5 siblings, 0 replies; 7+ messages in thread
From: Bart Van Assche @ 2013-07-05 13:27 UTC (permalink / raw)
To: James Bottomley
Cc: linux-scsi, Mike Christie, Hannes Reinecke, Chanho Min,
Joe Lawrence, David Milburn
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>
---
drivers/scsi/scsi_lib.c | 12 +++++++++++-
drivers/scsi/scsi_sysfs.c | 4 +++-
include/scsi/scsi_device.h | 4 ++++
3 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 9eb05a7..362855d 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1232,6 +1232,7 @@ int scsi_prep_state_check(struct scsi_device *sdev, struct request *req)
switch (sdev->sdev_state) {
case SDEV_OFFLINE:
case SDEV_TRANSPORT_OFFLINE:
+ case SDEV_CANCEL_OFFLINE:
/*
* If the device is offline we refuse to process any
* commands. The device must be brought online
@@ -2178,9 +2179,17 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
case SDEV_CREATED:
case SDEV_RUNNING:
case SDEV_QUIESCE:
+ case SDEV_BLOCK:
+ break;
+ default:
+ goto illegal;
+ }
+ break;
+
+ case SDEV_CANCEL_OFFLINE:
+ switch (oldstate) {
case SDEV_OFFLINE:
case SDEV_TRANSPORT_OFFLINE:
- case SDEV_BLOCK:
break;
default:
goto illegal;
@@ -2194,6 +2203,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_CANCEL_OFFLINE:
case SDEV_CREATED_BLOCK:
break;
default:
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 931a7d9..f6ce38e 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -32,6 +32,7 @@ static const struct {
{ SDEV_CREATED, "created" },
{ SDEV_RUNNING, "running" },
{ SDEV_CANCEL, "cancel" },
+ { SDEV_CANCEL_OFFLINE, "cancel-offline" },
{ SDEV_DEL, "deleted" },
{ SDEV_QUIESCE, "quiesce" },
{ SDEV_OFFLINE, "offline" },
@@ -955,7 +956,8 @@ 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)
+ if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0 &&
+ scsi_device_set_state(sdev, SDEV_CANCEL_OFFLINE) != 0)
return;
bsg_unregister_queue(sdev->request_queue);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index fead252..4ecf7ed1 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -35,6 +35,8 @@ enum scsi_device_state {
* All commands allowed */
SDEV_CANCEL, /* beginning to delete device
* Only error handler commands allowed */
+ SDEV_CANCEL_OFFLINE, /* beginning to delete offline device
+ * No commands allowed */
SDEV_DEL, /* device deleted
* no commands allowed */
SDEV_QUIESCE, /* Device quiescent. No block commands
@@ -442,6 +444,7 @@ static inline int scsi_device_online(struct scsi_device *sdev)
{
return (sdev->sdev_state != SDEV_OFFLINE &&
sdev->sdev_state != SDEV_TRANSPORT_OFFLINE &&
+ sdev->sdev_state != SDEV_CANCEL_OFFLINE &&
sdev->sdev_state != SDEV_DEL);
}
static inline int scsi_device_blocked(struct scsi_device *sdev)
@@ -457,6 +460,7 @@ static inline int scsi_device_created(struct scsi_device *sdev)
static inline int scsi_device_being_removed(struct scsi_device *sdev)
{
return sdev->sdev_state == SDEV_CANCEL ||
+ sdev->sdev_state == SDEV_CANCEL_OFFLINE ||
sdev->sdev_state == SDEV_DEL;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v14 6/6] Disallow changing the device state via sysfs into "deleted"
2013-07-05 13:22 [PATCH v14 0/6] SCSI device removal fixes Bart Van Assche
` (4 preceding siblings ...)
2013-07-05 13:27 ` [PATCH v14 5/6] Avoid re-enabling I/O after the transport became offline Bart Van Assche
@ 2013-07-05 13:28 ` Bart Van Assche
5 siblings, 0 replies; 7+ messages in thread
From: Bart Van Assche @ 2013-07-05 13:28 UTC (permalink / raw)
To: James Bottomley
Cc: linux-scsi, Mike Christie, Hannes Reinecke, Chanho Min,
Joe Lawrence, David Milburn
Changing the state of a SCSI device via sysfs into "cancel",
"cancel-offline" or "deleted" prevents removal of these devices by
scsi_remove_host(). Hence do not allow this.
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: James Bottomley <JBottomley@Parallels.com>
Cc: Mike Christie <michaelc@cs.wisc.edu>
Cc: Hannes Reinecke <hare@suse.de>
Cc: David Milburn <dmilburn@redhat.com>
---
drivers/scsi/scsi_sysfs.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index f6ce38e..f9db9c7 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -606,10 +606,9 @@ store_state_field(struct device *dev, struct device_attribute *attr,
break;
}
}
- if (!state)
- return -EINVAL;
-
- if (scsi_device_set_state(sdev, state))
+ if (state == 0 || state == SDEV_CANCEL ||
+ state == SDEV_CANCEL_OFFLINE || state == SDEV_DEL ||
+ scsi_device_set_state(sdev, state) != 0)
return -EINVAL;
return count;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 7+ messages in thread