* XFS: prevent deadlock in quota code when recursing into filesystem @ 2009-05-29 18:10 Felix Blyakher 2009-05-29 18:10 ` [PATCH] xfs: prevent deadlock in xfs_qm_shake() Felix Blyakher 0 siblings, 1 reply; 7+ messages in thread From: Felix Blyakher @ 2009-05-29 18:10 UTC (permalink / raw) To: xfs We experinced the hang in the quota code when the memory allocation recursed into filesystem, and ended up in the xfs_qm_shake(), which deadlocked itself on an already taken lock. STACK TRACE FOR TASK: 0xe00000306a0f8000 (pdflush) 0 schedule+0x26ec [0xa0000001005a32ac] 1 __mutex_lock_slowpath+0xfc [0xa0000001005a5b3c] 2 mutex_lock+0x4c [0xa0000001005a5c4c] 3 xfs_dqlock+0x1c [0xa000000207d8035c] 4 xfs_qm_shake_freelist+0x9c [0xa000000207d9061c] 5 xfs_qm_shake+0xfc [0xa000000207d90d7c] 6 shrink_slab+0x10c [0xa00000010012d78c] 7 zone_reclaim+0x51c [0xa00000010012df9c] 8 get_page_from_freelist+0x1bc [0xa00000010011f93c] 9 __alloc_pages+0x9c [0xa00000010012037c] 10 alloc_page_interleave+0xfc [0xa000000100159e9c] 11 alloc_pages_current+0x12c [0xa00000010015a34c] 12 find_or_create_page+0x5c [0xa00000010011425c] 13 _xfs_buf_lookup_pages+0x18c [0xa0000002062ecc0c] 14 xfs_buf_get_flags+0xac [0xa0000002062ee90c] 15 xfs_buf_read_flags+0x3c [0xa0000002062f0b9c] 16 xfs_trans_read_buf+0x5c [0xa0000002062cdddc] 17 xfs_qm_dqtobp+0x4bc [0xa000000207d826fc] 18 xfs_qm_dqflush+0x1ac [0xa000000207d82a0c] 19 xfs_qm_sync+0x27c [0xa000000207d8c8fc] 20 xfs_qm_syncall+0x9c [0xa000000207d8a11c] 21 vfs_sync+0x9c [0xa0000002063026bc] 22 dsvfs_sync+0x3c [0xa0000002082d65bc] 23 vfs_sync+0x9c [0xa0000002063026bc] 24 bhvlock_vfsop_sync+0xcc [0xa0000002082b244c] 25 vfs_sync+0x9c [0xa0000002063026bc] 26 xfs_fs_write_super+0x7c [0xa0000002062fef3c] 27 sync_supers+0x1dc [0xa0000001001896bc] 28 wb_kupdate+0x6c [0xa00000010012280c] 29 pdflush+0x3cc [0xa0000001001247cc] 30 kthread+0x23c [0xa0000001000d401c] 31 kernel_thread_helper+0xcc [0xa0000001000133ec] 32 start_kernel_thread+0x1c [0xa0000001000094bc] The following patch (in a separate mail) addresses this problem. Felix _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] xfs: prevent deadlock in xfs_qm_shake() 2009-05-29 18:10 XFS: prevent deadlock in quota code when recursing into filesystem Felix Blyakher @ 2009-05-29 18:10 ` Felix Blyakher 2009-05-29 19:25 ` Christoph Hellwig 0 siblings, 1 reply; 7+ messages in thread From: Felix Blyakher @ 2009-05-29 18:10 UTC (permalink / raw) To: xfs; +Cc: Hedi Berriche It's possible to recurse into filesystem from the memory allocation, which deadlocks in xfs_qm_shake(). Add check for __GFP_FS, and bailout if it is not set. Signed-off-by: Felix Blyakher <felixb@sgi.com> Signed-off-by: Hedi Berriche <hedi@sgi.com> --- fs/xfs/linux-2.6/kmem.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/fs/xfs/linux-2.6/kmem.h b/fs/xfs/linux-2.6/kmem.h index af6843c..d8d2321 100644 --- a/fs/xfs/linux-2.6/kmem.h +++ b/fs/xfs/linux-2.6/kmem.h @@ -103,7 +103,7 @@ extern void *kmem_zone_zalloc(kmem_zone_t *, unsigned int __nocast); static inline int kmem_shake_allow(gfp_t gfp_mask) { - return (gfp_mask & __GFP_WAIT) != 0; + return ((gfp_mask & __GFP_WAIT && gfp_mask & __GFP_FS) != 0; } #endif /* __XFS_SUPPORT_KMEM_H__ */ -- 1.5.4.rc3 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] xfs: prevent deadlock in xfs_qm_shake() 2009-05-29 18:10 ` [PATCH] xfs: prevent deadlock in xfs_qm_shake() Felix Blyakher @ 2009-05-29 19:25 ` Christoph Hellwig 2009-05-29 19:29 ` Felix Blyakher 2009-05-30 10:43 ` Andi Kleen 0 siblings, 2 replies; 7+ messages in thread From: Christoph Hellwig @ 2009-05-29 19:25 UTC (permalink / raw) To: Felix Blyakher; +Cc: Hedi Berriche, xfs On Fri, May 29, 2009 at 01:10:31PM -0500, Felix Blyakher wrote: > It's possible to recurse into filesystem from the memory > allocation, which deadlocks in xfs_qm_shake(). Add check > for __GFP_FS, and bailout if it is not set. > > Signed-off-by: Felix Blyakher <felixb@sgi.com> > Signed-off-by: Hedi Berriche <hedi@sgi.com> > --- > fs/xfs/linux-2.6/kmem.h | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/fs/xfs/linux-2.6/kmem.h b/fs/xfs/linux-2.6/kmem.h > index af6843c..d8d2321 100644 > --- a/fs/xfs/linux-2.6/kmem.h > +++ b/fs/xfs/linux-2.6/kmem.h > @@ -103,7 +103,7 @@ extern void *kmem_zone_zalloc(kmem_zone_t *, unsigned int __nocast); > static inline int > kmem_shake_allow(gfp_t gfp_mask) > { > - return (gfp_mask & __GFP_WAIT) != 0; > + return ((gfp_mask & __GFP_WAIT && gfp_mask & __GFP_FS) != 0; Looks good to me. But this could be written simpler as: return ((gfp_mask & (__GFP_WAIT|__GFP_FS)) != 0; _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xfs: prevent deadlock in xfs_qm_shake() 2009-05-29 19:25 ` Christoph Hellwig @ 2009-05-29 19:29 ` Felix Blyakher 2009-05-30 10:43 ` Andi Kleen 1 sibling, 0 replies; 7+ messages in thread From: Felix Blyakher @ 2009-05-29 19:29 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Hedi Berriche, xfs On May 29, 2009, at 2:25 PM, Christoph Hellwig wrote: > On Fri, May 29, 2009 at 01:10:31PM -0500, Felix Blyakher wrote: >> It's possible to recurse into filesystem from the memory >> allocation, which deadlocks in xfs_qm_shake(). Add check >> for __GFP_FS, and bailout if it is not set. >> >> Signed-off-by: Felix Blyakher <felixb@sgi.com> >> Signed-off-by: Hedi Berriche <hedi@sgi.com> >> --- >> fs/xfs/linux-2.6/kmem.h | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/fs/xfs/linux-2.6/kmem.h b/fs/xfs/linux-2.6/kmem.h >> index af6843c..d8d2321 100644 >> --- a/fs/xfs/linux-2.6/kmem.h >> +++ b/fs/xfs/linux-2.6/kmem.h >> @@ -103,7 +103,7 @@ extern void *kmem_zone_zalloc(kmem_zone_t *, >> unsigned int __nocast); >> static inline int >> kmem_shake_allow(gfp_t gfp_mask) >> { >> - return (gfp_mask & __GFP_WAIT) != 0; >> + return ((gfp_mask & __GFP_WAIT && gfp_mask & __GFP_FS) != 0; > > Looks good to me. But this could be written simpler as: > > return ((gfp_mask & (__GFP_WAIT|__GFP_FS)) != 0; Yeah, sure. Weird it didn't occur to me :) Thanks, Felix _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xfs: prevent deadlock in xfs_qm_shake() 2009-05-29 19:25 ` Christoph Hellwig 2009-05-29 19:29 ` Felix Blyakher @ 2009-05-30 10:43 ` Andi Kleen 2009-05-30 14:57 ` Felix Blyakher 1 sibling, 1 reply; 7+ messages in thread From: Andi Kleen @ 2009-05-30 10:43 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Hedi Berriche, xfs Christoph Hellwig <hch@infradead.org> writes: >> >> diff --git a/fs/xfs/linux-2.6/kmem.h b/fs/xfs/linux-2.6/kmem.h >> index af6843c..d8d2321 100644 >> --- a/fs/xfs/linux-2.6/kmem.h >> +++ b/fs/xfs/linux-2.6/kmem.h >> @@ -103,7 +103,7 @@ extern void *kmem_zone_zalloc(kmem_zone_t *, unsigned int __nocast); >> static inline int >> kmem_shake_allow(gfp_t gfp_mask) >> { >> - return (gfp_mask & __GFP_WAIT) != 0; >> + return ((gfp_mask & __GFP_WAIT && gfp_mask & __GFP_FS) != 0; > > Looks good to me. But this could be written simpler as: > > return ((gfp_mask & (__GFP_WAIT|__GFP_FS)) != 0; That's actually not equivalent. You would need to write (gfp_mask & (__GFP_WAIT|__GFP_FS)) == (__GFP_WAIT|__GFP_FS) which is not simpler. BTW you can usually trust gcc to do all the obvious logical simplifications, so it's best to just write the most readable code (like Felix's original version) And also it can depend on the target what is actually better in machine language, and gcc has more knowledge about that than the programmer. int f(int x) { return x & 1 && x & 2; } gives for gcc 4.3 andl $3, %edi xorl %eax, %eax cmpl $3, %edi sete %al ret -Andi -- ak@linux.intel.com -- Speaking for myself only. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xfs: prevent deadlock in xfs_qm_shake() 2009-05-30 10:43 ` Andi Kleen @ 2009-05-30 14:57 ` Felix Blyakher 2009-05-30 16:14 ` Andi Kleen 0 siblings, 1 reply; 7+ messages in thread From: Felix Blyakher @ 2009-05-30 14:57 UTC (permalink / raw) To: Andi Kleen; +Cc: Christoph Hellwig, Hedi Berriche, xfs On May 30, 2009, at 5:43 AM, Andi Kleen wrote: > Christoph Hellwig <hch@infradead.org> writes: >>> >>> diff --git a/fs/xfs/linux-2.6/kmem.h b/fs/xfs/linux-2.6/kmem.h >>> index af6843c..d8d2321 100644 >>> --- a/fs/xfs/linux-2.6/kmem.h >>> +++ b/fs/xfs/linux-2.6/kmem.h >>> @@ -103,7 +103,7 @@ extern void *kmem_zone_zalloc(kmem_zone_t *, >>> unsigned int __nocast); >>> static inline int >>> kmem_shake_allow(gfp_t gfp_mask) >>> { >>> - return (gfp_mask & __GFP_WAIT) != 0; >>> + return ((gfp_mask & __GFP_WAIT && gfp_mask & __GFP_FS) != 0; >> >> Looks good to me. But this could be written simpler as: >> >> return ((gfp_mask & (__GFP_WAIT|__GFP_FS)) != 0; > > That's actually not equivalent. You would need to write > > (gfp_mask & (__GFP_WAIT|__GFP_FS)) == (__GFP_WAIT|__GFP_FS) Indeed. (gfp_mask & (__GFP_WAIT|__GFP_FS)) != 0 is equivalent to (gfp_mask & __GFP_WAIT || gfp_mask & __GFP_FS) != 0; or "if _any_ of the flags set". In the fix we need "_both_ flags set", which brings us to equally (IMHO) readable (gfp_mask & __GFP_WAIT && gfp_mask & __GFP_FS) != 0 or as Andi noted (gfp_mask & (__GFP_WAIT|__GFP_FS)) == (__GFP_WAIT|__GFP_FS) I'd prefer the former, as in my original patch. Also, I accidentally put an extra open brace in a statement. After a successful build I started playing with braces for more readability, and left it in inconsistent state. Seems like the preferred style in the kernel is as following: return ((gfp_mask & __GFP_WAIT) && (gfp_mask & __GFP_FS)) != 0; If there is no objection, I'll use that. Felix > > > which is not simpler. > > BTW you can usually trust gcc to do all the obvious logical > simplifications, so it's best to just write the most readable code > (like Felix's original version) And also it can depend on the target > what is actually better in machine language, and gcc has more > knowledge about that than the programmer. > > int f(int x) > { > return x & 1 && x & 2; > } > > gives for gcc 4.3 > > andl $3, %edi > xorl %eax, %eax > cmpl $3, %edi > sete %al > ret > > -Andi > > > -- > ak@linux.intel.com -- Speaking for myself only. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xfs: prevent deadlock in xfs_qm_shake() 2009-05-30 14:57 ` Felix Blyakher @ 2009-05-30 16:14 ` Andi Kleen 0 siblings, 0 replies; 7+ messages in thread From: Andi Kleen @ 2009-05-30 16:14 UTC (permalink / raw) To: Felix Blyakher; +Cc: Christoph Hellwig, Andi Kleen, Hedi Berriche, xfs On Sat, May 30, 2009 at 09:57:20AM -0500, Felix Blyakher wrote: > > (gfp_mask & __GFP_WAIT && gfp_mask & __GFP_FS) != 0 or as Andi noted > (gfp_mask & (__GFP_WAIT|__GFP_FS)) == (__GFP_WAIT|__GFP_FS) > > I'd prefer the former, as in my original patch. > > Also, I accidentally put an extra open brace in a statement. After a > successful build I started playing with braces for more readability, > and left it in inconsistent state. > Seems like the preferred style in the kernel is as following: > > return ((gfp_mask & __GFP_WAIT) && (gfp_mask & __GFP_FS)) != 0; I would say it is return (gfp_mask & __GFP_WAIT) && (gfp_mask & __GFP_FS); The != 0 is completely superfluous. -Andi -- ak@linux.intel.com -- Speaking for myself only. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2009-05-30 16:07 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-05-29 18:10 XFS: prevent deadlock in quota code when recursing into filesystem Felix Blyakher 2009-05-29 18:10 ` [PATCH] xfs: prevent deadlock in xfs_qm_shake() Felix Blyakher 2009-05-29 19:25 ` Christoph Hellwig 2009-05-29 19:29 ` Felix Blyakher 2009-05-30 10:43 ` Andi Kleen 2009-05-30 14:57 ` Felix Blyakher 2009-05-30 16:14 ` Andi Kleen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox