[PATCH v1] exfat: do not clear volume dirty flag during sync

[PATCH v1] exfat: do not clear volume dirty flag during sync

[Yuezhang.Mo]

[-- Attachment #1: Type: text/plain, Size: 2787 bytes --]

xfstests generic/482 tests the file system consistency after each
FUA operation. It fails when run on exfat.

exFAT clears the volume dirty flag with a FUA operation during sync.
Since s_lock is not held when data is being written to a file, sync
can be executed at the same time. When data is being written to a
file, the FAT chain is updated first, and then the file size is
updated. If sync is executed between updating them, the length of the
FAT chain may be inconsistent with the file size.

To avoid the situation where the file system is inconsistent but the
volume dirty flag is cleared, this commit moves the clearing of the
volume dirty flag from exfat_fs_sync() to exfat_put_super(), so that
the volume dirty flag is not cleared until unmounting. After the
move, there is no additional action during sync, so exfat_fs_sync()
can be deleted.

Signed-off-by: Yuezhang Mo <Yuezhang.Mo@sony.com>
---
 fs/exfat/super.c | 30 +++++++-----------------------
 1 file changed, 7 insertions(+), 23 deletions(-)

diff --git a/fs/exfat/super.c b/fs/exfat/super.c
index 8465033a6cf0..7ed858937d45 100644
--- a/fs/exfat/super.c
+++ b/fs/exfat/super.c
@@ -36,31 +36,12 @@ static void exfat_put_super(struct super_block *sb)
 	struct exfat_sb_info *sbi = EXFAT_SB(sb);
 
 	mutex_lock(&sbi->s_lock);
+	exfat_clear_volume_dirty(sb);
 	exfat_free_bitmap(sbi);
 	brelse(sbi->boot_bh);
 	mutex_unlock(&sbi->s_lock);
 }
 
-static int exfat_sync_fs(struct super_block *sb, int wait)
-{
-	struct exfat_sb_info *sbi = EXFAT_SB(sb);
-	int err = 0;
-
-	if (unlikely(exfat_forced_shutdown(sb)))
-		return 0;
-
-	if (!wait)
-		return 0;
-
-	/* If there are some dirty buffers in the bdev inode */
-	mutex_lock(&sbi->s_lock);
-	sync_blockdev(sb->s_bdev);
-	if (exfat_clear_volume_dirty(sb))
-		err = -EIO;
-	mutex_unlock(&sbi->s_lock);
-	return err;
-}
-
 static int exfat_statfs(struct dentry *dentry, struct kstatfs *buf)
 {
 	struct super_block *sb = dentry->d_sb;
@@ -219,7 +200,6 @@ static const struct super_operations exfat_sops = {
 	.write_inode	= exfat_write_inode,
 	.evict_inode	= exfat_evict_inode,
 	.put_super	= exfat_put_super,
-	.sync_fs	= exfat_sync_fs,
 	.statfs		= exfat_statfs,
 	.show_options	= exfat_show_options,
 	.shutdown	= exfat_shutdown,
@@ -751,10 +731,14 @@ static void exfat_free(struct fs_context *fc)
 
 static int exfat_reconfigure(struct fs_context *fc)
 {
+	struct super_block *sb = fc->root->d_sb;
 	fc->sb_flags |= SB_NODIRATIME;
 
-	/* volume flag will be updated in exfat_sync_fs */
-	sync_filesystem(fc->root->d_sb);
+	sync_filesystem(sb);
+	mutex_lock(&EXFAT_SB(sb)->s_lock);
+	exfat_clear_volume_dirty(sb);
+	mutex_unlock(&EXFAT_SB(sb)->s_lock);
+
 	return 0;
 }
 
-- 
2.43.0

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: v1-0001-exfat-do-not-clear-volume-dirty-flag-during-sync.patch --]
[-- Type: text/x-patch; name="v1-0001-exfat-do-not-clear-volume-dirty-flag-during-sync.patch", Size: 2923 bytes --]

From df0f35df9374000cdfdff45da41953f6699eec63 Mon Sep 17 00:00:00 2001
From: Yuezhang Mo <Yuezhang.Mo@sony.com>
Date: Thu, 10 Apr 2025 17:26:14 -0600
Subject: [PATCH v1] exfat: do not clear volume dirty flag during sync

xfstests generic/482 tests the file system consistency after each
FUA operation. It fails when run on exfat.

exFAT clears the volume dirty flag with a FUA operation during sync.
Since s_lock is not held when data is being written to a file, sync
can be executed at the same time. When data is being written to a
file, the FAT chain is updated first, and then the file size is
updated. If sync is executed between updating them, the length of the
FAT chain may be inconsistent with the file size.

To avoid the situation where the file system is inconsistent but the
volume dirty flag is cleared, this commit moves the clearing of the
volume dirty flag from exfat_fs_sync() to exfat_put_super(), so that
the volume dirty flag is not cleared until unmounting. After the
move, there is no additional action during sync, so exfat_fs_sync()
can be deleted.

Signed-off-by: Yuezhang Mo <Yuezhang.Mo@sony.com>
---
 fs/exfat/super.c | 30 +++++++-----------------------
 1 file changed, 7 insertions(+), 23 deletions(-)

diff --git a/fs/exfat/super.c b/fs/exfat/super.c
index 8465033a6cf0..7ed858937d45 100644
--- a/fs/exfat/super.c
+++ b/fs/exfat/super.c
@@ -36,31 +36,12 @@ static void exfat_put_super(struct super_block *sb)
 	struct exfat_sb_info *sbi = EXFAT_SB(sb);
 
 	mutex_lock(&sbi->s_lock);
+	exfat_clear_volume_dirty(sb);
 	exfat_free_bitmap(sbi);
 	brelse(sbi->boot_bh);
 	mutex_unlock(&sbi->s_lock);
 }
 
