Re: [f2fs-dev] [PATCH] f2fs: quota: fix quota flush failure during filesystem freeze

[Chao Yu via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net>]

On 9/8/26 11:48, Jianan Huang wrote:
> Quota flush retries can be exhausted during filesystem freeze.
> 
> freeze_super() and the quota checkpoint may race as below.
> 
> freeze_super()                       f2fs_ckpt
> - down_write(&sb->s_umount)
> - sb->s_writers.frozen = SB_FREEZE_PAGEFAULT
> - sync_filesystem()
>  - f2fs_sync_fs()
>   - f2fs_issue_checkpoint()
>    - queue CP_SYNC -----------------> - block_operations()
>                                       - down_read_trylock(s_umount)
>                                         : fails; freeze holds write lock
>                                       - quota flush retries exhausted
>                                       - set CP_QUOTA_NEED_FSCK_FLAG
> 
> During the PAGEFAULT sync pass, freeze_super() holds s_umount for
> write, but umount_lock_holder is not set until f2fs_freeze().  Thus
> quota writeback tries to acquire s_umount again and can exhaust retries.
> 
> With checkpoint merge enabled, f2fs_issue_checkpoint() may dispatch
> CP_SYNC to f2fs_ckpt.  That thread also cannot acquire the freeze lock.
> 
> Split the F2FS-internal sync implementation from the super-operation
> callback.  For the VFS PAGEFAULT freeze sync, record current as holder
> around the internal sync.  This lets quota writeback use the existing
> lock and makes CP_SYNC run directly in the freeze caller.
> 
> Convert F2FS-internal callers to the internal helper so they cannot be
> mistaken for the freeze owner while the filesystem is frozen.
> 
> Fixes: eb85c2410d6f ("f2fs: quota: fix to avoid warning in dquot_writeback_dquots()")
> Signed-off-by: Jianan Huang <jnhuang95@gmail.com>
> ---
>  fs/f2fs/f2fs.h    |  2 +-
>  fs/f2fs/file.c    |  8 ++++----
>  fs/f2fs/namei.c   | 16 ++++++++--------
>  fs/f2fs/segment.c |  4 ++--
>  fs/f2fs/super.c   | 25 ++++++++++++++++++++++---
>  5 files changed, 37 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 9940a6cecf1a..169792a08a82 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -3977,7 +3977,7 @@ void f2fs_quota_off_umount(struct super_block *sb);
>  void f2fs_save_errors(struct f2fs_sb_info *sbi, unsigned char flag);
>  void f2fs_handle_error(struct f2fs_sb_info *sbi, unsigned char error);
>  int f2fs_commit_super(struct f2fs_sb_info *sbi, bool recover);
> -int f2fs_sync_fs(struct super_block *sb, int sync);
> +int __f2fs_sync_fs(struct super_block *sb, int sync);
>  int f2fs_sanity_check_ckpt(struct f2fs_sb_info *sbi);
>  
>  /*
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index edc352569e87..83546a053656 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -408,7 +408,7 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
>  
>  	if (cp_reason) {
>  		/* all the dirty node pages should be flushed for POR */
> -		ret = f2fs_sync_fs(inode->i_sb, 1);
> +		ret = __f2fs_sync_fs(inode->i_sb, 1);
>  
>  		/*
>  		 * We've secured consistency through sync_fs. Following pino
> @@ -2548,7 +2548,7 @@ int f2fs_do_shutdown(struct f2fs_sb_info *sbi, unsigned int flag,
>  		break;
>  	case F2FS_GOING_DOWN_METASYNC:
>  		/* do checkpoint only */
> -		ret = f2fs_sync_fs(sb, 1);
> +		ret = __f2fs_sync_fs(sb, 1);
>  		if (ret) {
>  			if (ret == -EIO)
>  				ret = 0;
> @@ -2568,7 +2568,7 @@ int f2fs_do_shutdown(struct f2fs_sb_info *sbi, unsigned int flag,
>  		set_sbi_flag(sbi, SBI_CP_DISABLED_QUICK);
>  		set_sbi_flag(sbi, SBI_IS_DIRTY);
>  		/* do checkpoint only */
> -		ret = f2fs_sync_fs(sb, 1);
> +		ret = __f2fs_sync_fs(sb, 1);
>  		if (ret == -EIO)
>  			ret = 0;
>  		goto out;
> @@ -2988,7 +2988,7 @@ static int f2fs_ioc_write_checkpoint(struct file *filp)
>  	if (ret)
>  		return ret;
>  
> -	ret = f2fs_sync_fs(sbi->sb, 1);
> +	ret = __f2fs_sync_fs(sbi->sb, 1);
>  
>  	mnt_drop_write_file(filp);
>  	return ret;
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index ff86ee07290d..5f6f5db9e849 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -403,7 +403,7 @@ static int f2fs_create(struct mnt_idmap *idmap, struct inode *dir,
>  	d_instantiate_new(dentry, inode);
>  
>  	if (IS_DIRSYNC(dir)) {
> -		err = f2fs_sync_fs(sbi->sb, 1);
> +		err = __f2fs_sync_fs(sbi->sb, 1);
>  		if (err)
>  			return err;
>  	}
> @@ -459,7 +459,7 @@ static int f2fs_link(struct dentry *old_dentry, struct inode *dir,
>  	d_instantiate(dentry, inode);
>  
>  	if (IS_DIRSYNC(dir)) {
> -		err = f2fs_sync_fs(sbi->sb, 1);
> +		err = __f2fs_sync_fs(sbi->sb, 1);
>  		if (err)
>  			return err;
>  	}
> @@ -641,7 +641,7 @@ static int f2fs_unlink(struct inode *dir, struct dentry *dentry)
>  		d_invalidate(dentry);
>  
>  	if (IS_DIRSYNC(dir))
> -		err = f2fs_sync_fs(F2FS_I_SB(dir)->sb, 1);
> +		err = __f2fs_sync_fs(F2FS_I_SB(dir)->sb, 1);
>  out:
>  	trace_f2fs_unlink_exit(d_inode(dentry), err);
>  	return err;
> @@ -729,7 +729,7 @@ static int f2fs_symlink(struct mnt_idmap *idmap, struct inode *dir,
>  	ret = filemap_write_and_wait_range(inode->i_mapping, 0,
>  					   disk_link.len - 1);
>  	if (!ret && IS_DIRSYNC(dir))
> -		err = f2fs_sync_fs(sbi->sb, 1);
> +		err = __f2fs_sync_fs(sbi->sb, 1);
>  
>  	f2fs_balance_fs(sbi, true);
>  out:
> @@ -787,7 +787,7 @@ static struct dentry *f2fs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
>  	d_instantiate_new(dentry, inode);
>  
>  	if (IS_DIRSYNC(dir)) {
> -		err = f2fs_sync_fs(sbi->sb, 1);
> +		err = __f2fs_sync_fs(sbi->sb, 1);
>  		if (err)
>  			return ERR_PTR(err);
>  	}
> @@ -845,7 +845,7 @@ static int f2fs_mknod(struct mnt_idmap *idmap, struct inode *dir,
>  	d_instantiate_new(dentry, inode);
>  
>  	if (IS_DIRSYNC(dir)) {
> -		err = f2fs_sync_fs(sbi->sb, 1);
> +		err = __f2fs_sync_fs(sbi->sb, 1);
>  		if (err)
>  			return err;
>  	}
> @@ -1148,7 +1148,7 @@ static int f2fs_rename(struct mnt_idmap *idmap, struct inode *old_dir,
>  	f2fs_unlock_op(sbi, &lc);
>  
>  	if (IS_DIRSYNC(old_dir) || IS_DIRSYNC(new_dir)) {
> -		err = f2fs_sync_fs(sbi->sb, 1);
> +		err = __f2fs_sync_fs(sbi->sb, 1);
>  		if (err)
>  			return err;
>  	}
> @@ -1318,7 +1318,7 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
>  	f2fs_unlock_op(sbi, &lc);
>  
>  	if (IS_DIRSYNC(old_dir) || IS_DIRSYNC(new_dir)) {
> -		err = f2fs_sync_fs(sbi->sb, 1);
> +		err = __f2fs_sync_fs(sbi->sb, 1);
>  		if (err)
>  			return err;
>  	}
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 63b712d3d599..9a09f2525b70 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -550,7 +550,7 @@ void f2fs_balance_fs_bg(struct f2fs_sb_info *sbi, bool from_bg)
>  		mutex_unlock(&sbi->flush_lock);
>  	}
>  	stat_inc_cp_call_count(sbi, BACKGROUND);
> -	f2fs_sync_fs(sbi->sb, 1);
> +	__f2fs_sync_fs(sbi->sb, 1);
>  }
>  
>  static int __submit_flush_wait(struct f2fs_sb_info *sbi,
> @@ -3478,7 +3478,7 @@ int f2fs_allocate_pinning_section(struct f2fs_sb_info *sbi)
>  				true, ZONED_PIN_SEC_REQUIRED_COUNT, true);
>  		if (err)
>  			return err;
> -		err = f2fs_sync_fs(sbi->sb, 1);
> +		err = __f2fs_sync_fs(sbi->sb, 1);
>  		if (!err) {
>  			gc_required = false;
>  			goto retry;
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 1314b6ccced9..a82065146849 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -2131,7 +2131,7 @@ static void f2fs_put_super(struct super_block *sb)
>  	}
>  }
>  
> -int f2fs_sync_fs(struct super_block *sb, int sync)
> +int __f2fs_sync_fs(struct super_block *sb, int sync)
>  {
>  	struct f2fs_sb_info *sbi = F2FS_SB(sb);
>  	int err = 0;
> @@ -2154,6 +2154,25 @@ int f2fs_sync_fs(struct super_block *sb, int sync)
>  	return err;
>  }
>  
> +static int f2fs_sync_fs(struct super_block *sb, int sync)
> +{
> +	struct f2fs_sb_info *sbi = F2FS_SB(sb);
> +	bool freeze_sync = sync &&
> +			sb->s_writers.frozen == SB_FREEZE_PAGEFAULT;

How about this to make sure freeze_sync will only be true in freeze_super()?

bool freeze_sync = sync && rwsem_is_locked(&sb->s_umount) &&
			sb->s_writers.frozen == SB_FREEZE_PAGEFAULT;

And it's not needed to rename f2fs_sync_fs() to __f2fs_sync_fs()?

Thanks,

> +	int err;
> +
> +	/* freeze_super() holds s_umount for write during this sync pass. */
> +	if (freeze_sync)
> +		sbi->umount_lock_holder = current;
> +
> +	err = __f2fs_sync_fs(sb, sync);
> +
> +	if (freeze_sync)
> +		sbi->umount_lock_holder = NULL;
> +
> +	return err;
> +}
> +
>  static int f2fs_freeze(struct super_block *sb)
>  {
>  	struct f2fs_sb_info *sbi = F2FS_SB(sb);
> @@ -2807,7 +2826,7 @@ static int f2fs_enable_checkpoint(struct f2fs_sb_info *sbi)
>  	set_sbi_flag(sbi, SBI_IS_DIRTY);
>  	f2fs_up_write_trace(&sbi->gc_lock, &lc);
>  
> -	ret = f2fs_sync_fs(sbi->sb, 1);
> +	ret = __f2fs_sync_fs(sbi->sb, 1);
>  	if (ret)
>  		f2fs_err(sbi, "%s sync_fs failed, ret: %d", __func__, ret);
>  
> @@ -2993,7 +3012,7 @@ static int __f2fs_remount(struct fs_context *fc, struct super_block *sb)
>  
>  		set_sbi_flag(sbi, SBI_IS_DIRTY);
>  		set_sbi_flag(sbi, SBI_IS_CLOSE);
> -		err = f2fs_sync_fs(sb, 1);
> +		err = __f2fs_sync_fs(sb, 1);
>  		if (err)
>  			goto restore_gc;
>  		clear_sbi_flag(sbi, SBI_IS_CLOSE);



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel