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 19:26, Yang Shi wrote:
> On Wed, Apr 26, 2023 at 10:38 AM Phillip Lougher
> <phillip@squashfs.org.uk> wrote:
>>
>> On 26/04/2023 17:44, Phillip Lougher wrote:
>>> 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).
>> You could try this patch which does that.  Compile tested only.
> The kmalloc_array() may call alloc_page() to trigger this problem too
> IIUC. It should be pre-allocated as well?


That is a much smaller allocation, and so it entirely depends whether it 
is an issue or not.  There are also a number of other small memory 
allocations in the path as well.

The whole point of this patch is to move the *biggest* allocation which 
is the reported issue and then see what happens.   No point in making 
this test patch more involved and complex than necessary at this point.

Phillip

>>    fs/squashfs/page_actor.c     | 10 +---------
>>    fs/squashfs/page_actor.h     |  1 -
>>    fs/squashfs/squashfs_fs_sb.h |  1 +
>>    fs/squashfs/super.c          | 10 ++++++++++
>>    4 files changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/squashfs/page_actor.c b/fs/squashfs/page_actor.c
>> index 81af6c4ca115..6cce239eca66 100644
>> --- a/fs/squashfs/page_actor.c
>> +++ b/fs/squashfs/page_actor.c
>> @@ -110,15 +110,7 @@ struct squashfs_page_actor *squashfs_page_actor_init_special(struct squashfs_sb_
>>          if (actor == NULL)
>>                  return NULL;
>>
>> -       if (msblk->decompressor->alloc_buffer) {
>> -               actor->tmp_buffer = kmalloc(PAGE_SIZE, GFP_KERNEL);
>> -
>> -               if (actor->tmp_buffer == NULL) {
>> -                       kfree(actor);
>> -                       return NULL;
>> -               }
>> -       } else
>> -               actor->tmp_buffer = NULL;
>> +       actor->tmp_buffer = msblk->actor_page;
>>
>>          actor->length = length ? : pages * PAGE_SIZE;
>>          actor->page = page;
>> diff --git a/fs/squashfs/page_actor.h b/fs/squashfs/page_actor.h
>> index 97d4983559b1..df5e999afa42 100644
>> --- a/fs/squashfs/page_actor.h
>> +++ b/fs/squashfs/page_actor.h
>> @@ -34,7 +34,6 @@ static inline struct page *squashfs_page_actor_free(struct squashfs_page_actor *
>>    {
>>          struct page *last_page = actor->last_page;
>>
>> -       kfree(actor->tmp_buffer);
>>          kfree(actor);
>>          return last_page;
>>    }
>> diff --git a/fs/squashfs/squashfs_fs_sb.h b/fs/squashfs/squashfs_fs_sb.h
>> index 72f6f4b37863..8feddc9e6cce 100644
>> --- a/fs/squashfs/squashfs_fs_sb.h
>> +++ b/fs/squashfs/squashfs_fs_sb.h
>> @@ -47,6 +47,7 @@ struct squashfs_sb_info {
>>          struct squashfs_cache                   *block_cache;
>>          struct squashfs_cache                   *fragment_cache;
>>          struct squashfs_cache                   *read_page;
>> +       void                                    *actor_page;
>>          int                                     next_meta_index;
>>          __le64                                  *id_table;
>>          __le64                                  *fragment_index;
>> diff --git a/fs/squashfs/super.c b/fs/squashfs/super.c
>> index e090fae48e68..674dc187d961 100644
>> --- a/fs/squashfs/super.c
>> +++ b/fs/squashfs/super.c
>> @@ -329,6 +329,15 @@ static int squashfs_fill_super(struct super_block *sb, struct fs_context *fc)
>>                  goto failed_mount;
>>          }
>>
>> +
>> +       /* Allocate page for squashfs_readahead()/squashfs_read_folio() */
>> +       if (msblk->decompressor->alloc_buffer) {
>> +               msblk->actor_page = kmalloc(PAGE_SIZE, GFP_KERNEL);
>> +
>> +               if(msblk->actor_page == NULL)
>> +                       goto failed_mount;
>> +       }
>> +
>>          msblk->stream = squashfs_decompressor_setup(sb, flags);
>>          if (IS_ERR(msblk->stream)) {
>>                  err = PTR_ERR(msblk->stream);
>> @@ -454,6 +463,7 @@ static int squashfs_fill_super(struct super_block *sb, struct fs_context *fc)
>>          squashfs_cache_delete(msblk->block_cache);
>>          squashfs_cache_delete(msblk->fragment_cache);
>>          squashfs_cache_delete(msblk->read_page);
>> +       kfree(msblk->actor_page);
>>          msblk->thread_ops->destroy(msblk);
>>          kfree(msblk->inode_lookup_table);
>>          kfree(msblk->fragment_index);
>> --
>> 2.35.1
>>
>>> 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
>>>
>>>
>>>
>>>