diff mbox series

[v2] exfat: do not clear VolumeDirty in writeback

Message ID HK2PR04MB3891D1D0AFAD9CA98B67706B810B9@HK2PR04MB3891.apcprd04.prod.outlook.com (mailing list archive)
State New, archived
Headers show
Series [v2] exfat: do not clear VolumeDirty in writeback | expand

Commit Message

Yuezhang.Mo@sony.com March 10, 2022, 8:18 a.m. UTC 
Before this commit, VolumeDirty will be cleared first in
writeback if 'dirsync' or 'sync' is not enabled. If the power
is suddenly cut off after cleaning VolumeDirty but other
updates are not written, the exFAT filesystem will not be able
to detect the power failure in the next mount.

And VolumeDirty will be set again but not cleared when updating
the parent directory. It means that BootSector will be written at
least once in each write-back, which will shorten the life of the
device.

Reviewed-by: Andy.Wu <Andy.Wu@sony.com>
Reviewed-by: Aoyama, Wataru <wataru.aoyama@sony.com>
Signed-off-by: Yuezhang.Mo <Yuezhang.Mo@sony.com>
---
 fs/exfat/file.c  |  2 --
 fs/exfat/namei.c |  5 -----
 fs/exfat/super.c | 10 ++--------
 3 files changed, 2 insertions(+), 15 deletions(-)

Comments

Kohada.Tetsuhiro@dc.MitsubishiElectric.co.jp March 11, 2022, 4:34 a.m. UTC | #1 
Hi, Yuezhang,Mo

I think it's good.
It will not be possible to clear dirty automatically, but I think device life and reliable integrity are more important.


> -       if (sync)
> -               sync_dirty_buffer(sbi->boot_bh);
> +       sync_dirty_buffer(sbi->boot_bh);
> +

Use __sync_dirty_buffer() with REQ_FUA/REQ_PREFLUSH instead to guarantee a strict write order (including devices).

BR
T .Kohada
Namjae Jeon March 14, 2022, 11:20 p.m. UTC | #2 
2022-03-11 13:34 GMT+09:00,
Kohada.Tetsuhiro@dc.MitsubishiElectric.co.jp
<Kohada.Tetsuhiro@dc.mitsubishielectric.co.jp>:
> Hi, Yuezhang,Mo
>
> I think it's good.
> It will not be possible to clear dirty automatically, but I think device
> life and reliable integrity are more important.
>
>
>> -       if (sync)
>> -               sync_dirty_buffer(sbi->boot_bh);
>> +       sync_dirty_buffer(sbi->boot_bh);
>> +
>
> Use __sync_dirty_buffer() with REQ_FUA/REQ_PREFLUSH instead to guarantee a
> strict write order (including devices).
Yuezhang, It seems to make sense. Can you check this ?

Thanks!
>
> BR
> T .Kohada
Yuezhang.Mo@sony.com March 16, 2022, 9:17 a.m. UTC | #3 
Hi Namjae, Kohada.Tetsuhiro,

> >> -       if (sync)
> >> -               sync_dirty_buffer(sbi->boot_bh);
> >> +       sync_dirty_buffer(sbi->boot_bh);
> >> +
> >
> > Use __sync_dirty_buffer() with REQ_FUA/REQ_PREFLUSH instead to
> > guarantee a strict write order (including devices).
> Yuezhang, It seems to make sense. Can you check this ?
> 

When call exfat_clear_volume_dirty(sb), all dirty buffers had synced by sync_blockdev(), so I think REQ_FUA/REQ_PREFLUSH is not needed.

```
        sync_blockdev(sb->s_bdev);                                                                                                                            
        if (exfat_clear_volume_dirty(sb))
```

exfat_clear_volume_dirty() is only called in sync or umount context.
In sync or umount context, all requests will be issued with REQ_SYNC regardless of whether REQ_SYNC is
set when submitting buffer.

And since the request of set VolumeDirty is issued with REQ_SYNC. So for simplicity, call sync_dirty_buffer()
unconditionally.

Best Regards,
Yuezhang Mo
Andy.Wu@sony.com March 17, 2022, 3:21 a.m. UTC | #4 
Hi Yuezhang

