* [f2fs-dev] [PATCH] f2fs: quota: fix quota flush failure during filesystem freeze
@ 2026-09-08 3:48 Jianan Huang
2026-09-09 11:42 ` Chao Yu via Linux-f2fs-devel
0 siblings, 1 reply; 5+ messages in thread
From: Jianan Huang @ 2026-09-08 3:48 UTC (permalink / raw)
To: linux-f2fs-devel, chao, jaegeuk; +Cc: 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: quota: fix quota flush failure during filesystem freeze
2026-09-08 3:48 [f2fs-dev] [PATCH] f2fs: quota: fix quota flush failure during filesystem freeze Jianan Huang
@ 2026-09-09 11:42 ` Chao Yu via Linux-f2fs-devel
2026-09-15 11:39 ` Jianan Huang
0 siblings, 1 reply; 5+ messages in thread
From: Chao Yu via Linux-f2fs-devel @ 2026-09-09 11:42 UTC (permalink / raw)
To: Jianan Huang, linux-f2fs-devel, jaegeuk
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: quota: fix quota flush failure during filesystem freeze
2026-09-09 11:42 ` Chao Yu via Linux-f2fs-devel
@ 2026-09-15 11:39 ` Jianan Huang
2026-09-16 1:59 ` Chao Yu via Linux-f2fs-devel
0 siblings, 1 reply; 5+ messages in thread
From: Jianan Huang @ 2026-09-15 11:39 UTC (permalink / raw)
To: Chao Yu; +Cc: jaegeuk, linux-f2fs-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: quota: fix quota flush failure during filesystem freeze
2026-09-15 11:39 ` Jianan Huang
@ 2026-09-16 1:59 ` Chao Yu via Linux-f2fs-devel
2026-09-16 3:25 ` Jianan Huang
0 siblings, 1 reply; 5+ messages in thread
From: Chao Yu via Linux-f2fs-devel @ 2026-09-16 1:59 UTC (permalink / raw)
To: Jianan Huang; +Cc: jaegeuk, 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: quota: fix quota flush failure during filesystem freeze
2026-09-16 1:59 ` Chao Yu via Linux-f2fs-devel
@ 2026-09-16 3:25 ` Jianan Huang
0 siblings, 0 replies; 5+ messages in thread
From: Jianan Huang @ 2026-09-16 3:25 UTC (permalink / raw)
To: Chao Yu; +Cc: jaegeuk, linux-f2fs-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-09-16 3:25 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-08 3:48 [f2fs-dev] [PATCH] f2fs: quota: fix quota flush failure during filesystem freeze Jianan Huang
2026-09-09 11:42 ` Chao Yu via Linux-f2fs-devel
2026-09-15 11:39 ` Jianan Huang
2026-09-16 1:59 ` Chao Yu via Linux-f2fs-devel
2026-09-16 3:25 ` Jianan Huang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.