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

["Sungjong Seo" <sj1557.seo@samsung.com>]

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