Skip to contentSkip to conversation

More device removal fixes

PATCH Superseded Bart Van Assche 3 participants 27 messages Dec 6, 2012 – Feb 9, 2013 1 message not in the archive SCSI

v7 of 14 Superseded Latest is v14 →

lore
Share & tools
lore

Bart Van Assche

author
#0 lore raw

v7 cover More device removal fixes

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.

author
#1 lore raw

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 <[email protected]>
Reported-and-tested-by Chanho Min <[email protected]>

Reference: http://lkml.org/lkml/2012/8/2/96

Acked-by Tejun Heo <[email protected]>
Reviewed-by Mike Christie <[email protected]>
Cc Jens Axboe <[email protected]>
Cc <[email protected]>
---
 drivers/scsi/scsi_lib.c   |   16 +++++++++++-----
 drivers/scsi/scsi_sysfs.c |   14 +++++++++++++-
 2 files changed, 24 insertions(+), 6 deletions(-)
drivers/scsi/scsi_lib.c +11 -5
index f1bf5af..5c67339 100644
@@ -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);
drivers/scsi/scsi_sysfs.c +13 -1
index ce5224c..2ff7ba5 100644
@@ -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);
Signature · 1 line
1.7.10.4

A message that is not in the archive (sent off-list or not yet received): 034101cdee08$2d67f870$8837e950$@[email protected]

author
#3 lore raw

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

Hello James,

Please consider this patch for inclusion in kernel 3.9.

Thanks,

Bart.

Quoted 99 lines · Chanho Min

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

Quoted 90 lines

-----Original Message-----
From: Bart Van Assche [mailto:[email protected]]
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 <[email protected]>
Reported-and-tested-by Chanho Min <[email protected]>

Reference: http://lkml.org/lkml/2012/8/2/96

Acked-by Tejun Heo <[email protected]>
Reviewed-by Mike Christie <[email protected]>
Cc Jens Axboe <[email protected]>
Cc <[email protected]>

---

drivers/scsi/scsi_lib.c   |   16 +++++++++++-----
drivers/scsi/scsi_sysfs.c |   14 +++++++++++++-
2 files changed, 24 insertions(+), 6 deletions(-)
drivers/scsi/scsi_lib.c +11 -5
index f1bf5af..5c67339 100644
@@ -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);
drivers/scsi/scsi_sysfs.c +13 -2
index ce5224c..2ff7ba5 100644
@@ -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

author
#4 lore raw

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

In reply to 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 <[email protected]>
Acked-by Tejun Heo <[email protected]>
Cc James Bottomley <[email protected]>
Cc Mike Christie <[email protected]>
Cc Jens Axboe <[email protected]>
---
 drivers/scsi/scsi_lib.c |   14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)
drivers/scsi/scsi_lib.c +3 -11
index 5c67339..db4836bc 100644
@@ -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)
Signature · 1 line
1.7.10.4
author
#5 lore raw

PATCH v7 3/9 Introduce scsi_device_being_removed()

In reply to Bart Van Assche
Signed-off-by Bart Van Assche <[email protected]>
Cc Hannes Reinecke <[email protected]>
Cc James Bottomley <[email protected]>
Cc Mike Christie <[email protected]>
Cc Tejun Heo <[email protected]>
---
 drivers/scsi/device_handler/scsi_dh.c |    7 ++-----
 include/scsi/scsi_device.h            |    5 +++++
 2 files changed, 7 insertions(+), 5 deletions(-)
drivers/scsi/device_handler/scsi_dh.c +2 -5
index 33e422e..78b3ddb 100644
@@ -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;
include/scsi/scsi_device.h +5 -0
index 55367b0..767dd16 100644
@@ -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)
Signature · 1 line
1.7.10.4
Acked-by on v7 3/9
#6 lore raw

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

Quoted 5 lines · Bart Van Assche
Signed-off-by Bart Van Assche <[email protected]>
Cc Hannes Reinecke <[email protected]>
Cc James Bottomley <[email protected]>
Cc Mike Christie <[email protected]>
Cc Tejun Heo <[email protected]>
Acked-by Hannes Reinecke <[email protected]>