> When call exfat_clear_volume_dirty(sb), all dirty buffers had synced by
> sync_blockdev(), so I think REQ_FUA/REQ_PREFLUSH is not needed.

I think Kohada-san's meaning is like below:

- sync_dirty_buffer(sbi->boot_bh);
+ __sync_dirty_buffer(sbi->boot_bh, REQ_SYNC | REQ_FUA | REQ_PREFLUSH);

I guess sync_blockdev() won't guarantee sync to non-volatile storage if disk contains cache.

Best Regards
Andy Wu

> -----Original Message-----
> From: Mo, Yuezhang <Yuezhang.Mo@sony.com>
> Sent: Wednesday, March 16, 2022 5:18 PM
> To: Namjae Jeon <linkinjeon@kernel.org>;
> Kohada.Tetsuhiro@dc.MitsubishiElectric.co.jp
> Cc: sj1557.seo@samsung.com; linux-fsdevel@vger.kernel.org;
> linux-kernel@vger.kernel.org; Wu, Andy <Andy.Wu@sony.com>; Aoyama,
> Wataru (SGC) <Wataru.Aoyama@sony.com>
> Subject: RE: [PATCH v2] exfat: do not clear VolumeDirty in writeback
> 
> Hi Namjae, Kohada.Tetsuhiro,
> 
> > >> -       if (sync)
> > >> -               sync_dirty_buffer(sbi->boot_bh);
> > >> +       sync_dirty_buffer(sbi->boot_bh);
> > >> +
> > >
> > > Use __sync_dirty_buffer() with REQ_FUA/REQ_PREFLUSH instead to
> > > guarantee a strict write order (including devices).
> > Yuezhang, It seems to make sense. Can you check this ?
> >
> 
> When call exfat_clear_volume_dirty(sb), all dirty buffers had synced by
> sync_blockdev(), so I think REQ_FUA/REQ_PREFLUSH is not needed.
> 
> ```
>         sync_blockdev(sb->s_bdev);
>         if (exfat_clear_volume_dirty(sb)) ```
> 
> exfat_clear_volume_dirty() is only called in sync or umount context.
> In sync or umount context, all requests will be issued with REQ_SYNC
> regardless of whether REQ_SYNC is set when submitting buffer.
> 
> And since the request of set VolumeDirty is issued with REQ_SYNC. So for
> simplicity, call sync_dirty_buffer() unconditionally.
> 
> Best Regards,
> Yuezhang Mo
>
Kohada.Tetsuhiro@dc.MitsubishiElectric.co.jp March 17, 2022, 9:47 a.m. UTC | #5 
Hi Yuezhang.Mo

> exfat_clear_volume_dirty() is only called in sync or umount context.
> In sync or umount context, all requests will be issued with REQ_SYNC regardless of whether REQ_SYNC is
> set when submitting buffer.
> 
> And since the request of set VolumeDirty is issued with REQ_SYNC. So for simplicity, call sync_dirty_buffer()
> unconditionally.

REQ_FUA and REQ_PREFLUSH may not make much sense on SD cards or USB sticks.
However, the behavior of SCSI/ATAPI type devices with lazy write cache is
 - Issue the SYNCHRONIZE_CACHE command to write all the data to the medium.
 - Issue a WRITE command with FORCE_UNIT_ACCESS (device does not use write cache) for the boot sector.
This guarantees a strict write order.

BR
T.Kohada
Yuezhang.Mo@sony.com March 18, 2022, 7:02 a.m. UTC | #6 
Hi T.Kohada,

> > exfat_clear_volume_dirty() is only called in sync or umount context.
> > In sync or umount context, all requests will be issued with REQ_SYNC
> > regardless of whether REQ_SYNC is set when submitting buffer.
> >
> > And since the request of set VolumeDirty is issued with REQ_SYNC. So
> > for simplicity, call sync_dirty_buffer() unconditionally.
> 
> REQ_FUA and REQ_PREFLUSH may not make much sense on SD cards or USB
> sticks.
> However, the behavior of SCSI/ATAPI type devices with lazy write cache is
>  - Issue the SYNCHRONIZE_CACHE command to write all the data to the
> medium.
>  - Issue a WRITE command with FORCE_UNIT_ACCESS (device does not use
> write cache) for the boot sector.
> This guarantees a strict write order.

