XFS: prevent deadlock in quota code when recursing into filesystem

XFS: prevent deadlock in quota code when recursing into filesystem

[Felix Blyakher]

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

[PATCH] xfs: prevent deadlock in xfs_qm_shake()

[Felix Blyakher]

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

Re: [PATCH] xfs: prevent deadlock in xfs_qm_shake()

[Christoph Hellwig]

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

Re: [PATCH] xfs: prevent deadlock in xfs_qm_shake()

[Felix Blyakher]

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

Re: [PATCH] xfs: prevent deadlock in xfs_qm_shake()

[Andi Kleen]

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

Re: [PATCH] xfs: prevent deadlock in xfs_qm_shake()

[Felix Blyakher]

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

Re: [PATCH] xfs: prevent deadlock in xfs_qm_shake()

[Andi Kleen]

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