On Tue, 2017-03-14 at 16:23 +0200, Israel Rukshin wrote:
> Patch number 5 doesn't handle the case when device_for_each_child() is
> called. device_for_each_child() calls to target_unblock() that uses also
> starget_for_each_device(). After applying also the following change the
> hang disappeared but it didn't fix the warning. Those fixes are not enough
> because if fast_io_fail_tmo is set to infinity then the hang will remain,
> because only __rport_fail_io_fast() calls to scsi_target_unblock() and
> terminate_rport_io() that free the sync cache command.
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index e5d4b50..09f9566 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -3068,9 +3068,15 @@ void scsi_device_resume(struct scsi_device *sdev)
> static int
> target_unblock(struct device *dev, void *data)
> {
> - if (scsi_is_target_device(dev))
> - starget_for_each_device(to_scsi_target(dev), data,
> - device_unblock);
> + if (scsi_is_target_device(dev)) {
> + struct scsi_target *starget = to_scsi_target(dev);
> + struct Scsi_Host *shost = dev_to_shost(dev->parent);
> + unsigned long flags;
> +
> + spin_lock_irqsave(shost->host_lock, flags);
> + __starget_for_each_device(starget, data, device_unblock);
> + spin_unlock_irqrestore(shost->host_lock, flags);
> + }
> return 0;
> }
Hello Israel,
Regarding setting fast_io_fail_tmo to infinity: that does not prevent
kernel module unloading because srp_timed_out() stops resetting the
timer as soon as the SCSI device is unblocked. The above patch should
realize that but suffers from the same issue as a patch attached to
my previous e-mail, namely lock inversion. For at least the following
call chain the block layer queue lock is the outer lock and the SCSI
host lock is the inner lock:
ata_qc_schedule_eh()
-> blk_abort_request()
-> blk_mq_rq_timed_out()
-> scsi_timeout()
-> scsi_times_out()
-> scsi_eh_scmd_add()
So I think we should avoid introducing code with the SCSI host lock
as outer lock and the block layer queue lock as inner lock. How about
the attached four patches?
Thanks,
Bart.From 458959938476788710738039a7c195e9c48ff338 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanass...@sandisk.com>
Date: Mon, 13 Mar 2017 10:06:13 -0700
Subject: [PATCH 1/4] Warn if __scsi_remove_device() is called for a stopped
queue
Calling __scsi_remove_device() for a stopped queue is a bug because
the device_del() call can trigger I/O and will trigger e.g. the
following hang:
Call Trace:
[<ffffffff815dd985>] schedule+0x35/0x80
[<ffffffff815e02c7>] schedule_timeout+0x237/0x2d0
[<ffffffff815dcf46>] io_schedule_timeout+0xa6/0x110
[<ffffffff815de2f3>] wait_for_completion_io+0xa3/0x110
[<ffffffff812e66ff>] blk_execute_rq+0xdf/0x120
[<ffffffffa00135de>] scsi_execute+0xce/0x150 [scsi_mod]
[<ffffffffa001548f>] scsi_execute_req_flags+0x8f/0xf0 [scsi_mod]
[<ffffffffa0154849>] sd_sync_cache+0xa9/0x190 [sd_mod]
[<ffffffffa0154c3a>] sd_shutdown+0x6a/0x100 [sd_mod]
[<ffffffffa0154d34>] sd_remove+0x64/0xc0 [sd_mod]
[<ffffffff8144d3fd>] __device_release_driver+0x8d/0x120
[<ffffffff8144d4ae>] device_release_driver+0x1e/0x30
[<ffffffff8144c239>] bus_remove_device+0xf9/0x170
[<ffffffff81448bc7>] device_del+0x127/0x240
[<ffffffffa001c0f1>] __scsi_remove_device+0xc1/0xd0 [scsi_mod]
[<ffffffffa001a5d7>] scsi_forget_host+0x57/0x60 [scsi_mod]
[<ffffffffa000e3d2>] scsi_remove_host+0x72/0x110 [scsi_mod]
[<ffffffffa06f95ab>] srp_remove_work+0x8b/0x200 [ib_srp]
Reported-by: Israel Rukshin <isra...@mellanox.com>
Signed-off-by: Bart Van Assche <bart.vanass...@sandisk.com>
---
drivers/scsi/scsi_sysfs.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 82dfe07b1d47..bbe7efd144b2 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1274,6 +1274,12 @@ void __scsi_remove_device(struct scsi_device *sdev)
struct device *dev = &sdev->sdev_gendev;
/*
+ * Calling __scsi_remove_device() for a stopped queue is a bug because
+ * the device_del() call can trigger I/O. See also sd_remove().
+ */
+ WARN_ON_ONCE(blk_queue_stopped(sdev->request_queue));
+
+ /*
* This cleanup path is not reentrant and while it is impossible
* to get a new reference with scsi_device_get() someone can still
* hold a previously acquired one.
--
2.12.0
From b86b5087698c73f5fdd8cc9fa18ed3f1e9e174bb Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanass...@sandisk.com>
Date: Wed, 15 Mar 2017 15:12:43 -0700
Subject: [PATCH 2/4] __scsi_iterate_devices(): Make the get and put functions
arguments
This patch does not change any functionality.
Signed-off-by: Bart Van Assche <bart.vanass...@sandisk.com>
Cc: <sta...@vger.kernel.org>
---
drivers/scsi/scsi.c | 8 +++++---
include/scsi/scsi_device.h | 16 +++++++++++-----
2 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index b3bb49d06943..45c266009f20 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -609,7 +609,9 @@ EXPORT_SYMBOL(scsi_device_put);
/* helper for shost_for_each_device, see that for documentation */
struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *shost,
- struct scsi_device *prev)
+ struct scsi_device *prev,
+ int (*get)(struct scsi_device *),
+ void (*put)(struct scsi_device *))
{
struct list_head *list = (prev ? &prev->siblings : &shost->__devices);
struct scsi_device *next = NULL;
@@ -619,7 +621,7 @@ struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *shost,
while (list->next != &shost->__devices) {
next = list_entry(list->next, struct scsi_device, siblings);
/* skip devices that we can't get a reference to */
- if (!scsi_device_get(next))
+ if (!get(next))
break;
next = NULL;
list = list->next;
@@ -627,7 +629,7 @@ struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *shost,
spin_unlock_irqrestore(shost->host_lock, flags);
if (prev)
- scsi_device_put(prev);
+ put(prev);
return next;
}
EXPORT_SYMBOL(__scsi_iterate_devices);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index cda620ed5922..812dd1cdfb6f 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -335,7 +335,9 @@ extern void __starget_for_each_device(struct scsi_target *, void *,
/* only exposed to implement shost_for_each_device */
extern struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *,
- struct scsi_device *);
+ struct scsi_device *,
+ int (*get)(struct scsi_device *),
+ void (*put)(struct scsi_device *));
/**
* shost_for_each_device - iterate over all devices of a host
@@ -346,10 +348,14 @@ extern struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *,
* takes a reference on each device and releases it at the end. If
* you break out of the loop, you must call scsi_device_put(sdev).
*/
-#define shost_for_each_device(sdev, shost) \
- for ((sdev) = __scsi_iterate_devices((shost), NULL); \
- (sdev); \
- (sdev) = __scsi_iterate_devices((shost), (sdev)))
+#define shost_for_each_device(sdev, shost) \
+ for ((sdev) = __scsi_iterate_devices((shost), NULL, \
+ scsi_device_get, \
+ scsi_device_put); \
+ (sdev); \
+ (sdev) = __scsi_iterate_devices((shost), (sdev), \
+ scsi_device_get, \
+ scsi_device_put))
/**
* __shost_for_each_device - iterate over all devices of a host (UNLOCKED)
--
2.12.0
From 91921ac966f52aef8236fdf657cbbcb31aa2381c Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanass...@sandisk.com>
Date: Wed, 15 Mar 2017 15:13:20 -0700
Subject: [PATCH 3/4] Introduce starget_for_all_devices() and
shost_for_all_devices()
This patch does not change any functionality.
Signed-off-by: Bart Van Assche <bart.vanass...@sandisk.com>
Cc: <sta...@vger.kernel.org>
---
drivers/scsi/scsi.c | 52 +++++++++++++++++++++++++++++++++++++++++++---
include/scsi/scsi_device.h | 28 ++++++++++++++++++-------
2 files changed, 70 insertions(+), 10 deletions(-)
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 45c266009f20..2aeaebdd50bc 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -640,9 +640,11 @@ EXPORT_SYMBOL(__scsi_iterate_devices);
* @data: Opaque passed to each function call.
* @fn: Function to call on each device
*
- * This traverses over each device of @starget. The devices have
- * a reference that must be released by scsi_host_put when breaking
- * out of the loop.
+ * This traverses over each device of @starget except the devices that are in
+ * state SDEV_DEL or SDEV_CANCEL. The devices have a reference that must be
+ * released by scsi_device_put() when breaking out of the loop. If the LLD
+ * associated with the devices is being unloaded, @fn is not called for any
+ * device.
*/
void starget_for_each_device(struct scsi_target *starget, void *data,
void (*fn)(struct scsi_device *, void *))
@@ -659,6 +661,50 @@ void starget_for_each_device(struct scsi_target *starget, void *data,
EXPORT_SYMBOL(starget_for_each_device);
/**
+ * scsi_device_get_any() - get a reference to @sdev even if it is being deleted
+ *
+ * See also scsi_device_get().
+ */
+static int scsi_device_get_any(struct scsi_device *sdev)
+{
+ return get_device(&sdev->sdev_gendev) ? 0 : -ENXIO;
+}
+
+/**
+ * scsi_device_put_any() - drop a reference obtained by scsi_device_get_any()
+ *
+ * See also scsi_device_put().
+ */
+static void scsi_device_put_any(struct scsi_device *sdev)
+{
+ put_device(&sdev->sdev_gendev);
+}
+
+/**
+ * starget_for_all_devices - helper to walk all devices of a target
+ * @starget: target whose devices we want to iterate over.
+ * @data: Pointer passed to each function call.
+ * @fn: Function to call on each device
+ *
+ * This traverses over each device of @starget, including the devices in state
+ * SDEV_DEL or SDEV_ANY. The devices have a reference that must be released by
+ * scsi_device_put_any() when breaking out of the loop.
+ */
+void starget_for_all_devices(struct scsi_target *starget, void *data,
+ void (*fn)(struct scsi_device *, void *))
+{
+ struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
+ struct scsi_device *sdev;
+
+ shost_for_all_devices(sdev, shost, scsi_device_get_any,
+ scsi_device_put_any)
+ if (sdev->channel == starget->channel &&
+ sdev->id == starget->id)
+ fn(sdev, data);
+}
+EXPORT_SYMBOL(starget_for_all_devices);
+
+/**
* __starget_for_each_device - helper to walk all devices of a target (UNLOCKED)
* @starget: target whose devices we want to iterate over.
* @data: parameter for callback @fn()
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 812dd1cdfb6f..057d7376dafc 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -329,6 +329,8 @@ extern struct scsi_device *__scsi_device_lookup_by_target(struct scsi_target *,
u64);
extern void starget_for_each_device(struct scsi_target *, void *,
void (*fn)(struct scsi_device *, void *));
+extern void starget_for_all_devices(struct scsi_target *, void *,
+ void (*fn)(struct scsi_device *, void *));
extern void __starget_for_each_device(struct scsi_target *, void *,
void (*fn)(struct scsi_device *,
void *));
@@ -340,6 +342,23 @@ extern struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *,
void (*put)(struct scsi_device *));
/**
+ * shost_for_all_devices - iterate over all devices of a host
+ * @sdev: the &struct scsi_device to use as a cursor
+ * @shost: the &struct scsi_host to iterate over
+ * @get: function that obtains a reference to a device and returns 0 upon
+ * success
+ * @put: function that drops a device reference.
+ *
+ * Iterator that returns each device attached to @shost. This loop
+ * takes a reference on each device and releases it at the end. If
+ * you break out of the loop, you must call @put(sdev).
+ */
+#define shost_for_all_devices(sdev, shost, get, put) \
+ for ((sdev) = NULL; \
+ ((sdev) = __scsi_iterate_devices((shost), (sdev), \
+ (get), (put))) != NULL; )
+
+/**
* shost_for_each_device - iterate over all devices of a host
* @sdev: the &struct scsi_device to use as a cursor
* @shost: the &struct scsi_host to iterate over
@@ -349,13 +368,8 @@ extern struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *,
* you break out of the loop, you must call scsi_device_put(sdev).
*/
#define shost_for_each_device(sdev, shost) \
- for ((sdev) = __scsi_iterate_devices((shost), NULL, \
- scsi_device_get, \
- scsi_device_put); \
- (sdev); \
- (sdev) = __scsi_iterate_devices((shost), (sdev), \
- scsi_device_get, \
- scsi_device_put))
+ shost_for_all_devices((sdev), (shost), scsi_device_get, \
+ scsi_device_put)
/**
* __shost_for_each_device - iterate over all devices of a host (UNLOCKED)
--
2.12.0
From 44959a030c1538a38be3a7f2ef0f7488368b2568 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanass...@sandisk.com>
Date: Mon, 13 Mar 2017 10:46:08 -0700
Subject: [PATCH 4/4] Ensure that scsi_target_unblock() examines all devices
Since scsi_target_unblock() uses starget_for_each_device(), since
starget_for_each_device() uses scsi_device_get(), since
scsi_device_get() fails after unloading of the LLD kernel module
has been started scsi_target_unblock() may skip devices that were
affected by scsi_target_block(). Ensure that scsi_target_block()
examines all SCSI devices. This patch avoids that unloading the
ib_srp kernel module can trigger the following hang:
Call Trace:
schedule+0x35/0x80
schedule_timeout+0x237/0x2d0
io_schedule_timeout+0xa6/0x110
wait_for_completion_io+0xa3/0x110
blk_execute_rq+0xdf/0x120
scsi_execute+0xce/0x150 [scsi_mod]
scsi_execute_req_flags+0x8f/0xf0 [scsi_mod]
sd_sync_cache+0xa9/0x190 [sd_mod]
sd_shutdown+0x6a/0x100 [sd_mod]
sd_remove+0x64/0xc0 [sd_mod]
__device_release_driver+0x8d/0x120
device_release_driver+0x1e/0x30
bus_remove_device+0xf9/0x170
device_del+0x127/0x240
__scsi_remove_device+0xc1/0xd0 [scsi_mod]
scsi_forget_host+0x57/0x60 [scsi_mod]
scsi_remove_host+0x72/0x110 [scsi_mod]
srp_remove_work+0x8b/0x200 [ib_srp]
Reported-by: Israel Rukshin <isra...@mellanox.com>
Signed-off-by: Bart Van Assche <bart.vanass...@sandisk.com>
Cc: <sta...@vger.kernel.org>
---
drivers/scsi/scsi_lib.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ba2286652ff6..991763fda0af 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3065,21 +3065,37 @@ device_unblock(struct scsi_device *sdev, void *data)
scsi_internal_device_unblock(sdev, *(enum scsi_device_state *)data);
}
+/**
+ * target_unblock() - unblock all devices associated with a SCSI target
+ *
+ * Notes:
+ * - Do not use scsi_device_get() nor any of the macros that use this
+ * function from inside scsi_target_block() because otherwise this function
+ * won't have any effect when called while the SCSI LLD is being unloaded.
+ * - Do not hold the host lock around the device_unblock() calls because at
+ * least for blk-sq the block layer queue lock is the outer lock and the
+ * SCSI host lock is the inner lock.
+ */
static int
target_unblock(struct device *dev, void *data)
{
if (scsi_is_target_device(dev))
- starget_for_each_device(to_scsi_target(dev), data,
+ starget_for_all_devices(to_scsi_target(dev), data,
device_unblock);
return 0;
}
+/**
+ * scsi_target_unblock() - unblock all devices associated with a SCSI target
+ * @dev: Either a pointer to the dev member of struct scsi_target or a pointer
+ * to the shost_gendev member of struct Scsi_Host.
+ * @new_state: New SCSI device state.
+ */
void
scsi_target_unblock(struct device *dev, enum scsi_device_state new_state)
{
if (scsi_is_target_device(dev))
- starget_for_each_device(to_scsi_target(dev), &new_state,
- device_unblock);
+ target_unblock(dev, &new_state);
else
device_for_each_child(dev, &new_state, target_unblock);
}
--
2.12.0