Cheers,

Hannes

Signature · 8 lines
Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#7 lore raw

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

In reply to Bart Van Assche
Quoted 16 lines
include/scsi/scsi_device.h +5 -0
index 55367b0..767dd16 100644
@@ -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;
+}
Quoted 2 lines

/* accessor functions for the SCSI parameters */ static inline int scsi_device_sync(struct scsi_device *sdev)

Newline missing before the new function.

Greetings,

Eike

author
#8 lore raw

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

Quoted 15 lines · Rolf Eike Beer
Quoted 15 lines
include/scsi/scsi_device.h +5 -0
index 55367b0..767dd16 100644
@@ -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;
+}
Quoted 4 lines
Quoted 2 lines
include/scsi/scsi_device.h +0 -0
 /* 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.

#9 lore raw

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

Am , schrieb Bart Van Assche:

Quoted 22 lines · Rolf Eike Beer
Quoted 16 lines
Quoted 16 lines
include/scsi/scsi_device.h +5 -0
index 55367b0..767dd16 100644
@@ -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;
+}
Quoted 4 lines
Quoted 2 lines

/* accessor functions for the SCSI parameters */ static inline int scsi_device_sync(struct scsi_device *sdev)

Newline missing before the new function.

Quoted 4 lines

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

author
#10 lore raw

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

Quoted 22 lines · Rolf Eike Beer

Am , schrieb Bart Van Assche:

Quoted 21 lines · Rolf Eike Beer
Quoted 19 lines
Quoted 16 lines
include/scsi/scsi_device.h +5 -0
index 55367b0..767dd16 100644
@@ -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 */
Quoted 1 line

static inline int scsi_device_sync(struct scsi_device *sdev)

Newline missing before the new function.

Quoted 8 lines
Quoted 4 lines

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(-)
drivers/scsi/device_handler/scsi_dh.c +2 -5
index 33e422e..78b3ddb 100644
@@ -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;
include/scsi/scsi_device.h +15 -0
index 55367b0..b9b19ca 100644
@@ -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;
Signature · 1 line
1.7.10.4
#11 lore raw

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

Am , schrieb Bart Van Assche:

Quoted 29 lines · Rolf Eike Beer
Quoted 28 lines

Am , schrieb Bart Van Assche:

Quoted 26 lines · Rolf Eike Beer
Quoted 16 lines
Quoted 16 lines
include/scsi/scsi_device.h +5 -0
index 55367b0..767dd16 100644
@@ -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;
+}
Quoted 4 lines
Quoted 2 lines

