[PATCH v7 0/9] More device removal fixes

[PATCH v7 0/9] More device removal fixes

[Bart Van Assche]

Fix a few race conditions that can be triggered by removing a device:
- Fix a race between starved list processing and device removal.
- Avoid that a SCSI LLD callback can get invoked after
   scsi_remove_host() finished.
- Speed up device removal by stopping error handling as soon as
   the SHOST_DEL or SHOST_DEL_RECOVERY state has been reached.

These patches have been tested on top of kernel v3.7-rc7.

Changes compared to v6:
- Dropped the first six patches since Jens queued these for 3.8.
- Added patch to avoid that __scsi_remove_device() is invoked twice.
- Restore error recovery in the SHOST_CANCEL state.

Changes compared to v5:
- Avoid that block layer work can be scheduled on a dead queue.
- Do not invoke any SCSI LLD callback after scsi_remove_host() finished.
- Stop error handling as soon as scsi_remove_host() started.
- Remove the unused function bsg_goose_queue().
- Avoid that scsi_device_set_state() triggers a race condition.

Changes compared to v4:
- Moved queue_flag_set(QUEUE_FLAG_DEAD, q) from blk_drain_queue() into
   blk_cleanup_queue().
- Declared the new __blk_run_queue_uncond() function inline. Checked in
   the generated assembler code that this function is really inlined in
   __blk_run_queue().
- Elaborated several patch descriptions.
- Added sparse annotations to scsi_request_fn().
- Split several patches.

Changes compared to v3:
- Fixed a race condition by setting QUEUE_FLAG_DEAD earlier.
- Added a patch for fixing a race between starved list processing
   and device removal to this series.

Changes compared to v2:
- Split second patch into two patches.
- Refined patch descriptions.

Changes compared to v1:
- Included a patch to rename QUEUE_FLAG_DEAD.
- Refined the descriptions of the __blk_run_queue_uncond() and
   blk_cleanup_queue() functions.

[PATCH v7 1/9] Fix race between starved list processing and device removal

[Bart Van Assche]

Avoid that the sdev reference count can drop to zero before
a queue is run by scsi_run_queue().

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Reported-and-tested-by: Chanho Min <chanho.min@lge.com>
Reference: http://lkml.org/lkml/2012/8/2/96
Acked-by: Tejun Heo <tj@kernel.org>
Reviewed-by: Mike Christie <michaelc@cs.wisc.edu>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: <stable@vger.kernel.org>
---
 drivers/scsi/scsi_lib.c   |   16 +++++++++++-----
 drivers/scsi/scsi_sysfs.c |   14 +++++++++++++-
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f1bf5af..5c67339 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -452,11 +452,17 @@ 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);
+		/*
+		 * Obtain a reference before unlocking the host_lock such that
+		 * the sdev can't disappear before blk_run_queue() is invoked.
+		 */
+		get_device(&sdev->sdev_gendev);
+		spin_unlock_irqrestore(shost->host_lock, flags);
+
+		blk_run_queue(sdev->request_queue);
+		put_device(&sdev->sdev_gendev);
+
+		spin_lock_irqsave(shost->host_lock, flags);
 	}
 	/* put any unprocessed entries back */
 	list_splice(&starved_list, &shost->starved_list);
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index ce5224c..2ff7ba5 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -348,7 +348,6 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 	starget->reap_ref++;
 	list_del(&sdev->siblings);
 	list_del(&sdev->same_target_siblings);
-	list_del(&sdev->starved_entry);
 	spin_unlock_irqrestore(sdev->host->host_lock, flags);
 
 	cancel_work_sync(&sdev->event_work);
