Re: [PATCH 0/3] SCSI: Fix hard lockup in scsi_remove_target()
On Wed, 2015-10-14 at 11:18 -0700, Christoph Hellwig wrote:
> On Wed, Oct 14, 2015 at 08:45:56AM -0700, James Bottomley wrote:
> > OK, so I really need you to separate the problems. Fixing the bug
[..]
>
> Johannes, can you test the patch below?
I've tested your patch and it doesn't show the lockup anymore, so far
so good. But it seems as if I have a problem in my test setup, because
I can't reproduce the bug on vanilla 4.3-rc5 either. I will ask the
original reporter if it is possible to test your patch on their side.
Appart from that it looks good to me (and much simpler than my changes)
.
>
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index b89f..d3b34d8 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -1158,31 +1158,23 @@ static void __scsi_remove_target(struct
> scsi_target *starget)
> void scsi_remove_target(struct device *dev)
> {
> struct Scsi_Host *shost = dev_to_shost(dev->parent);
> - struct scsi_target *starget, *last = NULL;
> + struct scsi_target *starget;
> unsigned long flags;
>
> - /* remove targets being careful to lookup next entry before
> - * deleting the last
> - */
> +restart:
> spin_lock_irqsave(shost->host_lock, flags);
> list_for_each_entry(starget, &shost->__targets, siblings) {
> if (starget->state == STARGET_DEL)
> continue;
> if (starget->dev.parent == dev || &starget->dev ==
> dev) {
> - /* assuming new targets arrive at the end */
> kref_get(&starget->reap_ref);
> spin_unlock_irqrestore(shost->host_lock,
> flags);
> - if (last)
> - scsi_target_reap(last);
> - last = starget;
> __scsi_remove_target(starget);
> - spin_lock_irqsave(shost->host_lock, flags);
> + scsi_target_reap(starget);
> + goto restart;
> }
> }
> spin_unlock_irqrestore(shost->host_lock, flags);
> -
> - if (last)
> - scsi_target_reap(last);
> }
> EXPORT_SYMBOL(scsi_remove_target);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi"
> in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] SCSI: Fix hard lockup in scsi_remove_target()
On Wed, 2015-10-14 at 16:22 -0400, Ewan Milne wrote:
> On Wed, 2015-10-14 at 07:30 -0700, James Bottomley wrote:
> > On Wed, 2015-10-14 at 15:50 +0200, Johannes Thumshirn wrote:
> > > Removing a SCSI target via scsi_remove_target() suspected to be
> > > racy. When a
> > > sibling get's removed from the list it can occassionly happen
> > > that one CPU is
> > > stuck endlessly looping around this code block
> > >
> > > list_for_each_entry(starget, &shost->__targets, siblings) {
> > > if (starget->state == STARGET_DEL)
> > > continue;
> >
> > How long is the __targets list? It seems a bit unlikely that this
> > is
> > the exact cause, because for a short list all in STARGET_DEL that
> > loop
> > should exit very quickly. Where in the code does
> > scsi_remove_target
> > +0x68/0x240 actually point to?
> >
> > Is it not a bit more likely that we're following a removed list
> > element?
> > Since that points back to itself, the list_for_each_entry() would
> > then
> > circulate forever. If that's the case the simple fix would be to
> > use
> > the safe version of the list traversal macro.
> >
> > James
>
> For what it's worth, I've seen a dump where this was exactly the
> case.
> starget was in STARGET_DEL state, starget->siblings pointed to
> itself,
> kref was 0, reap_ref was 0 (this was a while back).
>
That's exactly what I have here as well.
I'll give Christoph's patch a shot today and report back.
> The problem was not able to be reproduced at the time.
>
> -Ewan
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] SCSI: Fix hard lockup in scsi_remove_target()
On Wed, 2015-10-14 at 07:30 -0700, James Bottomley wrote:
> On Wed, 2015-10-14 at 15:50 +0200, Johannes Thumshirn wrote:
> > Removing a SCSI target via scsi_remove_target() suspected to be racy. When a
> > sibling get's removed from the list it can occassionly happen that one CPU
> > is
> > stuck endlessly looping around this code block
> >
> > list_for_each_entry(starget, &shost->__targets, siblings) {
> > if (starget->state == STARGET_DEL)
> > continue;
>
> How long is the __targets list? It seems a bit unlikely that this is
> the exact cause, because for a short list all in STARGET_DEL that loop
> should exit very quickly. Where in the code does scsi_remove_target
> +0x68/0x240 actually point to?
>
> Is it not a bit more likely that we're following a removed list element?
> Since that points back to itself, the list_for_each_entry() would then
> circulate forever. If that's the case the simple fix would be to use
> the safe version of the list traversal macro.
>
> James
For what it's worth, I've seen a dump where this was exactly the case.
starget was in STARGET_DEL state, starget->siblings pointed to itself,
kref was 0, reap_ref was 0 (this was a while back).
The problem was not able to be reproduced at the time.
-Ewan
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] SCSI: Fix hard lockup in scsi_remove_target()
On Wed, Oct 14, 2015 at 08:45:56AM -0700, James Bottomley wrote:
> OK, so I really need you to separate the problems. Fixing the bug
> you're reporting does not require a complete rework of the locking
> infrastructure; it just requires replacing the traversal macro with the
> safe version, can you verify that and it can go into fixes?
_safe only protects against deletions from yourself, it does not protect
against other threads once a lock is dropped. After auditing the
target reap code I fear the list_move trick isn't safe either, as
scsi_target_alloc relies on a being able to find a target that is
currently in the process of being deleted. So the only safe variant
we have is to keep the same sequence we currently have and restart the
loop once we've deleted the target. Given that we'd normally only
ever delete a single target anyway (not sure when we'd even get a second
one ever) this does not seem to be a major efficieny problem.
Johannes, can you test the patch below?
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index b89f..d3b34d8 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1158,31 +1158,23 @@ static void __scsi_remove_target(struct scsi_target
*starget)
void scsi_remove_target(struct device *dev)
{
struct Scsi_Host *shost = dev_to_shost(dev->parent);
- struct scsi_target *starget, *last = NULL;
+ struct scsi_target *starget;
unsigned long flags;
- /* remove targets being careful to lookup next entry before
-* deleting the last
-*/
+restart:
spin_lock_irqsave(shost->host_lock, flags);
list_for_each_entry(starget, &shost->__targets, siblings) {
if (starget->state == STARGET_DEL)
continue;
if (starget->dev.parent == dev || &starget->dev == dev) {
- /* assuming new targets arrive at the end */
kref_get(&starget->reap_ref);
spin_unlock_irqrestore(shost->host_lock, flags);
- if (last)
- scsi_target_reap(last);
- last = starget;
__scsi_remove_target(starget);
- spin_lock_irqsave(shost->host_lock, flags);
+ scsi_target_reap(starget);
+ goto restart;
}
}
spin_unlock_irqrestore(shost->host_lock, flags);
-
- if (last)
- scsi_target_reap(last);
}
EXPORT_SYMBOL(scsi_remove_target);
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] SCSI: Fix hard lockup in scsi_remove_target()
Zitat von James Bottomley :
On Wed, 2015-10-14 at 16:39 +0200, Johannes Thumshirn wrote:
On Wed, 2015-10-14 at 07:30 -0700, James Bottomley wrote:
> On Wed, 2015-10-14 at 15:50 +0200, Johannes Thumshirn wrote:
> > Removing a SCSI target via scsi_remove_target() suspected to be
> > racy. When a
> > sibling get's removed from the list it can occassionly happen that
> > one CPU is
> > stuck endlessly looping around this code block
> >
> > list_for_each_entry(starget, &shost->__targets, siblings) {
> > if (starget->state == STARGET_DEL)
> > continue;
>
> How long is the __targets list? It seems a bit unlikely that this is
> the exact cause, because for a short list all in STARGET_DEL that
> loop
> should exit very quickly. Where in the code does scsi_remove_target
> +0x68/0x240 actually point to?
>
> Is it not a bit more likely that we're following a removed list
> element?
> Since that points back to itself, the list_for_each_entry() would
> then
> circulate forever. If that's the case the simple fix would be to use
> the safe version of the list traversal macro.
Yes it is traversing a removed element and yes the patches 2/3 and 3/3
are introducing the safe version of list_for_each_entry(), but they
also decouple the search for elements to be removed from the actual
removal. This is what my initial proposal did as well. Christoph wanted
me to decouple the whole process from the host_lock though and this is
what this patches do as well now.
OK, so I really need you to separate the problems. Fixing the bug
you're reporting does not require a complete rework of the locking
infrastructure; it just requires replacing the traversal macro with the
safe version, can you verify that and it can go into fixes?
Yes. I can sent a patch for it tomorrow.
Then we can discuss the merits of doing a locking rework in this area
separately from the idea that it's some sort of bug fix.
James
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] SCSI: Fix hard lockup in scsi_remove_target()
Zitat von Christoph Hellwig : On Wed, Oct 14, 2015 at 04:39:07PM +0200, Johannes Thumshirn wrote: removal. This is what my initial proposal did as well. Christoph wanted me to decouple the whole process from the host_lock though and this is what this patches do as well now. I think we've talked past each other, I didn't intend to say that and my replies to you don't seem to imply that either. OK, I think I misunderstood you then. I'll draft a patch for what I meant. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] SCSI: Fix hard lockup in scsi_remove_target()
On Wed, Oct 14, 2015 at 04:39:07PM +0200, Johannes Thumshirn wrote: > removal. This is what my initial proposal did as well. Christoph wanted > me to decouple the whole process from the host_lock though and this is > what this patches do as well now. I think we've talked past each other, I didn't intend to say that and my replies to you don't seem to imply that either. I'll draft a patch for what I meant. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] SCSI: Fix hard lockup in scsi_remove_target()
On Wed, 2015-10-14 at 16:39 +0200, Johannes Thumshirn wrote:
> On Wed, 2015-10-14 at 07:30 -0700, James Bottomley wrote:
> > On Wed, 2015-10-14 at 15:50 +0200, Johannes Thumshirn wrote:
> > > Removing a SCSI target via scsi_remove_target() suspected to be
> > > racy. When a
> > > sibling get's removed from the list it can occassionly happen that
> > > one CPU is
> > > stuck endlessly looping around this code block
> > >
> > > list_for_each_entry(starget, &shost->__targets, siblings) {
> > > if (starget->state == STARGET_DEL)
> > > continue;
> >
> > How long is the __targets list? It seems a bit unlikely that this is
> > the exact cause, because for a short list all in STARGET_DEL that
> > loop
> > should exit very quickly. Where in the code does scsi_remove_target
> > +0x68/0x240 actually point to?
> >
> > Is it not a bit more likely that we're following a removed list
> > element?
> > Since that points back to itself, the list_for_each_entry() would
> > then
> > circulate forever. If that's the case the simple fix would be to use
> > the safe version of the list traversal macro.
>
> Yes it is traversing a removed element and yes the patches 2/3 and 3/3
> are introducing the safe version of list_for_each_entry(), but they
> also decouple the search for elements to be removed from the actual
> removal. This is what my initial proposal did as well. Christoph wanted
> me to decouple the whole process from the host_lock though and this is
> what this patches do as well now.
OK, so I really need you to separate the problems. Fixing the bug
you're reporting does not require a complete rework of the locking
infrastructure; it just requires replacing the traversal macro with the
safe version, can you verify that and it can go into fixes?
Then we can discuss the merits of doing a locking rework in this area
separately from the idea that it's some sort of bug fix.
James
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] SCSI: Fix hard lockup in scsi_remove_target()
On Wed, 2015-10-14 at 07:30 -0700, James Bottomley wrote:
> On Wed, 2015-10-14 at 15:50 +0200, Johannes Thumshirn wrote:
> > Removing a SCSI target via scsi_remove_target() suspected to be
> > racy. When a
> > sibling get's removed from the list it can occassionly happen that
> > one CPU is
> > stuck endlessly looping around this code block
> >
> > list_for_each_entry(starget, &shost->__targets, siblings) {
> > if (starget->state == STARGET_DEL)
> > continue;
>
> How long is the __targets list? It seems a bit unlikely that this is
> the exact cause, because for a short list all in STARGET_DEL that
> loop
> should exit very quickly. Where in the code does scsi_remove_target
> +0x68/0x240 actually point to?
>
> Is it not a bit more likely that we're following a removed list
> element?
> Since that points back to itself, the list_for_each_entry() would
> then
> circulate forever. If that's the case the simple fix would be to use
> the safe version of the list traversal macro.
Yes it is traversing a removed element and yes the patches 2/3 and 3/3
are introducing the safe version of list_for_each_entry(), but they
also decouple the search for elements to be removed from the actual
removal. This is what my initial proposal did as well. Christoph wanted
me to decouple the whole process from the host_lock though and this is
what this patches do as well now.
>
> James
>
>
> > Resulting in the following hard lockup.
> >
> > Kernel panic - not syncing: Watchdog detected hard LOCKUP on cpu 0
> > [...]
> > Call Trace:
> > [] dump_trace+0x7d/0x2d0
> > [] show_stack_log_lvl+0x94/0x170
> > [] show_stack+0x21/0x50
> > [] dump_stack+0x41/0x51
> > [] panic+0xc8/0x1d7
> > [] watchdog_overflow_callback+0xba/0xc0
> > [] __perf_event_overflow+0x88/0x240
> > [] intel_pmu_handle_irq+0x1fa/0x3e0
> > [] perf_event_nmi_handler+0x26/0x40
> > [] nmi_handle.isra.2+0x8d/0x180
> > [] do_nmi+0x126/0x3c0
> > [] end_repeat_nmi+0x1a/0x1e
> > [] scsi_remove_target+0x68/0x240 [scsi_mod]
> > [] process_one_work+0x172/0x420
> > [] worker_thread+0x11a/0x3c0
> > [] kthread+0xb4/0xc0
> > [] ret_from_fork+0x58/0x90
> >
> > This series attacks the issue by 1) decoupling the __targets and
> > __devices
> > lists of struct Scsi_Host from the host_lock spinlock by
> > introducing spinlocks
> > for each list and 2) de-coupling the list traversals needed for
> > detecting
> > targets/devices to be removed from the actual removal by moving
> > list entries to
> > be deleted to a second list and perform the deletion there.
> >
> >
> > The whole series survived a nearly 48h test loop of:
> > while [ $not_done ]; do
> > umount $mountpoint;
> > rmmod $module;
> > modprobe $module;
> > mount $mountpoint;
> > done
> >
> > This is a follow up of the patch proposed here:
> > http://marc.info/?l=linux-scsi&m=144377409311774&w=2
> > incorporating Christoph's comment
> >
> > Johannes Thumshirn (3):
> > SCSI: Introduce device_lock and target_lock in Scsi_Host
> > SCSI: Rework list handling in scsi_target_remove
> > SCSI: Rework list handling in __scsi_target_remove
> >
> > drivers/scsi/53c700.c | 3 +++
> > drivers/scsi/hosts.c | 2 ++
> > drivers/scsi/scsi.c | 8
> > drivers/scsi/scsi_scan.c | 10 +-
> > drivers/scsi/scsi_sysfs.c | 43 +--
> >
> > include/scsi/scsi_host.h | 2 ++
> > 6 files changed, 37 insertions(+), 31 deletions(-)
> >
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi"
> in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] SCSI: Fix hard lockup in scsi_remove_target()
On Wed, 2015-10-14 at 15:50 +0200, Johannes Thumshirn wrote:
> Removing a SCSI target via scsi_remove_target() suspected to be racy. When a
> sibling get's removed from the list it can occassionly happen that one CPU is
> stuck endlessly looping around this code block
>
> list_for_each_entry(starget, &shost->__targets, siblings) {
> if (starget->state == STARGET_DEL)
> continue;
How long is the __targets list? It seems a bit unlikely that this is
the exact cause, because for a short list all in STARGET_DEL that loop
should exit very quickly. Where in the code does scsi_remove_target
+0x68/0x240 actually point to?
Is it not a bit more likely that we're following a removed list element?
Since that points back to itself, the list_for_each_entry() would then
circulate forever. If that's the case the simple fix would be to use
the safe version of the list traversal macro.
James
> Resulting in the following hard lockup.
>
> Kernel panic - not syncing: Watchdog detected hard LOCKUP on cpu 0
> [...]
> Call Trace:
> [] dump_trace+0x7d/0x2d0
> [] show_stack_log_lvl+0x94/0x170
> [] show_stack+0x21/0x50
> [] dump_stack+0x41/0x51
> [] panic+0xc8/0x1d7
> [] watchdog_overflow_callback+0xba/0xc0
> [] __perf_event_overflow+0x88/0x240
> [] intel_pmu_handle_irq+0x1fa/0x3e0
> [] perf_event_nmi_handler+0x26/0x40
> [] nmi_handle.isra.2+0x8d/0x180
> [] do_nmi+0x126/0x3c0
> [] end_repeat_nmi+0x1a/0x1e
> [] scsi_remove_target+0x68/0x240 [scsi_mod]
> [] process_one_work+0x172/0x420
> [] worker_thread+0x11a/0x3c0
> [] kthread+0xb4/0xc0
> [] ret_from_fork+0x58/0x90
>
> This series attacks the issue by 1) decoupling the __targets and __devices
> lists of struct Scsi_Host from the host_lock spinlock by introducing spinlocks
> for each list and 2) de-coupling the list traversals needed for detecting
> targets/devices to be removed from the actual removal by moving list entries
> to
> be deleted to a second list and perform the deletion there.
>
>
> The whole series survived a nearly 48h test loop of:
> while [ $not_done ]; do
> umount $mountpoint;
> rmmod $module;
> modprobe $module;
> mount $mountpoint;
> done
>
> This is a follow up of the patch proposed here:
> http://marc.info/?l=linux-scsi&m=144377409311774&w=2
> incorporating Christoph's comment
>
> Johannes Thumshirn (3):
> SCSI: Introduce device_lock and target_lock in Scsi_Host
> SCSI: Rework list handling in scsi_target_remove
> SCSI: Rework list handling in __scsi_target_remove
>
> drivers/scsi/53c700.c | 3 +++
> drivers/scsi/hosts.c | 2 ++
> drivers/scsi/scsi.c | 8
> drivers/scsi/scsi_scan.c | 10 +-
> drivers/scsi/scsi_sysfs.c | 43 +--
> include/scsi/scsi_host.h | 2 ++
> 6 files changed, 37 insertions(+), 31 deletions(-)
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/3] SCSI: Fix hard lockup in scsi_remove_target()
Removing a SCSI target via scsi_remove_target() suspected to be racy. When a
sibling get's removed from the list it can occassionly happen that one CPU is
stuck endlessly looping around this code block
list_for_each_entry(starget, &shost->__targets, siblings) {
if (starget->state == STARGET_DEL)
continue;
Resulting in the following hard lockup.
Kernel panic - not syncing: Watchdog detected hard LOCKUP on cpu 0
[...]
Call Trace:
[] dump_trace+0x7d/0x2d0
[] show_stack_log_lvl+0x94/0x170
[] show_stack+0x21/0x50
[] dump_stack+0x41/0x51
[] panic+0xc8/0x1d7
[] watchdog_overflow_callback+0xba/0xc0
[] __perf_event_overflow+0x88/0x240
[] intel_pmu_handle_irq+0x1fa/0x3e0
[] perf_event_nmi_handler+0x26/0x40
[] nmi_handle.isra.2+0x8d/0x180
[] do_nmi+0x126/0x3c0
[] end_repeat_nmi+0x1a/0x1e
[] scsi_remove_target+0x68/0x240 [scsi_mod]
[] process_one_work+0x172/0x420
[] worker_thread+0x11a/0x3c0
[] kthread+0xb4/0xc0
[] ret_from_fork+0x58/0x90
This series attacks the issue by 1) decoupling the __targets and __devices
lists of struct Scsi_Host from the host_lock spinlock by introducing spinlocks
for each list and 2) de-coupling the list traversals needed for detecting
targets/devices to be removed from the actual removal by moving list entries to
be deleted to a second list and perform the deletion there.
The whole series survived a nearly 48h test loop of:
while [ $not_done ]; do
umount $mountpoint;
rmmod $module;
modprobe $module;
mount $mountpoint;
done
This is a follow up of the patch proposed here:
http://marc.info/?l=linux-scsi&m=144377409311774&w=2
incorporating Christoph's comment
Johannes Thumshirn (3):
SCSI: Introduce device_lock and target_lock in Scsi_Host
SCSI: Rework list handling in scsi_target_remove
SCSI: Rework list handling in __scsi_target_remove
drivers/scsi/53c700.c | 3 +++
drivers/scsi/hosts.c | 2 ++
drivers/scsi/scsi.c | 8
drivers/scsi/scsi_scan.c | 10 +-
drivers/scsi/scsi_sysfs.c | 43 +--
include/scsi/scsi_host.h | 2 ++
6 files changed, 37 insertions(+), 31 deletions(-)
--
1.8.5.6
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
