From: Bart Van Assche <bvanassche@acm.org>
To: linux-scsi <linux-scsi@vger.kernel.org>
Cc: James Bottomley <jbottomley@parallels.com>,
Mike Christie <michaelc@cs.wisc.edu>, Tejun Heo <tj@kernel.org>,
Chanho Min <chanho.min@lge.com>,
David Milburn <dmilburn@redhat.com>
Subject: [PATCH v8 09/10] Avoid that scsi_device_set_state() triggers a race
Date: Tue, 05 Feb 2013 13:51:08 +0100 [thread overview]
Message-ID: <5111003C.1080603@acm.org> (raw)
In-Reply-To: <5110FE98.8030209@acm.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Acked-by: Hannes Reinecke <hare@suse.de>
Cc: James Bottomley <JBottomley@Parallels.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Mike Christie <michaelc@cs.wisc.edu>
---
drivers/scsi/scsi_error.c | 4 ++++
drivers/scsi/scsi_lib.c | 43 ++++++++++++++++++++++++++++++++++---------
drivers/scsi/scsi_scan.c | 15 ++++++++-------
drivers/scsi/scsi_sysfs.c | 18 ++++++++++++++----
4 files changed, 60 insertions(+), 20 deletions(-)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 266981a..d50af13 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1430,7 +1430,11 @@ static void scsi_eh_offline_sdevs(struct list_head *work_q,
list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
sdev_printk(KERN_INFO, scmd->device, "Device offlined - "
"not ready after error recovery\n");
+
+ spin_lock_irq(scmd->device->host->host_lock);
scsi_device_set_state(scmd->device, SDEV_OFFLINE);
+ spin_unlock_irq(scmd->device->host->host_lock);
+
if (scmd->eh_eflags & SCSI_EH_CANCEL_CMD) {
/*
* FIXME: Handle lost cmds.
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 27bd445..d826710 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2074,7 +2074,9 @@ EXPORT_SYMBOL(scsi_test_unit_ready);
* @state: state to change to.
*
* Returns zero if unsuccessful or an error if the requested
- * transition is illegal.
+ * transition is illegal. It is the responsibility of the caller to make
+ * sure that a call of this function does not race with other code that
+ * accesses the device state, e.g. by holding the host lock.
*/
int
scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
@@ -2353,7 +2355,13 @@ EXPORT_SYMBOL_GPL(sdev_evt_send_simple);
int
scsi_device_quiesce(struct scsi_device *sdev)
{
- int err = scsi_device_set_state(sdev, SDEV_QUIESCE);
+ struct Scsi_Host *host = sdev->host;
+ int err;
+
+ spin_lock_irq(host->host_lock);
+ err = scsi_device_set_state(sdev, SDEV_QUIESCE);
+ spin_unlock_irq(host->host_lock);
+
if (err)
return err;
@@ -2377,13 +2385,21 @@ EXPORT_SYMBOL(scsi_device_quiesce);
*/
void scsi_device_resume(struct scsi_device *sdev)
{
+ struct Scsi_Host *host = sdev->host;
+ int err;
+
/* check if the device state was mutated prior to resume, and if
* so assume the state is being managed elsewhere (for example
* device deleted during suspend)
*/
- if (sdev->sdev_state != SDEV_QUIESCE ||
- scsi_device_set_state(sdev, SDEV_RUNNING))
+ spin_lock_irq(host->host_lock);
+ err = sdev->sdev_state == SDEV_QUIESCE ?
+ scsi_device_set_state(sdev, SDEV_RUNNING) : -EINVAL;
+ spin_unlock_irq(host->host_lock);
+
+ if (err)
return;
+
scsi_run_queue(sdev->request_queue);
}
EXPORT_SYMBOL(scsi_device_resume);
@@ -2433,17 +2449,19 @@ EXPORT_SYMBOL(scsi_target_resume);
int
scsi_internal_device_block(struct scsi_device *sdev)
{
+ struct Scsi_Host *host = sdev->host;
struct request_queue *q = sdev->request_queue;
unsigned long flags;
int err = 0;
+ spin_lock_irqsave(host->host_lock, flags);
err = scsi_device_set_state(sdev, SDEV_BLOCK);
- if (err) {
+ if (err)
err = scsi_device_set_state(sdev, SDEV_CREATED_BLOCK);
+ spin_unlock_irqrestore(host->host_lock, flags);
- if (err)
- return err;
- }
+ if (err)
+ return err;
/*
* The device has transitioned to SDEV_BLOCK. Stop the
@@ -2478,13 +2496,16 @@ int
scsi_internal_device_unblock(struct scsi_device *sdev,
enum scsi_device_state new_state)
{
+ struct Scsi_Host *host = sdev->host;
struct request_queue *q = sdev->request_queue;
unsigned long flags;
+ int ret = 0;
/*
* Try to transition the scsi device to SDEV_RUNNING or one of the
* offlined states and goose the device queue if successful.
*/
+ spin_lock_irqsave(host->host_lock, flags);
if ((sdev->sdev_state == SDEV_BLOCK) ||
(sdev->sdev_state == SDEV_TRANSPORT_OFFLINE))
sdev->sdev_state = new_state;
@@ -2496,7 +2517,11 @@ 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;
+ ret = -EINVAL;
+ spin_unlock_irqrestore(host->host_lock, flags);
+
+ if (ret)
+ return ret;
spin_lock_irqsave(q->queue_lock, flags);
blk_start_queue(q);
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 0612fba..3fbdd7b 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -898,18 +898,19 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
if (*bflags & BLIST_USE_10_BYTE_MS)
sdev->use_10_for_ms = 1;
+ spin_lock_irq(sdev->host->host_lock);
/* set the device running here so that slave configure
* may do I/O */
ret = scsi_device_set_state(sdev, SDEV_RUNNING);
- if (ret) {
+ if (ret)
ret = scsi_device_set_state(sdev, SDEV_BLOCK);
+ spin_unlock_irq(sdev->host->host_lock);
- if (ret) {
- sdev_printk(KERN_ERR, sdev,
- "in wrong state %s to complete scan\n",
- scsi_device_state_name(sdev->sdev_state));
- return SCSI_SCAN_NO_RESPONSE;
- }
+ if (ret) {
+ sdev_printk(KERN_ERR, sdev,
+ "in wrong state %s to complete scan\n",
+ scsi_device_state_name(sdev->sdev_state));
+ return SCSI_SCAN_NO_RESPONSE;
}
if (*bflags & BLIST_MS_192_BYTES_FOR_3F)
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index bd51d65..6068cef 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -593,7 +593,7 @@ static ssize_t
store_state_field(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
- int i;
+ int i, ret;
struct scsi_device *sdev = to_scsi_device(dev);
enum scsi_device_state state = INVALID_SDEV_STATE;
@@ -609,9 +609,11 @@ store_state_field(struct device *dev, struct device_attribute *attr,
state == SDEV_DEL)
return -EINVAL;
- if (scsi_device_set_state(sdev, state))
- return -EINVAL;
- return count;
+ spin_lock_irq(sdev->host->host_lock);
+ ret = scsi_device_set_state(sdev, state);
+ spin_unlock_irq(sdev->host->host_lock);
+
+ return ret < 0 ? ret : count;
}
static ssize_t
@@ -871,7 +873,10 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
struct request_queue *rq = sdev->request_queue;
struct scsi_target *starget = sdev->sdev_target;
+ spin_lock_irq(sdev->host->host_lock);
error = scsi_device_set_state(sdev, SDEV_RUNNING);
+ spin_unlock_irq(sdev->host->host_lock);
+
if (error)
return error;
@@ -958,8 +963,10 @@ void __scsi_remove_device(struct scsi_device *sdev)
unsigned long flags;
if (sdev->is_visible) {
+ spin_lock_irqsave(shost->host_lock, flags);
WARN_ON_ONCE(scsi_device_set_state(sdev, SDEV_CANCEL) != 0 &&
scsi_device_set_state(sdev, SDEV_DEL) != 0);
+ spin_unlock_irqrestore(shost->host_lock, flags);
bsg_unregister_queue(sdev->request_queue);
device_unregister(&sdev->sdev_dev);
@@ -973,7 +980,10 @@ void __scsi_remove_device(struct scsi_device *sdev)
* scsi_run_queue() invocations have finished before tearing down the
* device.
*/
+ spin_lock_irqsave(shost->host_lock, flags);
scsi_device_set_state(sdev, SDEV_DEL);
+ spin_unlock_irqrestore(shost->host_lock, flags);
+
blk_cleanup_queue(sdev->request_queue);
cancel_work_sync(&sdev->requeue_work);
--
1.7.10.4
next prev parent reply other threads:[~2013-02-05 12:51 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-02-05 12:44 [PATCH v8 0/10] More device removal fixes Bart Van Assche
2013-02-05 12:45 ` [PATCH v8 01/10] Fix race between starved list processing and device removal Bart Van Assche
2013-02-05 12:46 ` [PATCH v8 02/10] Remove get_device() / put_device() pair from scsi_request_fn() Bart Van Assche
2013-02-05 12:47 ` [PATCH v8 03/10] Introduce scsi_device_being_removed() Bart Van Assche
2013-02-05 12:47 ` [PATCH v8 04/10] Remove offline devices when removing a host Bart Van Assche
2013-02-05 12:48 ` [PATCH v8 05/10] Disallow changing the device state via sysfs into "deleted" Bart Van Assche
2013-02-05 12:49 ` [PATCH v8 06/10] Avoid saving/restoring interrupt state inside scsi_remove_host() Bart Van Assche
2013-02-05 12:49 ` [PATCH v8 07/10] Make scsi_remove_host() wait for device removal Bart Van Assche
2013-02-05 12:50 ` [PATCH v8 08/10] Make scsi_remove_host() wait until error handling finished Bart Van Assche
2013-02-05 12:51 ` Bart Van Assche [this message]
2013-02-05 12:51 ` [PATCH v8 10/10] Save and restore host_scribble during error handling Bart Van Assche
2013-02-06 22:31 ` [PATCH v8 0/10] More device removal fixes Joe Lawrence
2013-02-07 10:40 ` Bart Van Assche
2013-02-07 11:33 ` Bart Van Assche
2013-02-08 23:29 ` Joe Lawrence
2013-02-09 9:28 ` Bart Van Assche
2013-02-11 20:36 ` Bart Van Assche
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5111003C.1080603@acm.org \
--to=bvanassche@acm.org \
--cc=chanho.min@lge.com \
--cc=dmilburn@redhat.com \
--cc=jbottomley@parallels.com \
--cc=linux-scsi@vger.kernel.org \
--cc=michaelc@cs.wisc.edu \
--cc=tj@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).