@@ -956,6 +955,8 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 void __scsi_remove_device(struct scsi_device *sdev)
 {
 	struct device *dev = &sdev->sdev_gendev;
+	struct Scsi_Host *shost = sdev->host;
+	unsigned long flags;
 
 	if (sdev->is_visible) {
 		if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
@@ -977,6 +978,17 @@ void __scsi_remove_device(struct scsi_device *sdev)
 	blk_cleanup_queue(sdev->request_queue);
 	cancel_work_sync(&sdev->requeue_work);
 
+	/*
+	 * Remove a SCSI device from the starved list after blk_cleanup_queue()
+	 * finished such that scsi_request_fn() can't add it back to that list.
+	 * Also remove an sdev from the starved list before invoking
+	 * put_device() such that get_device() is guaranteed to succeed for an
+	 * sdev present on the starved list.
+	 */
+	spin_lock_irqsave(shost->host_lock, flags);
+	list_del(&sdev->starved_entry);
+	spin_unlock_irqrestore(shost->host_lock, flags);
+
 	if (sdev->host->hostt->slave_destroy)
 		sdev->host->hostt->slave_destroy(sdev);
 	transport_destroy_device(dev);
-- 
1.7.10.4

[PATCH v7 2/9] Remove get_device() / put_device() pair from scsi_request_fn()

[Bart Van Assche]

Now that all scsi_request_fn() callers hold a reference on the
SCSI device that function is invoked for and since
blk_cleanup_queue() waits until scsi_request_fn() has finished
it is safe to remove the get_device() / put_device() pair from
scsi_request_fn().

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Acked-by: Tejun Heo <tj@kernel.org>
Cc: James Bottomley <JBottomley@Parallels.com>
Cc: Mike Christie <michaelc@cs.wisc.edu>
Cc: Jens Axboe <axboe@kernel.dk>
---
 drivers/scsi/scsi_lib.c |   14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 5c67339..db4836bc 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1529,16 +1529,14 @@ static void scsi_softirq_done(struct request *rq)
  * Lock status: IO request lock assumed to be held when called.
  */
 static void scsi_request_fn(struct request_queue *q)
+	__releases(q->queue_lock)
+	__acquires(q->queue_lock)
 {
 	struct scsi_device *sdev = q->queuedata;
 	struct Scsi_Host *shost;
 	struct scsi_cmnd *cmd;
 	struct request *req;
 
-	if(!get_device(&sdev->sdev_gendev))
-		/* We must be tearing the block queue down already */
-		return;
-
 	/*
 	 * To start with, we keep looping until the queue is empty, or until
 	 * the host is no longer able to accept any more requests.
@@ -1627,7 +1625,7 @@ static void scsi_request_fn(struct request_queue *q)
 			goto out_delay;
 	}
 
-	goto out;
+	return;
 
  not_ready:
 	spin_unlock_irq(shost->host_lock);
@@ -1646,12 +1644,6 @@ static void scsi_request_fn(struct request_queue *q)
 out_delay:
 	if (sdev->device_busy == 0)
 		blk_delay_queue(q, SCSI_QUEUE_DELAY);
-out:
-	/* must be careful here...if we trigger the ->remove() function
-	 * we cannot be holding the q lock */
-	spin_unlock_irq(q->queue_lock);
-	put_device(&sdev->sdev_gendev);
-	spin_lock_irq(q->queue_lock);
 }
 
 u64 scsi_calculate_bounce_limit(struct Scsi_Host *shost)
-- 
1.7.10.4

[PATCH v7 3/9] Introduce scsi_device_being_removed()

[Bart Van Assche]

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: Hannes Reinecke <hare@suse.de>
Cc: James Bottomley <JBottomley@Parallels.com>
Cc: Mike Christie <michaelc@cs.wisc.edu>
Cc: Tejun Heo <tj@kernel.org>
---
 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 55367b0..767dd16 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -442,6 +442,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

[PATCH v7 4/9] Remove offline devices when removing a host

[Bart Van Assche]

Currently __scsi_remove_device() skips devices that are visible and
offline. Make sure that these devices get removed by changing their
device state into SDEV_DEL at the start of __scsi_remove_device().
Also, avoid that __scsi_remove_device() gets called a second time
for devices that are in state SDEV_CANCEL when scsi_forget_host()
is invoked.

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: Tejun Heo <tj@kernel.org>
---
 drivers/scsi/scsi_scan.c  |    2 +-
 drivers/scsi/scsi_sysfs.c |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 3e58b22..0612fba 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1889,7 +1889,7 @@ void scsi_forget_host(struct Scsi_Host *shost)
  restart:
 	spin_lock_irqsave(shost->host_lock, flags);
 	list_for_each_entry(sdev, &shost->__devices, siblings) {
-		if (sdev->sdev_state == SDEV_DEL)
+		if (scsi_device_being_removed(sdev))
 			continue;
 		spin_unlock_irqrestore(shost->host_lock, flags);
 		__scsi_remove_device(sdev);
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 2ff7ba5..4348f12 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -959,8 +959,8 @@ void __scsi_remove_device(struct scsi_device *sdev)
 	unsigned long flags;
 
 	if (sdev->is_visible) {
-		if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
-			return;
+		WARN_ON_ONCE(scsi_device_set_state(sdev, SDEV_CANCEL) != 0 &&
+			     scsi_device_set_state(sdev, SDEV_DEL) != 0);
 
 		bsg_unregister_queue(sdev->request_queue);
 		device_unregister(&sdev->sdev_dev);
-- 
1.7.10.4

[PATCH v7 5/9] Disallow changing the device state via sysfs into "deleted"

[Bart Van Assche]

Changing the state of a SCSI device via sysfs into "cancel" or
"deleted" prevents scsi_remove_host() to remove these devices.
Hence do not allow this.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: James Bottomley <JBottomley@Parallels.com>
Cc: Mike Christie <michaelc@cs.wisc.edu>
Cc: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/scsi_sysfs.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 4348f12..b319c20 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -591,13 +591,15 @@ sdev_store_delete(struct device *dev, struct device_attribute *attr,
 };
 static DEVICE_ATTR(delete, S_IWUSR, NULL, sdev_store_delete);
 
+#define INVALID_SDEV_STATE 0
+
 static ssize_t
 store_state_field(struct device *dev, struct device_attribute *attr,
 		  const char *buf, size_t count)
 {
 	int i;
 	struct scsi_device *sdev = to_scsi_device(dev);
-	enum scsi_device_state state = 0;
+	enum scsi_device_state state = INVALID_SDEV_STATE;
 
 	for (i = 0; i < ARRAY_SIZE(sdev_states); i++) {
 		const int len = strlen(sdev_states[i].name);
@@ -607,7 +609,8 @@ store_state_field(struct device *dev, struct device_attribute *attr,
 			break;
 		}
 	}
-	if (!state)
+	if (state == INVALID_SDEV_STATE || state == SDEV_CANCEL ||
+	    state == SDEV_DEL)
 		return -EINVAL;
 
 	if (scsi_device_set_state(sdev, state))
-- 
1.7.10.4

[PATCH v7 6/9] Avoid saving/restoring interrupt state inside scsi_remove_host()

[Bart Van Assche]

Since it is not allowed to invoke scsi_remove_host() with interrupts
disabled, avoid saving and restoring the interrupt state inside
scsi_remove_host(). This patch does not change the functionality of
that function.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Acked-by: Tejun Heo <tj@kernel.org>
Cc: Mike Christie <michaelc@cs.wisc.edu>
Cc: Hannes Reinecke <hare@suse.de>
Cc: James Bottomley <JBottomley@Parallels.com>
---
 drivers/scsi/hosts.c |   12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 593085a..6ae16cd 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -156,27 +156,25 @@ EXPORT_SYMBOL(scsi_host_set_state);
  **/
 void scsi_remove_host(struct Scsi_Host *shost)
 {
-	unsigned long flags;
-
 	mutex_lock(&shost->scan_mutex);
-	spin_lock_irqsave(shost->host_lock, flags);
+	spin_lock_irq(shost->host_lock);
 	if (scsi_host_set_state(shost, SHOST_CANCEL))
 		if (scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY)) {
-			spin_unlock_irqrestore(shost->host_lock, flags);
+			spin_unlock_irq(shost->host_lock);
 			mutex_unlock(&shost->scan_mutex);
 			return;
 		}
-	spin_unlock_irqrestore(shost->host_lock, flags);
+	spin_unlock_irq(shost->host_lock);
 
 	scsi_autopm_get_host(shost);
 	scsi_forget_host(shost);
 	mutex_unlock(&shost->scan_mutex);
 	scsi_proc_host_rm(shost);
 
-	spin_lock_irqsave(shost->host_lock, flags);
+	spin_lock_irq(shost->host_lock);
 	if (scsi_host_set_state(shost, SHOST_DEL))
 		BUG_ON(scsi_host_set_state(shost, SHOST_DEL_RECOVERY));
-	spin_unlock_irqrestore(shost->host_lock, flags);
+	spin_unlock_irq(shost->host_lock);
 
 	transport_unregister_device(&shost->shost_gendev);
 	device_unregister(&shost->shost_dev);
-- 
1.7.10.4

[PATCH v7 7/9] Make scsi_remove_host() wait for device removal

[Bart Van Assche]

If a device is removed via sysfs before scsi_remove_host() is
invoked that device may still be in state SDEV_CANCEL when
scsi_remove_host() returns. SCSI LLDs may start cleaning up host
resources needed by their queuecommand() callback as soon as
scsi_remove_host() finished. Hence scsi_remove_host() must wait
until blk_cleanup_queue() for all devices associated with the
host has finished. That avoids that queuecommand() gets invoked
after scsi_remove_host() finished.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: James Bottomley <JBottomley@Parallels.com>
Cc: Mike Christie <michaelc@cs.wisc.edu>
Cc: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/hosts.c      |   30 ++++++++++++++++++++++++++++++
 drivers/scsi/scsi_priv.h  |    1 +
 drivers/scsi/scsi_sysfs.c |    1 +
 include/scsi/scsi_host.h  |    1 +
 4 files changed, 33 insertions(+)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 6ae16cd..b68a013 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -150,12 +150,31 @@ int scsi_host_set_state(struct Scsi_Host *shost, enum scsi_host_state state)
 }
 EXPORT_SYMBOL(scsi_host_set_state);
 
+/* Return true if and only if scsi_remove_host() is allowed to finish. */
+static bool scsi_remove_host_done(struct Scsi_Host *shost)
+{
+	lockdep_assert_held(shost->host_lock);
+
+	return list_empty(&shost->__devices);
+}
+
+/* Test whether scsi_remove_host() may finish, and if so, wake it up. */
+void scsi_check_remove_host_done(struct Scsi_Host *shost)
+{
+	lockdep_assert_held(shost->host_lock);
+
+	if (scsi_remove_host_done(shost))
+		wake_up(&shost->remove_host);
+}
+
 /**
  * scsi_remove_host - remove a scsi host
  * @shost:	a pointer to a scsi host to remove
  **/
 void scsi_remove_host(struct Scsi_Host *shost)
 {
+	DEFINE_WAIT(wait);
+
 	mutex_lock(&shost->scan_mutex);
 	spin_lock_irq(shost->host_lock);
 	if (scsi_host_set_state(shost, SHOST_CANCEL))
@@ -174,6 +193,16 @@ void scsi_remove_host(struct Scsi_Host *shost)
 	spin_lock_irq(shost->host_lock);
 	if (scsi_host_set_state(shost, SHOST_DEL))
 		BUG_ON(scsi_host_set_state(shost, SHOST_DEL_RECOVERY));
+	while (!scsi_remove_host_done(shost)) {
+		prepare_to_wait_exclusive(&shost->remove_host, &wait,
+					  TASK_INTERRUPTIBLE);
+		if (scsi_remove_host_done(shost))
+			break;
+		spin_unlock_irq(shost->host_lock);
+		schedule();
+		spin_lock_irq(shost->host_lock);
+	}
+	finish_wait(&shost->remove_host, &wait);
 	spin_unlock_irq(shost->host_lock);
 
 	transport_unregister_device(&shost->shost_gendev);
@@ -349,6 +378,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 	shost->shost_state = SHOST_CREATED;
 	INIT_LIST_HEAD(&shost->__devices);
 	INIT_LIST_HEAD(&shost->__targets);
+	init_waitqueue_head(&shost->remove_host);
 	INIT_LIST_HEAD(&shost->eh_cmd_q);
 	INIT_LIST_HEAD(&shost->starved_list);
 	init_waitqueue_head(&shost->host_wait);
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 8f9a0ca..882c823 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -26,6 +26,7 @@ struct scsi_nl_hdr;
 /* hosts.c */
 extern int scsi_init_hosts(void);
 extern void scsi_exit_hosts(void);
+extern void scsi_check_remove_host_done(struct Scsi_Host *shost);
 
 /* scsi.c */
 extern int scsi_dispatch_cmd(struct scsi_cmnd *cmd);
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index b319c20..6368576 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -348,6 +348,7 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 	starget->reap_ref++;
 	list_del(&sdev->siblings);
 	list_del(&sdev->same_target_siblings);
+	scsi_check_remove_host_done(sdev->host);
 	spin_unlock_irqrestore(sdev->host->host_lock, flags);
 
 	cancel_work_sync(&sdev->event_work);
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 4908480..1b7fd89 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -577,6 +577,7 @@ struct Scsi_Host {
 	struct completion     * eh_action; /* Wait for specific actions on the
 					      host. */
 	wait_queue_head_t       host_wait;
+	wait_queue_head_t	remove_host;
 	struct scsi_host_template *hostt;
 	struct scsi_transport_template *transportt;
 
-- 
1.7.10.4

[PATCH v7 8/9] Make scsi_remove_host() wait until error handling finished

[Bart Van Assche]

A SCSI LLD may start cleaning up host resources as soon as
scsi_remove_host() returns. These host resources may be needed by
the LLD in an implementation of one of the eh_* functions. So if
one of the eh_* functions is in progress when scsi_remove_host()
is invoked, wait until the eh_* function has finished. Also, do
not invoke any of the eh_* functions after scsi_remove_host() has
started.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Mike Christie <michaelc@cs.wisc.edu>
Cc: Tejun Heo <tj@kernel.org>
---
 drivers/scsi/hosts.c      |    2 +-
 drivers/scsi/scsi_error.c |   76 +++++++++++++++++++++++++++++++++++++++++++--
 include/scsi/scsi_host.h  |    1 +
 3 files changed, 76 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index b68a013..a941861 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -155,7 +155,7 @@ static bool scsi_remove_host_done(struct Scsi_Host *shost)
 {
 	lockdep_assert_held(shost->host_lock);
 
-	return list_empty(&shost->__devices);
+	return list_empty(&shost->__devices) && !shost->eh_active;
 }
 
 /* Test whether scsi_remove_host() may finish, and if so, wake it up. */
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index c1b05a8..76761aa 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -536,8 +536,59 @@ static void scsi_eh_done(struct scsi_cmnd *scmd)
 }
 
 /**
+ * scsi_begin_eh - start host-related error handling
+ *
+ * Must be called before invoking any of the scsi_host_template.eh_* functions
+ * to avoid that scsi_remove_host() returns while one of these callback
+ * functions is in progress.
+ *
+ * Returns 0 if invoking an eh_* function is allowed and a negative value if
+ * not. If this function returns 0 then scsi_end_eh() must be called
+ * eventually.
+ *
+ * Note: scsi_send_eh_cmnd() calls do not have to be protected by a
+ * scsi_begin_eh() / scsi_end_eh() pair since these operate on an unfinished
+ * block layer request. Since scsi_remove_host() waits until all SCSI devices
+ * have been removed and since blk_cleanup_queue() is invoked during SCSI
+ * device removal scsi_remove_host() won't finish while a scsi_send_eh_cmnd()
+ * call is in progress.
+ */
+static int scsi_begin_eh(struct Scsi_Host *host)
+{
+	int res;
+
+	spin_lock_irq(host->host_lock);
+	switch (host->shost_state) {
+	case SHOST_DEL:
+	case SHOST_DEL_RECOVERY:
+		res = -ENODEV;
+		break;
+	default:
+		WARN_ON_ONCE(host->eh_active < 0 || host->eh_active > 1);
+		host->eh_active++;
+		res = 0;
+		break;
+	}
+	spin_unlock_irq(host->host_lock);
+
+	return res;
+}
+
+/**
+ * scsi_end_eh - finish host-related error handling
+ */
+static void scsi_end_eh(struct Scsi_Host *host)
+{
+	spin_lock_irq(host->host_lock);
+	host->eh_active--;
+	WARN_ON_ONCE(host->eh_active < 0 || host->eh_active > 1);
+	scsi_check_remove_host_done(host);
+	spin_unlock_irq(host->host_lock);
+}
+
+/**
  * scsi_try_host_reset - ask host adapter to reset itself
- * @scmd:	SCSI cmd to send hsot reset.
+ * @scmd:	SCSI cmd to send host reset.
  */
 static int scsi_try_host_reset(struct scsi_cmnd *scmd)
 {
@@ -552,6 +603,9 @@ static int scsi_try_host_reset(struct scsi_cmnd *scmd)
 	if (!hostt->eh_host_reset_handler)
 		return FAILED;
 
+	if (scsi_begin_eh(host))
+		return FAST_IO_FAIL;
+
 	rtn = hostt->eh_host_reset_handler(scmd);
 
 	if (rtn == SUCCESS) {
@@ -561,6 +615,7 @@ static int scsi_try_host_reset(struct scsi_cmnd *scmd)
 		scsi_report_bus_reset(host, scmd_channel(scmd));
 		spin_unlock_irqrestore(host->host_lock, flags);
 	}
+	scsi_end_eh(host);
 
 	return rtn;
 }
@@ -582,6 +637,9 @@ static int scsi_try_bus_reset(struct scsi_cmnd *scmd)
 	if (!hostt->eh_bus_reset_handler)
 		return FAILED;
 
+	if (scsi_begin_eh(host))
+		return FAST_IO_FAIL;
+
 	rtn = hostt->eh_bus_reset_handler(scmd);
 
 	if (rtn == SUCCESS) {
@@ -591,6 +649,7 @@ static int scsi_try_bus_reset(struct scsi_cmnd *scmd)
 		scsi_report_bus_reset(host, scmd_channel(scmd));
 		spin_unlock_irqrestore(host->host_lock, flags);
 	}
+	scsi_end_eh(host);
 
 	return rtn;
 }
@@ -621,6 +680,9 @@ static int scsi_try_target_reset(struct scsi_cmnd *scmd)
 	if (!hostt->eh_target_reset_handler)
 		return FAILED;
 
+	if (scsi_begin_eh(host))
+		return FAST_IO_FAIL;
+
 	rtn = hostt->eh_target_reset_handler(scmd);
 	if (rtn == SUCCESS) {
 		spin_lock_irqsave(host->host_lock, flags);
@@ -628,6 +690,7 @@ static int scsi_try_target_reset(struct scsi_cmnd *scmd)
 					  __scsi_report_device_reset);
 		spin_unlock_irqrestore(host->host_lock, flags);
 	}
+	scsi_end_eh(host);
 
 	return rtn;
 }
@@ -645,14 +708,20 @@ static int scsi_try_target_reset(struct scsi_cmnd *scmd)
 static int scsi_try_bus_device_reset(struct scsi_cmnd *scmd)
 {
 	int rtn;
-	struct scsi_host_template *hostt = scmd->device->host->hostt;
+	struct Scsi_Host *host = scmd->device->host;
+	struct scsi_host_template *hostt = host->hostt;
 
 	if (!hostt->eh_device_reset_handler)
 		return FAILED;
 
+	if (scsi_begin_eh(host))
+		return FAST_IO_FAIL;
+
 	rtn = hostt->eh_device_reset_handler(scmd);
 	if (rtn == SUCCESS)
 		__scsi_report_device_reset(scmd->device, NULL);
+	scsi_end_eh(host);
+
 	return rtn;
 }
 
@@ -1877,6 +1946,9 @@ int scsi_error_handler(void *data)
 	}
 	__set_current_state(TASK_RUNNING);
 
+	WARN_ONCE(shost->eh_active, "scsi_eh_%d: eh_active = %d\n",
+		  shost->host_no, shost->eh_active);
+
 	SCSI_LOG_ERROR_RECOVERY(1,
 		printk("Error handler scsi_eh_%d exiting\n", shost->host_no));
 	shost->ehandler = NULL;
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 1b7fd89..5e2fcd2 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -576,6 +576,7 @@ struct Scsi_Host {
 	struct task_struct    * ehandler;  /* Error recovery thread. */
 	struct completion     * eh_action; /* Wait for specific actions on the
 					      host. */
+	int			eh_active;
 	wait_queue_head_t       host_wait;
 	wait_queue_head_t	remove_host;
 	struct scsi_host_template *hostt;
-- 
1.7.10.4

[PATCH v7 9/9] Avoid that scsi_device_set_state() triggers a race

[Bart Van Assche]

Make concurrent invocations of scsi_device_set_state() safe.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: James Bottomley <JBottomley@Parallels.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Hannes Reinecke <hare@suse.de>
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 |   19 ++++++++++++++++---
 4 files changed, 62 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 76761aa..dbc1fb86 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1432,7 +1432,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 db4836bc..f6e415d 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)
@@ -2351,7 +2353,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;
 
@@ -2375,13 +2383,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);
@@ -2431,17 +2447,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
@@ -2476,13 +2494,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;
@@ -2494,7 +2515,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 6368576..b2ebcfd 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -598,7 +598,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;
 