/* 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 ?

Quoted 7 lines
Quoted 3 lines

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.

Quoted 3 lines

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

author
#12 lore raw

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

In reply to 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 <[email protected]>
Cc James Bottomley <[email protected]>
Cc Mike Christie <[email protected]>
Cc Hannes Reinecke <[email protected]>
Cc Tejun Heo <[email protected]>
---
 drivers/scsi/scsi_scan.c  |    2 +-
 drivers/scsi/scsi_sysfs.c |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)
drivers/scsi/scsi_scan.c +1 -1
index 3e58b22..0612fba 100644
@@ -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);
drivers/scsi/scsi_sysfs.c +2 -2
index 2ff7ba5..4348f12 100644
@@ -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);
Signature · 1 line
1.7.10.4
#13 lore raw

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

Quoted 36 lines · 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 <[email protected]>
Cc James Bottomley <[email protected]>
Cc Mike Christie <[email protected]>
Cc Hannes Reinecke <[email protected]>
Cc Tejun Heo <[email protected]>
---
  drivers/scsi/scsi_scan.c  |    2 +-
  drivers/scsi/scsi_sysfs.c |    4 ++--
  2 files changed, 3 insertions(+), 3 deletions(-)
drivers/scsi/scsi_scan.c +1 -1
index 3e58b22..0612fba 100644
@@ -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);
drivers/scsi/scsi_sysfs.c +0 -0
index 2ff7ba5..4348f12 100644

drivers/scsi/scsi_sysfs.c void __scsi_remove_device(struct scsi_device *sdev) · line 966

Quoted 10 lines
drivers/scsi/scsi_sysfs.c +2 -2
@@ -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

Signature · 8 lines
Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#14 lore raw

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

Quoted 44 lines · Hannes Reinecke
Quoted 43 lines · 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 <[email protected]>
Cc James Bottomley <[email protected]>
Cc Mike Christie <[email protected]>
Cc Hannes Reinecke <[email protected]>
Cc Tejun Heo <[email protected]>
---
  drivers/scsi/scsi_scan.c  |    2 +-
  drivers/scsi/scsi_sysfs.c |    4 ++--
  2 files changed, 3 insertions(+), 3 deletions(-)
drivers/scsi/scsi_scan.c +1 -1
index 3e58b22..0612fba 100644
@@ -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);
drivers/scsi/scsi_sysfs.c +2 -2
index 2ff7ba5..4348f12 100644
@@ -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);
Quoted 7 lines

drivers/scsi/scsi_sysfs.c void __scsi_remove_device(struct scsi_device *sdev) · line 966

Quoted 3 lines
drivers/scsi/scsi_sysfs.c +0 -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.

author
#15 lore raw

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

Quoted 59 lines · Bart Van Assche
Quoted 44 lines · Hannes Reinecke
Quoted 43 lines · 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 <[email protected]>
Cc James Bottomley <[email protected]>
Cc Mike Christie <[email protected]>
Cc Hannes Reinecke <[email protected]>
Cc Tejun Heo <[email protected]>
---
  drivers/scsi/scsi_scan.c  |    2 +-
  drivers/scsi/scsi_sysfs.c |    4 ++--
  2 files changed, 3 insertions(+), 3 deletions(-)
drivers/scsi/scsi_scan.c +1 -1
index 3e58b22..0612fba 100644
@@ -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);
drivers/scsi/scsi_sysfs.c +2 -2
index 2ff7ba5..4348f12 100644
@@ -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);
Quoted 7 lines

drivers/scsi/scsi_sysfs.c void __scsi_remove_device(struct scsi_device *sdev) · line 966

Quoted 3 lines
drivers/scsi/scsi_sysfs.c +0 -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.

Quoted 6 lines

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.

author
#16 lore raw

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

In reply to 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 <[email protected]>
Cc Tejun Heo <[email protected]>
Cc James Bottomley <[email protected]>
Cc Mike Christie <[email protected]>
Cc Hannes Reinecke <[email protected]>
---
 drivers/scsi/scsi_sysfs.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/scsi/scsi_sysfs.c +5 -2
index 4348f12..b319c20 100644
@@ -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))
Signature · 1 line
1.7.10.4
#17 lore raw

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

Quoted 18 lines · 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 <[email protected]>
Cc Tejun Heo <[email protected]>
Cc James Bottomley <[email protected]>
Cc Mike Christie <[email protected]>
Cc Hannes Reinecke <[email protected]>
---
  drivers/scsi/scsi_sysfs.c |    7 +++++--
  1 file changed, 5 insertions(+), 2 deletions(-)
drivers/scsi/scsi_sysfs.c +0 -0
index 4348f12..b319c20 100644

drivers/scsi/scsi_sysfs.c sdev_store_delete(struct device *dev, struct device_attribute *attr, · line 595

Quoted 5 lines
drivers/scsi/scsi_sysfs.c +2 -0
@@ -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.

Quoted 13 lines
drivers/scsi/scsi_sysfs.c +1 -1
  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);

drivers/scsi/scsi_sysfs.c store_state_field(struct device *dev, struct device_attribute *attr, · line 616

Quoted 9 lines
drivers/scsi/scsi_sysfs.c +2 -1
@@ -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

Signature · 8 lines
Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#18 lore raw

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

Quoted 23 lines · Hannes Reinecke
Quoted 22 lines · 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 <[email protected]>
Cc Tejun Heo <[email protected]>
Cc James Bottomley <[email protected]>
Cc Mike Christie <[email protected]>
Cc Hannes Reinecke <[email protected]>
---
  drivers/scsi/scsi_sysfs.c |    7 +++++--
  1 file changed, 5 insertions(+), 2 deletions(-)
drivers/scsi/scsi_sysfs.c +0 -0
index 4348f12..b319c20 100644
@@ -591,13 +591,15 @@ sdev_store_delete(struct device *dev, struct
 device_attribute *attr,
  };
  static DEVICE_ATTR(delete, S_IWUSR, NULL, sdev_store_delete);
Quoted 5 lines

drivers/scsi/scsi_sysfs.c sdev_store_delete(struct device *dev, struct device_attribute *attr, · line 595

Quoted 2 lines
drivers/scsi/scsi_sysfs.c +2 -0
+#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.

author
#19 lore raw

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

Quoted 30 lines · Bart Van Assche
Quoted 23 lines · Hannes Reinecke
Quoted 22 lines · 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 <[email protected]>
Cc Tejun Heo <[email protected]>
Cc James Bottomley <[email protected]>
Cc Mike Christie <[email protected]>
Cc Hannes Reinecke <[email protected]>
---
  drivers/scsi/scsi_sysfs.c |    7 +++++--
  1 file changed, 5 insertions(+), 2 deletions(-)
drivers/scsi/scsi_sysfs.c +0 -0
index 4348f12..b319c20 100644
@@ -591,13 +591,15 @@ sdev_store_delete(struct device *dev, struct
 device_attribute *attr,
  };
  static DEVICE_ATTR(delete, S_IWUSR, NULL, sdev_store_delete);
Quoted 5 lines

drivers/scsi/scsi_sysfs.c sdev_store_delete(struct device *dev, struct · line 596

Quoted 2 lines
drivers/scsi/scsi_sysfs.c +2 -0
+#define INVALID_SDEV_STATE 0
+

Shouldn't this become part of the enum?
Defining it outside only confuses the compiler.
And the unsuspecting user.

Quoted 6 lines

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:

drivers/scsi/scsi_lib.c +2 -0
index d80714f..253fc30 100644
@@ -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;
drivers/scsi/scsi_sysfs.c +0 -2
index 3293ba7..81d9d55 100644
@@ -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)
include/scsi/scsi_device.h +5 -1
index 83a6532..4281ff4 100644
@@ -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 */
#20 lore raw

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

