[PATCH v2] exfat: do not clear VolumeDirty in writeback

[PATCH v2] exfat: do not clear VolumeDirty in writeback

[Yuezhang.Mo]

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

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(-)

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;
 }

[-- Attachment #2: v2-0001-exfat-do-not-clear-VolumeDirty-in-writeback.patch --]
[-- Type: application/octet-stream, Size: 3566 bytes --]

From a63688ed6b9149dbbce224808e903645c5fa72a8 Mon Sep 17 00:00:00 2001
From: "Yuezhang.Mo" <Yuezhang.Mo@sony.com>
Date: Thu, 10 Mar 2022 15:04:47 +0800
Subject: [PATCH v2] exfat: do not clear VolumeDirty in writeback

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(-)

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;
 }
 
-- 
2.25.1

Re: [PATCH v2] exfat: do not clear VolumeDirty in writeback

[Kohada.Tetsuhiro]

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

Re: [PATCH v2] exfat: do not clear VolumeDirty in writeback

[Namjae Jeon]

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

RE: [PATCH v2] exfat: do not clear VolumeDirty in writeback

[Yuezhang.Mo]

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

RE: [PATCH v2] exfat: do not clear VolumeDirty in writeback

[Andy.Wu]

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
> 

Re: [PATCH v2] exfat: do not clear VolumeDirty in writeback

[Kohada.Tetsuhiro]

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

RE: [PATCH v2] exfat: do not clear VolumeDirty in writeback

[Yuezhang.Mo]

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