@@ -614,9 +614,14 @@ store_state_field(struct device *dev, struct device_attribute *attr,
 	    state == SDEV_DEL)
 		return -EINVAL;
 
+	ret = count;
+
+	spin_lock_irq(sdev->host->host_lock);
 	if (scsi_device_set_state(sdev, state))
-		return -EINVAL;
-	return count;
+		ret = -EINVAL;
+	spin_unlock_irq(sdev->host->host_lock);
+
+	return ret;
 }
 
 static ssize_t
@@ -876,7 +881,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;
 
@@ -963,8 +971,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);
@@ -978,7 +988,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

Re: [PATCH v7 3/9] Introduce scsi_device_being_removed()

[Hannes Reinecke]

On 12/06/2012 04:55 PM, Bart Van Assche wrote:
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: James Bottomley <JBottomley@Parallels.com>
> Cc: Mike Christie <michaelc@cs.wisc.edu>
> Cc: Tejun Heo <tj@kernel.org>
Acked-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 5/9] Disallow changing the device state via sysfs into "deleted"

[Hannes Reinecke]

On 12/06/2012 04:56 PM, Bart Van Assche wrote:
> Changing the state of a SCSI device via sysfs into "cancel" or
> "deleted" prevents scsi_remove_host() to remove these devices.
> Hence do not allow this.
>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: James Bottomley <JBottomley@Parallels.com>
> Cc: Mike Christie <michaelc@cs.wisc.edu>
> Cc: Hannes Reinecke <hare@suse.de>
> ---
>   drivers/scsi/scsi_sysfs.c |    7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index 4348f12..b319c20 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -591,13 +591,15 @@ sdev_store_delete(struct device *dev, struct device_attribute *attr,
>   };
>   static DEVICE_ATTR(delete, S_IWUSR, NULL, sdev_store_delete);
>
> +#define INVALID_SDEV_STATE 0
> +
Shouldn't this become part of the enum?
Defining it outside only confuses the compiler.
And the unsuspecting user.