Quoted 77 lines · Bart Van Assche
Quoted 30 lines · Bart Van Assche
Quoted 23 lines · Hannes Reinecke
Quoted 22 lines · 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 <[email protected]>
Cc Tejun Heo <[email protected]>
Cc James Bottomley <[email protected]>
Cc Mike Christie <[email protected]>
Cc Hannes Reinecke <[email protected]>
---
   drivers/scsi/scsi_sysfs.c |    7 +++++--
   1 file changed, 5 insertions(+), 2 deletions(-)
drivers/scsi/scsi_sysfs.c +0 -0
index 4348f12..b319c20 100644
@@ -591,13 +591,15 @@ sdev_store_delete(struct device *dev, struct
 device_attribute *attr,
   };
   static DEVICE_ATTR(delete, S_IWUSR, NULL, sdev_store_delete);
Quoted 5 lines

drivers/scsi/scsi_sysfs.c sdev_store_delete(struct device *dev, struct · line 596

Quoted 2 lines
drivers/scsi/scsi_sysfs.c +2 -0
+#define INVALID_SDEV_STATE 0
+

Shouldn't this become part of the enum?
Defining it outside only confuses the compiler.
And the unsuspecting user.

Quoted 6 lines

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:

drivers/scsi/scsi_lib.c +2 -0
index d80714f..253fc30 100644
@@ -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;
drivers/scsi/scsi_sysfs.c +0 -2
index 3293ba7..81d9d55 100644
@@ -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)
include/scsi/scsi_device.h +0 -0
index 83a6532..4281ff4 100644
@@ -29,7 +29,11 @@ struct scsi_mode_data {
   * scsi_lib:scsi_device_set_state().
   */

include/scsi/scsi_device.h struct scsi_mode_data { · line 39

Quoted 10 lines
include/scsi/scsi_device.h +5 -1
  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

Signature · 8 lines
Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#21 lore raw

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

In reply to 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 <[email protected]>
Acked-by Tejun Heo <[email protected]>
Cc Mike Christie <[email protected]>
Cc Hannes Reinecke <[email protected]>
Cc James Bottomley <[email protected]>
---
 drivers/scsi/hosts.c |   12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)
drivers/scsi/hosts.c +5 -7
index 593085a..6ae16cd 100644
@@ -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);
Signature · 1 line
1.7.10.4
Acked-by on v7 6/9
#22 lore raw

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