-static int exfat_sync_fs(struct super_block *sb, int wait)
-{
-	struct exfat_sb_info *sbi = EXFAT_SB(sb);
-	int err = 0;
-
-	if (unlikely(exfat_forced_shutdown(sb)))
-		return 0;
-
-	if (!wait)
-		return 0;
-
-	/* If there are some dirty buffers in the bdev inode */
-	mutex_lock(&sbi->s_lock);
-	sync_blockdev(sb->s_bdev);
-	if (exfat_clear_volume_dirty(sb))
-		err = -EIO;
-	mutex_unlock(&sbi->s_lock);
-	return err;
-}
-
 static int exfat_statfs(struct dentry *dentry, struct kstatfs *buf)
 {
 	struct super_block *sb = dentry->d_sb;
@@ -219,7 +200,6 @@ static const struct super_operations exfat_sops = {
 	.write_inode	= exfat_write_inode,
 	.evict_inode	= exfat_evict_inode,
 	.put_super	= exfat_put_super,
-	.sync_fs	= exfat_sync_fs,
 	.statfs		= exfat_statfs,
 	.show_options	= exfat_show_options,
 	.shutdown	= exfat_shutdown,
@@ -751,10 +731,14 @@ static void exfat_free(struct fs_context *fc)
 
 static int exfat_reconfigure(struct fs_context *fc)
 {
+	struct super_block *sb = fc->root->d_sb;
 	fc->sb_flags |= SB_NODIRATIME;
 
-	/* volume flag will be updated in exfat_sync_fs */
-	sync_filesystem(fc->root->d_sb);
+	sync_filesystem(sb);
+	mutex_lock(&EXFAT_SB(sb)->s_lock);
+	exfat_clear_volume_dirty(sb);
+	mutex_unlock(&EXFAT_SB(sb)->s_lock);
+
 	return 0;
 }
 
-- 
2.43.0

RE: [PATCH v1] exfat: do not clear volume dirty flag during sync

[Sungjong Seo]

Hi, Yuezhang,
> xfstests generic/482 tests the file system consistency after each
> FUA operation. It fails when run on exfat.
> 
> exFAT clears the volume dirty flag with a FUA operation during sync.
> Since s_lock is not held when data is being written to a file, sync
> can be executed at the same time. When data is being written to a
> file, the FAT chain is updated first, and then the file size is
> updated. If sync is executed between updating them, the length of the
> FAT chain may be inconsistent with the file size.
> 
> To avoid the situation where the file system is inconsistent but the
> volume dirty flag is cleared, this commit moves the clearing of the
> volume dirty flag from exfat_fs_sync() to exfat_put_super(), so that
> the volume dirty flag is not cleared until unmounting. After the
> move, there is no additional action during sync, so exfat_fs_sync()
> can be deleted.

It doesn't seem like FUA is the core issue. To set the volume to a clear
state in sync_filesystem, it might be possible to block the writer_iter,
mkwrite, and truncate operations.

However, as of now, it seems that moving to put_super is the simplest
and most reliable method, and FAT-fs is currently operating in that manner.

However, it seems that a modification is also needed to keep the state
dirty if it is already dirty at the time of mount, as in the FAT-fs below.
commit b88a105802e9 ("fat: mark fs as dirty on mount and clean on umount")

Could you send additional patches along with this patch as a series?

> 
> Signed-off-by: Yuezhang Mo <Yuezhang.Mo@sony.com>
> ---
>  fs/exfat/super.c | 30 +++++++-----------------------
>  1 file changed, 7 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/exfat/super.c b/fs/exfat/super.c
> index 8465033a6cf0..7ed858937d45 100644
> --- a/fs/exfat/super.c
> +++ b/fs/exfat/super.c
> @@ -36,31 +36,12 @@ static void exfat_put_super(struct super_block *sb)
>  	struct exfat_sb_info *sbi = EXFAT_SB(sb);
> 
>  	mutex_lock(&sbi->s_lock);
> +	exfat_clear_volume_dirty(sb);
>  	exfat_free_bitmap(sbi);
>  	brelse(sbi->boot_bh);
>  	mutex_unlock(&sbi->s_lock);
>  }
> 
> -static int exfat_sync_fs(struct super_block *sb, int wait)
> -{
> -	struct exfat_sb_info *sbi = EXFAT_SB(sb);
> -	int err = 0;
> -
> -	if (unlikely(exfat_forced_shutdown(sb)))
> -		return 0;
> -
> -	if (!wait)
> -		return 0;
> -
> -	/* If there are some dirty buffers in the bdev inode */
> -	mutex_lock(&sbi->s_lock);
> -	sync_blockdev(sb->s_bdev);
> -	if (exfat_clear_volume_dirty(sb))
> -		err = -EIO;
> -	mutex_unlock(&sbi->s_lock);
> -	return err;
> -}
> -
>  static int exfat_statfs(struct dentry *dentry, struct kstatfs *buf)
>  {
>  	struct super_block *sb = dentry->d_sb;
> @@ -219,7 +200,6 @@ static const struct super_operations exfat_sops = {
>  	.write_inode	= exfat_write_inode,
>  	.evict_inode	= exfat_evict_inode,
>  	.put_super	= exfat_put_super,
> -	.sync_fs	= exfat_sync_fs,
>  	.statfs		= exfat_statfs,
>  	.show_options	= exfat_show_options,
>  	.shutdown	= exfat_shutdown,
> @@ -751,10 +731,14 @@ static void exfat_free(struct fs_context *fc)
> 
>  static int exfat_reconfigure(struct fs_context *fc)
>  {
> +	struct super_block *sb = fc->root->d_sb;
>  	fc->sb_flags |= SB_NODIRATIME;
> 
> -	/* volume flag will be updated in exfat_sync_fs */
> -	sync_filesystem(fc->root->d_sb);
> +	sync_filesystem(sb);
> +	mutex_lock(&EXFAT_SB(sb)->s_lock);
> +	exfat_clear_volume_dirty(sb);
> +	mutex_unlock(&EXFAT_SB(sb)->s_lock);
> +
>  	return 0;
>  }
> 
> --
> 2.43.0

Re: [PATCH v1] exfat: do not clear volume dirty flag during sync

[Yuezhang.Mo]

Hi Sungjong,

> However, it seems that a modification is also needed to keep the state
> dirty if it is already dirty at the time of mount, as in the FAT-fs below.
> commit b88a105802e9 ("fat: mark fs as dirty on mount and clean on umount")
> 
> Could you send additional patches along with this patch as a series?

This is already supported by the commit.

7018ec68f082 (tag: exfat-for-5.9-rc1) exfat: retain 'VolumeFlags' properly.

RE: [PATCH v1] exfat: do not clear volume dirty flag during sync

[Sungjong Seo]

> Hi Sungjong,
> 
> > However, it seems that a modification is also needed to keep the state
> > dirty if it is already dirty at the time of mount, as in the FAT-fs
> below.
> > commit b88a105802e9 ("fat: mark fs as dirty on mount and clean on
> umount")
> >
> > Could you send additional patches along with this patch as a series?
> 
> This is already supported by the commit.
Oh, sorry about that. I must have misread the code.
If so, your patch moving to put_super is enough.

Reviewed-by: Sungjong Seo <sj1557.seo@samsung.com>

> 
> 7018ec68f082 (tag: exfat-for-5.9-rc1) exfat: retain 'VolumeFlags'
properly.

Re: [PATCH v1] exfat: do not clear volume dirty flag during sync

[Namjae Jeon]

On Thu, Apr 10, 2025 at 6:41 PM Yuezhang.Mo@sony.com
<Yuezhang.Mo@sony.com> wrote:
>
> xfstests generic/482 tests the file system consistency after each
> FUA operation. It fails when run on exfat.
>
> exFAT clears the volume dirty flag with a FUA operation during sync.
> Since s_lock is not held when data is being written to a file, sync
> can be executed at the same time. When data is being written to a
> file, the FAT chain is updated first, and then the file size is
> updated. If sync is executed between updating them, the length of the
> FAT chain may be inconsistent with the file size.
>
> To avoid the situation where the file system is inconsistent but the
> volume dirty flag is cleared, this commit moves the clearing of the
> volume dirty flag from exfat_fs_sync() to exfat_put_super(), so that
> the volume dirty flag is not cleared until unmounting. After the
> move, there is no additional action during sync, so exfat_fs_sync()
> can be deleted.
>
> Signed-off-by: Yuezhang Mo <Yuezhang.Mo@sony.com>
Applied it to #dev with Sunjong's reviewed-by tag.
Thanks!