Re: [f2fs-dev] [PATCH] f2fs: fix livelock in f2fs_sync_inode_meta()

[Chao Yu <chao@kernel.org>]

On 9/4/26 22:59, Daeho Jeong wrote:
> From: Daeho Jeong <daehojeong@google.com>
> 
> During checkpoint, f2fs_sync_inode_meta() iterates over dirty inodes in
> the DIRTY_META list. If igrab() fails on an inode (e.g. because it is in
> the freeing state), the loop continues without moving the current inode to
> the tail of the list. As a result, subsequent iterations pick the same
> inode repeatedly, preventing other ready dirty inodes in the list from
> making forward progress and leading to a livelock.
> 
> Fix this by moving the current inode to the tail of the list
> (list_move_tail(&fi->gdirty_list, head)) before attempting igrab().
> 
> Additionally, if igrab() fails, the freeing inode may be waiting for its
> pending writeback data pages to complete during eviction.
> Submit any pending merged data writes and yield the
> CPU with cond_resched() to allow the eviction to make progress.
> 
> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> ---
>   fs/f2fs/checkpoint.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index ef22692cef0a..5597033533b0 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -1460,6 +1460,7 @@ static int f2fs_sync_inode_meta(struct f2fs_sb_info *sbi)
>   		}
>   		fi = list_first_entry(head, struct f2fs_inode_info,
>   							gdirty_list);
> +		list_move_tail(&fi->gdirty_list, head);

Seems fine, if so, do we need to do this in f2fs_sync_dirty_inodes() as well?

>   		inode = igrab(&fi->vfs_inode);
>   		spin_unlock(&sbi->inode_lock[DIRTY_META]);
>   		if (inode) {
> @@ -1469,6 +1470,13 @@ static int f2fs_sync_inode_meta(struct f2fs_sb_info *sbi)
>   			if (is_inode_flag_set(inode, FI_DIRTY_INODE))
>   				f2fs_update_inode_page(inode);
>   			iput(inode);
> +		} else {
> +			/*
> +			 * We should submit bio, since it exists several
> +			 * writebacking pages in the freeing inode.
> +			 */
> +			f2fs_submit_merged_write(sbi, DATA);
> +			cond_resched();

It uses the same implementation from f2fs_sync_dirty_inodes(), after git blame on
it, the implementation was introduce long time ago, I suspect we don't need this?
because in .writepages, we will submit cached bio anyway, right?

Thanks,

>   		}
>   	}
>   	return 0;