Quoted 10 lines · 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 <[email protected]>
Acked-by Tejun Heo <[email protected]>
Cc Mike Christie <[email protected]>
Cc Hannes Reinecke <[email protected]>
Cc James Bottomley <[email protected]>
Acked-by Hannes Reinecke <[email protected]>

Cheers,

Hannes

Signature · 8 lines
Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#23 lore raw

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

In reply to 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 <[email protected]>
Cc Tejun Heo <[email protected]>
Cc James Bottomley <[email protected]>
Cc Mike Christie <[email protected]>
Cc Hannes Reinecke <[email protected]>
---
 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(+)
drivers/scsi/hosts.c +30 -0
index 6ae16cd..b68a013 100644
@@ -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);
drivers/scsi/scsi_priv.h +1 -0
index 8f9a0ca..882c823 100644
@@ -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);
drivers/scsi/scsi_sysfs.c +1 -0
index b319c20..6368576 100644
@@ -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);
include/scsi/scsi_host.h +1 -0
index 4908480..1b7fd89 100644
@@ -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;
Signature · 1 line
1.7.10.4
author
#24 lore raw

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

In reply to 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 <[email protected]>
Cc Hannes Reinecke <[email protected]>
Cc Mike Christie <[email protected]>
Cc Tejun Heo <[email protected]>
---
 drivers/scsi/hosts.c      |    2 +-
 drivers/scsi/scsi_error.c |   76 +++++++++++++++++++++++++++++++++++++++++++--
 include/scsi/scsi_host.h  |    1 +
 3 files changed, 76 insertions(+), 3 deletions(-)
drivers/scsi/hosts.c +1 -1
index b68a013..a941861 100644
@@ -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. */
drivers/scsi/scsi_error.c +74 -2

138 lines not on this page · raw message

include/scsi/scsi_host.h +1 -0
index 1b7fd89..5e2fcd2 100644
@@ -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;
Signature · 1 line
1.7.10.4
Acked-by on v7 8/9
#25 lore raw

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

Quoted 8 lines · 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.

Quoted 4 lines
Signed-off-by Bart Van Assche <[email protected]>
Cc Hannes Reinecke <[email protected]>
Cc Mike Christie <[email protected]>
Cc Tejun Heo <[email protected]>
Acked-by Hannes Reinecke <[email protected]>

Cheers,

Hannes

Signature · 8 lines
Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#26 lore raw

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

In reply to Bart Van Assche

Make concurrent invocations of scsi_device_set_state() safe.

Signed-off-by Bart Van Assche <[email protected]>
Cc James Bottomley <[email protected]>
Cc Tejun Heo <[email protected]>
Cc Hannes Reinecke <[email protected]>
Cc Mike Christie <[email protected]>
---
 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(-)
drivers/scsi/scsi_error.c +4 -0
index 76761aa..dbc1fb86 100644
@@ -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.
drivers/scsi/scsi_lib.c +34 -9

98 lines not on this page · raw message

drivers/scsi/scsi_scan.c +8 -7
index 0612fba..3fbdd7b 100644
@@ -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)
drivers/scsi/scsi_sysfs.c +16 -3
index 6368576..b2ebcfd 100644
@@ -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);
Signature · 1 line
1.7.10.4
Acked-by on v7 9/9
#27 lore raw

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

Quoted 7 lines · Bart Van Assche

Make concurrent invocations of scsi_device_set_state() safe.

Signed-off-by Bart Van Assche <[email protected]>
Cc James Bottomley <[email protected]>
Cc Tejun Heo <[email protected]>
Cc Hannes Reinecke <[email protected]>
Cc Mike Christie <[email protected]>
Acked-by Hannes Reinecke <[email protected]>

Cheers,

Hannes

Signature · 8 lines
Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html