* [PATCH v8 0/10] More device removal fixes
@ 2013-02-05 12:44 Bart Van Assche
2013-02-05 12:45 ` [PATCH v8 01/10] Fix race between starved list processing and device removal Bart Van Assche
` (10 more replies)
0 siblings, 11 replies; 17+ messages in thread
From: Bart Van Assche @ 2013-02-05 12:44 UTC (permalink / raw)
To: linux-scsi, James Bottomley, Mike Christie, Tejun Heo, Chanho Min,
David Milburn
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.
- Save and restore the host_scribble field during error handling
These patches have been tested on top of kernel v3.8-rc6 and are also
available here:
http://github.com/bvanassche/linux/tree/device-removal-fixes.
Sorry for the delay between v7 and v8 - the past few weeks were really
hectic.
Changes compared to v7:
- Addressed the review comments posted by Hannes Reinecke and Rolf Eike
Beer.
- Modified patch "Make scsi_remove_host() wait until error handling
finished" such that it is also safe for SCSI timeout values below
the maximum LLD response time by modifying scsi_send_eh_cmnd() such
that it does not invoke any LLD code after scsi_remove_host() started.
- Added a patch to save and restore the host_scribble field.
- Refined / clarified several patch descriptions.
- Rebased and retested on top of kernel v3.8-rc6.
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.
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH v8 01/10] Fix race between starved list processing and device removal 2013-02-05 12:44 [PATCH v8 0/10] More device removal fixes Bart Van Assche @ 2013-02-05 12:45 ` Bart Van Assche 2013-02-05 12:46 ` [PATCH v8 02/10] Remove get_device() / put_device() pair from scsi_request_fn() Bart Van Assche ` (9 subsequent siblings) 10 siblings, 0 replies; 17+ messages in thread From: Bart Van Assche @ 2013-02-05 12:45 UTC (permalink / raw) Cc: linux-scsi, James Bottomley, Mike Christie, Tejun Heo, Chanho Min, David Milburn scsi_run_queue() examines all SCSI devices that are present on the starved list. Since scsi_run_queue() unlocks the SCSI host lock before running a queue a SCSI device can get removed after it has been removed from the starved list and before its queue is run. Protect against that race condition by increasing the sdev refcount before running the sdev queue. Also, remove a SCSI device from the starved list before __scsi_remove_device() decreases the sdev refcount such that the get_device() call added in scsi_run_queue() is guaranteed to succeed. 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: Hannes Reinecke <hare@suse.de> 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 931a7d9..34f1b39 100644 --- a/drivers/scsi/scsi_sysfs.c +++ b/drivers/scsi/scsi_sysfs.c @@ -345,7 +345,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); @@ -953,6 +952,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) @@ -974,6 +975,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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v8 02/10] Remove get_device() / put_device() pair from scsi_request_fn() 2013-02-05 12:44 [PATCH v8 0/10] More device removal fixes Bart Van Assche 2013-02-05 12:45 ` [PATCH v8 01/10] Fix race between starved list processing and device removal Bart Van Assche @ 2013-02-05 12:46 ` Bart Van Assche 2013-02-05 12:47 ` [PATCH v8 03/10] Introduce scsi_device_being_removed() Bart Van Assche ` (8 subsequent siblings) 10 siblings, 0 replies; 17+ messages in thread From: Bart Van Assche @ 2013-02-05 12:46 UTC (permalink / raw) Cc: linux-scsi, James Bottomley, Mike Christie, Tejun Heo, Chanho Min, David Milburn 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: Hannes Reinecke <hare@suse.de> Cc: Mike Christie <michaelc@cs.wisc.edu> --- 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v8 03/10] Introduce scsi_device_being_removed() 2013-02-05 12:44 [PATCH v8 0/10] More device removal fixes Bart Van Assche 2013-02-05 12:45 ` [PATCH v8 01/10] Fix race between starved list processing and device removal Bart Van Assche 2013-02-05 12:46 ` [PATCH v8 02/10] Remove get_device() / put_device() pair from scsi_request_fn() Bart Van Assche @ 2013-02-05 12:47 ` Bart Van Assche 2013-02-05 12:47 ` [PATCH v8 04/10] Remove offline devices when removing a host Bart Van Assche ` (7 subsequent siblings) 10 siblings, 0 replies; 17+ messages in thread From: Bart Van Assche @ 2013-02-05 12:47 UTC (permalink / raw) Cc: linux-scsi, James Bottomley, Mike Christie, Tejun Heo, Chanho Min, David Milburn This patch does not change any functionality. Signed-off-by: Bart Van Assche <bvanassche@acm.org> Acked-by: Hannes Reinecke <hare@suse.de> Cc: James Bottomley <JBottomley@Parallels.com> Cc: Mike Christie <michaelc@cs.wisc.edu> Cc: Tejun Heo <tj@kernel.org> --- 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 e65c62e..1dbf4ac 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -427,54 +427,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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v8 04/10] Remove offline devices when removing a host 2013-02-05 12:44 [PATCH v8 0/10] More device removal fixes Bart Van Assche ` (2 preceding siblings ...) 2013-02-05 12:47 ` [PATCH v8 03/10] Introduce scsi_device_being_removed() Bart Van Assche @ 2013-02-05 12:47 ` Bart Van Assche 2013-02-05 12:48 ` [PATCH v8 05/10] Disallow changing the device state via sysfs into "deleted" Bart Van Assche ` (6 subsequent siblings) 10 siblings, 0 replies; 17+ messages in thread From: Bart Van Assche @ 2013-02-05 12:47 UTC (permalink / raw) To: linux-scsi Cc: James Bottomley, Mike Christie, Tejun Heo, Chanho Min, David Milburn __scsi_remove_device() incorrectly skips devices that are visible and offline. Such devices get skipped because scsi_device_set_state() refuses the transition from SDEV_OFFLINE into SDEV_CANCEL. 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 by scsi_forget_host() for devices that are in state SDEV_CANCEL. 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 34f1b39..5e0c769 100644 --- a/drivers/scsi/scsi_sysfs.c +++ b/drivers/scsi/scsi_sysfs.c @@ -956,8 +956,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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v8 05/10] Disallow changing the device state via sysfs into "deleted" 2013-02-05 12:44 [PATCH v8 0/10] More device removal fixes Bart Van Assche ` (3 preceding siblings ...) 2013-02-05 12:47 ` [PATCH v8 04/10] Remove offline devices when removing a host Bart Van Assche @ 2013-02-05 12:48 ` Bart Van Assche 2013-02-05 12:49 ` [PATCH v8 06/10] Avoid saving/restoring interrupt state inside scsi_remove_host() Bart Van Assche ` (5 subsequent siblings) 10 siblings, 0 replies; 17+ messages in thread From: Bart Van Assche @ 2013-02-05 12:48 UTC (permalink / raw) Cc: linux-scsi, James Bottomley, Mike Christie, Tejun Heo, Chanho Min, David Milburn Changing the state of a SCSI device via sysfs into "cancel" or "deleted" prevents removal of these devices by scsi_remove_host(). Hence do not allow this. Also, introduce the symbolic name INVALID_SDEV_STATE, representing a value different from any valid SCSI device state. Update scsi_device_set_state() such that gcc does not issue a warning about an enumeration value not being handled inside a switch statement. 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> Cc: David Milburn <dmilburn@redhat.com> --- drivers/scsi/scsi_lib.c | 2 ++ drivers/scsi/scsi_sysfs.c | 5 +++-- include/scsi/scsi_device.h | 6 +++++- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index db4836bc..27bd445 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -2177,6 +2177,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 5e0c769..7c1935b 100644 --- a/drivers/scsi/scsi_sysfs.c +++ b/drivers/scsi/scsi_sysfs.c @@ -594,7 +594,7 @@ store_state_field(struct device *dev, struct device_attribute *attr, { 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); @@ -604,7 +604,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)) diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index 1dbf4ac..1f8723c 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 from + * 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 */ -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v8 06/10] Avoid saving/restoring interrupt state inside scsi_remove_host() 2013-02-05 12:44 [PATCH v8 0/10] More device removal fixes Bart Van Assche ` (4 preceding siblings ...) 2013-02-05 12:48 ` [PATCH v8 05/10] Disallow changing the device state via sysfs into "deleted" Bart Van Assche @ 2013-02-05 12:49 ` Bart Van Assche 2013-02-05 12:49 ` [PATCH v8 07/10] Make scsi_remove_host() wait for device removal Bart Van Assche ` (4 subsequent siblings) 10 siblings, 0 replies; 17+ messages in thread From: Bart Van Assche @ 2013-02-05 12:49 UTC (permalink / raw) To: linux-scsi Cc: James Bottomley, Mike Christie, Tejun Heo, Chanho Min, David Milburn 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 the function scsi_remove_host(). Signed-off-by: Bart Van Assche <bvanassche@acm.org> Acked-by: Tejun Heo <tj@kernel.org> Acked-by: Hannes Reinecke <hare@suse.de> Cc: Mike Christie <michaelc@cs.wisc.edu> 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v8 07/10] Make scsi_remove_host() wait for device removal 2013-02-05 12:44 [PATCH v8 0/10] More device removal fixes Bart Van Assche ` (5 preceding siblings ...) 2013-02-05 12:49 ` [PATCH v8 06/10] Avoid saving/restoring interrupt state inside scsi_remove_host() Bart Van Assche @ 2013-02-05 12:49 ` Bart Van Assche 2013-02-05 12:50 ` [PATCH v8 08/10] Make scsi_remove_host() wait until error handling finished Bart Van Assche ` (3 subsequent siblings) 10 siblings, 0 replies; 17+ messages in thread From: Bart Van Assche @ 2013-02-05 12:49 UTC (permalink / raw) To: linux-scsi Cc: James Bottomley, Mike Christie, Tejun Heo, Chanho Min, David Milburn If removal of a SCSI device has been triggered via sysfs that device may still be in state SDEV_CANCEL if scsi_remove_host() is invoked later on and after 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 7c1935b..bd51d65 100644 --- a/drivers/scsi/scsi_sysfs.c +++ b/drivers/scsi/scsi_sysfs.c @@ -345,6 +345,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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v8 08/10] Make scsi_remove_host() wait until error handling finished 2013-02-05 12:44 [PATCH v8 0/10] More device removal fixes Bart Van Assche ` (6 preceding siblings ...) 2013-02-05 12:49 ` [PATCH v8 07/10] Make scsi_remove_host() wait for device removal Bart Van Assche @ 2013-02-05 12:50 ` Bart Van Assche 2013-02-05 12:51 ` [PATCH v8 09/10] Avoid that scsi_device_set_state() triggers a race Bart Van Assche ` (2 subsequent siblings) 10 siblings, 0 replies; 17+ messages in thread From: Bart Van Assche @ 2013-02-05 12:50 UTC (permalink / raw) To: linux-scsi Cc: James Bottomley, Mike Christie, Tejun Heo, Chanho Min, David Milburn 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 | 74 +++++++++++++++++++++++++++++++++++++++++++-- include/scsi/scsi_host.h | 1 + 3 files changed, 74 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..266981a 100644 --- a/drivers/scsi/scsi_error.c +++ b/drivers/scsi/scsi_error.c @@ -536,8 +536,52 @@ static void scsi_eh_done(struct scsi_cmnd *scmd) } /** + * scsi_begin_eh - start host-related error handling + * + * Must be called before invoking an LLD callback function 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. + */ +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 +596,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 +608,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 +630,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 +642,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 +673,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 +683,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 +701,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; } @@ -795,6 +857,9 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd, struct scsi_eh_save ses; int rtn; + if (scsi_begin_eh(shost)) + return FAILED; + scsi_eh_prep_cmnd(scmd, &ses, cmnd, cmnd_size, sense_bytes); shost->eh_action = &done; @@ -850,6 +915,8 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd, rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn); } + scsi_end_eh(shost); + return rtn; } @@ -1877,6 +1944,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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v8 09/10] Avoid that scsi_device_set_state() triggers a race 2013-02-05 12:44 [PATCH v8 0/10] More device removal fixes Bart Van Assche ` (7 preceding siblings ...) 2013-02-05 12:50 ` [PATCH v8 08/10] Make scsi_remove_host() wait until error handling finished Bart Van Assche @ 2013-02-05 12:51 ` Bart Van Assche 2013-02-05 12:51 ` [PATCH v8 10/10] Save and restore host_scribble during error handling Bart Van Assche 2013-02-06 22:31 ` [PATCH v8 0/10] More device removal fixes Joe Lawrence 10 siblings, 0 replies; 17+ messages in thread From: Bart Van Assche @ 2013-02-05 12:51 UTC (permalink / raw) To: linux-scsi Cc: James Bottomley, Mike Christie, Tejun Heo, Chanho Min, David Milburn Signed-off-by: Bart Van Assche <bvanassche@acm.org> Acked-by: Hannes Reinecke <hare@suse.de> Cc: James Bottomley <JBottomley@Parallels.com> Cc: Tejun Heo <tj@kernel.org> Cc: Mike Christie <michaelc@cs.wisc.edu> --- drivers/scsi/scsi_error.c | 4 ++++ drivers/scsi/scsi_lib.c | 43 ++++++++++++++++++++++++++++++++++--------- drivers/scsi/scsi_scan.c | 15 ++++++++------- drivers/scsi/scsi_sysfs.c | 18 ++++++++++++++---- 4 files changed, 60 insertions(+), 20 deletions(-) diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c index 266981a..d50af13 100644 --- a/drivers/scsi/scsi_error.c +++ b/drivers/scsi/scsi_error.c @@ -1430,7 +1430,11 @@ static void scsi_eh_offline_sdevs(struct list_head *work_q, list_for_each_entry_safe(scmd, next, work_q, eh_entry) { sdev_printk(KERN_INFO, scmd->device, "Device offlined - " "not ready after error recovery\n"); + + spin_lock_irq(scmd->device->host->host_lock); scsi_device_set_state(scmd->device, SDEV_OFFLINE); + spin_unlock_irq(scmd->device->host->host_lock); + if (scmd->eh_eflags & SCSI_EH_CANCEL_CMD) { /* * FIXME: Handle lost cmds. diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 27bd445..d826710 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -2074,7 +2074,9 @@ EXPORT_SYMBOL(scsi_test_unit_ready); * @state: state to change to. * * Returns zero if unsuccessful or an error if the requested - * transition is illegal. + * transition is illegal. It is the responsibility of the caller to make + * sure that a call of this function does not race with other code that + * accesses the device state, e.g. by holding the host lock. */ int scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state) @@ -2353,7 +2355,13 @@ EXPORT_SYMBOL_GPL(sdev_evt_send_simple); int scsi_device_quiesce(struct scsi_device *sdev) { - int err = scsi_device_set_state(sdev, SDEV_QUIESCE); + struct Scsi_Host *host = sdev->host; + int err; + + spin_lock_irq(host->host_lock); + err = scsi_device_set_state(sdev, SDEV_QUIESCE); + spin_unlock_irq(host->host_lock); + if (err) return err; @@ -2377,13 +2385,21 @@ EXPORT_SYMBOL(scsi_device_quiesce); */ void scsi_device_resume(struct scsi_device *sdev) { + struct Scsi_Host *host = sdev->host; + int err; + /* check if the device state was mutated prior to resume, and if * so assume the state is being managed elsewhere (for example * device deleted during suspend) */ - if (sdev->sdev_state != SDEV_QUIESCE || - scsi_device_set_state(sdev, SDEV_RUNNING)) + spin_lock_irq(host->host_lock); + err = sdev->sdev_state == SDEV_QUIESCE ? + scsi_device_set_state(sdev, SDEV_RUNNING) : -EINVAL; + spin_unlock_irq(host->host_lock); + + if (err) return; + scsi_run_queue(sdev->request_queue); } EXPORT_SYMBOL(scsi_device_resume); @@ -2433,17 +2449,19 @@ EXPORT_SYMBOL(scsi_target_resume); int scsi_internal_device_block(struct scsi_device *sdev) { + struct Scsi_Host *host = sdev->host; struct request_queue *q = sdev->request_queue; unsigned long flags; int err = 0; + spin_lock_irqsave(host->host_lock, flags); err = scsi_device_set_state(sdev, SDEV_BLOCK); - if (err) { + if (err) err = scsi_device_set_state(sdev, SDEV_CREATED_BLOCK); + spin_unlock_irqrestore(host->host_lock, flags); - if (err) - return err; - } + if (err) + return err; /* * The device has transitioned to SDEV_BLOCK. Stop the @@ -2478,13 +2496,16 @@ int scsi_internal_device_unblock(struct scsi_device *sdev, enum scsi_device_state new_state) { + struct Scsi_Host *host = sdev->host; struct request_queue *q = sdev->request_queue; unsigned long flags; + int ret = 0; /* * Try to transition the scsi device to SDEV_RUNNING or one of the * offlined states and goose the device queue if successful. */ + spin_lock_irqsave(host->host_lock, flags); if ((sdev->sdev_state == SDEV_BLOCK) || (sdev->sdev_state == SDEV_TRANSPORT_OFFLINE)) sdev->sdev_state = new_state; @@ -2496,7 +2517,11 @@ scsi_internal_device_unblock(struct scsi_device *sdev, sdev->sdev_state = SDEV_CREATED; } else if (sdev->sdev_state != SDEV_CANCEL && sdev->sdev_state != SDEV_OFFLINE) - return -EINVAL; + ret = -EINVAL; + spin_unlock_irqrestore(host->host_lock, flags); + + if (ret) + return ret; spin_lock_irqsave(q->queue_lock, flags); blk_start_queue(q); diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c index 0612fba..3fbdd7b 100644 --- a/drivers/scsi/scsi_scan.c +++ b/drivers/scsi/scsi_scan.c @@ -898,18 +898,19 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result, if (*bflags & BLIST_USE_10_BYTE_MS) sdev->use_10_for_ms = 1; + spin_lock_irq(sdev->host->host_lock); /* set the device running here so that slave configure * may do I/O */ ret = scsi_device_set_state(sdev, SDEV_RUNNING); - if (ret) { + if (ret) ret = scsi_device_set_state(sdev, SDEV_BLOCK); + spin_unlock_irq(sdev->host->host_lock); - if (ret) { - sdev_printk(KERN_ERR, sdev, - "in wrong state %s to complete scan\n", - scsi_device_state_name(sdev->sdev_state)); - return SCSI_SCAN_NO_RESPONSE; - } + if (ret) { + sdev_printk(KERN_ERR, sdev, + "in wrong state %s to complete scan\n", + scsi_device_state_name(sdev->sdev_state)); + return SCSI_SCAN_NO_RESPONSE; } if (*bflags & BLIST_MS_192_BYTES_FOR_3F) diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c index bd51d65..6068cef 100644 --- a/drivers/scsi/scsi_sysfs.c +++ b/drivers/scsi/scsi_sysfs.c @@ -593,7 +593,7 @@ static ssize_t store_state_field(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { - int i; + int i, ret; struct scsi_device *sdev = to_scsi_device(dev); enum scsi_device_state state = INVALID_SDEV_STATE; @@ -609,9 +609,11 @@ store_state_field(struct device *dev, struct device_attribute *attr, state == SDEV_DEL) return -EINVAL; - if (scsi_device_set_state(sdev, state)) - return -EINVAL; - return count; + spin_lock_irq(sdev->host->host_lock); + ret = scsi_device_set_state(sdev, state); + spin_unlock_irq(sdev->host->host_lock); + + return ret < 0 ? ret : count; } static ssize_t @@ -871,7 +873,10 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev) struct request_queue *rq = sdev->request_queue; struct scsi_target *starget = sdev->sdev_target; + spin_lock_irq(sdev->host->host_lock); error = scsi_device_set_state(sdev, SDEV_RUNNING); + spin_unlock_irq(sdev->host->host_lock); + if (error) return error; @@ -958,8 +963,10 @@ void __scsi_remove_device(struct scsi_device *sdev) unsigned long flags; if (sdev->is_visible) { + spin_lock_irqsave(shost->host_lock, flags); WARN_ON_ONCE(scsi_device_set_state(sdev, SDEV_CANCEL) != 0 && scsi_device_set_state(sdev, SDEV_DEL) != 0); + spin_unlock_irqrestore(shost->host_lock, flags); bsg_unregister_queue(sdev->request_queue); device_unregister(&sdev->sdev_dev); @@ -973,7 +980,10 @@ void __scsi_remove_device(struct scsi_device *sdev) * scsi_run_queue() invocations have finished before tearing down the * device. */ + spin_lock_irqsave(shost->host_lock, flags); scsi_device_set_state(sdev, SDEV_DEL); + spin_unlock_irqrestore(shost->host_lock, flags); + blk_cleanup_queue(sdev->request_queue); cancel_work_sync(&sdev->requeue_work); -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v8 10/10] Save and restore host_scribble during error handling 2013-02-05 12:44 [PATCH v8 0/10] More device removal fixes Bart Van Assche ` (8 preceding siblings ...) 2013-02-05 12:51 ` [PATCH v8 09/10] Avoid that scsi_device_set_state() triggers a race Bart Van Assche @ 2013-02-05 12:51 ` Bart Van Assche 2013-02-06 22:31 ` [PATCH v8 0/10] More device removal fixes Joe Lawrence 10 siblings, 0 replies; 17+ messages in thread From: Bart Van Assche @ 2013-02-05 12:51 UTC (permalink / raw) To: linux-scsi Cc: James Bottomley, Mike Christie, Tejun Heo, Chanho Min, David Milburn A SCSI LLD may overwrite host_scribble in its queuecommand() implementation. Several drivers need that field to process requests and aborts correctly. Hence this field must be saved by scsi_eh_prep_cmnd() and must be restored by scsi_eh_restore_cmnd(). 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_error.c | 2 ++ include/scsi/scsi_eh.h | 1 + 2 files changed, 3 insertions(+) diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c index d50af13..a4a9ec9 100644 --- a/drivers/scsi/scsi_error.c +++ b/drivers/scsi/scsi_error.c @@ -769,6 +769,7 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct scsi_eh_save *ses, ses->result = scmd->result; ses->underflow = scmd->underflow; ses->prot_op = scmd->prot_op; + ses->host_scribble = scmd->host_scribble; scmd->prot_op = SCSI_PROT_NORMAL; scmd->cmnd = ses->eh_cmnd; @@ -830,6 +831,7 @@ void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd, struct scsi_eh_save *ses) scmd->result = ses->result; scmd->underflow = ses->underflow; scmd->prot_op = ses->prot_op; + scmd->host_scribble = ses->host_scribble; } EXPORT_SYMBOL(scsi_eh_restore_cmnd); diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h index 06a8790..7bdb02f 100644 --- a/include/scsi/scsi_eh.h +++ b/include/scsi/scsi_eh.h @@ -83,6 +83,7 @@ struct scsi_eh_save { /* new command support */ unsigned char eh_cmnd[BLK_MAX_CDB]; struct scatterlist sense_sgl; + unsigned char *host_scribble; }; extern void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v8 0/10] More device removal fixes 2013-02-05 12:44 [PATCH v8 0/10] More device removal fixes Bart Van Assche ` (9 preceding siblings ...) 2013-02-05 12:51 ` [PATCH v8 10/10] Save and restore host_scribble during error handling Bart Van Assche @ 2013-02-06 22:31 ` Joe Lawrence 2013-02-07 10:40 ` Bart Van Assche 2013-02-07 11:33 ` Bart Van Assche 10 siblings, 2 replies; 17+ messages in thread From: Joe Lawrence @ 2013-02-06 22:31 UTC (permalink / raw) To: Bart Van Assche Cc: linux-scsi, James Bottomley, Mike Christie, Tejun Heo, Chanho Min, David Milburn On Tue, 5 Feb 2013, Bart Van Assche wrote: > 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. > - Save and restore the host_scribble field during error handling > > These patches have been tested on top of kernel v3.8-rc6 and are also > available here: http://github.com/bvanassche/linux/tree/device-removal-fixes. Hi Bart, I gave your patchset a spin on our ftServer, which supports hotplug of PCI devices. Before attempting a surprise HW removal of an LSI Fusion SAS-2 SCSI controller, I issued an orderly removal through our hotplug driver. The device removal appeared to hang (this does not occur with vanilla 3.8.0-rc6): PID: 3331 TASK: ffff88083e968000 CPU: 28 COMMAND: "java" #0 [ffff88084fd5da48] __schedule at ffffffff8165e8f4 #1 [ffff88084fd5dac0] schedule at ffffffff8165f0d9 #3 [ffff88084fd5db30] _scsih_remove at ffffffffa008416d [mpt2sas] #4 [ffff88084fd5db80] pci_device_remove at ffffffff81335e66 #5 [ffff88084fd5dbb0] __device_release_driver at ffffffff813fd23c #6 [ffff88084fd5dbd0] device_release_driver at ffffffff813fd57c #7 [ffff88084fd5dbf0] bus_remove_device at ffffffff813fccde #8 [ffff88084fd5dc20] device_del at ffffffff813fa0ba #9 [ffff88084fd5dc50] device_unregister at ffffffff813fa172 #10 [ffff88084fd5dc70] pci_stop_bus_device at ffffffff8132fce4 #11 [ffff88084fd5dca0] pci_stop_bus_device at ffffffff8132fc8d #12 [ffff88084fd5dcd0] pci_stop_bus_device at ffffffff8132fc8d #13 [ffff88084fd5dd00] pci_stop_bus_device at ffffffff8132fc8d #14 [ffff88084fd5dd30] pci_stop_bus_device at ffffffff8132fc8d #15 [ffff88084fd5dd60] pci_stop_and_remove_bus_device at ffffffff8132fe46 And to further pin down where we are... crash> sym _scsih_remove ffffffffa0084010 (t) _scsih_remove [mpt2sas] crash> p/x 0xffffffffa008416d - 0xffffffffa0084010 $9 = 0x15d crash> dis -l _scsih_remove <_scsih_remove+0x140>: callq 0xffffffffa0011ea0 <sas_remove_host> <_scsih_remove+0x145>: mov %r13,%rdi <_scsih_remove+0x148>: callq 0xffffffffa00766c0 <mpt2sas_base_detach> <_scsih_remove+0x14d>: mov %r13,%rdi <_scsih_remove+0x150>: callq 0xffffffff8131b930 <list_del> <_scsih_remove+0x155>: mov %r12,%rdi <_scsih_remove+0x158>: callq 0xffffffff8141dcf0 <scsi_remove_host> <_scsih_remove+0x15d>: mov %r12,%rdi <_scsih_remove+0x160>: callq 0xffffffff8141d770 <scsi_host_put> <_scsih_remove+0x165>: add $0x18,%rsp We're stuck in the mpt2sas driver's call to scsi_remove_host(), which "[PATCH v8 07/10] Make scsi_remove_host() wait for device removal" modifies. I re-ran the test with some additional debugging so I could figure out the Scsi_Host it was operating on and found: crash> struct Scsi_Host 0xffff8808513a4290 | grep eh_active eh_active = 0 However, its __devices list was not empty: crash> struct Scsi_Host 0xffff8808513a4290 | grep __devices -A3 __devices = { next = 0xffff880851232530, prev = 0xffff880851235398 }, So scsi_remove_host loops while the __devices list is never cleared. See a complete dump of Scsi_Host and the __devices_list scsi_devices below. Regards, -- Joe crash> struct Scsi_Host 0xffff8808513a4290 > shost struct Scsi_Host { __devices = { next = 0xffff880851232530, prev = 0xffff880851235398 }, __targets = { next = 0xffff881054377980, prev = 0xffff88084ed99528 }, cmd_pool = 0xffffffff81c956c0, free_list_lock = { { rlock = { raw_lock = { { head_tail = 0x2e2e, tickets = { head = 0x2e, tail = 0x2e } } } } } }, free_list = { next = 0xffff88084fb6c008, prev = 0xffff88084fb6c008 }, starved_list = { next = 0xffff8808513a42d0, prev = 0xffff8808513a42d0 }, default_lock = { { rlock = { raw_lock = { { head_tail = 0xe5e5, tickets = { head = 0xe5, tail = 0xe5 } } } } } }, host_lock = 0xffff8808513a42e0, scan_mutex = { count = { counter = 0x1 }, wait_lock = { { rlock = { raw_lock = { { head_tail = 0x0, tickets = { head = 0x0, tail = 0x0 } } } } } }, wait_list = { next = 0xffff8808513a42f8, prev = 0xffff8808513a42f8 }, owner = 0x0 }, eh_cmd_q = { next = 0xffff8808513a4310, prev = 0xffff8808513a4310 }, ehandler = 0xffff8808512d1890, eh_action = 0x0, eh_active = 0x0, host_wait = { lock = { { rlock = { raw_lock = { { head_tail = 0x0, tickets = { head = 0x0, tail = 0x0 } } } } } }, task_list = { next = 0xffff8808513a4340, prev = 0xffff8808513a4340 } }, remove_host = { lock = { { rlock = { raw_lock = { { head_tail = 0x101, tickets = { head = 0x1, tail = 0x1 } } } } } }, task_list = { next = 0xffff88103712faa8, prev = 0xffff88103712faa8 } }, hostt = 0xffffffffa00cc1c0, transportt = 0xffff880850c233d8, bqt = 0x0, host_busy = 0x0, host_failed = 0x0, host_eh_scheduled = 0x0, host_no = 0x0, resetting = 0x0, last_reset = 0x0, max_id = 0xffffffff, max_lun = 0x41ff, max_channel = 0x0, unique_id = 0x0, max_cmd_len = 0x20, this_id = 0xffffffff, can_queue = 0xdfb, cmd_per_lun = 0x7, sg_tablesize = 0x80, sg_prot_tablesize = 0x0, max_sectors = 0x7fff, dma_boundary = 0xffffffff, cmd_serial_number = 0x152f, active_mode = 0x1, unchecked_isa_dma = 0x0, use_clustering = 0x1, use_blk_tcq = 0x0, host_self_blocked = 0x0, reverse_ordering = 0x0, ordered_tag = 0x0, tmf_in_progress = 0x0, async_scan = 0x0, eh_noresume = 0x0, work_q_name = "\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000", work_q = 0x0, host_blocked = 0x0, max_host_blocked = 0x7, prot_capabilities = 0x7, prot_guard_type = 0x1, uspace_req_q = 0x0, base = 0x0, io_port = 0x0, n_io_port = 0x0, dma_channel = 0xff, irq = 0x0, shost_state = SHOST_DEL, shost_gendev = { parent = 0xffff881053a92328, p = 0xffff880850ccca28, kobj = { name = 0xffff88084fa2e220 "host0", entry = { next = 0xffff8808513a46c8, prev = 0xffff880851225498 }, parent = 0xffff881053a92338, kset = 0xffff88085f878b98, ktype = 0xffffffff81c92240, sd = 0xffff88084fb65658, kref = { refcount = { counter = 0xe } }, state_initialized = 0x1, state_in_sysfs = 0x1, state_add_uevent_sent = 0x1, state_remove_uevent_sent = 0x0, uevent_suppress = 0x0 }, init_name = 0x0, type = 0xffffffff81c95780, mutex = { count = { counter = 0x1 }, wait_lock = { { rlock = { raw_lock = { { head_tail = 0x0, tickets = { head = 0x0, tail = 0x0 } } } } } }, wait_list = { next = 0xffff8808513a4498, prev = 0xffff8808513a4498 }, owner = 0x0 }, bus = 0xffffffff81c95960, driver = 0x0, platform_data = 0x0, power = { power_state = { event = 0x0 }, can_wakeup = 0x0, async_suspend = 0x1, is_prepared = 0x0, is_suspended = 0x0, ignore_children = 0x0, early_init = 0x1, lock = { { rlock = { raw_lock = { { head_tail = 0xf0f, tickets = { head = 0xf, tail = 0xf } } } } } }, entry = { next = 0xffff8808513a4750, prev = 0xffff880851225520 }, completion = { done = 0x7fffffff, wait = { lock = { { rlock = { raw_lock = { { head_tail = 0x101, tickets = { head = 0x1, tail = 0x1 } } } } } }, task_list = { next = 0xffff8808513a44f0, prev = 0xffff8808513a44f0 } } }, wakeup = 0x0, wakeup_path = 0x0, syscore = 0x0, suspend_timer = { entry = { next = 0x0, prev = 0x0 }, expires = 0x0, base = 0xffff880854090000, function = 0xffffffff814092d0 <pm_suspend_timer_fn>, data = 0xffff8808513a4430, slack = 0xffffffff, start_pid = 0xffffffff, start_site = 0x0, start_comm = "\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000" }, timer_expires = 0x0, work = { data = { counter = 0x10200 }, entry = { next = 0xffff8808513a4570, prev = 0xffff8808513a4570 }, func = 0xffffffff8140a6b0 <pm_runtime_work> }, wait_queue = { lock = { { rlock = { raw_lock = { { head_tail = 0x303, tickets = { head = 0x3, tail = 0x3 } } } } } }, task_list = { next = 0xffff8808513a4590, prev = 0xffff8808513a4590 } }, usage_count = { counter = 0x1 }, child_count = { counter = 0x0 }, disable_depth = 0x0, idle_notification = 0x0, request_pending = 0x0, deferred_resume = 0x0, run_wake = 0x0, runtime_auto = 0x1, no_callbacks = 0x0, irq_safe = 0x0, use_autosuspend = 0x0, timer_autosuspends = 0x0, request = RPM_REQ_NONE, runtime_status = RPM_ACTIVE, runtime_error = 0x0, autosuspend_delay = 0x0, last_busy = 0x0, active_jiffies = 0x119, suspended_jiffies = 0x199ea, accounting_timestamp = 0xfffd2888, subsys_data = 0x0, qos = 0x0 }, pm_domain = 0x0, numa_node = 0xffffffff, dma_mask = 0x0, coherent_dma_mask = 0x0, dma_parms = 0x0, dma_pools = { next = 0xffff8808513a4618, prev = 0xffff8808513a4618 }, dma_mem = 0x0, archdata = { dma_ops = 0x0, iommu = 0x0 }, of_node = 0x0, acpi_node = { handle = 0x0 }, devt = 0x0, id = 0x0, devres_lock = { { rlock = { raw_lock = { { head_tail = 0x0, tickets = { head = 0x0, tail = 0x0 } } } } } }, devres_head = { next = 0xffff8808513a4660, prev = 0xffff8808513a4660 }, knode_class = { n_klist = 0x0, n_node = { next = 0x0, prev = 0x0 }, n_ref = { refcount = { counter = 0x0 } } }, class = 0x0, groups = 0x0, release = 0, iommu_group = 0x0 }, shost_dev = { parent = 0xffff8808513a4430, p = 0xffff880850cccc30, kobj = { name = 0xffff88084fa2dce0 "host0", entry = { next = 0xffff88084fb87448, prev = 0xffff8808513a4448 }, parent = 0xffff8808513e31b0, kset = 0xffff88085f878b98, ktype = 0xffffffff81c92240, sd = 0xffff88084fb78370, kref = { refcount = { counter = 0x3 } }, state_initialized = 0x1, state_in_sysfs = 0x1, state_add_uevent_sent = 0x1, state_remove_uevent_sent = 0x0, uevent_suppress = 0x0 }, init_name = 0x0, type = 0x0, mutex = { count = { counter = 0x1 }, wait_lock = { { rlock = { raw_lock = { { head_tail = 0x0, tickets = { head = 0x0, tail = 0x0 } } } } } }, wait_list = { next = 0xffff8808513a4718, prev = 0xffff8808513a4718 }, owner = 0x0 }, bus = 0x0, driver = 0x0, platform_data = 0x0, power = { power_state = { event = 0x0 }, can_wakeup = 0x0, async_suspend = 0x1, is_prepared = 0x0, is_suspended = 0x0, ignore_children = 0x0, early_init = 0x1, lock = { { rlock = { raw_lock = { { head_tail = 0x0, tickets = { head = 0x0, tail = 0x0 } } } } } }, entry = { next = 0xffff88084fb874d0, prev = 0xffff8808513a44d0 }, completion = { done = 0x7fffffff, wait = { lock = { { rlock = { raw_lock = { { head_tail = 0x101, tickets = { head = 0x1, tail = 0x1 } } } } } }, task_list = { next = 0xffff8808513a4770, prev = 0xffff8808513a4770 } } }, wakeup = 0x0, wakeup_path = 0x0, syscore = 0x0, suspend_timer = { entry = { next = 0x0, prev = 0x0 }, expires = 0x0, base = 0xffff880854090000, function = 0xffffffff814092d0 <pm_suspend_timer_fn>, data = 0xffff8808513a46b0, slack = 0xffffffff, start_pid = 0xffffffff, start_site = 0x0, start_comm = "\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000" }, timer_expires = 0x0, work = { data = { counter = 0x10200 }, entry = { next = 0xffff8808513a47f0, prev = 0xffff8808513a47f0 }, func = 0xffffffff8140a6b0 <pm_runtime_work> }, wait_queue = { lock = { { rlock = { raw_lock = { { head_tail = 0x0, tickets = { head = 0x0, tail = 0x0 } } } } } }, task_list = { next = 0xffff8808513a4810, prev = 0xffff8808513a4810 } }, usage_count = { counter = 0x0 }, child_count = { counter = 0x0 }, disable_depth = 0x1, idle_notification = 0x0, request_pending = 0x0, deferred_resume = 0x0, run_wake = 0x0, runtime_auto = 0x1, no_callbacks = 0x0, irq_safe = 0x0, use_autosuspend = 0x0, timer_autosuspends = 0x0, request = RPM_REQ_NONE, runtime_status = RPM_SUSPENDED, runtime_error = 0x0, autosuspend_delay = 0x0, last_busy = 0x0, active_jiffies = 0x0, suspended_jiffies = 0x0, accounting_timestamp = 0xfffb8d5b, subsys_data = 0x0, qos = 0x0 }, pm_domain = 0x0, numa_node = 0xffffffff, dma_mask = 0x0, coherent_dma_mask = 0x0, dma_parms = 0x0, dma_pools = { next = 0xffff8808513a4898, prev = 0xffff8808513a4898 }, dma_mem = 0x0, archdata = { dma_ops = 0x0, iommu = 0x0 }, of_node = 0x0, acpi_node = { handle = 0x0 }, devt = 0x0, id = 0x0, devres_lock = { { rlock = { raw_lock = { { head_tail = 0x0, tickets = { head = 0x0, tail = 0x0 } } } } } }, devres_head = { next = 0xffff8808513a48e0, prev = 0xffff8808513a48e0 }, knode_class = { n_klist = 0xffff880853dc8730, n_node = { next = 0xffff8808513a27b0, prev = 0xffff880853dc8738 }, n_ref = { refcount = { counter = 0x1 } } }, class = 0xffffffff81c957c0, groups = 0xffffffff81c959e0, release = 0, iommu_group = 0x0 }, sht_legacy_list = { next = 0x0, prev = 0x0 }, shost_data = 0xffff8808513e3008, dma_dev = 0xffff881053a92328, hostdata = 0xffff8808513a4950 } crash> list scsi_device.siblings -H 0xffff8808513a4290 -s scsi_device ffff880851232520 struct scsi_device { host = 0xffff8808513a4290, request_queue = 0xffff88085107ad78, siblings = { next = 0xffff880851235398, prev = 0xffff8808513a4290 }, same_target_siblings = { next = 0xffff881054377990, prev = 0xffff881054377990 }, device_busy = 0x0, list_lock = { { rlock = { raw_lock = { { head_tail = 0x2626, tickets = { head = 0x26, tail = 0x26 } } } } } }, cmd_list = { next = 0xffff880851232558, prev = 0xffff880851232558 }, starved_entry = { next = 0xdead000000100100, prev = 0xdead000000200200 }, current_cmnd = 0x0, queue_depth = 0xfe, max_queue_depth = 0xfe, last_queue_full_depth = 0x0, last_queue_full_count = 0x0, last_queue_full_time = 0x0, queue_ramp_up_period = 0x1d4c0, last_queue_ramp_up = 0x0, id = 0x0, lun = 0x0, channel = 0x0, manufacturer = 0x0, sector_size = 0x200, hostdata = 0x0, type = 0x0, scsi_level = 0x7, inq_periph_qual = 0x0, inquiry_len = 0x90, inquiry = 0xffff88084ec2e8a0 "", vendor = 0xffff88084ec2e8a8 "SEAGATE ST9300653SS 00026XN020GG", model = 0xffff88084ec2e8b0 "ST9300653SS 00026XN020GG", rev = 0xffff88084ec2e8c0 "00026XN020GG", current_tag = 0x0, sdev_target = 0xffff881054377978, sdev_bflags = 0x0, writeable = 0x1, removable = 0x0, changed = 0x0, busy = 0x0, lockable = 0x0, locked = 0x0, borken = 0x0, disconnect = 0x0, soft_reset = 0x0, sdtr = 0x0, wdtr = 0x0, ppr = 0x1, tagged_supported = 0x1, simple_tags = 0x0, ordered_tags = 0x0, was_reset = 0x0, expecting_cc_ua = 0x0, use_10_for_rw = 0x1, use_10_for_ms = 0x0, no_report_opcodes = 0x0, no_write_same = 0x0, use_16_for_rw = 0x0, skip_ms_page_8 = 0x0, skip_ms_page_3f = 0x0, skip_vpd_pages = 0x0, use_192_bytes_for_3f = 0x0, no_start_on_add = 0x0, allow_restart = 0x0, manage_start_stop = 0x0, start_stop_pwr_cond = 0x0, no_uld_attach = 0x0, select_no_atn = 0x0, fix_capacity = 0x0, guess_capacity = 0x0, retry_hwerror = 0x0, last_sector_bug = 0x0, no_read_disc_info = 0x0, no_read_capacity_16 = 0x0, try_rc_10_first = 0x0, is_visible = 0x1, can_power_off = 0x0, wce_default_on = 0x0, no_dif = 0x0, supported_events = {0x0}, event_list = { next = 0xffff880851232610, prev = 0xffff880851232610 }, event_work = { data = { counter = 0x10200 }, entry = { next = 0xffff880851232628, prev = 0xffff880851232628 }, func = 0xffffffff81426020 <scsi_evt_thread> }, device_blocked = 0x0, max_device_blocked = 0x3, iorequest_cnt = { counter = 0x1313 }, iodone_cnt = { counter = 0x1313 }, ioerr_cnt = { counter = 0x0 }, sdev_gendev = { parent = 0xffff8810543779a0, p = 0xffff88084edab6d8, kobj = { name = 0xffff88084ec48540 "0:0:0:0", entry = { next = 0xffff880851232670, prev = 0xffff880851232670 }, parent = 0x0, kset = 0xffff88085f878b98, ktype = 0xffffffff81c92240, sd = 0x0, kref = { refcount = { counter = 0x2 } }, state_initialized = 0x1, state_in_sysfs = 0x0, state_add_uevent_sent = 0x1, state_remove_uevent_sent = 0x1, uevent_suppress = 0x0 }, init_name = 0x0, type = 0xffffffff81c95ac0, mutex = { count = { counter = 0x1 }, wait_lock = { { rlock = { raw_lock = { { head_tail = 0x0, tickets = { head = 0x0, tail = 0x0 } } } } } }, wait_list = { next = 0xffff8808512326c0, prev = 0xffff8808512326c0 }, owner = 0x0 }, bus = 0xffffffff81c95960, driver = 0x0, platform_data = 0x0, power = { power_state = { event = 0xffffffff }, can_wakeup = 0x0, async_suspend = 0x1, is_prepared = 0x0, is_suspended = 0x0, ignore_children = 0x0, early_init = 0x1, lock = { { rlock = { raw_lock = { { head_tail = 0x2b2b, tickets = { head = 0x2b, tail = 0x2b } } } } } }, entry = { next = 0xffff8808512326f8, prev = 0xffff8808512326f8 }, completion = { done = 0xfffffffe, wait = { lock = { { rlock = { raw_lock = { { head_tail = 0x202, tickets = { head = 0x2, tail = 0x2 } } } } } }, task_list = { next = 0xffff880851232718, prev = 0xffff880851232718 } } }, wakeup = 0x0, wakeup_path = 0x0, syscore = 0x0, suspend_timer = { entry = { next = 0x0, prev = 0x0 }, expires = 0x0, base = 0xffff8808546f4000, function = 0xffffffff814092d0 <pm_suspend_timer_fn>, data = 0xffff880851232658, slack = 0xffffffff, start_pid = 0xffffffff, start_site = 0x0, start_comm = "\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000" }, timer_expires = 0x0, work = { data = { counter = 0x10200 }, entry = { next = 0xffff880851232798, prev = 0xffff880851232798 }, func = 0xffffffff8140a6b0 <pm_runtime_work> }, wait_queue = { lock = { { rlock = { raw_lock = { { head_tail = 0x0, tickets = { head = 0x0, tail = 0x0 } } } } } }, task_list = { next = 0xffff8808512327b8, prev = 0xffff8808512327b8 } }, usage_count = { counter = 0x3 }, child_count = { counter = 0x0 }, disable_depth = 0x1, idle_notification = 0x0, request_pending = 0x0, deferred_resume = 0x0, run_wake = 0x0, runtime_auto = 0x0, no_callbacks = 0x0, irq_safe = 0x0, use_autosuspend = 0x0, timer_autosuspends = 0x0, request = RPM_REQ_NONE, runtime_status = RPM_SUSPENDED, runtime_error = 0x0, autosuspend_delay = 0x0, last_busy = 0x0, active_jiffies = 0x0, suspended_jiffies = 0x0, accounting_timestamp = 0xfffd24fd, subsys_data = 0x0, qos = 0x0 }, pm_domain = 0x0, numa_node = 0xffffffff, dma_mask = 0x0, coherent_dma_mask = 0x0, dma_parms = 0x0, dma_pools = { next = 0xffff880851232840, prev = 0xffff880851232840 }, dma_mem = 0x0, archdata = { dma_ops = 0x0, iommu = 0x0 }, of_node = 0x0, acpi_node = { handle = 0x0 }, devt = 0x0, id = 0x0, devres_lock = { { rlock = { raw_lock = { { head_tail = 0x202, tickets = { head = 0x2, tail = 0x2 } } } } } }, devres_head = { next = 0xffff880851232888, prev = 0xffff880851232888 }, knode_class = { n_klist = 0x0, n_node = { next = 0x0, prev = 0x0 }, n_ref = { refcount = { counter = 0x0 } } }, class = 0x0, groups = 0x0, release = 0, iommu_group = 0x0 }, sdev_dev = { parent = 0xffff880851232658, p = 0xffff88084edab2c8, kobj = { name = 0xffff88084ec48a80 "kkkkkkk\245\273\273\273\273\273\273\273\273p\216\304N\b\210\377\377\261\222\060\201\377\377\377\377y\216e\201\377\377\377\377?\356\030\201\377\377\377\377{U1\201\377\377\377\377\261\222\060\201\377\377\377\377\241\221?\201\377\377\377\377\250\245B\201\377\377\377\377\256fB\201\377\377\377\377\247kB\201\377\377\377\377\201{B\201\377\377\377\377", entry = { next = 0xffff8808512328f0, prev = 0xffff8808512328f0 }, parent = 0x0, kset = 0xffff88085f878b98, ktype = 0xffffffff81c92240, sd = 0x0, kref = { refcount = { counter = 0x0 } }, state_initialized = 0x1, state_in_sysfs = 0x0, state_add_uevent_sent = 0x1, state_remove_uevent_sent = 0x1, uevent_suppress = 0x0 }, init_name = 0x0, type = 0x0, mutex = { count = { counter = 0x1 }, wait_lock = { { rlock = { raw_lock = { { head_tail = 0x0, tickets = { head = 0x0, tail = 0x0 } } } } } }, wait_list = { next = 0xffff880851232940, prev = 0xffff880851232940 }, owner = 0x0 }, bus = 0x0, driver = 0x0, platform_data = 0x0, power = { power_state = { event = 0xffffffff }, can_wakeup = 0x0, async_suspend = 0x1, is_prepared = 0x0, is_suspended = 0x0, ignore_children = 0x0, early_init = 0x1, lock = { { rlock = { raw_lock = { { head_tail = 0x101, tickets = { head = 0x1, tail = 0x1 } } } } } }, entry = { next = 0xffff880851232978, prev = 0xffff880851232978 }, completion = { done = 0xfffffffe, wait = { lock = { { rlock = { raw_lock = { { head_tail = 0x202, tickets = { head = 0x2, tail = 0x2 } } } } } }, task_list = { next = 0xffff880851232998, prev = 0xffff880851232998 } } }, wakeup = 0x0, wakeup_path = 0x0, syscore = 0x0, suspend_timer = { entry = { next = 0x0, prev = 0x0 }, expires = 0x0, base = 0xffff8808546f4000, function = 0xffffffff814092d0 <pm_suspend_timer_fn>, data = 0xffff8808512328d8, slack = 0xffffffff, start_pid = 0xffffffff, start_site = 0x0, start_comm = "\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000" }, timer_expires = 0x0, work = { data = { counter = 0x10200 }, entry = { next = 0xffff880851232a18, prev = 0xffff880851232a18 }, func = 0xffffffff8140a6b0 <pm_runtime_work> }, wait_queue = { lock = { { rlock = { raw_lock = { { head_tail = 0x0, tickets = { head = 0x0, tail = 0x0 } } } } } }, task_list = { next = 0xffff880851232a38, prev = 0xffff880851232a38 } }, usage_count = { counter = 0x0 }, child_count = { counter = 0x0 }, disable_depth = 0x2, idle_notification = 0x0, request_pending = 0x0, deferred_resume = 0x0, run_wake = 0x0, runtime_auto = 0x1, no_callbacks = 0x0, irq_safe = 0x0, use_autosuspend = 0x0, timer_autosuspends = 0x0, request = RPM_REQ_NONE, runtime_status = RPM_SUSPENDED, runtime_error = 0x0, autosuspend_delay = 0x0, last_busy = 0x0, active_jiffies = 0x0, suspended_jiffies = 0x0, accounting_timestamp = 0xfffb8e97, subsys_data = 0x0, qos = 0x0 }, pm_domain = 0x0, numa_node = 0xffffffff, dma_mask = 0x0, coherent_dma_mask = 0x0, dma_parms = 0x0, dma_pools = { next = 0xffff880851232ac0, prev = 0xffff880851232ac0 }, dma_mem = 0x0, archdata = { dma_ops = 0x0, iommu = 0x0 }, of_node = 0x0, acpi_node = { handle = 0x0 }, devt = 0x0, id = 0x0, devres_lock = { { rlock = { raw_lock = { { head_tail = 0x101, tickets = { head = 0x1, tail = 0x1 } } } } } }, devres_head = { next = 0xffff880851232b08, prev = 0xffff880851232b08 }, knode_class = { n_klist = 0x0, n_node = { next = 0xdead000000100100, prev = 0xdead000000200200 }, n_ref = { refcount = { counter = 0x0 } } }, class = 0xffffffff81c95a40, groups = 0x0, release = 0, iommu_group = 0x0 }, ew = { work = { data = { counter = 0x0 }, entry = { next = 0x0, prev = 0x0 }, func = 0 } }, requeue_work = { data = { counter = 0x10200 }, entry = { next = 0xffff880851232b80, prev = 0xffff880851232b80 }, func = 0xffffffff814257c0 <scsi_requeue_run_queue> }, scsi_dh_data = 0x0, sdev_state = SDEV_DEL, sdev_data = 0xffff880851232ba8 } ffff880851235388 struct scsi_device { host = 0xffff8808513a4290, request_queue = 0xffff88085107b690, siblings = { next = 0xffff8808513a4290, prev = 0xffff880851232530 }, same_target_siblings = { next = 0xffff88084ed99538, prev = 0xffff88084ed99538 }, device_busy = 0x0, list_lock = { { rlock = { raw_lock = { { head_tail = 0x3636, tickets = { head = 0x36, tail = 0x36 } } } } } }, cmd_list = { next = 0xffff8808512353c0, prev = 0xffff8808512353c0 }, starved_entry = { next = 0xdead000000100100, prev = 0xdead000000200200 }, current_cmnd = 0x0, queue_depth = 0xfe, max_queue_depth = 0xfe, last_queue_full_depth = 0x0, last_queue_full_count = 0x0, last_queue_full_time = 0x0, queue_ramp_up_period = 0x1d4c0, last_queue_ramp_up = 0x0, id = 0x1, lun = 0x0, channel = 0x0, manufacturer = 0x0, sector_size = 0x200, hostdata = 0x0, type = 0x0, scsi_level = 0x7, inq_periph_qual = 0x0, inquiry_len = 0x90, inquiry = 0xffff88084eda8410 "", vendor = 0xffff88084eda8418 "SEAGATE ST9146853SS 00026XM02L18", model = 0xffff88084eda8420 "ST9146853SS 00026XM02L18", rev = 0xffff88084eda8430 "00026XM02L18", current_tag = 0x0, sdev_target = 0xffff88084ed99520, sdev_bflags = 0x0, writeable = 0x1, removable = 0x0, changed = 0x0, busy = 0x0, lockable = 0x0, locked = 0x0, borken = 0x0, disconnect = 0x0, soft_reset = 0x0, sdtr = 0x0, wdtr = 0x0, ppr = 0x1, tagged_supported = 0x1, simple_tags = 0x0, ordered_tags = 0x0, was_reset = 0x0, expecting_cc_ua = 0x0, use_10_for_rw = 0x1, use_10_for_ms = 0x0, no_report_opcodes = 0x0, no_write_same = 0x0, use_16_for_rw = 0x0, skip_ms_page_8 = 0x0, skip_ms_page_3f = 0x0, skip_vpd_pages = 0x0, use_192_bytes_for_3f = 0x0, no_start_on_add = 0x0, allow_restart = 0x0, manage_start_stop = 0x0, start_stop_pwr_cond = 0x0, no_uld_attach = 0x0, select_no_atn = 0x0, fix_capacity = 0x0, guess_capacity = 0x0, retry_hwerror = 0x0, last_sector_bug = 0x0, no_read_disc_info = 0x0, no_read_capacity_16 = 0x0, try_rc_10_first = 0x0, is_visible = 0x1, can_power_off = 0x0, wce_default_on = 0x0, no_dif = 0x0, supported_events = {0x0}, event_list = { next = 0xffff880851235478, prev = 0xffff880851235478 }, event_work = { data = { counter = 0x10200 }, entry = { next = 0xffff880851235490, prev = 0xffff880851235490 }, func = 0xffffffff81426020 <scsi_evt_thread> }, device_blocked = 0x0, max_device_blocked = 0x3, iorequest_cnt = { counter = 0x21b }, iodone_cnt = { counter = 0x21b }, ioerr_cnt = { counter = 0x0 }, sdev_gendev = { parent = 0xffff88084ed99548, p = 0xffff88084eda8e38, kobj = { name = 0xffff88084eda0540 "0:0:1:0", entry = { next = 0xffff8808512354d8, prev = 0xffff8808512354d8 }, parent = 0x0, kset = 0xffff88085f878b98, ktype = 0xffffffff81c92240, sd = 0x0, kref = { refcount = { counter = 0x2 } }, state_initialized = 0x1, state_in_sysfs = 0x0, state_add_uevent_sent = 0x1, state_remove_uevent_sent = 0x1, uevent_suppress = 0x0 }, init_name = 0x0, type = 0xffffffff81c95ac0, mutex = { count = { counter = 0x1 }, wait_lock = { { rlock = { raw_lock = { { head_tail = 0x0, tickets = { head = 0x0, tail = 0x0 } } } } } }, wait_list = { next = 0xffff880851235528, prev = 0xffff880851235528 }, owner = 0x0 }, bus = 0xffffffff81c95960, driver = 0x0, platform_data = 0x0, power = { power_state = { event = 0xffffffff }, can_wakeup = 0x0, async_suspend = 0x1, is_prepared = 0x0, is_suspended = 0x0, ignore_children = 0x0, early_init = 0x1, lock = { { rlock = { raw_lock = { { head_tail = 0xc4c4, tickets = { head = 0xc4, tail = 0xc4 } } } } } }, entry = { next = 0xffff880851235560, prev = 0xffff880851235560 }, completion = { done = 0xfffffffe, wait = { lock = { { rlock = { raw_lock = { { head_tail = 0x202, tickets = { head = 0x2, tail = 0x2 } } } } } }, task_list = { next = 0xffff880851235580, prev = 0xffff880851235580 } } }, wakeup = 0x0, wakeup_path = 0x0, syscore = 0x0, suspend_timer = { entry = { next = 0x0, prev = 0x0 }, expires = 0x0, base = 0xffff8808546f4000, function = 0xffffffff814092d0 <pm_suspend_timer_fn>, data = 0xffff8808512354c0, slack = 0xffffffff, start_pid = 0xffffffff, start_site = 0x0, start_comm = "\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000" }, timer_expires = 0x0, work = { data = { counter = 0x10200 }, entry = { next = 0xffff880851235600, prev = 0xffff880851235600 }, func = 0xffffffff8140a6b0 <pm_runtime_work> }, wait_queue = { lock = { { rlock = { raw_lock = { { head_tail = 0x0, tickets = { head = 0x0, tail = 0x0 } } } } } }, task_list = { next = 0xffff880851235620, prev = 0xffff880851235620 } }, usage_count = { counter = 0x3 }, child_count = { counter = 0x0 }, disable_depth = 0x1, idle_notification = 0x0, request_pending = 0x0, deferred_resume = 0x0, run_wake = 0x0, runtime_auto = 0x0, no_callbacks = 0x0, irq_safe = 0x0, use_autosuspend = 0x0, timer_autosuspends = 0x0, request = RPM_REQ_NONE, runtime_status = RPM_SUSPENDED, runtime_error = 0x0, autosuspend_delay = 0x0, last_busy = 0x0, active_jiffies = 0x0, suspended_jiffies = 0x0, accounting_timestamp = 0xfffd2515, subsys_data = 0x0, qos = 0x0 }, pm_domain = 0x0, numa_node = 0xffffffff, dma_mask = 0x0, coherent_dma_mask = 0x0, dma_parms = 0x0, dma_pools = { next = 0xffff8808512356a8, prev = 0xffff8808512356a8 }, dma_mem = 0x0, archdata = { dma_ops = 0x0, iommu = 0x0 }, of_node = 0x0, acpi_node = { handle = 0x0 }, devt = 0x0, id = 0x0, devres_lock = { { rlock = { raw_lock = { { head_tail = 0x202, tickets = { head = 0x2, tail = 0x2 } } } } } }, devres_head = { next = 0xffff8808512356f0, prev = 0xffff8808512356f0 }, knode_class = { n_klist = 0x0, n_node = { next = 0x0, prev = 0x0 }, n_ref = { refcount = { counter = 0x0 } } }, class = 0x0, groups = 0x0, release = 0, iommu_group = 0x0 }, sdev_dev = { parent = 0xffff8808512354c0, p = 0xffff88084eda9248, kobj = { name = 0xffff88084eda3870 "kkkkkkk\245\273\273\273\273\273\273\273\273\060\063\332N\b\210\377\377\261\222\060\201\377\377\377\377y\216e\201\377\377\377\377?\356\030\201\377\377\377\377{U1\201\377\377\377\377\261\222\060\201\377\377\377\377\241\221?\201\377\377\377\377\250\245B\201\377\377\377\377\256fB\201\377\377\377\377\247kB\201\377\377\377\377\201{B\201\377\377\377\377", entry = { next = 0xffff880851235758, prev = 0xffff880851235758 }, parent = 0x0, kset = 0xffff88085f878b98, ktype = 0xffffffff81c92240, sd = 0x0, kref = { refcount = { counter = 0x0 } }, state_initialized = 0x1, state_in_sysfs = 0x0, state_add_uevent_sent = 0x1, state_remove_uevent_sent = 0x1, uevent_suppress = 0x0 }, init_name = 0x0, type = 0x0, mutex = { count = { counter = 0x1 }, wait_lock = { { rlock = { raw_lock = { { head_tail = 0x0, tickets = { head = 0x0, tail = 0x0 } } } } } }, wait_list = { next = 0xffff8808512357a8, prev = 0xffff8808512357a8 }, owner = 0x0 }, bus = 0x0, driver = 0x0, platform_data = 0x0, power = { power_state = { event = 0xffffffff }, can_wakeup = 0x0, async_suspend = 0x1, is_prepared = 0x0, is_suspended = 0x0, ignore_children = 0x0, early_init = 0x1, lock = { { rlock = { raw_lock = { { head_tail = 0x101, tickets = { head = 0x1, tail = 0x1 } } } } } }, entry = { next = 0xffff8808512357e0, prev = 0xffff8808512357e0 }, completion = { done = 0xfffffffe, wait = { lock = { { rlock = { raw_lock = { { head_tail = 0x202, tickets = { head = 0x2, tail = 0x2 } } } } } }, task_list = { next = 0xffff880851235800, prev = 0xffff880851235800 } } }, wakeup = 0x0, wakeup_path = 0x0, syscore = 0x0, suspend_timer = { entry = { next = 0x0, prev = 0x0 }, expires = 0x0, base = 0xffff8808546f4000, function = 0xffffffff814092d0 <pm_suspend_timer_fn>, data = 0xffff880851235740, slack = 0xffffffff, start_pid = 0xffffffff, start_site = 0x0, start_comm = "\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000" }, timer_expires = 0x0, work = { data = { counter = 0x10200 }, entry = { next = 0xffff880851235880, prev = 0xffff880851235880 }, func = 0xffffffff8140a6b0 <pm_runtime_work> }, wait_queue = { lock = { { rlock = { raw_lock = { { head_tail = 0x0, tickets = { head = 0x0, tail = 0x0 } } } } } }, task_list = { next = 0xffff8808512358a0, prev = 0xffff8808512358a0 } }, usage_count = { counter = 0x0 }, child_count = { counter = 0x0 }, disable_depth = 0x2, idle_notification = 0x0, request_pending = 0x0, deferred_resume = 0x0, run_wake = 0x0, runtime_auto = 0x1, no_callbacks = 0x0, irq_safe = 0x0, use_autosuspend = 0x0, timer_autosuspends = 0x0, request = RPM_REQ_NONE, runtime_status = RPM_SUSPENDED, runtime_error = 0x0, autosuspend_delay = 0x0, last_busy = 0x0, active_jiffies = 0x0, suspended_jiffies = 0x0, accounting_timestamp = 0xfffb8e9a, subsys_data = 0x0, qos = 0x0 }, pm_domain = 0x0, numa_node = 0xffffffff, dma_mask = 0x0, coherent_dma_mask = 0x0, dma_parms = 0x0, dma_pools = { next = 0xffff880851235928, prev = 0xffff880851235928 }, dma_mem = 0x0, archdata = { dma_ops = 0x0, iommu = 0x0 }, of_node = 0x0, acpi_node = { handle = 0x0 }, devt = 0x0, id = 0x0, devres_lock = { { rlock = { raw_lock = { { head_tail = 0x101, tickets = { head = 0x1, tail = 0x1 } } } } } }, devres_head = { next = 0xffff880851235970, prev = 0xffff880851235970 }, knode_class = { n_klist = 0x0, n_node = { next = 0xdead000000100100, prev = 0xdead000000200200 }, n_ref = { refcount = { counter = 0x0 } } }, class = 0xffffffff81c95a40, groups = 0x0, release = 0, iommu_group = 0x0 }, ew = { work = { data = { counter = 0x0 }, entry = { next = 0x0, prev = 0x0 }, func = 0 } }, requeue_work = { data = { counter = 0x10200 }, entry = { next = 0xffff8808512359e8, prev = 0xffff8808512359e8 }, func = 0xffffffff814257c0 <scsi_requeue_run_queue> }, scsi_dh_data = 0x0, sdev_state = SDEV_DEL, sdev_data = 0xffff880851235a10 } ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v8 0/10] More device removal fixes 2013-02-06 22:31 ` [PATCH v8 0/10] More device removal fixes Joe Lawrence @ 2013-02-07 10:40 ` Bart Van Assche 2013-02-07 11:33 ` Bart Van Assche 1 sibling, 0 replies; 17+ messages in thread From: Bart Van Assche @ 2013-02-07 10:40 UTC (permalink / raw) To: Joe Lawrence Cc: linux-scsi, James Bottomley, Mike Christie, Tejun Heo, Chanho Min, David Milburn On 02/06/13 23:31, Joe Lawrence wrote: > On Tue, 5 Feb 2013, Bart Van Assche wrote: >> 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. >> - Save and restore the host_scribble field during error handling >> >> These patches have been tested on top of kernel v3.8-rc6 and are also >> available here: http://github.com/bvanassche/linux/tree/device-removal-fixes. > > Hi Bart, > > I gave your patchset a spin on our ftServer, which supports hotplug of PCI > devices. Before attempting a surprise HW removal of an LSI Fusion SAS-2 > SCSI controller, I issued an orderly removal through our hotplug driver. > The device removal appeared to hang (this does not occur with vanilla > 3.8.0-rc6). Hello Joe, Thanks for running this test. I have had a quick look at the mpt2sas _scsih_remove() implementation but I have not yet had the time to analyze that function entirely. My proposal is to defer further analysis of the interaction between the two patches that make scsi_remove_host() wait and the mpt2sas driver until a later time. It would be appreciated if you could run the following test: * Revert the two patches that make scsi_remove_host() wait (these two patches should revert cleanly). * Repeat your device removal test. If that test runs fine, I will retest and repost this patch series without the two patches that make scsi_remove_host() wait. Thanks, Bart. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v8 0/10] More device removal fixes 2013-02-06 22:31 ` [PATCH v8 0/10] More device removal fixes Joe Lawrence 2013-02-07 10:40 ` Bart Van Assche @ 2013-02-07 11:33 ` Bart Van Assche 2013-02-08 23:29 ` Joe Lawrence 1 sibling, 1 reply; 17+ messages in thread From: Bart Van Assche @ 2013-02-07 11:33 UTC (permalink / raw) To: Joe Lawrence Cc: linux-scsi, James Bottomley, Mike Christie, Tejun Heo, Chanho Min, David Milburn On 02/06/13 23:31, Joe Lawrence wrote: > crash> list scsi_device.siblings -H 0xffff8808513a4290 -s scsi_device > > ffff880851232520 > struct scsi_device { > is_visible = 0x1, > sdev_state = SDEV_DEL, > } > ffff880851235388 > struct scsi_device { > is_visible = 0x1, > sdev_state = SDEV_DEL, > } This is interesting. This probably means that one or more threads got stuck in __scsi_remove_device(). If you still have the crash dump available it would be appreciated if you could verify whether this is correct. If so, there might be an issue in the mpt2sas driver where scsi_done() does not get invoked for all outstanding commands after a surprise removal. Bart. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v8 0/10] More device removal fixes 2013-02-07 11:33 ` Bart Van Assche @ 2013-02-08 23:29 ` Joe Lawrence 2013-02-09 9:28 ` Bart Van Assche 0 siblings, 1 reply; 17+ messages in thread From: Joe Lawrence @ 2013-02-08 23:29 UTC (permalink / raw) To: Bart Van Assche Cc: Joe Lawrence, linux-scsi, James Bottomley, Mike Christie, Tejun Heo, Chanho Min, David Milburn On Thu, 7 Feb 2013, Bart Van Assche wrote: > On 02/06/13 23:31, Joe Lawrence wrote: > > crash> list scsi_device.siblings -H 0xffff8808513a4290 -s scsi_device > > > > ffff880851232520 > > struct scsi_device { > > is_visible = 0x1, > > sdev_state = SDEV_DEL, > > } > > ffff880851235388 > > struct scsi_device { > > is_visible = 0x1, > > sdev_state = SDEV_DEL, > > } > > This is interesting. This probably means that one or more threads got stuck in > __scsi_remove_device(). If you still have the crash dump available it would be > appreciated if you could verify whether this is correct. If so, there might be > an issue in the mpt2sas driver where scsi_done() does not get invoked for all > outstanding commands after a surprise removal. Hi Bart, I haven't had time to rerun the test without the two patches that wait in scsi_remove_host(), however I did rerun the test and verify the same behavior as in my earlier mail. I didn't see any __scsi_remove_device() instances running. Some more investigation revealed that MD RAID was holding a reference to the removed device. (In short, mdadm --remove had failed and left the device as a faulty member of the array.) When I did finally manage to kick that disk from the MD device, scsi host/device removal continued to completion as expected. There's a bit more context to the MD situation that I'll post to the raid list once I get the details together for Neil. I will CC you if you are interested in following. Regards, -- Joe ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v8 0/10] More device removal fixes 2013-02-08 23:29 ` Joe Lawrence @ 2013-02-09 9:28 ` Bart Van Assche 2013-02-11 20:36 ` Bart Van Assche 0 siblings, 1 reply; 17+ messages in thread From: Bart Van Assche @ 2013-02-09 9:28 UTC (permalink / raw) To: Joe Lawrence Cc: linux-scsi, James Bottomley, Mike Christie, Tejun Heo, Chanho Min, David Milburn On 02/09/13 00:29, Joe Lawrence wrote: > I haven't had time to rerun the test without the two patches that wait in > scsi_remove_host(), however I did rerun the test and verify the same > behavior as in my earlier mail. I didn't see any __scsi_remove_device() > instances running. > > Some more investigation revealed that MD RAID was holding a reference to > the removed device. (In short, mdadm --remove had failed and left the > device as a faulty member of the array.) When I did finally manage to > kick that disk from the MD device, scsi host/device removal continued to > completion as expected. > > There's a bit more context to the MD situation that I'll post to the raid > list once I get the details together for Neil. I will CC you if you are > interested in following. The loop in scsi_remove_host() waits too long. It should stop waiting as soon as the blk_cleanup_queue() calls for all sdev's have finished instead of waiting until all sdev users have closed these sdev's. I will repost patches 07/10 and 08/10. Bart. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v8 0/10] More device removal fixes 2013-02-09 9:28 ` Bart Van Assche @ 2013-02-11 20:36 ` Bart Van Assche 0 siblings, 0 replies; 17+ messages in thread From: Bart Van Assche @ 2013-02-11 20:36 UTC (permalink / raw) Cc: Joe Lawrence, linux-scsi, James Bottomley, Mike Christie, Tejun Heo, Chanho Min, David Milburn On 02/09/13 10:28, Bart Van Assche wrote: > On 02/09/13 00:29, Joe Lawrence wrote: >> I haven't had time to rerun the test without the two patches that wait in >> scsi_remove_host(), however I did rerun the test and verify the same >> behavior as in my earlier mail. I didn't see any __scsi_remove_device() >> instances running. >> >> Some more investigation revealed that MD RAID was holding a reference to >> the removed device. (In short, mdadm --remove had failed and left the >> device as a faulty member of the array.) When I did finally manage to >> kick that disk from the MD device, scsi host/device removal continued to >> completion as expected. >> >> There's a bit more context to the MD situation that I'll post to the raid >> list once I get the details together for Neil. I will CC you if you are >> interested in following. > > The loop in scsi_remove_host() waits too long. It should stop waiting as > soon as the blk_cleanup_queue() calls for all sdev's have finished > instead of waiting until all sdev users have closed these sdev's. I will > repost patches 07/10 and 08/10. (replying to my own e-mail) Hello Joe, It would be appreciated if you could repeat your test with this kernel tree (tested with iSCSI and SRP): http://github.com/bvanassche/linux/tree/device-removal-fixes. If that test succeed I will repost the patches in that tree. Thanks, Bart. ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2013-02-11 20:36 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-02-05 12:44 [PATCH v8 0/10] More device removal fixes Bart Van Assche 2013-02-05 12:45 ` [PATCH v8 01/10] Fix race between starved list processing and device removal Bart Van Assche 2013-02-05 12:46 ` [PATCH v8 02/10] Remove get_device() / put_device() pair from scsi_request_fn() Bart Van Assche 2013-02-05 12:47 ` [PATCH v8 03/10] Introduce scsi_device_being_removed() Bart Van Assche 2013-02-05 12:47 ` [PATCH v8 04/10] Remove offline devices when removing a host Bart Van Assche 2013-02-05 12:48 ` [PATCH v8 05/10] Disallow changing the device state via sysfs into "deleted" Bart Van Assche 2013-02-05 12:49 ` [PATCH v8 06/10] Avoid saving/restoring interrupt state inside scsi_remove_host() Bart Van Assche 2013-02-05 12:49 ` [PATCH v8 07/10] Make scsi_remove_host() wait for device removal Bart Van Assche 2013-02-05 12:50 ` [PATCH v8 08/10] Make scsi_remove_host() wait until error handling finished Bart Van Assche 2013-02-05 12:51 ` [PATCH v8 09/10] Avoid that scsi_device_set_state() triggers a race Bart Van Assche 2013-02-05 12:51 ` [PATCH v8 10/10] Save and restore host_scribble during error handling Bart Van Assche 2013-02-06 22:31 ` [PATCH v8 0/10] More device removal fixes Joe Lawrence 2013-02-07 10:40 ` Bart Van Assche 2013-02-07 11:33 ` Bart Van Assche 2013-02-08 23:29 ` Joe Lawrence 2013-02-09 9:28 ` Bart Van Assche 2013-02-11 20:36 ` Bart Van Assche
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).