>   static ssize_t
>   store_state_field(struct device *dev, struct device_attribute *attr,
>   		  const char *buf, size_t count)
>   {
>   	int i;
>   	struct scsi_device *sdev = to_scsi_device(dev);
> -	enum scsi_device_state state = 0;
> +	enum scsi_device_state state = INVALID_SDEV_STATE;
>
>   	for (i = 0; i < ARRAY_SIZE(sdev_states); i++) {
>   		const int len = strlen(sdev_states[i].name);
> @@ -607,7 +609,8 @@ store_state_field(struct device *dev, struct device_attribute *attr,
>   			break;
>   		}
>   	}
> -	if (!state)
> +	if (state == INVALID_SDEV_STATE || state == SDEV_CANCEL ||
> +	    state == SDEV_DEL)
>   		return -EINVAL;
>
>   	if (scsi_device_set_state(sdev, state))
>
Why not scsi_device_being_removed() here?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 6/9] Avoid saving/restoring interrupt state inside scsi_remove_host()

[Hannes Reinecke]

On 12/06/2012 04:57 PM, Bart Van Assche wrote:
> Since it is not allowed to invoke scsi_remove_host() with interrupts
> disabled, avoid saving and restoring the interrupt state inside
> scsi_remove_host(). This patch does not change the functionality of
> that function.
>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> Acked-by: Tejun Heo <tj@kernel.org>
> Cc: Mike Christie <michaelc@cs.wisc.edu>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: James Bottomley <JBottomley@Parallels.com>
Acked-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 8/9] Make scsi_remove_host() wait until error handling finished

[Hannes Reinecke]

On 12/06/2012 04:59 PM, Bart Van Assche wrote:
> A SCSI LLD may start cleaning up host resources as soon as
> scsi_remove_host() returns. These host resources may be needed by
> the LLD in an implementation of one of the eh_* functions. So if
> one of the eh_* functions is in progress when scsi_remove_host()
> is invoked, wait until the eh_* function has finished. Also, do
> not invoke any of the eh_* functions after scsi_remove_host() has
> started.
>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Mike Christie <michaelc@cs.wisc.edu>
> Cc: Tejun Heo <tj@kernel.org>
Acked-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 9/9] Avoid that scsi_device_set_state() triggers a race

[Hannes Reinecke]

On 12/06/2012 05:00 PM, Bart Van Assche wrote:
> Make concurrent invocations of scsi_device_set_state() safe.
>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> Cc: James Bottomley <JBottomley@Parallels.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Mike Christie <michaelc@cs.wisc.edu>
Acked-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 3/9] Introduce scsi_device_being_removed()

