linux-kernel - Re: [PATCH] exfat: do not clear VolumeDirty in writeback

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <TYAPR01MB53531DB8B19324F7D60EE9AA90039@TYAPR01MB5353.jpnprd01.prod.outlook.com>
Date:   Wed, 2 Mar 2022 09:29:45 +0000
From:   "Kohada.Tetsuhiro@...MitsubishiElectric.co.jp" 
        <Kohada.Tetsuhiro@...MitsubishiElectric.co.jp>
To:     "Yuezhang.Mo@...y.com" <Yuezhang.Mo@...y.com>,
        "linkinjeon@...nel.org" <linkinjeon@...nel.org>,
        "sj1557.seo@...sung.com" <sj1557.seo@...sung.com>
CC:     "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] exfat: do not clear VolumeDirty in writeback

Hi, Yuezhang,Mo

Thank for your comments.

>>> And VolumeDirty will be set again when updating the parent directory. 
>>> It means that BootSector will be written twice in each writeback, that will shorten the life of the device.
>> 
>> I have the same concern.
>> From a lifespan point of view, we should probably clear dirty with just sync_fs().
>
>If it is acceptable for VolumeDirty to remain dirty after all updates are complete, I think it is a good idea.

This patch will keep VOL_DIRTY until sync_fs or umount when default mount.
It's a preferred change for device life and VOL_DIRTY integrity. 
On the other hand, we should think more about the behavior when SB_SYNCHRONOUS is set.
For example, FATFS keep VOL_DIRTY until umount regardless of SB_SYNCHRONOUS. 
When SB_SYNCHRONOUS is enabled, updating VOL_DIRTY every time will increase the number of writes to the boot-sector.
For NAND flash devices, mounts with 'sync' are a dangerous option that can drastically wear out their lifespan.

>(PS: The original logic is to clear VolumeDirty after BitMap, FAT and directory entries are updated.)

However, the writing order was not guaranteed.
More synchronous writes are needed to guarantee the write order.

>>>  	sync_blockdev(sb->s_bdev);
>>> -	if (exfat_clear_volume_dirty(sb))
>>> +	if (__exfat_clear_volume_dirty(sb))
>> 
>> If SB_SYNCHRONOUS or SB_DIRSYNC is not present, isn't dirty cleared?
>
>With this patch, exfat_clear_volume_dirty() will not clear VolumeDirty if SB_SYNCHRONOUS or SB_DIRSYNC is not present, and __exfat_clear_volume_dirty() will clear VolumeDirty unconditionally.

__exfat_clear_volume_dirty() only mark_buffer_dirty() to boot-sector, it doesn't sync.
It should sync in here or exfat_set_vol_flags().

>>> +int exfat_clear_volume_dirty(struct super_block *sb) {
>>> +	if (sb->s_flags & (SB_SYNCHRONOUS | SB_DIRSYNC))
>>> +		return __exfat_clear_volume_dirty(sb);
>> 
>> Even when only one of SB or DIR is synced, dirty will be cleared.
>> Isn't it necessary to have both SB_SYNCHRONOUS and SB_DIRSYNC?
>
>VolumeDirty will be cleared if one of SB_SYNCHRONOUS and SB_DIRSYNC is set.
>The condition of (sb->s_flags & (SB_SYNCHRONOUS | SB_DIRSYNC)) is exactly that.

Even if dir-entries is synced, dirty must not be cleared when FAT / mirrorFAT is not synced.
Also, it is not necessary to clear VOL_DIRTY even if SB_DIRSYNC is set.
I don't think it is necessary to check SB_DIRSYNC.

>> And, I think it would be better to use IS_SYNC or IS_DIRSYNC macro here.
>
>If use IS_SYNC or IS_DIRSYNC, we should pass `inode` as an argument, it will be a big change for code.
>And if open a file with O_SYNC, IS_DIRSYNC and IS_SYNC will be true, VolumeDirty will be cleared. 
>So I think it is not necessary to use IS_DIRSYNC and IS_SYNC.

exactly.

BR