| Message ID | PUZPR04MB63168406D20B7CF3B287812281B72@PUZPR04MB6316.apcprd04.prod.outlook.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] exfat: do not clear volume dirty flag during sync | expand |
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
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.
> 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.
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!
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
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(-)