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

[Daeho Jeong]

On Mon, Sep 7, 2026 at 3:23 AM Chao Yu <chao@xxxxxxxxxx> wrote:
>
> On 9/4/26 22:59, Daeho Jeong wrote:
> > From: Daeho Jeong <daehojeong@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> >   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,


Thanks for the review.

1. Regarding list_move_tail in f2fs_sync_dirty_inodes():
I will apply list_move_tail() to f2fs_sync_dirty_inodes() as well in v2.

2. Regarding f2fs_submit_merged_write(sbi, DATA):
The original intention was just a defensive measure in case the freeing inode
was waiting in f2fs_evict_inode() -> filemap_fdatawait() for writeback
completion.
I will remove f2fs_submit_merged_write(sbi, DATA) and keep only cond_resched()
in the else block, and update both f2fs_sync_inode_meta() and
f2fs_sync_dirty_inodes() in v2.

Thanks,
Daeho

>
> >               }
> >       }
> >       return 0;
>