Thank you for your detailed explanation.
I will update my patch.

Best Regards,
Yuezhang Mo
diff mbox series

Patch

diff --git a/fs/exfat/file.c b/fs/exfat/file.c
index d890fd34bb2d..2f5130059236 100644
--- a/fs/exfat/file.c
+++ b/fs/exfat/file.c
@@ -218,8 +218,6 @@  int __exfat_truncate(struct inode *inode, loff_t new_size)
 	if (exfat_free_cluster(inode, &clu))
 		return -EIO;
 
-	exfat_clear_volume_dirty(sb);
-
 	return 0;
 }
 
diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c
index af4eb39cc0c3..39c9bdd6b6aa 100644
--- a/fs/exfat/namei.c
+++ b/fs/exfat/namei.c
@@ -554,7 +554,6 @@  static int exfat_create(struct user_namespace *mnt_userns, struct inode *dir,
 	exfat_set_volume_dirty(sb);
 	err = exfat_add_entry(dir, dentry->d_name.name, &cdir, TYPE_FILE,
 		&info);
-	exfat_clear_volume_dirty(sb);
 	if (err)
 		goto unlock;
 
@@ -812,7 +811,6 @@  static int exfat_unlink(struct inode *dir, struct dentry *dentry)
 
 	/* This doesn't modify ei */
 	ei->dir.dir = DIR_DELETED;
-	exfat_clear_volume_dirty(sb);
 
 	inode_inc_iversion(dir);
 	dir->i_mtime = dir->i_atime = current_time(dir);
@@ -846,7 +844,6 @@  static int exfat_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
 	exfat_set_volume_dirty(sb);
 	err = exfat_add_entry(dir, dentry->d_name.name, &cdir, TYPE_DIR,
 		&info);
-	exfat_clear_volume_dirty(sb);
 	if (err)
 		goto unlock;
 
@@ -976,7 +973,6 @@  static int exfat_rmdir(struct inode *dir, struct dentry *dentry)
 		goto unlock;
 	}
 	ei->dir.dir = DIR_DELETED;
-	exfat_clear_volume_dirty(sb);
 
 	inode_inc_iversion(dir);
 	dir->i_mtime = dir->i_atime = current_time(dir);
@@ -1311,7 +1307,6 @@  static int __exfat_rename(struct inode *old_parent_inode,
 		 */
 		new_ei->dir.dir = DIR_DELETED;
 	}
-	exfat_clear_volume_dirty(sb);
 out:
 	return ret;
 }
diff --git a/fs/exfat/super.c b/fs/exfat/super.c
index 8c9fb7dcec16..cb6b87c1d6b9 100644
--- a/fs/exfat/super.c
+++ b/fs/exfat/super.c
@@ -100,7 +100,6 @@  static int exfat_set_vol_flags(struct super_block *sb, unsigned short new_flags)
 {
 	struct exfat_sb_info *sbi = EXFAT_SB(sb);
 	struct boot_sector *p_boot = (struct boot_sector *)sbi->boot_bh->b_data;
-	bool sync;
 
 	/* retain persistent-flags */
 	new_flags |= sbi->vol_flags_persistent;
@@ -119,16 +118,11 @@  static int exfat_set_vol_flags(struct super_block *sb, unsigned short new_flags)
 
 	p_boot->vol_flags = cpu_to_le16(new_flags);
 
-	if ((new_flags & VOLUME_DIRTY) && !buffer_dirty(sbi->boot_bh))
-		sync = true;
-	else
-		sync = false;
-
 	set_buffer_uptodate(sbi->boot_bh);
 	mark_buffer_dirty(sbi->boot_bh);
 
-	if (sync)
-		sync_dirty_buffer(sbi->boot_bh);
+	sync_dirty_buffer(sbi->boot_bh);
+
 	return 0;
 }