Re: [PATCH] f2fs: avoid deadlock in gc thread under low memory

[Jaegeuk Kim]

On 04/13, Rokudo Yan wrote:
> There is a potential deadlock in gc thread may happen
> under low memory as below:
> 
> gc_thread_func
>  -f2fs_gc
>   -do_garbage_collect
>    -gc_data_segment
>     -move_data_block
>      -set_page_writeback(fio.encrypted_page);
>      -f2fs_submit_page_write
> as f2fs_submit_page_write try to do io merge when possible, so the
> encrypted_page is marked PG_writeback but may not submit to block
> layer immediately, if system enter low memory when gc thread try
> to move next data block, it may do direct reclaim and enter fs layer
> as below:
>    -move_data_block
>     -f2fs_grab_cache_page(index=?, for_write=false)
>      -grab_cache_page
>       -find_or_create_page
>        -pagecache_get_page
>         -__page_cache_alloc --  __GFP_FS is set
>          -alloc_pages_node
>           -__alloc_pages
>            -__alloc_pages_slowpath
>             -__alloc_pages_direct_reclaim
>              -__perform_reclaim
>               -try_to_free_pages
>                -do_try_to_free_pages
>                 -shrink_zones
>                  -mem_cgroup_soft_limit_reclaim
>                   -mem_cgroup_soft_reclaim
>                    -mem_cgroup_shrink_node
>                     -shrink_node_memcg
>                      -shrink_list
>                       -shrink_inactive_list
>                        -shrink_page_list
>                         -wait_on_page_writeback -- the page is marked
>                        writeback during previous move_data_block call
> 
> the gc thread wait for the encrypted_page writeback complete,
> but as gc thread held sbi->gc_lock, the writeback & sync thread
> may blocked waiting for sbi->gc_lock, so the bio contain the
> encrypted_page may nerver submit to block layer and complete the
> writeback, which cause deadlock. To avoid this deadlock condition,
> we mark the gc thread with PF_MEMALLOC_NOFS flag, then it will nerver
> enter fs layer when try to alloc cache page during move_data_block.
> 
> Signed-off-by: Rokudo Yan <wu-yan@xxxxxxx>
> ---
>  fs/f2fs/gc.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index e020804f7b07..cc71f77b98c8 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -38,6 +38,12 @@ static int gc_thread_func(void *data)
>  
>  	wait_ms = gc_th->min_sleep_time;
>  
> +	/*
> +	 * Make sure that no allocations from gc thread will ever
> +	 * recurse to the fs layer to avoid deadlock as it will
> +	 * hold sbi->gc_lock during garbage collection
> +	 */
> +	memalloc_nofs_save();

I think this cannot cover all the f2fs_gc() call cases. Can we just avoid by:

--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -1233,7 +1233,7 @@ static int move_data_block(struct inode *inode, block_t bidx,
                                CURSEG_ALL_DATA_ATGC : CURSEG_COLD_DATA;

        /* do not read out */
-       page = f2fs_grab_cache_page(inode->i_mapping, bidx, false);
+       page = f2fs_grab_cache_page(inode->i_mapping, bidx, true);
        if (!page)
                return -ENOMEM;

Thanks,

>  	set_freezable();
>  	do {
>  		bool sync_mode, foreground = false;
> -- 
> 2.25.1