* [PATCH] dax: Fix deadlock in dax_lock_mapping_entry() @ 2018-09-27 11:23 Jan Kara 2018-09-27 13:28 ` Matthew Wilcox 0 siblings, 1 reply; 11+ messages in thread From: Jan Kara @ 2018-09-27 11:23 UTC (permalink / raw) To: Dan Williams; +Cc: linux-fsdevel, Barret Rhoden, Jan Kara, linux-nvdimm When dax_lock_mapping_entry() has to sleep to obtain entry lock, it will fail to unlock mapping->i_pages spinlock and thus immediately deadlock against itself when retrying to grab the entry lock again. Fix the problem by unlocking mapping->i_pages before retrying. Fixes: c2a7d2a115525d3501d38e23d24875a79a07e15e Reported-by: Barret Rhoden <brho@google.com> Signed-off-by: Jan Kara <jack@suse.cz> --- fs/dax.c | 1 + 1 file changed, 1 insertion(+) Dan, can you please get this merged? Otherwise dax_lock_mapping_entry() deadlocks as soon as there's any contention. diff --git a/fs/dax.c b/fs/dax.c index b68ce484e1be..4becbf168b7f 100644 --- a/fs/dax.c +++ b/fs/dax.c @@ -447,6 +447,7 @@ bool dax_lock_mapping_entry(struct page *page) xa_unlock_irq(&mapping->i_pages); break; } else if (IS_ERR(entry)) { + xa_unlock_irq(&mapping->i_pages); WARN_ON_ONCE(PTR_ERR(entry) != -EAGAIN); continue; } -- 2.16.4 _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] dax: Fix deadlock in dax_lock_mapping_entry() 2018-09-27 11:23 [PATCH] dax: Fix deadlock in dax_lock_mapping_entry() Jan Kara @ 2018-09-27 13:28 ` Matthew Wilcox 2018-09-27 13:41 ` Jan Kara 0 siblings, 1 reply; 11+ messages in thread From: Matthew Wilcox @ 2018-09-27 13:28 UTC (permalink / raw) To: Jan Kara; +Cc: linux-fsdevel, Barret Rhoden, linux-nvdimm On Thu, Sep 27, 2018 at 01:23:32PM +0200, Jan Kara wrote: > When dax_lock_mapping_entry() has to sleep to obtain entry lock, it will > fail to unlock mapping->i_pages spinlock and thus immediately deadlock > against itself when retrying to grab the entry lock again. Fix the > problem by unlocking mapping->i_pages before retrying. It seems weird that xfstests doesn't provoke this ... _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] dax: Fix deadlock in dax_lock_mapping_entry() 2018-09-27 13:28 ` Matthew Wilcox @ 2018-09-27 13:41 ` Jan Kara 2018-09-27 18:22 ` Dan Williams 0 siblings, 1 reply; 11+ messages in thread From: Jan Kara @ 2018-09-27 13:41 UTC (permalink / raw) To: Matthew Wilcox; +Cc: linux-fsdevel, Jan Kara, Barret Rhoden, linux-nvdimm On Thu 27-09-18 06:28:43, Matthew Wilcox wrote: > On Thu, Sep 27, 2018 at 01:23:32PM +0200, Jan Kara wrote: > > When dax_lock_mapping_entry() has to sleep to obtain entry lock, it will > > fail to unlock mapping->i_pages spinlock and thus immediately deadlock > > against itself when retrying to grab the entry lock again. Fix the > > problem by unlocking mapping->i_pages before retrying. > > It seems weird that xfstests doesn't provoke this ... The function currently gets called only from mm/memory-failure.c. And yes, we are lacking DAX hwpoison error tests in fstests... Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] dax: Fix deadlock in dax_lock_mapping_entry() 2018-09-27 13:41 ` Jan Kara @ 2018-09-27 18:22 ` Dan Williams 2018-10-04 16:27 ` Jan Kara 0 siblings, 1 reply; 11+ messages in thread From: Dan Williams @ 2018-09-27 18:22 UTC (permalink / raw) To: Jan Kara; +Cc: linux-fsdevel, brho, Matthew Wilcox, linux-nvdimm On Thu, Sep 27, 2018 at 6:41 AM Jan Kara <jack@suse.cz> wrote: > > On Thu 27-09-18 06:28:43, Matthew Wilcox wrote: > > On Thu, Sep 27, 2018 at 01:23:32PM +0200, Jan Kara wrote: > > > When dax_lock_mapping_entry() has to sleep to obtain entry lock, it will > > > fail to unlock mapping->i_pages spinlock and thus immediately deadlock > > > against itself when retrying to grab the entry lock again. Fix the > > > problem by unlocking mapping->i_pages before retrying. > > > > It seems weird that xfstests doesn't provoke this ... > > The function currently gets called only from mm/memory-failure.c. And yes, > we are lacking DAX hwpoison error tests in fstests... I have an item on my backlog to port the ndctl unit test that does memory_failure() injection vs ext4 over to fstests. That said I've been investigating a deadlock on ext4 caused by this test. When I saw this patch I hoped it was root cause, but the test is still failing for me. Vishal is able to pass the test on his system, so the failure mode is timing dependent. I'm running this patch on top of -rc5 and still seeing the following deadlock. EXT4-fs (pmem0): DAX enabled. Warning: EXPERIMENTAL, use at your own risk EXT4-fs (pmem0): mounted filesystem with ordered data mode. Opts: dax Injecting memory failure for pfn 0x208900 at process virtual address 0x7f5872900000 Memory failure: 0x208900: Killing dax-pmd:7095 due to hardware memory corruption Memory failure: 0x208900: recovery action for dax page: Recovered watchdog: BUG: soft lockup - CPU#35 stuck for 22s! [dax-pmd:7095] [..] irq event stamp: 121911146 hardirqs last enabled at (121911145): [<ffffffff81aa1bd9>] _raw_spin_unlock_irq+0x29/0x40 hardirqs last disabled at (121911146): [<ffffffff810037a3>] trace_hardirqs_off_thunk+0x1a/0x1c softirqs last enabled at (78238674): [<ffffffff81e0032e>] __do_softirq+0x32e/0x428 softirqs last disabled at (78238627): [<ffffffff810bc6f6>] irq_exit+0xf6/0x100 CPU: 35 PID: 7095 Comm: dax-pmd Tainted: G OE 4.19.0-rc5+ #2394 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.11.1-0-g0551a4be2c-prebuilt.qemu-project.org 04/01/2014 RIP: 0010:lock_release+0x134/0x2a0 [..] Call Trace: find_get_entries+0x299/0x3c0 pagevec_lookup_entries+0x1a/0x30 dax_layout_busy_page+0x9c/0x280 ? __lock_acquire+0x12fa/0x1310 ext4_break_layouts+0x48/0x100 ? ext4_punch_hole+0x108/0x5a0 ext4_punch_hole+0x110/0x5a0 ext4_fallocate+0x189/0xa40 ? rcu_read_lock_sched_held+0x6b/0x80 ? rcu_sync_lockdep_assert+0x2e/0x60 vfs_fallocate+0x13f/0x270 The same test against xfs is not failing for me. I have been seeking some focus time to dig in on this. _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] dax: Fix deadlock in dax_lock_mapping_entry() 2018-09-27 18:22 ` Dan Williams @ 2018-10-04 16:27 ` Jan Kara 2018-10-05 1:57 ` Dan Williams 0 siblings, 1 reply; 11+ messages in thread From: Jan Kara @ 2018-10-04 16:27 UTC (permalink / raw) To: Dan Williams; +Cc: linux-fsdevel, brho, Jan Kara, Matthew Wilcox, linux-nvdimm On Thu 27-09-18 11:22:22, Dan Williams wrote: > On Thu, Sep 27, 2018 at 6:41 AM Jan Kara <jack@suse.cz> wrote: > > > > On Thu 27-09-18 06:28:43, Matthew Wilcox wrote: > > > On Thu, Sep 27, 2018 at 01:23:32PM +0200, Jan Kara wrote: > > > > When dax_lock_mapping_entry() has to sleep to obtain entry lock, it will > > > > fail to unlock mapping->i_pages spinlock and thus immediately deadlock > > > > against itself when retrying to grab the entry lock again. Fix the > > > > problem by unlocking mapping->i_pages before retrying. > > > > > > It seems weird that xfstests doesn't provoke this ... > > > > The function currently gets called only from mm/memory-failure.c. And yes, > > we are lacking DAX hwpoison error tests in fstests... > > I have an item on my backlog to port the ndctl unit test that does > memory_failure() injection vs ext4 over to fstests. That said I've > been investigating a deadlock on ext4 caused by this test. When I saw > this patch I hoped it was root cause, but the test is still failing > for me. Vishal is able to pass the test on his system, so the failure > mode is timing dependent. I'm running this patch on top of -rc5 and > still seeing the following deadlock. I went through the code but I don't see where the problem could be. How can I run that test? Is KVM enough or do I need hardware with AEP dimms? Honza > > EXT4-fs (pmem0): DAX enabled. Warning: EXPERIMENTAL, use at your own risk > EXT4-fs (pmem0): mounted filesystem with ordered data mode. Opts: dax > Injecting memory failure for pfn 0x208900 at process virtual > address 0x7f5872900000 > Memory failure: 0x208900: Killing dax-pmd:7095 due to hardware > memory corruption > Memory failure: 0x208900: recovery action for dax page: Recovered > watchdog: BUG: soft lockup - CPU#35 stuck for 22s! [dax-pmd:7095] > [..] > irq event stamp: 121911146 > hardirqs last enabled at (121911145): [<ffffffff81aa1bd9>] > _raw_spin_unlock_irq+0x29/0x40 hardirqs last disabled at > (121911146): [<ffffffff810037a3>] trace_hardirqs_off_thunk+0x1a/0x1c > softirqs last enabled at (78238674): [<ffffffff81e0032e>] > __do_softirq+0x32e/0x428 > softirqs last disabled at (78238627): [<ffffffff810bc6f6>] > irq_exit+0xf6/0x100 > CPU: 35 PID: 7095 Comm: dax-pmd Tainted: G OE > 4.19.0-rc5+ #2394 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS > rel-1.11.1-0-g0551a4be2c-prebuilt.qemu-project.org 04/01/2014 > RIP: 0010:lock_release+0x134/0x2a0 > [..] > Call Trace: > find_get_entries+0x299/0x3c0 > pagevec_lookup_entries+0x1a/0x30 > dax_layout_busy_page+0x9c/0x280 > ? __lock_acquire+0x12fa/0x1310 > ext4_break_layouts+0x48/0x100 > ? ext4_punch_hole+0x108/0x5a0 > ext4_punch_hole+0x110/0x5a0 > ext4_fallocate+0x189/0xa40 > ? rcu_read_lock_sched_held+0x6b/0x80 > ? rcu_sync_lockdep_assert+0x2e/0x60 > vfs_fallocate+0x13f/0x270 > > The same test against xfs is not failing for me. I have been seeking > some focus time to dig in on this. -- Jan Kara <jack@suse.com> SUSE Labs, CR _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] dax: Fix deadlock in dax_lock_mapping_entry() 2018-10-04 16:27 ` Jan Kara @ 2018-10-05 1:57 ` Dan Williams 2018-10-05 2:52 ` Matthew Wilcox 0 siblings, 1 reply; 11+ messages in thread From: Dan Williams @ 2018-10-05 1:57 UTC (permalink / raw) To: Jan Kara; +Cc: linux-fsdevel, Barret Rhoden, Matthew Wilcox, linux-nvdimm On Thu, Oct 4, 2018 at 9:27 AM Jan Kara <jack@suse.cz> wrote: > > On Thu 27-09-18 11:22:22, Dan Williams wrote: > > On Thu, Sep 27, 2018 at 6:41 AM Jan Kara <jack@suse.cz> wrote: > > > > > > On Thu 27-09-18 06:28:43, Matthew Wilcox wrote: > > > > On Thu, Sep 27, 2018 at 01:23:32PM +0200, Jan Kara wrote: > > > > > When dax_lock_mapping_entry() has to sleep to obtain entry lock, it will > > > > > fail to unlock mapping->i_pages spinlock and thus immediately deadlock > > > > > against itself when retrying to grab the entry lock again. Fix the > > > > > problem by unlocking mapping->i_pages before retrying. > > > > > > > > It seems weird that xfstests doesn't provoke this ... > > > > > > The function currently gets called only from mm/memory-failure.c. And yes, > > > we are lacking DAX hwpoison error tests in fstests... > > > > I have an item on my backlog to port the ndctl unit test that does > > memory_failure() injection vs ext4 over to fstests. That said I've > > been investigating a deadlock on ext4 caused by this test. When I saw > > this patch I hoped it was root cause, but the test is still failing > > for me. Vishal is able to pass the test on his system, so the failure > > mode is timing dependent. I'm running this patch on top of -rc5 and > > still seeing the following deadlock. > > I went through the code but I don't see where the problem could be. How can > I run that test? Is KVM enough or do I need hardware with AEP dimms? KVM is enough... however, I have found a hack that makes the test pass: diff --git a/mm/filemap.c b/mm/filemap.c index 52517f28e6f4..d7f035b1846e 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -1668,6 +1668,9 @@ unsigned find_get_entries(struct address_space *mapping, goto repeat; } export: + if (iter.index < start) + continue; + indices[ret] = iter.index; entries[ret] = page; if (++ret == nr_entries) Is this a radix bug? I would never expect: radix_tree_for_each_slot(slot, &mapping->i_pages, &iter, start) ...to return entries with an index < start. Without that change above we loop forever because dax_layout_busy_page() can't make forward progress. I'll dig into the radix code tomorrow, but maybe someone else will be me to it overnight. _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] dax: Fix deadlock in dax_lock_mapping_entry() 2018-10-05 1:57 ` Dan Williams @ 2018-10-05 2:52 ` Matthew Wilcox 2018-10-05 4:01 ` Dan Williams 0 siblings, 1 reply; 11+ messages in thread From: Matthew Wilcox @ 2018-10-05 2:52 UTC (permalink / raw) To: Dan Williams; +Cc: linux-fsdevel, Barret Rhoden, Jan Kara, linux-nvdimm On Thu, Oct 04, 2018 at 06:57:52PM -0700, Dan Williams wrote: > On Thu, Oct 4, 2018 at 9:27 AM Jan Kara <jack@suse.cz> wrote: > > > > On Thu 27-09-18 11:22:22, Dan Williams wrote: > > > On Thu, Sep 27, 2018 at 6:41 AM Jan Kara <jack@suse.cz> wrote: > > > > > > > > On Thu 27-09-18 06:28:43, Matthew Wilcox wrote: > > > > > On Thu, Sep 27, 2018 at 01:23:32PM +0200, Jan Kara wrote: > > > > > > When dax_lock_mapping_entry() has to sleep to obtain entry lock, it will > > > > > > fail to unlock mapping->i_pages spinlock and thus immediately deadlock > > > > > > against itself when retrying to grab the entry lock again. Fix the > > > > > > problem by unlocking mapping->i_pages before retrying. > > > > > > > > > > It seems weird that xfstests doesn't provoke this ... > > > > > > > > The function currently gets called only from mm/memory-failure.c. And yes, > > > > we are lacking DAX hwpoison error tests in fstests... > > > > > > I have an item on my backlog to port the ndctl unit test that does > > > memory_failure() injection vs ext4 over to fstests. That said I've > > > been investigating a deadlock on ext4 caused by this test. When I saw > > > this patch I hoped it was root cause, but the test is still failing > > > for me. Vishal is able to pass the test on his system, so the failure > > > mode is timing dependent. I'm running this patch on top of -rc5 and > > > still seeing the following deadlock. > > > > I went through the code but I don't see where the problem could be. How can > > I run that test? Is KVM enough or do I need hardware with AEP dimms? > > KVM is enough... however, I have found a hack that makes the test pass: > > diff --git a/mm/filemap.c b/mm/filemap.c > index 52517f28e6f4..d7f035b1846e 100644 > --- a/mm/filemap.c > +++ b/mm/filemap.c > @@ -1668,6 +1668,9 @@ unsigned find_get_entries(struct address_space *mapping, > goto repeat; > } > export: > + if (iter.index < start) > + continue; > + > indices[ret] = iter.index; > entries[ret] = page; > if (++ret == nr_entries) > > Is this a radix bug? I would never expect: > > radix_tree_for_each_slot(slot, &mapping->i_pages, &iter, start) > > ...to return entries with an index < start. Without that change above > we loop forever because dax_layout_busy_page() can't make forward > progress. I'll dig into the radix code tomorrow, but maybe someone > else will be me to it overnight. If 'start' is within a 2MB entry, iter.index can absolutely be less than start. I forget exactly what the radix tree code does, but I think it returns index set to the canonical/base index of the entry. _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] dax: Fix deadlock in dax_lock_mapping_entry() 2018-10-05 2:52 ` Matthew Wilcox @ 2018-10-05 4:01 ` Dan Williams 2018-10-05 4:28 ` Dan Williams 0 siblings, 1 reply; 11+ messages in thread From: Dan Williams @ 2018-10-05 4:01 UTC (permalink / raw) To: Matthew Wilcox; +Cc: linux-fsdevel, Barret Rhoden, Jan Kara, linux-nvdimm On Thu, Oct 4, 2018 at 7:52 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Thu, Oct 04, 2018 at 06:57:52PM -0700, Dan Williams wrote: > > On Thu, Oct 4, 2018 at 9:27 AM Jan Kara <jack@suse.cz> wrote: > > > > > > On Thu 27-09-18 11:22:22, Dan Williams wrote: > > > > On Thu, Sep 27, 2018 at 6:41 AM Jan Kara <jack@suse.cz> wrote: > > > > > > > > > > On Thu 27-09-18 06:28:43, Matthew Wilcox wrote: > > > > > > On Thu, Sep 27, 2018 at 01:23:32PM +0200, Jan Kara wrote: > > > > > > > When dax_lock_mapping_entry() has to sleep to obtain entry lock, it will > > > > > > > fail to unlock mapping->i_pages spinlock and thus immediately deadlock > > > > > > > against itself when retrying to grab the entry lock again. Fix the > > > > > > > problem by unlocking mapping->i_pages before retrying. > > > > > > > > > > > > It seems weird that xfstests doesn't provoke this ... > > > > > > > > > > The function currently gets called only from mm/memory-failure.c. And yes, > > > > > we are lacking DAX hwpoison error tests in fstests... > > > > > > > > I have an item on my backlog to port the ndctl unit test that does > > > > memory_failure() injection vs ext4 over to fstests. That said I've > > > > been investigating a deadlock on ext4 caused by this test. When I saw > > > > this patch I hoped it was root cause, but the test is still failing > > > > for me. Vishal is able to pass the test on his system, so the failure > > > > mode is timing dependent. I'm running this patch on top of -rc5 and > > > > still seeing the following deadlock. > > > > > > I went through the code but I don't see where the problem could be. How can > > > I run that test? Is KVM enough or do I need hardware with AEP dimms? > > > > KVM is enough... however, I have found a hack that makes the test pass: > > > > diff --git a/mm/filemap.c b/mm/filemap.c > > index 52517f28e6f4..d7f035b1846e 100644 > > --- a/mm/filemap.c > > +++ b/mm/filemap.c > > @@ -1668,6 +1668,9 @@ unsigned find_get_entries(struct address_space *mapping, > > goto repeat; > > } > > export: > > + if (iter.index < start) > > + continue; > > + > > indices[ret] = iter.index; > > entries[ret] = page; > > if (++ret == nr_entries) > > > > Is this a radix bug? I would never expect: > > > > radix_tree_for_each_slot(slot, &mapping->i_pages, &iter, start) > > > > ...to return entries with an index < start. Without that change above > > we loop forever because dax_layout_busy_page() can't make forward > > progress. I'll dig into the radix code tomorrow, but maybe someone > > else will be me to it overnight. > > If 'start' is within a 2MB entry, iter.index can absolutely be less > than start. I forget exactly what the radix tree code does, but I think > it returns index set to the canonical/base index of the entry. Ok, that makes sense. Then the bug is in dax_layout_busy_page() which needs to increment 'index' by the entry size. This might also explain why not every run sees it because you may get lucky and have a 4K entry. _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] dax: Fix deadlock in dax_lock_mapping_entry() 2018-10-05 4:01 ` Dan Williams @ 2018-10-05 4:28 ` Dan Williams 2018-10-05 9:54 ` Jan Kara 0 siblings, 1 reply; 11+ messages in thread From: Dan Williams @ 2018-10-05 4:28 UTC (permalink / raw) To: Matthew Wilcox; +Cc: linux-fsdevel, Barret Rhoden, Jan Kara, linux-nvdimm On Thu, Oct 4, 2018 at 9:01 PM Dan Williams <dan.j.williams@intel.com> wrote: > > On Thu, Oct 4, 2018 at 7:52 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > On Thu, Oct 04, 2018 at 06:57:52PM -0700, Dan Williams wrote: > > > On Thu, Oct 4, 2018 at 9:27 AM Jan Kara <jack@suse.cz> wrote: > > > > > > > > On Thu 27-09-18 11:22:22, Dan Williams wrote: > > > > > On Thu, Sep 27, 2018 at 6:41 AM Jan Kara <jack@suse.cz> wrote: > > > > > > > > > > > > On Thu 27-09-18 06:28:43, Matthew Wilcox wrote: > > > > > > > On Thu, Sep 27, 2018 at 01:23:32PM +0200, Jan Kara wrote: > > > > > > > > When dax_lock_mapping_entry() has to sleep to obtain entry lock, it will > > > > > > > > fail to unlock mapping->i_pages spinlock and thus immediately deadlock > > > > > > > > against itself when retrying to grab the entry lock again. Fix the > > > > > > > > problem by unlocking mapping->i_pages before retrying. > > > > > > > > > > > > > > It seems weird that xfstests doesn't provoke this ... > > > > > > > > > > > > The function currently gets called only from mm/memory-failure.c. And yes, > > > > > > we are lacking DAX hwpoison error tests in fstests... > > > > > > > > > > I have an item on my backlog to port the ndctl unit test that does > > > > > memory_failure() injection vs ext4 over to fstests. That said I've > > > > > been investigating a deadlock on ext4 caused by this test. When I saw > > > > > this patch I hoped it was root cause, but the test is still failing > > > > > for me. Vishal is able to pass the test on his system, so the failure > > > > > mode is timing dependent. I'm running this patch on top of -rc5 and > > > > > still seeing the following deadlock. > > > > > > > > I went through the code but I don't see where the problem could be. How can > > > > I run that test? Is KVM enough or do I need hardware with AEP dimms? > > > > > > KVM is enough... however, I have found a hack that makes the test pass: > > > > > > diff --git a/mm/filemap.c b/mm/filemap.c > > > index 52517f28e6f4..d7f035b1846e 100644 > > > --- a/mm/filemap.c > > > +++ b/mm/filemap.c > > > @@ -1668,6 +1668,9 @@ unsigned find_get_entries(struct address_space *mapping, > > > goto repeat; > > > } > > > export: > > > + if (iter.index < start) > > > + continue; > > > + > > > indices[ret] = iter.index; > > > entries[ret] = page; > > > if (++ret == nr_entries) > > > > > > Is this a radix bug? I would never expect: > > > > > > radix_tree_for_each_slot(slot, &mapping->i_pages, &iter, start) > > > > > > ...to return entries with an index < start. Without that change above > > > we loop forever because dax_layout_busy_page() can't make forward > > > progress. I'll dig into the radix code tomorrow, but maybe someone > > > else will be me to it overnight. > > > > If 'start' is within a 2MB entry, iter.index can absolutely be less > > than start. I forget exactly what the radix tree code does, but I think > > it returns index set to the canonical/base index of the entry. > > Ok, that makes sense. Then the bug is in dax_layout_busy_page() which > needs to increment 'index' by the entry size. This might also explain > why not every run sees it because you may get lucky and have a 4K > entry. Hmm, no 2MB entry here. We go through the first find_get_entries and export: export start: 0x0 index: 0x0 page: 0x822000a export start: 0x0 index: 0x200 page: 0xcc3801a Then dax_layout_busy_page sets 'start' to 0x201, and find_get_entries returns: export start: 0x201 index: 0x200 page: 0xcc3801a ...forevermore. _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] dax: Fix deadlock in dax_lock_mapping_entry() 2018-10-05 4:28 ` Dan Williams @ 2018-10-05 9:54 ` Jan Kara 2018-10-06 18:04 ` Dan Williams 0 siblings, 1 reply; 11+ messages in thread From: Jan Kara @ 2018-10-05 9:54 UTC (permalink / raw) To: Dan Williams Cc: linux-fsdevel, Barret Rhoden, Jan Kara, Matthew Wilcox, linux-nvdimm On Thu 04-10-18 21:28:14, Dan Williams wrote: > On Thu, Oct 4, 2018 at 9:01 PM Dan Williams <dan.j.williams@intel.com> wrote: > > > > On Thu, Oct 4, 2018 at 7:52 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > On Thu, Oct 04, 2018 at 06:57:52PM -0700, Dan Williams wrote: > > > > On Thu, Oct 4, 2018 at 9:27 AM Jan Kara <jack@suse.cz> wrote: > > > > > > > > > > On Thu 27-09-18 11:22:22, Dan Williams wrote: > > > > > > On Thu, Sep 27, 2018 at 6:41 AM Jan Kara <jack@suse.cz> wrote: > > > > > > > > > > > > > > On Thu 27-09-18 06:28:43, Matthew Wilcox wrote: > > > > > > > > On Thu, Sep 27, 2018 at 01:23:32PM +0200, Jan Kara wrote: > > > > > > > > > When dax_lock_mapping_entry() has to sleep to obtain entry lock, it will > > > > > > > > > fail to unlock mapping->i_pages spinlock and thus immediately deadlock > > > > > > > > > against itself when retrying to grab the entry lock again. Fix the > > > > > > > > > problem by unlocking mapping->i_pages before retrying. > > > > > > > > > > > > > > > > It seems weird that xfstests doesn't provoke this ... > > > > > > > > > > > > > > The function currently gets called only from mm/memory-failure.c. And yes, > > > > > > > we are lacking DAX hwpoison error tests in fstests... > > > > > > > > > > > > I have an item on my backlog to port the ndctl unit test that does > > > > > > memory_failure() injection vs ext4 over to fstests. That said I've > > > > > > been investigating a deadlock on ext4 caused by this test. When I saw > > > > > > this patch I hoped it was root cause, but the test is still failing > > > > > > for me. Vishal is able to pass the test on his system, so the failure > > > > > > mode is timing dependent. I'm running this patch on top of -rc5 and > > > > > > still seeing the following deadlock. > > > > > > > > > > I went through the code but I don't see where the problem could be. How can > > > > > I run that test? Is KVM enough or do I need hardware with AEP dimms? > > > > > > > > KVM is enough... however, I have found a hack that makes the test pass: > > > > > > > > diff --git a/mm/filemap.c b/mm/filemap.c > > > > index 52517f28e6f4..d7f035b1846e 100644 > > > > --- a/mm/filemap.c > > > > +++ b/mm/filemap.c > > > > @@ -1668,6 +1668,9 @@ unsigned find_get_entries(struct address_space *mapping, > > > > goto repeat; > > > > } > > > > export: > > > > + if (iter.index < start) > > > > + continue; > > > > + > > > > indices[ret] = iter.index; > > > > entries[ret] = page; > > > > if (++ret == nr_entries) > > > > > > > > Is this a radix bug? I would never expect: > > > > > > > > radix_tree_for_each_slot(slot, &mapping->i_pages, &iter, start) > > > > > > > > ...to return entries with an index < start. Without that change above > > > > we loop forever because dax_layout_busy_page() can't make forward > > > > progress. I'll dig into the radix code tomorrow, but maybe someone > > > > else will be me to it overnight. > > > > > > If 'start' is within a 2MB entry, iter.index can absolutely be less > > > than start. I forget exactly what the radix tree code does, but I think > > > it returns index set to the canonical/base index of the entry. > > > > Ok, that makes sense. Then the bug is in dax_layout_busy_page() which > > needs to increment 'index' by the entry size. This might also explain > > why not every run sees it because you may get lucky and have a 4K > > entry. > > Hmm, no 2MB entry here. > > We go through the first find_get_entries and export: > > export start: 0x0 index: 0x0 page: 0x822000a > export start: 0x0 index: 0x200 page: 0xcc3801a > > Then dax_layout_busy_page sets 'start' to 0x201, and find_get_entries returns: > > export start: 0x201 index: 0x200 page: 0xcc3801a > > ...forevermore. Are you sure there's not 2MB entry starting at index 0x200? Because if there was, we'd get infinite loop exactly as you describe in dax_layout_busy_page() AFAICT. And it seems to me lot of other places iterating over entries are borked in a similar way as they all assume that doing +1 to the current index is guaranteeing them forward progress. Now actual breakage resulting from this is limited as only DAX uses multiorder entries and thus not many of these iterators actually ever get called for radix tree with multiorder entries (e.g. tmpfs still inserts every 4k subpage of THP into the radix tree and iteration functions usually handles tail subpages in a special way). But this would really deserve larger cleanup. Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] dax: Fix deadlock in dax_lock_mapping_entry() 2018-10-05 9:54 ` Jan Kara @ 2018-10-06 18:04 ` Dan Williams 0 siblings, 0 replies; 11+ messages in thread From: Dan Williams @ 2018-10-06 18:04 UTC (permalink / raw) To: Jan Kara; +Cc: linux-fsdevel, Barret Rhoden, Matthew Wilcox, linux-nvdimm On Fri, Oct 5, 2018 at 2:56 AM Jan Kara <jack@suse.cz> wrote: > > On Thu 04-10-18 21:28:14, Dan Williams wrote: > > On Thu, Oct 4, 2018 at 9:01 PM Dan Williams <dan.j.williams@intel.com> wrote: > > > > > > On Thu, Oct 4, 2018 at 7:52 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > > > On Thu, Oct 04, 2018 at 06:57:52PM -0700, Dan Williams wrote: > > > > > On Thu, Oct 4, 2018 at 9:27 AM Jan Kara <jack@suse.cz> wrote: > > > > > > > > > > > > On Thu 27-09-18 11:22:22, Dan Williams wrote: > > > > > > > On Thu, Sep 27, 2018 at 6:41 AM Jan Kara <jack@suse.cz> wrote: > > > > > > > > > > > > > > > > On Thu 27-09-18 06:28:43, Matthew Wilcox wrote: > > > > > > > > > On Thu, Sep 27, 2018 at 01:23:32PM +0200, Jan Kara wrote: > > > > > > > > > > When dax_lock_mapping_entry() has to sleep to obtain entry lock, it will > > > > > > > > > > fail to unlock mapping->i_pages spinlock and thus immediately deadlock > > > > > > > > > > against itself when retrying to grab the entry lock again. Fix the > > > > > > > > > > problem by unlocking mapping->i_pages before retrying. > > > > > > > > > > > > > > > > > > It seems weird that xfstests doesn't provoke this ... > > > > > > > > > > > > > > > > The function currently gets called only from mm/memory-failure.c. And yes, > > > > > > > > we are lacking DAX hwpoison error tests in fstests... > > > > > > > > > > > > > > I have an item on my backlog to port the ndctl unit test that does > > > > > > > memory_failure() injection vs ext4 over to fstests. That said I've > > > > > > > been investigating a deadlock on ext4 caused by this test. When I saw > > > > > > > this patch I hoped it was root cause, but the test is still failing > > > > > > > for me. Vishal is able to pass the test on his system, so the failure > > > > > > > mode is timing dependent. I'm running this patch on top of -rc5 and > > > > > > > still seeing the following deadlock. > > > > > > > > > > > > I went through the code but I don't see where the problem could be. How can > > > > > > I run that test? Is KVM enough or do I need hardware with AEP dimms? > > > > > > > > > > KVM is enough... however, I have found a hack that makes the test pass: > > > > > > > > > > diff --git a/mm/filemap.c b/mm/filemap.c > > > > > index 52517f28e6f4..d7f035b1846e 100644 > > > > > --- a/mm/filemap.c > > > > > +++ b/mm/filemap.c > > > > > @@ -1668,6 +1668,9 @@ unsigned find_get_entries(struct address_space *mapping, > > > > > goto repeat; > > > > > } > > > > > export: > > > > > + if (iter.index < start) > > > > > + continue; > > > > > + > > > > > indices[ret] = iter.index; > > > > > entries[ret] = page; > > > > > if (++ret == nr_entries) > > > > > > > > > > Is this a radix bug? I would never expect: > > > > > > > > > > radix_tree_for_each_slot(slot, &mapping->i_pages, &iter, start) > > > > > > > > > > ...to return entries with an index < start. Without that change above > > > > > we loop forever because dax_layout_busy_page() can't make forward > > > > > progress. I'll dig into the radix code tomorrow, but maybe someone > > > > > else will be me to it overnight. > > > > > > > > If 'start' is within a 2MB entry, iter.index can absolutely be less > > > > than start. I forget exactly what the radix tree code does, but I think > > > > it returns index set to the canonical/base index of the entry. > > > > > > Ok, that makes sense. Then the bug is in dax_layout_busy_page() which > > > needs to increment 'index' by the entry size. This might also explain > > > why not every run sees it because you may get lucky and have a 4K > > > entry. > > > > Hmm, no 2MB entry here. > > > > We go through the first find_get_entries and export: > > > > export start: 0x0 index: 0x0 page: 0x822000a > > export start: 0x0 index: 0x200 page: 0xcc3801a > > > > Then dax_layout_busy_page sets 'start' to 0x201, and find_get_entries returns: > > > > export start: 0x201 index: 0x200 page: 0xcc3801a > > > > ...forevermore. > > Are you sure there's not 2MB entry starting at index 0x200? Because if > there was, we'd get infinite loop exactly as you describe in > dax_layout_busy_page() My debug code was buggy, these are 2MB entries, I'll send out a fix. > AFAICT. And it seems to me lot of other places > iterating over entries are borked in a similar way as they all assume that > doing +1 to the current index is guaranteeing them forward progress. Now > actual breakage resulting from this is limited as only DAX uses multiorder > entries and thus not many of these iterators actually ever get called for > radix tree with multiorder entries (e.g. tmpfs still inserts every 4k subpage > of THP into the radix tree and iteration functions usually handles > tail subpages in a special way). But this would really deserve larger > cleanup. Yeah, it's a subtle detail waiting to trip up new multi-order-radix users. The shift reported in the iterator is 6 in this case. _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-10-06 18:05 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-09-27 11:23 [PATCH] dax: Fix deadlock in dax_lock_mapping_entry() Jan Kara 2018-09-27 13:28 ` Matthew Wilcox 2018-09-27 13:41 ` Jan Kara 2018-09-27 18:22 ` Dan Williams 2018-10-04 16:27 ` Jan Kara 2018-10-05 1:57 ` Dan Williams 2018-10-05 2:52 ` Matthew Wilcox 2018-10-05 4:01 ` Dan Williams 2018-10-05 4:28 ` Dan Williams 2018-10-05 9:54 ` Jan Kara 2018-10-06 18:04 ` Dan Williams
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).