[PATCH] xfs: prevent deadlock in xfs_qm_shake()

[Felix Blyakher]

On May 30, 2009, at 5:43 AM, Andi Kleen wrote:

> Christoph Hellwig <hch at 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 at linux.intel.com -- Speaking for myself only.