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

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

[Jianan Huang]

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;
+	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);
-- 
2.43.0



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

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

[Chao Yu via Linux-f2fs-devel]

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

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

[Jianan Huang]

Chao Yu <chao@kernel.org> 于2026年9月9日周三 19:43写道：
>
> 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()?
>

Sorry for the late reply. rwsem_is_locked() doesn't tell us whether
current owns the lock, so I'm concerned about the following race:

Freezer A                          Directory fsync B
holds s_umount write lock
stage = SB_FREEZE_PAGEFAULT
holder = current_A
locks gc_lock
                                   holder = current_B
                                   waits for gc_lock
holder != current_A
s_umount read trylock fails
retry limit -> QUOTA_SKIP_FLUSH
sets QUOTA_NEED_FLUSH
commits CP with quota_need_fsck
clears IS_DIRTY
unlocks gc_lock
holder = NULL
stage = SB_FREEZE_FS
                                   locks gc_lock
                                   !IS_DIRTY && CP_SYNC
                                     -> returns before quota flush
                                   unlocks gc_lock
                                   holder = NULL
completes f2fs_freeze()
unlocks s_umount
FIFREEZE returns

Could we use a separate f2fs_sync_fs_super() wrapper for the VFS callback
and keep freeze holder management there, leaving f2fs_sync_fs() for
internal callers?

Thanks,

> 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

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

[Chao Yu via Linux-f2fs-devel]

On 9/15/26 19:39, Jianan Huang wrote:
> Chao Yu <chao@kernel.org> 于2026年9月9日周三 19:43写道：
>>
>> 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()?
>>
> 
> Sorry for the late reply. rwsem_is_locked() doesn't tell us whether
> current owns the lock, so I'm concerned about the following race:
> 
> Freezer A                          Directory fsync B
> holds s_umount write lock
> stage = SB_FREEZE_PAGEFAULT
> holder = current_A
> locks gc_lock
>                                    holder = current_B
>                                    waits for gc_lock

Oh, right, fsync won't call sb_start_*().

One more question, if we set holder under gc_lock, will it resolve this race case?

Thanks,

> holder != current_A
> s_umount read trylock fails
> retry limit -> QUOTA_SKIP_FLUSH
> sets QUOTA_NEED_FLUSH
> commits CP with quota_need_fsck
> clears IS_DIRTY
> unlocks gc_lock
> holder = NULL
> stage = SB_FREEZE_FS
>                                    locks gc_lock
>                                    !IS_DIRTY && CP_SYNC
>                                      -> returns before quota flush
>                                    unlocks gc_lock
>                                    holder = NULL
> completes f2fs_freeze()
> unlocks s_umount
> FIFREEZE returns
> 
> Could we use a separate f2fs_sync_fs_super() wrapper for the VFS callback
> and keep freeze holder management there, leaving f2fs_sync_fs() for
> internal callers?
> 
> Thanks,
> 
>> 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

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

[Jianan Huang]

Chao Yu <chao@kernel.org> 于2026年9月16日周三 09:59写道：
>
> On 9/15/26 19:39, Jianan Huang wrote:
> > Chao Yu <chao@kernel.org> 于2026年9月9日周三 19:43写道：
> >>
> >> 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()?
> >>
> >
> > Sorry for the late reply. rwsem_is_locked() doesn't tell us whether
> > current owns the lock, so I'm concerned about the following race:
> >
> > Freezer A                          Directory fsync B
> > holds s_umount write lock
> > stage = SB_FREEZE_PAGEFAULT
> > holder = current_A
> > locks gc_lock
> >                                    holder = current_B
> >                                    waits for gc_lock
>
> Oh, right, fsync won't call sb_start_*().
>
> One more question, if we set holder under gc_lock, will it resolve this race case?
>

Setting holder under gc_lock prevents the overwrite race, but a later
checkpoint, even from a direct fsync, may outlive the freezer's s_umount
write lock:

Freezer A                       Fsync task / checkpoint worker B
holds s_umount for write
finishes its own checkpoint
                                acquires gc_lock
                                sees SB_FREEZE_PAGEFAULT
                                sets holder = current B
                                computes need_lock = false
freeze may fails on dirty check
without waiting for B
releases s_umount
                                runs quota sync without s_umount

So I think we still need to identify whether the current task actually
holds s_umount.

Thanks,

> Thanks,
>
> > holder != current_A
> > s_umount read trylock fails
> > retry limit -> QUOTA_SKIP_FLUSH
> > sets QUOTA_NEED_FLUSH
> > commits CP with quota_need_fsck
> > clears IS_DIRTY
> > unlocks gc_lock
> > holder = NULL
> > stage = SB_FREEZE_FS
> >                                    locks gc_lock
> >                                    !IS_DIRTY && CP_SYNC
> >                                      -> returns before quota flush
> >                                    unlocks gc_lock
> >                                    holder = NULL
> > completes f2fs_freeze()
> > unlocks s_umount
> > FIFREEZE returns
> >
> > Could we use a separate f2fs_sync_fs_super() wrapper for the VFS callback
> > and keep freeze holder management there, leaving f2fs_sync_fs() for
> > internal callers?
> >
> > Thanks,
> >
> >> 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