[Rolf Eike Beer]

> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 55367b0..767dd16 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -442,6 +442,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)

Newline missing before the new function.

Greetings,

Eike

Re: [PATCH v7 3/9] Introduce scsi_device_being_removed()

[Bart Van Assche]

On 12/07/12 09:40, Rolf Eike Beer wrote:
>> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
>> index 55367b0..767dd16 100644
>> --- a/include/scsi/scsi_device.h
>> +++ b/include/scsi/scsi_device.h
>> @@ -442,6 +442,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)
>
> Newline missing before the new function.

Hello Rolf,

Well spotted. Did you realize I had left out the newline on purpose, to 
preserve consistency with the rest of the header file ?

Thanks,

Bart.

Re: [PATCH v7 3/9] Introduce scsi_device_being_removed()

[Rolf Eike Beer]

Am , schrieb Bart Van Assche:
> On 12/07/12 09:40, Rolf Eike Beer wrote:
>>> diff --git a/include/scsi/scsi_device.h 
>>> b/include/scsi/scsi_device.h
>>> index 55367b0..767dd16 100644
>>> --- a/include/scsi/scsi_device.h
>>> +++ b/include/scsi/scsi_device.h
>>> @@ -442,6 +442,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)
>>
>> Newline missing before the new function.
>
> Hello Rolf,
>
> Well spotted. Did you realize I had left out the newline on purpose,
> to preserve consistency with the rest of the header file ?

Well, there is already a newline between scsi_device_created() and 
scsi_device_sync() where you insert your stuff so this sort of 
consistency is hard to spot.

Eike

Re: [PATCH v7 3/9] Introduce scsi_device_being_removed()

[Bart Van Assche]

On 12/07/12 11:02, Rolf Eike Beer wrote:
> Am , schrieb Bart Van Assche:
>> On 12/07/12 09:40, Rolf Eike Beer wrote:
>>>> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
>>>> index 55367b0..767dd16 100644
>>>> --- a/include/scsi/scsi_device.h
>>>> +++ b/include/scsi/scsi_device.h
>>>> @@ -442,6 +442,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)
>>>
>>> Newline missing before the new function.
>>
>> Hello Rolf,
>>
>> Well spotted. Did you realize I had left out the newline on purpose,
>> to preserve consistency with the rest of the header file ?
> 
> Well, there is already a newline between scsi_device_created() and 
> scsi_device_sync() where you insert your stuff so this sort of 
> consistency is hard to spot.

Hello Eike,

So you really care about these blank lines ? How about the patch below ?
It is identical to the patch at the start of this thread except that it
adds a blank line between each pair of functions in <scsi/scsi_device.h>
where there was not yet a blank line.

---
 drivers/scsi/device_handler/scsi_dh.c |    7 ++-----
 include/scsi/scsi_device.h            |   15 +++++++++++++++
 2 files changed, 17 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 55367b0..b9b19ca 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -426,54 +426,69 @@ static inline unsigned int sdev_id(struct scsi_device *sdev)
 /*
  * checks for positions of the SCSI state machine
  */
+
 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_DEL);
 }
+
 static inline int scsi_device_blocked(struct scsi_device *sdev)
 {
 	return sdev->sdev_state == SDEV_BLOCK ||
 		sdev->sdev_state == SDEV_CREATED_BLOCK;
 }
+
 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)
 {
 	return sdev->sdtr;
 }
+
 static inline int scsi_device_wide(struct scsi_device *sdev)
 {
 	return sdev->wdtr;
 }
+
 static inline int scsi_device_dt(struct scsi_device *sdev)
 {
 	return sdev->ppr;
 }
+
 static inline int scsi_device_dt_only(struct scsi_device *sdev)
 {
 	if (sdev->inquiry_len < 57)
 		return 0;
 	return (sdev->inquiry[56] & 0x0c) == 0x04;
 }
+
 static inline int scsi_device_ius(struct scsi_device *sdev)
 {
 	if (sdev->inquiry_len < 57)
 		return 0;
 	return sdev->inquiry[56] & 0x01;
 }
+
 static inline int scsi_device_qas(struct scsi_device *sdev)
 {
 	if (sdev->inquiry_len < 57)
 		return 0;
 	return sdev->inquiry[56] & 0x02;
 }
+
 static inline int scsi_device_enclosure(struct scsi_device *sdev)
 {
 	return sdev->inquiry ? (sdev->inquiry[6] & (1<<6)) : 1;
-- 
1.7.10.4

Re: [PATCH v7 5/9] Disallow changing the device state via sysfs into "deleted"

[Bart Van Assche]

On 12/07/12 07:55, Hannes Reinecke wrote:
> On 12/06/2012 04:56 PM, Bart Van Assche wrote:
>> Changing the state of a SCSI device via sysfs into "cancel" or
>> "deleted" prevents scsi_remove_host() to remove these devices.
>> Hence do not allow this.
>>
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>> Cc: Tejun Heo <tj@kernel.org>
>> Cc: James Bottomley <JBottomley@Parallels.com>
>> Cc: Mike Christie <michaelc@cs.wisc.edu>
>> Cc: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/scsi/scsi_sysfs.c |    7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
>> index 4348f12..b319c20 100644
>> --- a/drivers/scsi/scsi_sysfs.c
>> +++ b/drivers/scsi/scsi_sysfs.c
>> @@ -591,13 +591,15 @@ sdev_store_delete(struct device *dev, struct
>> device_attribute *attr,
>>   };
>>   static DEVICE_ATTR(delete, S_IWUSR, NULL, sdev_store_delete);
>>
>> +#define INVALID_SDEV_STATE 0
>> +
> Shouldn't this become part of the enum?
> Defining it outside only confuses the compiler.
> And the unsuspecting user.

I can do that, but that will require changes in every switch statement 
on enum scsi_device_state because the kernel code is compiled with 
-Wswitch. From the gcc manual: <quote>-Wswitch: Warn whenever a switch 
statement has an index of enumerated type and lacks a case for one or 
more of the named codes of that enumeration. (The presence of a default 
label prevents this warning.)</quote>

Bart.

Re: [PATCH v7 5/9] Disallow changing the device state via sysfs into "deleted"

[Bart Van Assche]

On 12/07/12 13:46, Bart Van Assche wrote:
> On 12/07/12 07:55, Hannes Reinecke wrote:
>> On 12/06/2012 04:56 PM, Bart Van Assche wrote:
>>> Changing the state of a SCSI device via sysfs into "cancel" or
>>> "deleted" prevents scsi_remove_host() to remove these devices.
>>> Hence do not allow this.
>>>
>>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>>> Cc: Tejun Heo <tj@kernel.org>
>>> Cc: James Bottomley <JBottomley@Parallels.com>
>>> Cc: Mike Christie <michaelc@cs.wisc.edu>
>>> Cc: Hannes Reinecke <hare@suse.de>
>>> ---
>>>   drivers/scsi/scsi_sysfs.c |    7 +++++--
>>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
>>> index 4348f12..b319c20 100644
>>> --- a/drivers/scsi/scsi_sysfs.c
>>> +++ b/drivers/scsi/scsi_sysfs.c
>>> @@ -591,13 +591,15 @@ sdev_store_delete(struct device *dev, struct
>>> device_attribute *attr,
>>>   };
>>>   static DEVICE_ATTR(delete, S_IWUSR, NULL, sdev_store_delete);
>>>
>>> +#define INVALID_SDEV_STATE 0
>>> +
>> Shouldn't this become part of the enum?
>> Defining it outside only confuses the compiler.
>> And the unsuspecting user.
> 
> I can do that, but that will require changes in every switch statement 
> on enum scsi_device_state because the kernel code is compiled with 
> -Wswitch. From the gcc manual: <quote>-Wswitch: Warn whenever a switch 
> statement has an index of enumerated type and lacks a case for one or 
> more of the named codes of that enumeration. (The presence of a default 
> label prevents this warning.)</quote>

(replying to my own e-mail)

Apparently there is only one such switch statement that has to be
updated. The add-on patch below realizes the above proposal:

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index d80714f..253fc30 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2158,6 +2158,8 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
 		}
 		break;
 
+	case INVALID_SDEV_STATE:
+		goto illegal;
 	}
 	sdev->sdev_state = state;
 	return 0;
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 3293ba7..81d9d55 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -592,8 +592,6 @@ sdev_store_delete(struct device *dev, struct device_attribute *attr,
 };
 static DEVICE_ATTR(delete, S_IWUSR, NULL, sdev_store_delete);
 
-#define INVALID_SDEV_STATE 0
-
 static ssize_t
 store_state_field(struct device *dev, struct device_attribute *attr,
 		  const char *buf, size_t count)
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 83a6532..4281ff4 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -29,7 +29,11 @@ struct scsi_mode_data {
  * scsi_lib:scsi_device_set_state().
  */
 enum scsi_device_state {
-	SDEV_CREATED = 1,	/* device created but not added to sysfs
+	INVALID_SDEV_STATE,	/* Not a valid SCSI device state but a
+				 * symbolic name that can be used wherever
+				 * a value is needed that is different of
+				 * any valid SCSI device state. */
+	SDEV_CREATED,		/* device created but not added to sysfs
 				 * Only internal commands allowed (for inq) */
 	SDEV_RUNNING,		/* device properly configured
 				 * All commands allowed */

Re: [PATCH v7 5/9] Disallow changing the device state via sysfs into "deleted"

[Hannes Reinecke]

On 12/07/2012 02:33 PM, Bart Van Assche wrote:
> On 12/07/12 13:46, Bart Van Assche wrote:
>> On 12/07/12 07:55, Hannes Reinecke wrote:
>>> On 12/06/2012 04:56 PM, Bart Van Assche wrote:
>>>> Changing the state of a SCSI device via sysfs into "cancel" or
>>>> "deleted" prevents scsi_remove_host() to remove these devices.
>>>> Hence do not allow this.
>>>>
>>>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>>>> Cc: Tejun Heo <tj@kernel.org>
>>>> Cc: James Bottomley <JBottomley@Parallels.com>
>>>> Cc: Mike Christie <michaelc@cs.wisc.edu>
>>>> Cc: Hannes Reinecke <hare@suse.de>
>>>> ---
>>>>    drivers/scsi/scsi_sysfs.c |    7 +++++--
>>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
>>>> index 4348f12..b319c20 100644
>>>> --- a/drivers/scsi/scsi_sysfs.c
>>>> +++ b/drivers/scsi/scsi_sysfs.c
>>>> @@ -591,13 +591,15 @@ sdev_store_delete(struct device *dev, struct
>>>> device_attribute *attr,
>>>>    };
>>>>    static DEVICE_ATTR(delete, S_IWUSR, NULL, sdev_store_delete);
>>>>
>>>> +#define INVALID_SDEV_STATE 0
>>>> +
>>> Shouldn't this become part of the enum?
>>> Defining it outside only confuses the compiler.
>>> And the unsuspecting user.
>>
>> I can do that, but that will require changes in every switch statement
>> on enum scsi_device_state because the kernel code is compiled with
>> -Wswitch. From the gcc manual: <quote>-Wswitch: Warn whenever a switch
>> statement has an index of enumerated type and lacks a case for one or
>> more of the named codes of that enumeration. (The presence of a default
>> label prevents this warning.)</quote>
>
> (replying to my own e-mail)
>
> Apparently there is only one such switch statement that has to be
> updated. The add-on patch below realizes the above proposal:
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index d80714f..253fc30 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2158,6 +2158,8 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
>   		}
>   		break;
>
> +	case INVALID_SDEV_STATE:
> +		goto illegal;
>   	}
>   	sdev->sdev_state = state;
>   	return 0;
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index 3293ba7..81d9d55 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -592,8 +592,6 @@ sdev_store_delete(struct device *dev, struct device_attribute *attr,
>   };
>   static DEVICE_ATTR(delete, S_IWUSR, NULL, sdev_store_delete);
>
> -#define INVALID_SDEV_STATE 0
> -
>   static ssize_t
>   store_state_field(struct device *dev, struct device_attribute *attr,
>   		  const char *buf, size_t count)
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 83a6532..4281ff4 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -29,7 +29,11 @@ struct scsi_mode_data {
>    * scsi_lib:scsi_device_set_state().
>    */
>   enum scsi_device_state {
> -	SDEV_CREATED = 1,	/* device created but not added to sysfs
> +	INVALID_SDEV_STATE,	/* Not a valid SCSI device state but a
> +				 * symbolic name that can be used wherever
> +				 * a value is needed that is different of
> +				 * any valid SCSI device state. */
> +	SDEV_CREATED,		/* device created but not added to sysfs
>   				 * Only internal commands allowed (for inq) */
>   	SDEV_RUNNING,		/* device properly configured
>   				 * All commands allowed */
>

Much better.
enum checking was the main intention for this, after all :-)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 3/9] Introduce scsi_device_being_removed()

[Rolf Eike Beer]

Am , schrieb Bart Van Assche:
> On 12/07/12 11:02, Rolf Eike Beer wrote:
>> Am , schrieb Bart Van Assche:
>>> On 12/07/12 09:40, Rolf Eike Beer wrote:
>>>>> diff --git a/include/scsi/scsi_device.h 
>>>>> b/include/scsi/scsi_device.h
>>>>> index 55367b0..767dd16 100644
>>>>> --- a/include/scsi/scsi_device.h
>>>>> +++ b/include/scsi/scsi_device.h
>>>>> @@ -442,6 +442,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)
>>>>
>>>> Newline missing before the new function.
>>>
>>> Hello Rolf,
>>>
>>> Well spotted. Did you realize I had left out the newline on 
>>> purpose,
>>> to preserve consistency with the rest of the header file ?
>>
>> Well, there is already a newline between scsi_device_created() and
>> scsi_device_sync() where you insert your stuff so this sort of
>> consistency is hard to spot.
>
> Hello Eike,
>
> So you really care about these blank lines ? How about the patch 
> below ?

Well, that was not the intent of the message. I just saw that you 
introduced something that looked like an error. You explained why you 
did it, I explained why I did not notice that before.

> It is identical to the patch at the start of this thread except that 
> it
> adds a blank line between each pair of functions in 
> <scsi/scsi_device.h>
> where there was not yet a blank line.

Since you already did the patch I think we should take this one ;)

Eike

Re: [PATCH v7 4/9] Remove offline devices when removing a host

[Hannes Reinecke]

