Re: [PATCH 1/1] mm/oom_kill: trigger the oom killer if oom occurs without __GFP_FS

[Phillip Lougher <phillip@squashfs.org.uk>]

On 26/04/2023 12:07, Hui Wang wrote:
>
> On 4/26/23 16:33, Michal Hocko wrote:
>> [CC squashfs maintainer]
>>
>> On Wed 26-04-23 13:10:30, Hui Wang wrote:
>>> If we run the stress-ng in the filesystem of squashfs, the system
>>> will be in a state something like hang, the stress-ng couldn't
>>> finish running and the console couldn't react to users' input.
>>>
>>> This issue happens on all arm/arm64 platforms we are working on,
>>> through debugging, we found this issue is introduced by oom handling
>>> in the kernel.
>>>
>>> The fs->readahead() is called between memalloc_nofs_save() and
>>> memalloc_nofs_restore(), and the squashfs_readahead() calls
>>> alloc_page(), in this case, if there is no memory left, the
>>> out_of_memory() will be called without __GFP_FS, then the oom killer
>>> will not be triggered and this process will loop endlessly and wait
>>> for others to trigger oom killer to release some memory. But for a
>>> system with the whole root filesystem constructed by squashfs,
>>> nearly all userspace processes will call out_of_memory() without
>>> __GFP_FS, so we will see that the system enters a state something like
>>> hang when running stress-ng.
>>>
>>> To fix it, we could trigger a kthread to call page_alloc() with
>>> __GFP_FS before returning from out_of_memory() due to without
>>> __GFP_FS.
>> I do not think this is an appropriate way to deal with this issue.
>> Does it even make sense to trigger OOM killer for something like
>> readahead? Would it be more mindful to fail the allocation instead?
>> That being said should allocations from squashfs_readahead use
>> __GFP_RETRY_MAYFAIL instead?
>
> Thanks for your comment, and this issue could hardly be reproduced on 
> ext4 filesystem, that is because the ext4->readahead() doesn't call 
> alloc_page(). If changing the ext4->readahead() as below, it will be 
> easy to reproduce this issue with the ext4 filesystem (repeatedly run: 
> $stress-ng --bigheap ${num_of_cpu_threads} --sequential 0 --timeout 
> 30s --skip-silent --verbose)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index ffbbd9626bd8..8b9db0b9d0b8 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -3114,12 +3114,18 @@ static int ext4_read_folio(struct file *file, 
> struct folio *folio)
>  static void ext4_readahead(struct readahead_control *rac)
>  {
>         struct inode *inode = rac->mapping->host;
> +       struct page *tmp_page;
>
>         /* If the file has inline data, no need to do readahead. */
>         if (ext4_has_inline_data(inode))
>                 return;
>
> +       tmp_page = alloc_page(GFP_KERNEL);
> +
>         ext4_mpage_readpages(inode, rac, NULL);
> +
> +       if (tmp_page)
> +               __free_page(tmp_page);
>  }
>
>
> BTW, I applied my patch to the linux-next and ran the oom stress-ng 
> testcases overnight, there is no hang, oops or crash, looks like there 
> is no big problem to use a kthread to trigger the oom killer in this 
> case.
>
> And Hi squashfs maintainer, I checked the code of filesystem, looks 
> like most filesystems will not call alloc_page() in the readahead(), 
> could you please help take a look at this issue, thanks.


This will be because most filesystems don't need to do so. Squashfs is a 
compressed filesystem with large blocks covering much more than one 
page, and it decompresses these blocks in squashfs_readahead().   If 
__readahead_batch() does not return the full set of pages covering the 
Squashfs block, it allocates a temporary page for the decompressors to 
decompress into to "fill in the hole".

What can be done here as far as Squashfs is concerned ....  I could move 
the page allocation out of the readahead path (e.g. do it at mount time).

Adding __GFP_RETRY_MAYFAIL so the alloc() can fail will mean Squashfs 
returning I/O failures due to no memory.  That will cause a lot of 
applications to crash in a low memory situation.  So a crash rather than 
a hang.

Phillip