Re: [PATCH] lockdep: Teach lockdep about memalloc_noio_save

[Nikolay Borisov]

On  1.03.2017 12:31, Michal Hocko wrote:
> On Wed 01-03-17 11:22:51, Vlastimil Babka wrote:
>> On 03/01/2017 08:48 AM, Nikolay Borisov wrote:
>>> Commit 21caf2fc1931 ("mm: teach mm by current context info to not do I/O
>>> during memory allocation") added the memalloc_noio_(save|restore) functions
>>> to enable people to modify the MM behavior by disbaling I/O during memory
>>> allocation. This prevents allocation paths recursing back into the filesystem
>>> without explicitly changing the flags for every allocation site. Yet, lockdep
>>> not being aware of that is prone to showing false positives. Fix this
>>> by teaching it that the presence of PF_MEMALLOC_NOIO flag mean we are not
>>> going to issue any I/O
>>>
>>> Signed-off-by: Nikolay Borisov <nborisov@xxxxxxxx>
>>> ---
>>>  kernel/locking/lockdep.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
>>> index 9812e5dd409e..5715fdcede28 100644
>>> --- a/kernel/locking/lockdep.c
>>> +++ b/kernel/locking/lockdep.c
>>> @@ -2866,7 +2866,8 @@ static void __lockdep_trace_alloc(gfp_t gfp_mask, unsigned long flags)
>>>  		return;
>>>  
>>>  	/* this guy won't enter reclaim */
>>> -	if ((curr->flags & PF_MEMALLOC) && !(gfp_mask & __GFP_NOMEMALLOC))
>>> +	if (((curr->flags & PF_MEMALLOC) && !(gfp_mask & __GFP_NOMEMALLOC)) ||
>>> +			curr->flags & PF_MEMALLOC_NOIO)
>>
>> It would be slightly better to use memalloc_noio_flags() here. Michal is
>> planning to convert it to take also a new PF_MEMALLOC_NOFS flag into
>> account, and there would be less chance of forgetting to update this place.
> 
> Yes, you are right. The following should do the trick. I am really
> surprised we haven't noticed this before. I thought we were shaving the
> gfp_mask before the allocator goes the lockdep_trace_alloc way. But it
> is not and what is worse SLAB tracks this as well so we cannot rely on
> the proper gfp mask. The positive thing is that the recursion avoidance
> works because we always clear GFP_IO and GFP_FS when doing reclaim.

Okay I will send a revised patch, doing it the way you suggested.

> 
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index 7c38f8f3d97b..0c70b26849ce 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -2861,6 +2861,8 @@ static void __lockdep_trace_alloc(gfp_t gfp_mask, unsigned long flags)
>  	if (unlikely(!debug_locks))
>  		return;
>  
> +	gfp_mask = memalloc_noio_flags(gfp_mask);
> +
>  	/* no reclaim without waiting on it */
>  	if (!(gfp_mask & __GFP_DIRECT_RECLAIM))
>  		return;
>