[PATCH] f2fs: fix livelock in f2fs_sync_inode_meta()

[PATCH] f2fs: fix livelock in f2fs_sync_inode_meta()

[Daeho Jeong]

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);
 		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();
 		}
 	}
 	return 0;
-- 
2.55.0.979.g7e5102b832-goog

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

[Chao Yu]

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;

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@kernel.org> wrote:
>
> 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,


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;
>