On 12/06/2012 04:55 PM, Bart Van Assche wrote:
> Currently __scsi_remove_device() skips devices that are visible and
> offline. Make sure that these devices get removed by changing their
> device state into SDEV_DEL at the start of __scsi_remove_device().
> Also, avoid that __scsi_remove_device() gets called a second time
> for devices that are in state SDEV_CANCEL when scsi_forget_host()
> is invoked.
>
> 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: Tejun Heo <tj@kernel.org>
> ---
>   drivers/scsi/scsi_scan.c  |    2 +-
>   drivers/scsi/scsi_sysfs.c |    4 ++--
>   2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 3e58b22..0612fba 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -1889,7 +1889,7 @@ void scsi_forget_host(struct Scsi_Host *shost)
>    restart:
>   	spin_lock_irqsave(shost->host_lock, flags);
>   	list_for_each_entry(sdev, &shost->__devices, siblings) {
> -		if (sdev->sdev_state == SDEV_DEL)
> +		if (scsi_device_being_removed(sdev))
>   			continue;
>   		spin_unlock_irqrestore(shost->host_lock, flags);
>   		__scsi_remove_device(sdev);
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index 2ff7ba5..4348f12 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -959,8 +959,8 @@ void __scsi_remove_device(struct scsi_device *sdev)
>   	unsigned long flags;
>
>   	if (sdev->is_visible) {
> -		if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
> -			return;
> +		WARN_ON_ONCE(scsi_device_set_state(sdev, SDEV_CANCEL) != 0 &&
> +			     scsi_device_set_state(sdev, SDEV_DEL) != 0);
>
>   		bsg_unregister_queue(sdev->request_queue);
>   		device_unregister(&sdev->sdev_dev);
>
Hmm. Then we would be getting a warning if the device is already in 
SDEV_DEL, wouldn't we?
And what about offlined devices?
We should be safe to remove them, or?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 4/9] Remove offline devices when removing a host

[Bart Van Assche]

On 12/07/12 16:10, Hannes Reinecke wrote:
> On 12/06/2012 04:55 PM, Bart Van Assche wrote:
>> Currently __scsi_remove_device() skips devices that are visible and
>> offline. Make sure that these devices get removed by changing their
>> device state into SDEV_DEL at the start of __scsi_remove_device().
>> Also, avoid that __scsi_remove_device() gets called a second time
>> for devices that are in state SDEV_CANCEL when scsi_forget_host()
>> is invoked.
>>
>> 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: Tejun Heo <tj@kernel.org>
>> ---
>>   drivers/scsi/scsi_scan.c  |    2 +-
>>   drivers/scsi/scsi_sysfs.c |    4 ++--
>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
>> index 3e58b22..0612fba 100644
>> --- a/drivers/scsi/scsi_scan.c
>> +++ b/drivers/scsi/scsi_scan.c
>> @@ -1889,7 +1889,7 @@ void scsi_forget_host(struct Scsi_Host *shost)
>>    restart:
>>       spin_lock_irqsave(shost->host_lock, flags);
>>       list_for_each_entry(sdev, &shost->__devices, siblings) {
>> -        if (sdev->sdev_state == SDEV_DEL)
>> +        if (scsi_device_being_removed(sdev))
>>               continue;
>>           spin_unlock_irqrestore(shost->host_lock, flags);
>>           __scsi_remove_device(sdev);
>> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
>> index 2ff7ba5..4348f12 100644
>> --- a/drivers/scsi/scsi_sysfs.c
>> +++ b/drivers/scsi/scsi_sysfs.c
>> @@ -959,8 +959,8 @@ void __scsi_remove_device(struct scsi_device *sdev)
>>       unsigned long flags;
>>
>>       if (sdev->is_visible) {
>> -        if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
>> -            return;
>> +        WARN_ON_ONCE(scsi_device_set_state(sdev, SDEV_CANCEL) != 0 &&
>> +                 scsi_device_set_state(sdev, SDEV_DEL) != 0);
>>
>>           bsg_unregister_queue(sdev->request_queue);
>>           device_unregister(&sdev->sdev_dev);
>>
> Hmm. Then we would be getting a warning if the device is already in
> SDEV_DEL, wouldn't we?
> And what about offlined devices?
> We should be safe to remove them, or?

Hello Hannes,

The intent of this patch is that __scsi_remove_device() gets invoked 
exactly once per device. This function shouldn't be invoked for devices 
already in state SDEV_DEL.

Offlined devices will be transitioned directly from one of the two 
offline states into state SDEV_DEL.

The above patch fixes a nasty crash by avoiding that a second 
__scsi_remove_device() call queues I/O (sd_shutdown()) after 
scsi_remove_host() has already finished.

Bart.

Re: [PATCH v7 4/9] Remove offline devices when removing a host

[Bart Van Assche]

On 12/07/12 16:33, Bart Van Assche wrote:
> On 12/07/12 16:10, Hannes Reinecke wrote:
>> On 12/06/2012 04:55 PM, Bart Van Assche wrote:
>>> Currently __scsi_remove_device() skips devices that are visible and
>>> offline. Make sure that these devices get removed by changing their
>>> device state into SDEV_DEL at the start of __scsi_remove_device().
>>> Also, avoid that __scsi_remove_device() gets called a second time
>>> for devices that are in state SDEV_CANCEL when scsi_forget_host()
>>> is invoked.
>>>
>>> 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: Tejun Heo <tj@kernel.org>
>>> ---
>>>   drivers/scsi/scsi_scan.c  |    2 +-
>>>   drivers/scsi/scsi_sysfs.c |    4 ++--
>>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
>>> index 3e58b22..0612fba 100644
>>> --- a/drivers/scsi/scsi_scan.c
>>> +++ b/drivers/scsi/scsi_scan.c
>>> @@ -1889,7 +1889,7 @@ void scsi_forget_host(struct Scsi_Host *shost)
>>>    restart:
>>>       spin_lock_irqsave(shost->host_lock, flags);
>>>       list_for_each_entry(sdev, &shost->__devices, siblings) {
>>> -        if (sdev->sdev_state == SDEV_DEL)
>>> +        if (scsi_device_being_removed(sdev))
>>>               continue;
>>>           spin_unlock_irqrestore(shost->host_lock, flags);
>>>           __scsi_remove_device(sdev);
>>> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
>>> index 2ff7ba5..4348f12 100644
>>> --- a/drivers/scsi/scsi_sysfs.c
>>> +++ b/drivers/scsi/scsi_sysfs.c
>>> @@ -959,8 +959,8 @@ void __scsi_remove_device(struct scsi_device *sdev)
>>>       unsigned long flags;
>>>
>>>       if (sdev->is_visible) {
>>> -        if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
>>> -            return;
>>> +        WARN_ON_ONCE(scsi_device_set_state(sdev, SDEV_CANCEL) != 0 &&
>>> +                 scsi_device_set_state(sdev, SDEV_DEL) != 0);
>>>
>>>           bsg_unregister_queue(sdev->request_queue);
>>>           device_unregister(&sdev->sdev_dev);
>>>
>> Hmm. Then we would be getting a warning if the device is already in
>> SDEV_DEL, wouldn't we?
>> And what about offlined devices?
>> We should be safe to remove them, or?
>
> Hello Hannes,
>
> The intent of this patch is that __scsi_remove_device() gets invoked
> exactly once per device. This function shouldn't be invoked for devices
> already in state SDEV_DEL.
>
> Offlined devices will be transitioned directly from one of the two
> offline states into state SDEV_DEL.
>
> The above patch fixes a nasty crash by avoiding that a second
> __scsi_remove_device() call queues I/O (sd_shutdown()) after
> scsi_remove_host() has already finished.

(replying to my own e-mail)

Please ignore the above comment about sd_shutdown() - that didn't make 
sense. What I would like to add to the above is that it's only after I 
included the above patch in my tests that the following two call stacks 
could no longer be triggered:

BUG: spinlock bad magic on CPU#0, kworker/0:1H/178
  lock: 0xffff880177880c28, .magic: ffff8801, .owner: <none>/-1, 
.owner_cpu: 2006506176
Pid: 178, comm: kworker/0:1H Tainted: G        W  O 3.7.0-rc7-debug+ #2
Call Trace:
  [<ffffffff814120ef>] spin_dump+0x8c/0x91
  [<ffffffff81412115>] spin_bug+0x21/0x26
  [<ffffffff81218aef>] do_raw_spin_lock+0x13f/0x150
  [<ffffffff81417bb8>] _raw_spin_lock_irqsave+0x78/0xa0
  [<ffffffffa0766c6c>] srp_queuecommand+0x3c/0xc80 [ib_srp]
  [<ffffffffa0002f18>] scsi_dispatch_cmd+0x148/0x310 [scsi_mod]
  [<ffffffffa000a390>] scsi_request_fn+0x320/0x520 [scsi_mod]
  [<ffffffff811ec427>] __blk_run_queue+0x37/0x50
  [<ffffffff811ec539>] blk_delay_work+0x29/0x40
  [<ffffffff81059283>] process_one_work+0x1c3/0x5c0
  [<ffffffff8105b1be>] worker_thread+0x15e/0x440
  [<ffffffff8106137b>] kthread+0xdb/0xe0
  [<ffffffff81420d5c>] ret_from_fork+0x7c/0xb0
------------[ cut here ]------------

BUG: spinlock bad magic on CPU#1, udevd/1518
  lock: 0xffff8801a2384c28, .magic: ffff8801, .owner: <none>/-1, 
.owner_cpu: -1519491200
Pid: 1518, comm: udevd Not tainted 3.7.0-rc8-debug+ #2
Call Trace:
  [<ffffffff81411a9d>] spin_dump+0x8c/0x91
  [<ffffffff81411ac3>] spin_bug+0x21/0x26
  [<ffffffff812184ff>] do_raw_spin_lock+0x13f/0x150
  [<ffffffff81417568>] _raw_spin_lock_irqsave+0x78/0xa0
  [<ffffffffa04a0d1c>] srp_queuecommand+0x3c/0xc80 [ib_srp]
  [<ffffffffa0002f18>] scsi_dispatch_cmd+0x148/0x310 [scsi_mod]
  [<ffffffffa000a6cc>] scsi_request_fn+0x46c/0x570 [scsi_mod]
  [<ffffffff811ebe26>] __blk_run_queue+0x46/0x60
  [<ffffffff811ebe7e>] queue_unplugged+0x3e/0xd0
  [<ffffffff811ee9c3>] blk_flush_plug_list+0x1c3/0x240
  [<ffffffff811eea58>] blk_finish_plug+0x18/0x50
  [<ffffffff8110511c>] __do_page_cache_readahead+0x24c/0x2e0
  [<ffffffff811052e9>] force_page_cache_readahead+0x79/0xb0
  [<ffffffff8110573b>] page_cache_sync_readahead+0x4b/0x50
  [<ffffffff810fad30>] generic_file_aio_read+0x590/0x710
  [<ffffffff8114b127>] do_sync_read+0xa7/0xe0
  [<ffffffff8114b878>] vfs_read+0xa8/0x170
  [<ffffffff8114b995>] sys_read+0x55/0xa0
  [<ffffffff81420782>] system_call_fastpath+0x16/0x1b
------------[ cut here ]------------

Bart.

Re: [PATCH v7 1/9] Fix race between starved list processing and device removal

[Bart Van Assche]

Hello James,

Please consider this patch for inclusion in kernel 3.9.

Thanks,

Bart.

On 01/09/13 02:25, Chanho Min wrote:
> Is there any progress in this patch?
> Oops is still occurred from a torn down device due to this cause.
> We look forward to this patch is applied to mainline ASAP.
>
> Thanks
> Chanho Min
>
>> -----Original Message-----
>> From: Bart Van Assche [mailto:bvanassche@acm.org]
>> Sent: Friday, December 07, 2012 12:53 AM
>> To: undisclosed-recipients:
>> Cc: linux-scsi; James Bottomley; Mike Christie; Tejun Heo; Chanho Min; Jens Axboe
>> Subject: [PATCH v7 1/9] Fix race between starved list processing and device removal
>>
>> Avoid that the sdev reference count can drop to zero before
>> a queue is run by scsi_run_queue().
>>
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>> Reported-and-tested-by: Chanho Min <chanho.min@lge.com>
>> Reference: http://lkml.org/lkml/2012/8/2/96
>> Acked-by: Tejun Heo <tj@kernel.org>
>> Reviewed-by: Mike Christie <michaelc@cs.wisc.edu>
>> Cc: Jens Axboe <axboe@kernel.dk>
>> Cc: <stable@vger.kernel.org>
>> ---
>> drivers/scsi/scsi_lib.c   |   16 +++++++++++-----
>> drivers/scsi/scsi_sysfs.c |   14 +++++++++++++-
>> 2 files changed, 24 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index f1bf5af..5c67339 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -452,11 +452,17 @@ 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);
>> +		/*
>> +		 * Obtain a reference before unlocking the host_lock such that
>> +		 * the sdev can't disappear before blk_run_queue() is invoked.
>> +		 */
>> +		get_device(&sdev->sdev_gendev);
>> +		spin_unlock_irqrestore(shost->host_lock, flags);
>> +
>> +		blk_run_queue(sdev->request_queue);
>> +		put_device(&sdev->sdev_gendev);
>> +
>> +		spin_lock_irqsave(shost->host_lock, flags);
>> 	}
>> 	/* put any unprocessed entries back */
>> 	list_splice(&starved_list, &shost->starved_list);
>> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
>> index ce5224c..2ff7ba5 100644
>> --- a/drivers/scsi/scsi_sysfs.c
>> +++ b/drivers/scsi/scsi_sysfs.c
>> @@ -348,7 +348,6 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
>> 	starget->reap_ref++;
>> 	list_del(&sdev->siblings);
>> 	list_del(&sdev->same_target_siblings);
>> -	list_del(&sdev->starved_entry);
>> 	spin_unlock_irqrestore(sdev->host->host_lock, flags);
>>
>> 	cancel_work_sync(&sdev->event_work);
>> @@ -956,6 +955,8 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
>> void __scsi_remove_device(struct scsi_device *sdev)
>> {
>> 	struct device *dev = &sdev->sdev_gendev;
>> +	struct Scsi_Host *shost = sdev->host;
>> +	unsigned long flags;
>>
>> 	if (sdev->is_visible) {
>> 		if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
>> @@ -977,6 +978,17 @@ void __scsi_remove_device(struct scsi_device *sdev)
>> 	blk_cleanup_queue(sdev->request_queue);
>> 	cancel_work_sync(&sdev->requeue_work);
>>
>> +	/*
>> +	 * Remove a SCSI device from the starved list after blk_cleanup_queue()
>> +	 * finished such that scsi_request_fn() can't add it back to that list.
>> +	 * Also remove an sdev from the starved list before invoking
>> +	 * put_device() such that get_device() is guaranteed to succeed for an
>> +	 * sdev present on the starved list.
>> +	 */
>> +	spin_lock_irqsave(shost->host_lock, flags);
>> +	list_del(&sdev->starved_entry);
>> +	spin_unlock_irqrestore(shost->host_lock, flags);
>> +
>> 	if (sdev->host->hostt->slave_destroy)
>> 		sdev->host->hostt->slave_destroy(sdev);
>> 	transport_destroy_device(dev);
>> --
>> 1.7.10.4
>
>