Re: [PATCH] ext4: Fix deadlock on page reclaim

[Damien Le Moal]

On 2019/07/25 20:54, Christoph Hellwig wrote:
> On Thu, Jul 25, 2019 at 06:33:58PM +0900, Damien Le Moal wrote:
>> +	gfp_t gfp_mask;
>> +
>>  	switch (ext4_inode_journal_mode(inode)) {
>>  	case EXT4_INODE_ORDERED_DATA_MODE:
>>  	case EXT4_INODE_WRITEBACK_DATA_MODE:
>> @@ -4019,6 +4019,14 @@ void ext4_set_aops(struct inode *inode)
>>  		inode->i_mapping->a_ops = &ext4_da_aops;
>>  	else
>>  		inode->i_mapping->a_ops = &ext4_aops;
>> +
>> +	/*
>> +	 * Ensure all page cache allocations are done from GFP_NOFS context to
>> +	 * prevent direct reclaim recursion back into the filesystem and blowing
>> +	 * stacks or deadlocking.
>> +	 */
>> +	gfp_mask = mapping_gfp_mask(inode->i_mapping);
>> +	mapping_set_gfp_mask(inode->i_mapping, (gfp_mask & ~(__GFP_FS)));
> 
> This looks like something that could hit every file systems, so
> shouldn't we fix this in common code?  We could also look into
> just using memalloc_nofs_save for the page cache allocation path
> instead of the per-mapping gfp_mask.

I agree. I did a quick scan and it looks to me like not all file systems are
using GFP_NOFS when grabbing pages for read or write. XFS, btrfs, f2fs, jfs,
gfs2 and ramfs seem OK, but I did not dig very deep for others where the use of
GFP_NOFS is not obvious.

I am not sure though how to approach a global fix. At the very least, it may be
good to have GFP_NOFS set by default in the inode mapping flags in
__address_space_init_once() which is called from inode_init_once(). But I am not
sure if that is enough nor if all file systems are using this function.

The other method as you suggest would be to add calls to
memalloc_nofs_save/restore() in functions like grab_cache_page_write_begin(),
but since that would cover writes only, we may want to do that at a higher level
in the various generic_xxx() and mpage_xxx() helper functions to cover more
ground. But that would still not be enough for the files systems not using these
helpers (plenty of examples for that for the write path).

Or do we go as high to VFS layer to add memalloc_nofs_save/restore() calls ?
That would be a big hammer fix... I personally think this would be OK though,
but I may be missing some points here.

Thoughts on the best approach ?

Best regards.

-- 
Damien Le Moal
Western Digital Research