| Message ID | 20200616021808.5222-1-kohada.t2@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] exfat: remove EXFAT_SB_DIRTY flag | expand |
> remove EXFAT_SB_DIRTY flag and related codes. > > This flag is set/reset in exfat_put_super()/exfat_sync_fs() to avoid sync_blockdev(). > However ... > - exfat_put_super(): > Before calling this, the VFS has already called sync_filesystem(), so sync is never performed here. > - exfat_sync_fs(): > After calling this, the VFS calls sync_blockdev(), so, it is meaningless to check EXFAT_SB_DIRTY or to > bypass sync_blockdev() here. > Not only that, but in some cases can't clear VOL_DIRTY. > ex: > VOL_DIRTY is set when rmdir starts, but when non-empty-dir is detected, return error without setting > EXFAT_SB_DIRTY. > If performe 'sync' in this state, VOL_DIRTY will not be cleared. There is still a problem if system reboot or device is unplugged when sync is not called yet. I will remove this mention in the patch.
> remove EXFAT_SB_DIRTY flag and related codes. > > This flag is set/reset in exfat_put_super()/exfat_sync_fs() to avoid > sync_blockdev(). > However ... > - exfat_put_super(): > Before calling this, the VFS has already called sync_filesystem(), so sync > is never performed here. > - exfat_sync_fs(): > After calling this, the VFS calls sync_blockdev(), so, it is meaningless > to check EXFAT_SB_DIRTY or to bypass sync_blockdev() here. > Not only that, but in some cases can't clear VOL_DIRTY. > ex: > VOL_DIRTY is set when rmdir starts, but when non-empty-dir is detected, > return error without setting EXFAT_SB_DIRTY. > If performe 'sync' in this state, VOL_DIRTY will not be cleared. > Since this patch does not resolve 'VOL_DIRTY in ENOTEMPTY' problem you mentioned, it would be better to remove the description above for that and to make new patch. > Remove the EXFAT_SB_DIRTY check to ensure synchronization. > And, remove the code related to the flag. > > Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com> Reviewed-by: Sungjong Seo <sj1557.seo@samsung.com> > --- > Changes in v2: > - exfat_sync_fs() avoids synchronous processing when wait=0 Changes in v3: > - fix return value in exfat_sync_fs() > > fs/exfat/balloc.c | 4 ++-- > fs/exfat/dir.c | 16 ++++++++-------- > fs/exfat/exfat_fs.h | 5 +---- > fs/exfat/fatent.c | 7 ++----- > fs/exfat/misc.c | 3 +-- > fs/exfat/namei.c | 12 ++++++------ > fs/exfat/super.c | 14 ++++++-------- > 7 files changed, 26 insertions(+), 35 deletions(-) > > diff --git a/fs/exfat/balloc.c b/fs/exfat/balloc.c index > 4055eb00ea9b..a987919686c0 100644 > --- a/fs/exfat/balloc.c > +++ b/fs/exfat/balloc.c > @@ -158,7 +158,7 @@ int exfat_set_bitmap(struct inode *inode, unsigned int > clu) > b = BITMAP_OFFSET_BIT_IN_SECTOR(sb, ent_idx); > > set_bit_le(b, sbi->vol_amap[i]->b_data); > - exfat_update_bh(sb, sbi->vol_amap[i], IS_DIRSYNC(inode)); > + exfat_update_bh(sbi->vol_amap[i], IS_DIRSYNC(inode)); > return 0; > } > > @@ -180,7 +180,7 @@ void exfat_clear_bitmap(struct inode *inode, unsigned > int clu) > b = BITMAP_OFFSET_BIT_IN_SECTOR(sb, ent_idx); > > clear_bit_le(b, sbi->vol_amap[i]->b_data); > - exfat_update_bh(sb, sbi->vol_amap[i], IS_DIRSYNC(inode)); > + exfat_update_bh(sbi->vol_amap[i], IS_DIRSYNC(inode)); > > if (opts->discard) { > int ret_discard; > diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index > 8e775bd5d523..02acbb6ddf02 100644 > --- a/fs/exfat/dir.c > +++ b/fs/exfat/dir.c > @@ -470,7 +470,7 @@ int exfat_init_dir_entry(struct inode *inode, struct > exfat_chain *p_dir, > &ep->dentry.file.access_date, > NULL); > > - exfat_update_bh(sb, bh, IS_DIRSYNC(inode)); > + exfat_update_bh(bh, IS_DIRSYNC(inode)); > brelse(bh); > > ep = exfat_get_dentry(sb, p_dir, entry + 1, &bh, §or); @@ - > 480,7 +480,7 @@ int exfat_init_dir_entry(struct inode *inode, struct > exfat_chain *p_dir, > exfat_init_stream_entry(ep, > (type == TYPE_FILE) ? ALLOC_FAT_CHAIN : ALLOC_NO_FAT_CHAIN, > start_clu, size); > - exfat_update_bh(sb, bh, IS_DIRSYNC(inode)); > + exfat_update_bh(bh, IS_DIRSYNC(inode)); > brelse(bh); > > return 0; > @@ -516,7 +516,7 @@ int exfat_update_dir_chksum(struct inode *inode, > struct exfat_chain *p_dir, > } > > fep->dentry.file.checksum = cpu_to_le16(chksum); > - exfat_update_bh(sb, fbh, IS_DIRSYNC(inode)); > + exfat_update_bh(fbh, IS_DIRSYNC(inode)); > release_fbh: > brelse(fbh); > return ret; > @@ -538,7 +538,7 @@ int exfat_init_ext_entry(struct inode *inode, struct > exfat_chain *p_dir, > return -EIO; > > ep->dentry.file.num_ext = (unsigned char)(num_entries - 1); > - exfat_update_bh(sb, bh, sync); > + exfat_update_bh(bh, sync); > brelse(bh); > > ep = exfat_get_dentry(sb, p_dir, entry + 1, &bh, §or); @@ - > 547,7 +547,7 @@ int exfat_init_ext_entry(struct inode *inode, struct > exfat_chain *p_dir, > > ep->dentry.stream.name_len = p_uniname->name_len; > ep->dentry.stream.name_hash = cpu_to_le16(p_uniname->name_hash); > - exfat_update_bh(sb, bh, sync); > + exfat_update_bh(bh, sync); > brelse(bh); > > for (i = EXFAT_FIRST_CLUSTER; i < num_entries; i++) { @@ -556,7 > +556,7 @@ int exfat_init_ext_entry(struct inode *inode, struct exfat_chain > *p_dir, > return -EIO; > > exfat_init_name_entry(ep, uniname); > - exfat_update_bh(sb, bh, sync); > + exfat_update_bh(bh, sync); > brelse(bh); > uniname += EXFAT_FILE_NAME_LEN; > } > @@ -580,7 +580,7 @@ int exfat_remove_entries(struct inode *inode, struct > exfat_chain *p_dir, > return -EIO; > > exfat_set_entry_type(ep, TYPE_DELETED); > - exfat_update_bh(sb, bh, IS_DIRSYNC(inode)); > + exfat_update_bh(bh, IS_DIRSYNC(inode)); > brelse(bh); > } > > @@ -610,7 +610,7 @@ void exfat_free_dentry_set(struct > exfat_entry_set_cache *es, int sync) > > for (i = 0; i < es->num_bh; i++) { > if (es->modified) > - exfat_update_bh(es->sb, es->bh[i], sync); > + exfat_update_bh(es->bh[i], sync); > brelse(es->bh[i]); > } > kfree(es); > diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h index > 595f3117f492..84664024e51e 100644 > --- a/fs/exfat/exfat_fs.h > +++ b/fs/exfat/exfat_fs.h > @@ -13,8 +13,6 @@ > #define EXFAT_SUPER_MAGIC 0x2011BAB0UL > #define EXFAT_ROOT_INO 1 > > -#define EXFAT_SB_DIRTY 0 > - > #define EXFAT_CLUSTERS_UNTRACKED (~0u) > > /* > @@ -238,7 +236,6 @@ struct exfat_sb_info { > unsigned int clu_srch_ptr; /* cluster search pointer */ > unsigned int used_clusters; /* number of used clusters */ > > - unsigned long s_state; > struct mutex s_lock; /* superblock lock */ > struct exfat_mount_options options; > struct nls_table *nls_io; /* Charset used for input and display */ > @@ -514,7 +511,7 @@ void exfat_set_entry_time(struct exfat_sb_info *sbi, > struct timespec64 *ts, > u8 *tz, __le16 *time, __le16 *date, u8 *time_cs); > u16 exfat_calc_chksum16(void *data, int len, u16 chksum, int type); > u32 exfat_calc_chksum32(void *data, int len, u32 chksum, int type); -void > exfat_update_bh(struct super_block *sb, struct buffer_head *bh, int sync); > +void exfat_update_bh(struct buffer_head *bh, int sync); > void exfat_chain_set(struct exfat_chain *ec, unsigned int dir, > unsigned int size, unsigned char flags); void > exfat_chain_dup(struct exfat_chain *dup, struct exfat_chain *ec); diff -- > git a/fs/exfat/fatent.c b/fs/exfat/fatent.c index > 4e5c5c9c0f2d..82ee8246c080 100644 > --- a/fs/exfat/fatent.c > +++ b/fs/exfat/fatent.c > @@ -75,7 +75,7 @@ int exfat_ent_set(struct super_block *sb, unsigned int > loc, > > fat_entry = (__le32 *)&(bh->b_data[off]); > *fat_entry = cpu_to_le32(content); > - exfat_update_bh(sb, bh, sb->s_flags & SB_SYNCHRONOUS); > + exfat_update_bh(bh, sb->s_flags & SB_SYNCHRONOUS); > exfat_mirror_bh(sb, sec, bh); > brelse(bh); > return 0; > @@ -174,7 +174,6 @@ int exfat_free_cluster(struct inode *inode, struct > exfat_chain *p_chain) > return -EIO; > } > > - set_bit(EXFAT_SB_DIRTY, &sbi->s_state); > clu = p_chain->dir; > > if (p_chain->flags == ALLOC_NO_FAT_CHAIN) { @@ -274,7 +273,7 @@ int > exfat_zeroed_cluster(struct inode *dir, unsigned int clu) > goto release_bhs; > } > memset(bhs[n]->b_data, 0, sb->s_blocksize); > - exfat_update_bh(sb, bhs[n], 0); > + exfat_update_bh(bhs[n], 0); > > n++; > blknr++; > @@ -358,8 +357,6 @@ int exfat_alloc_cluster(struct inode *inode, unsigned > int num_alloc, > } > } > > - set_bit(EXFAT_SB_DIRTY, &sbi->s_state); > - > p_chain->dir = EXFAT_EOF_CLUSTER; > > while ((new_clu = exfat_find_free_bitmap(sb, hint_clu)) != diff -- > git a/fs/exfat/misc.c b/fs/exfat/misc.c index 17d41f3d3709..8a3dde59052b > 100644 > --- a/fs/exfat/misc.c > +++ b/fs/exfat/misc.c > @@ -163,9 +163,8 @@ u32 exfat_calc_chksum32(void *data, int len, u32 > chksum, int type) > return chksum; > } > > -void exfat_update_bh(struct super_block *sb, struct buffer_head *bh, int > sync) > +void exfat_update_bh(struct buffer_head *bh, int sync) > { > - set_bit(EXFAT_SB_DIRTY, &EXFAT_SB(sb)->s_state); > set_buffer_uptodate(bh); > mark_buffer_dirty(bh); > > diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c index > edd8023865a0..5eef2217fcf2 100644 > --- a/fs/exfat/namei.c > +++ b/fs/exfat/namei.c > @@ -387,7 +387,7 @@ static int exfat_find_empty_entry(struct inode *inode, > ep->dentry.stream.valid_size = cpu_to_le64(size); > ep->dentry.stream.size = ep->dentry.stream.valid_size; > ep->dentry.stream.flags = p_dir->flags; > - exfat_update_bh(sb, bh, IS_DIRSYNC(inode)); > + exfat_update_bh(bh, IS_DIRSYNC(inode)); > brelse(bh); > if (exfat_update_dir_chksum(inode, &(ei->dir), > ei->entry)) > @@ -1071,7 +1071,7 @@ static int exfat_rename_file(struct inode *inode, > struct exfat_chain *p_dir, > epnew->dentry.file.attr |= cpu_to_le16(ATTR_ARCHIVE); > ei->attr |= ATTR_ARCHIVE; > } > - exfat_update_bh(sb, new_bh, sync); > + exfat_update_bh(new_bh, sync); > brelse(old_bh); > brelse(new_bh); > > @@ -1087,7 +1087,7 @@ static int exfat_rename_file(struct inode *inode, > struct exfat_chain *p_dir, > } > > memcpy(epnew, epold, DENTRY_SIZE); > - exfat_update_bh(sb, new_bh, sync); > + exfat_update_bh(new_bh, sync); > brelse(old_bh); > brelse(new_bh); > > @@ -1104,7 +1104,7 @@ static int exfat_rename_file(struct inode *inode, > struct exfat_chain *p_dir, > epold->dentry.file.attr |= cpu_to_le16(ATTR_ARCHIVE); > ei->attr |= ATTR_ARCHIVE; > } > - exfat_update_bh(sb, old_bh, sync); > + exfat_update_bh(old_bh, sync); > brelse(old_bh); > ret = exfat_init_ext_entry(inode, p_dir, oldentry, > num_new_entries, p_uniname); > @@ -1159,7 +1159,7 @@ static int exfat_move_file(struct inode *inode, > struct exfat_chain *p_olddir, > epnew->dentry.file.attr |= cpu_to_le16(ATTR_ARCHIVE); > ei->attr |= ATTR_ARCHIVE; > } > - exfat_update_bh(sb, new_bh, IS_DIRSYNC(inode)); > + exfat_update_bh(new_bh, IS_DIRSYNC(inode)); > brelse(mov_bh); > brelse(new_bh); > > @@ -1175,7 +1175,7 @@ static int exfat_move_file(struct inode *inode, > struct exfat_chain *p_olddir, > } > > memcpy(epnew, epmov, DENTRY_SIZE); > - exfat_update_bh(sb, new_bh, IS_DIRSYNC(inode)); > + exfat_update_bh(new_bh, IS_DIRSYNC(inode)); > brelse(mov_bh); > brelse(new_bh); > > diff --git a/fs/exfat/super.c b/fs/exfat/super.c index > e650e65536f8..8cb146376d6b 100644 > --- a/fs/exfat/super.c > +++ b/fs/exfat/super.c > @@ -45,9 +45,6 @@ static void exfat_put_super(struct super_block *sb) > struct exfat_sb_info *sbi = EXFAT_SB(sb); > > mutex_lock(&sbi->s_lock); > - if (test_and_clear_bit(EXFAT_SB_DIRTY, &sbi->s_state)) > - sync_blockdev(sb->s_bdev); > - exfat_set_vol_flags(sb, VOL_CLEAN); > exfat_free_bitmap(sbi); > brelse(sbi->boot_bh); > mutex_unlock(&sbi->s_lock); > @@ -60,13 +57,14 @@ static int exfat_sync_fs(struct super_block *sb, int > wait) > struct exfat_sb_info *sbi = EXFAT_SB(sb); > int err = 0; > > + if (!wait) > + return 0; > + > /* If there are some dirty buffers in the bdev inode */ > mutex_lock(&sbi->s_lock); > - if (test_and_clear_bit(EXFAT_SB_DIRTY, &sbi->s_state)) { > - sync_blockdev(sb->s_bdev); > - if (exfat_set_vol_flags(sb, VOL_CLEAN)) > - err = -EIO; > - } > + sync_blockdev(sb->s_bdev); > + if (exfat_set_vol_flags(sb, VOL_CLEAN)) > + err = -EIO; > mutex_unlock(&sbi->s_lock); > return err; > } > -- > 2.25.1
> > remove EXFAT_SB_DIRTY flag and related codes. > > > > This flag is set/reset in exfat_put_super()/exfat_sync_fs() to avoid > > sync_blockdev(). > > However ... > > - exfat_put_super(): > > Before calling this, the VFS has already called sync_filesystem(), so > > sync is never performed here. > > - exfat_sync_fs(): > > After calling this, the VFS calls sync_blockdev(), so, it is > > meaningless to check EXFAT_SB_DIRTY or to bypass sync_blockdev() here. > > Not only that, but in some cases can't clear VOL_DIRTY. > > ex: > > VOL_DIRTY is set when rmdir starts, but when non-empty-dir is > > detected, return error without setting EXFAT_SB_DIRTY. > > If performe 'sync' in this state, VOL_DIRTY will not be cleared. > > > > Since this patch does not resolve 'VOL_DIRTY in ENOTEMPTY' problem you mentioned, it would be better > to remove the description above for that and to make new patch. > > > Remove the EXFAT_SB_DIRTY check to ensure synchronization. > > And, remove the code related to the flag. > > > > Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com> > > Reviewed-by: Sungjong Seo <sj1557.seo@samsung.com> Applied. Thanks!
Thank you for your comment. On 2020/06/17 16:20, Sungjong Seo wrote: >> remove EXFAT_SB_DIRTY flag and related codes. >> >> This flag is set/reset in exfat_put_super()/exfat_sync_fs() to avoid >> sync_blockdev(). >> However ... >> - exfat_put_super(): >> Before calling this, the VFS has already called sync_filesystem(), so sync >> is never performed here. >> - exfat_sync_fs(): >> After calling this, the VFS calls sync_blockdev(), so, it is meaningless >> to check EXFAT_SB_DIRTY or to bypass sync_blockdev() here. >> Not only that, but in some cases can't clear VOL_DIRTY. >> ex: >> VOL_DIRTY is set when rmdir starts, but when non-empty-dir is detected, >> return error without setting EXFAT_SB_DIRTY. >> If performe 'sync' in this state, VOL_DIRTY will not be cleared. >> > > Since this patch does not resolve 'VOL_DIRTY in ENOTEMPTY' problem you > mentioned, > it would be better to remove the description above for that and to make new > patch. I mentioned rmdir as an example. However, this problem is not only with rmdirs. VOL_DIRTY remains when some functions abort with an error. In original, VOL_DIRTY is not cleared even if performe 'sync'. With this patch, it ensures that VOL_DIRTY will be cleared by 'sync'. Is my description insufficient? BTW Even with this patch applied, VOL_DIRTY remains until synced in the above case. It's not easy to reproduce as rmdir, but I'll try to fix it in the future. BR --- Tetsuhiro Kohada <kohada.t2@gmail.com>
> > Since this patch does not resolve 'VOL_DIRTY in ENOTEMPTY' problem you > > mentioned, it would be better to remove the description above for that > > and to make new patch. > > I mentioned rmdir as an example. > However, this problem is not only with rmdirs. > VOL_DIRTY remains when some functions abort with an error. > In original, VOL_DIRTY is not cleared even if performe 'sync'. > With this patch, it ensures that VOL_DIRTY will be cleared by 'sync'. > > Is my description insufficient? I understood what you said. However, it is a natural result when deleting the related code with EXFAT_SB_DIRTY flag. So I thought it would be better to separate it into new problems related to VOL_DIRTY-set under not real errors. > > > BTW > Even with this patch applied, VOL_DIRTY remains until synced in the above > case. > It's not easy to reproduce as rmdir, but I'll try to fix it in the future. I think it's not a problem not to clear VOL_DIRTY under real errors, because VOL_DIRTY is just like a hint to note that write was not finished clearly. If you mean there are more situation like ENOTEMPTY you mentioned, please make new patch to fix them. Thanks. > > > BR > --- > Tetsuhiro Kohada <kohada.t2@gmail.com> > >
>> I mentioned rmdir as an example. >> However, this problem is not only with rmdirs. >> VOL_DIRTY remains when some functions abort with an error. >> In original, VOL_DIRTY is not cleared even if performe 'sync'. >> With this patch, it ensures that VOL_DIRTY will be cleared by 'sync'. >> >> Is my description insufficient? > > I understood what you said. However, it is a natural result > when deleting the related code with EXFAT_SB_DIRTY flag. > > So I thought it would be better to separate it into new problems > related to VOL_DIRTY-set under not real errors. I see. It seems that it is better to consider separately when consistency is corrupted and when it is kept. >> BTW >> Even with this patch applied, VOL_DIRTY remains until synced in the above >> case. >> It's not easy to reproduce as rmdir, but I'll try to fix it in the future. > > I think it's not a problem not to clear VOL_DIRTY under real errors, > because VOL_DIRTY is just like a hint to note that write was not finished clearly. > > If you mean there are more situation like ENOTEMPTY you mentioned, > please make new patch to fix them. Hmm. VOL_DIRTY is easily cleared by another write operation. For that purpose, I think MediaFailure is more appropriate. BR --- Tetsuhiro Kohada <kohada.t2@gmail.com>
On 2020/06/18 22:11, Sungjong Seo wrote: >> BTW >> Even with this patch applied, VOL_DIRTY remains until synced in the above >> case. >> It's not easy to reproduce as rmdir, but I'll try to fix it in the future. > > I think it's not a problem not to clear VOL_DIRTY under real errors, > because VOL_DIRTY is just like a hint to note that write was not finished clearly. > > If you mean there are more situation like ENOTEMPTY you mentioned, > please make new patch to fix them. When should VOL_DIRTY be cleared? The current behavior is ... Case of mkdir, rmdir, rename: - set VOL_DIRTY before operation - set VOL_CLEAN after operating. In async mode, it is actually written to the media after 30 seconds. Case of cp, touch: - set VOL_DIRTY before operation - however, VOL_CLEAN is not called in this context. VOL_CLEAN will call by sync_fs or unmount. I added VOL_CLEAN in last of __exfat_write_inode() and exfat_map_cluster(). As a result, VOL_DIRTY is cleared with cp and touch. However, when copying a many files ... - Async mode: VOL_DIRTY is written to the media twice every 30 seconds. - Sync mode: Of course, VOL_DIRTY and VOL_CLEAN to the media for each file. Frequent writing VOL_DIRTY and VOL_CLEAN increases the risk of boot-sector curruption. If the boot-sector corrupted, it causes the following serious problems on some OSs. - misjudge as unformatted - can't judge as exfat - can't repair I want to minimize boot sector writes, to reduce these risk. I looked vfat/udf implementation, which manages similar dirty information on linux, and found that they ware mark-dirty at mount and cleared at unmount. Here are some ways to clear VOL_DIRTY. (A) VOL_CLEAN after every write operation. :-) Ejectable at any time after a write operation. :-( Many times write to Boot-sector. (B) dirty at mount, clear at unmount (same as vfat/udf) :-) Write to boot-sector twice. :-( It remains dirty unless unmounted. :-( Write to boot-sector even if there is no write operation. (C) dirty on first write operation, clear on unmount :-) Writing to boot-sector is minimal. :-) Will not write to the boot-sector if there is no write operation. :-( It remains dirty unless unmounted. (D) dirty on first write operation, clear on sync-fs/unmount :-) Writing to boot-sector can be reduced. :-) Will not write to the boot-sector if there is no write operation. :-) sync-fs makes it clean and ejectable immidiately. :-( It remains dirty unless sync-fs or unmount. :-( Frequent sync-fs will increases writes to boot-sector. I think it should be (C) or(D). What do you think? BR --- Tetsuhiro Kohada <kohada.t2@gmail.com>
> On 2020/06/18 22:11, Sungjong Seo wrote: > >> BTW > >> Even with this patch applied, VOL_DIRTY remains until synced in the > >> above case. > >> It's not easy to reproduce as rmdir, but I'll try to fix it in the > future. > > > > I think it's not a problem not to clear VOL_DIRTY under real errors, > > because VOL_DIRTY is just like a hint to note that write was not > finished clearly. > > > > If you mean there are more situation like ENOTEMPTY you mentioned, > > please make new patch to fix them. > > > When should VOL_DIRTY be cleared? > > The current behavior is ... > > Case of mkdir, rmdir, rename: > - set VOL_DIRTY before operation > - set VOL_CLEAN after operating. > In async mode, it is actually written to the media after 30 seconds. > > Case of cp, touch: > - set VOL_DIRTY before operation > - however, VOL_CLEAN is not called in this context. > VOL_CLEAN will call by sync_fs or unmount. > > I added VOL_CLEAN in last of __exfat_write_inode() and exfat_map_cluster(). > As a result, VOL_DIRTY is cleared with cp and touch. > However, when copying a many files ... > - Async mode: VOL_DIRTY is written to the media twice every 30 seconds. > - Sync mode: Of course, VOL_DIRTY and VOL_CLEAN to the media for each > file. > > Frequent writing VOL_DIRTY and VOL_CLEAN increases the risk of boot- > sector curruption. > If the boot-sector corrupted, it causes the following serious problems on > some OSs. > - misjudge as unformatted > - can't judge as exfat > - can't repair > > I want to minimize boot sector writes, to reduce these risk. > > I looked vfat/udf implementation, which manages similar dirty information > on linux, and found that they ware mark-dirty at mount and cleared at > unmount. > > Here are some ways to clear VOL_DIRTY. > > (A) VOL_CLEAN after every write operation. > :-) Ejectable at any time after a write operation. > :-( Many times write to Boot-sector. > > (B) dirty at mount, clear at unmount (same as vfat/udf) > :-) Write to boot-sector twice. > :-( It remains dirty unless unmounted. > :-( Write to boot-sector even if there is no write operation. > > (C) dirty on first write operation, clear on unmount > :-) Writing to boot-sector is minimal. > :-) Will not write to the boot-sector if there is no write operation. > :-( It remains dirty unless unmounted. > > (D) dirty on first write operation, clear on sync-fs/unmount > :-) Writing to boot-sector can be reduced. > :-) Will not write to the boot-sector if there is no write operation. > :-) sync-fs makes it clean and ejectable immidiately. > :-( It remains dirty unless sync-fs or unmount. > :-( Frequent sync-fs will increases writes to boot-sector. > > I think it should be (C) or(D). > What do you think? > First of all, I'm sorry for the late reply. And thank you for the suggestion. Most of the NAND flash devices and HDDs have wear leveling and bad sector replacement algorithms applied. So I think that the life of the boot sector will not be exhausted first. Currently the volume dirty/clean policy of exfat-fs is not perfect, but I think it behaves similarly to the policy of MS Windows. Therefore, I think code improvements should be made to reduce volume flag records while maintaining the current policy. BR Sungjong Seo > > > BR > --- > Tetsuhiro Kohada <kohada.t2@gmail.com>
>> >> When should VOL_DIRTY be cleared? >> >> The current behavior is ... >> >> Case of mkdir, rmdir, rename: >> - set VOL_DIRTY before operation >> - set VOL_CLEAN after operating. >> In async mode, it is actually written to the media after 30 seconds. >> >> Case of cp, touch: >> - set VOL_DIRTY before operation >> - however, VOL_CLEAN is not called in this context. >> VOL_CLEAN will call by sync_fs or unmount. >> >> I added VOL_CLEAN in last of __exfat_write_inode() and exfat_map_cluster(). >> As a result, VOL_DIRTY is cleared with cp and touch. >> However, when copying a many files ... >> - Async mode: VOL_DIRTY is written to the media twice every 30 seconds. >> - Sync mode: Of course, VOL_DIRTY and VOL_CLEAN to the media for each >> file. >> >> Frequent writing VOL_DIRTY and VOL_CLEAN increases the risk of boot- >> sector curruption. >> If the boot-sector corrupted, it causes the following serious problems on >> some OSs. >> - misjudge as unformatted >> - can't judge as exfat >> - can't repair >> >> I want to minimize boot sector writes, to reduce these risk. >> >> I looked vfat/udf implementation, which manages similar dirty information >> on linux, and found that they ware mark-dirty at mount and cleared at >> unmount. >> >> Here are some ways to clear VOL_DIRTY. >> >> (A) VOL_CLEAN after every write operation. >> :-) Ejectable at any time after a write operation. >> :-( Many times write to Boot-sector. >> >> (B) dirty at mount, clear at unmount (same as vfat/udf) >> :-) Write to boot-sector twice. >> :-( It remains dirty unless unmounted. >> :-( Write to boot-sector even if there is no write operation. >> >> (C) dirty on first write operation, clear on unmount >> :-) Writing to boot-sector is minimal. >> :-) Will not write to the boot-sector if there is no write operation. >> :-( It remains dirty unless unmounted. >> >> (D) dirty on first write operation, clear on sync-fs/unmount >> :-) Writing to boot-sector can be reduced. >> :-) Will not write to the boot-sector if there is no write operation. >> :-) sync-fs makes it clean and ejectable immidiately. >> :-( It remains dirty unless sync-fs or unmount. >> :-( Frequent sync-fs will increases writes to boot-sector. >> >> I think it should be (C) or(D). >> What do you think? >> > > First of all, I'm sorry for the late reply. > And thank you for the suggestion. Thanks for thinking on this complicated issue. > Most of the NAND flash devices and HDDs have wear leveling and bad sector replacement algorithms applied. > So I think that the life of the boot sector will not be exhausted first. I'm not too worried about the life of the boot-sector. I'm worried about write failures caused by external factors. (power failure/system down/vibration/etc. during writing) They rarely occur on SD cards, but occur on many HDDs, some SSDs and USB storages by 0.1% or more. Especially with AFT-HDD, not only boot-sector but also the following multiple sectors become unreadable. It is not possible to completely solve this problem, as long as writing to the boot-sector. (I think it's a exFAT's specification defect) The only effective way to reduce this problem is to reduce writes to the boot-sector. > Currently the volume dirty/clean policy of exfat-fs is not perfect, Thank you for sharing the problem with you. > but I think it behaves similarly to the policy of MS Windows. On Windows10, the dirty flag is cleared after more than 15 seconds after all write operations are completed. (dirty-flag is never updated during the write operation continues) > Therefore, > I think code improvements should be made to reduce volume flag records while maintaining the current policy. Current policy is inconsistent. As I wrote last mail, the problem with the current implementation is that the dirty-flag may not be cleared after the write operation.(even if sync is enabled or disabled) Because, some write operations clear the dirty-flag but some don't clear. Unmount or sync command is the only way to ensure that the dirty-flag is cleared. This has no effect on clearing the dirty-flag after a write operations, it only increases risk of destroying the boot-sector. - Clear the dirty-flag after every write operation. - Never clear the dirty-flag after every write operation. Unless unified to either one, I think that sync policy cannot be consistent. How do you think? BR --- etsuhiro Kohada <kohada.t2@gmail.com>
> Thanks for thinking on this complicated issue. > > > > Most of the NAND flash devices and HDDs have wear leveling and bad sector replacement algorithms > applied. > > So I think that the life of the boot sector will not be exhausted first. > > I'm not too worried about the life of the boot-sector. > I'm worried about write failures caused by external factors. > (power failure/system down/vibration/etc. during writing) They rarely occur on SD cards, but occur on > many HDDs, some SSDs and USB storages by 0.1% or more. Hard disk and SSD do not guarantee atomic write of a sector unit? > Especially with AFT-HDD, not only boot-sector but also the following multiple sectors become > unreadable. Other file systems will also be unstable on a such HW. > It is not possible to completely solve this problem, as long as writing to the boot-sector. > (I think it's a exFAT's specification defect) The only effective way to reduce this problem is to > reduce writes to the boot-sector. exFAT's specification defect... Well.. Even though the boot sector is corrupted, It can be recovered using the backup boot sector through fsck. > > > > Currently the volume dirty/clean policy of exfat-fs is not perfect, > > Thank you for sharing the problem with you. > > > > but I think it behaves similarly to the policy of MS Windows. > > On Windows10, the dirty flag is cleared after more than 15 seconds after all write operations are > completed. > (dirty-flag is never updated during the write operation continues) > > > > Therefore, > > I think code improvements should be made to reduce volume flag records while maintaining the current > policy. > > Current policy is inconsistent. > As I wrote last mail, the problem with the current implementation is that the dirty-flag may not be > cleared after the write operation.(even if sync is enabled or disabled) Because, some write operations > clear the dirty-flag but some don't clear. > Unmount or sync command is the only way to ensure that the dirty-flag is cleared. > This has no effect on clearing the dirty-flag after a write operations, it only increases risk of > destroying the boot-sector. > - Clear the dirty-flag after every write operation. > - Never clear the dirty-flag after every write operation. > Unless unified to either one, I think that sync policy cannot be consistent. > > How do you think? > > > BR > --- > etsuhiro Kohada <kohada.t2@gmail.com>
Thank you for your reply. >>> Most of the NAND flash devices and HDDs have wear leveling and bad sector replacement algorithms >> applied. >>> So I think that the life of the boot sector will not be exhausted first. >> >> I'm not too worried about the life of the boot-sector. >> I'm worried about write failures caused by external factors. >> (power failure/system down/vibration/etc. during writing) They rarely occur on SD cards, but occur on >> many HDDs, some SSDs and USB storages by 0.1% or more. > Hard disk and SSD do not guarantee atomic write of a sector unit? In the case of SD, the sector-data will be either new or old when unexpected write interruption occurs. Almost HDD, the sector-data will be either new, old, or unreadable. And, some SSD products have similar problem. >> Especially with AFT-HDD, not only boot-sector but also the following multiple sectors become >> unreadable. > Other file systems will also be unstable on a such HW. A well-designed FileSystems never rewrite critical regions. >> It is not possible to completely solve this problem, as long as writing to the boot-sector. >> (I think it's a exFAT's specification defect) The only effective way to reduce this problem is to >> reduce writes to the boot-sector. > exFAT's specification defect... Well.. > Even though the boot sector is corrupted, It can be recovered using the backup boot sector > through fsck. Exactly. However, in order to execute fsck, it is necessary to recognize the partition/volume with broken boot-sector as exfat. Can linux(or fsck) correctly recognize the FileSystem even if the boot-sector cannot be read? (I don't yet know how linux recognizes FileSystem) In fact, a certain system recognize it as 'Unknown format'. Nowadays, exfat is often used for removable storage. This problem is not only for linux. BR --- etsuhiro Kohada <kohada.t2@gmail.com>
diff --git a/fs/exfat/balloc.c b/fs/exfat/balloc.c index 4055eb00ea9b..a987919686c0 100644 --- a/fs/exfat/balloc.c +++ b/fs/exfat/balloc.c @@ -158,7 +158,7 @@ int exfat_set_bitmap(struct inode *inode, unsigned int clu) b = BITMAP_OFFSET_BIT_IN_SECTOR(sb, ent_idx); set_bit_le(b, sbi->vol_amap[i]->b_data); - exfat_update_bh(sb, sbi->vol_amap[i], IS_DIRSYNC(inode)); + exfat_update_bh(sbi->vol_amap[i], IS_DIRSYNC(inode)); return 0; } @@ -180,7 +180,7 @@ void exfat_clear_bitmap(struct inode *inode, unsigned int clu) b = BITMAP_OFFSET_BIT_IN_SECTOR(sb, ent_idx); clear_bit_le(b, sbi->vol_amap[i]->b_data); - exfat_update_bh(sb, sbi->vol_amap[i], IS_DIRSYNC(inode)); + exfat_update_bh(sbi->vol_amap[i], IS_DIRSYNC(inode)); if (opts->discard) { int ret_discard; diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index 8e775bd5d523..02acbb6ddf02 100644 --- a/fs/exfat/dir.c +++ b/fs/exfat/dir.c @@ -470,7 +470,7 @@ int exfat_init_dir_entry(struct inode *inode, struct exfat_chain *p_dir, &ep->dentry.file.access_date, NULL); - exfat_update_bh(sb, bh, IS_DIRSYNC(inode)); + exfat_update_bh(bh, IS_DIRSYNC(inode)); brelse(bh); ep = exfat_get_dentry(sb, p_dir, entry + 1, &bh, §or); @@ -480,7 +480,7 @@ int exfat_init_dir_entry(struct inode *inode, struct exfat_chain *p_dir, exfat_init_stream_entry(ep, (type == TYPE_FILE) ? ALLOC_FAT_CHAIN : ALLOC_NO_FAT_CHAIN, start_clu, size); - exfat_update_bh(sb, bh, IS_DIRSYNC(inode)); + exfat_update_bh(bh, IS_DIRSYNC(inode)); brelse(bh); return 0; @@ -516,7 +516,7 @@ int exfat_update_dir_chksum(struct inode *inode, struct exfat_chain *p_dir, } fep->dentry.file.checksum = cpu_to_le16(chksum); - exfat_update_bh(sb, fbh, IS_DIRSYNC(inode)); + exfat_update_bh(fbh, IS_DIRSYNC(inode)); release_fbh: brelse(fbh); return ret; @@ -538,7 +538,7 @@ int exfat_init_ext_entry(struct inode *inode, struct exfat_chain *p_dir, return -EIO; ep->dentry.file.num_ext = (unsigned char)(num_entries - 1); - exfat_update_bh(sb, bh, sync); + exfat_update_bh(bh, sync); brelse(bh); ep = exfat_get_dentry(sb, p_dir, entry + 1, &bh, §or); @@ -547,7 +547,7 @@ int exfat_init_ext_entry(struct inode *inode, struct exfat_chain *p_dir, ep->dentry.stream.name_len = p_uniname->name_len; ep->dentry.stream.name_hash = cpu_to_le16(p_uniname->name_hash); - exfat_update_bh(sb, bh, sync); + exfat_update_bh(bh, sync); brelse(bh); for (i = EXFAT_FIRST_CLUSTER; i < num_entries; i++) { @@ -556,7 +556,7 @@ int exfat_init_ext_entry(struct inode *inode, struct exfat_chain *p_dir, return -EIO; exfat_init_name_entry(ep, uniname); - exfat_update_bh(sb, bh, sync); + exfat_update_bh(bh, sync); brelse(bh); uniname += EXFAT_FILE_NAME_LEN; } @@ -580,7 +580,7 @@ int exfat_remove_entries(struct inode *inode, struct exfat_chain *p_dir, return -EIO; exfat_set_entry_type(ep, TYPE_DELETED); - exfat_update_bh(sb, bh, IS_DIRSYNC(inode)); + exfat_update_bh(bh, IS_DIRSYNC(inode)); brelse(bh); } @@ -610,7 +610,7 @@ void exfat_free_dentry_set(struct exfat_entry_set_cache *es, int sync) for (i = 0; i < es->num_bh; i++) { if (es->modified) - exfat_update_bh(es->sb, es->bh[i], sync); + exfat_update_bh(es->bh[i], sync); brelse(es->bh[i]); } kfree(es); diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h index 595f3117f492..84664024e51e 100644 --- a/fs/exfat/exfat_fs.h +++ b/fs/exfat/exfat_fs.h @@ -13,8 +13,6 @@ #define EXFAT_SUPER_MAGIC 0x2011BAB0UL #define EXFAT_ROOT_INO 1 -#define EXFAT_SB_DIRTY 0 - #define EXFAT_CLUSTERS_UNTRACKED (~0u) /* @@ -238,7 +236,6 @@ struct exfat_sb_info { unsigned int clu_srch_ptr; /* cluster search pointer */ unsigned int used_clusters; /* number of used clusters */ - unsigned long s_state; struct mutex s_lock; /* superblock lock */ struct exfat_mount_options options; struct nls_table *nls_io; /* Charset used for input and display */ @@ -514,7 +511,7 @@ void exfat_set_entry_time(struct exfat_sb_info *sbi, struct timespec64 *ts, u8 *tz, __le16 *time, __le16 *date, u8 *time_cs); u16 exfat_calc_chksum16(void *data, int len, u16 chksum, int type); u32 exfat_calc_chksum32(void *data, int len, u32 chksum, int type); -void exfat_update_bh(struct super_block *sb, struct buffer_head *bh, int sync); +void exfat_update_bh(struct buffer_head *bh, int sync); void exfat_chain_set(struct exfat_chain *ec, unsigned int dir, unsigned int size, unsigned char flags); void exfat_chain_dup(struct exfat_chain *dup, struct exfat_chain *ec); diff --git a/fs/exfat/fatent.c b/fs/exfat/fatent.c index 4e5c5c9c0f2d..82ee8246c080 100644 --- a/fs/exfat/fatent.c +++ b/fs/exfat/fatent.c @@ -75,7 +75,7 @@ int exfat_ent_set(struct super_block *sb, unsigned int loc, fat_entry = (__le32 *)&(bh->b_data[off]); *fat_entry = cpu_to_le32(content); - exfat_update_bh(sb, bh, sb->s_flags & SB_SYNCHRONOUS); + exfat_update_bh(bh, sb->s_flags & SB_SYNCHRONOUS); exfat_mirror_bh(sb, sec, bh); brelse(bh); return 0; @@ -174,7 +174,6 @@ int exfat_free_cluster(struct inode *inode, struct exfat_chain *p_chain) return -EIO; } - set_bit(EXFAT_SB_DIRTY, &sbi->s_state); clu = p_chain->dir; if (p_chain->flags == ALLOC_NO_FAT_CHAIN) { @@ -274,7 +273,7 @@ int exfat_zeroed_cluster(struct inode *dir, unsigned int clu) goto release_bhs; } memset(bhs[n]->b_data, 0, sb->s_blocksize); - exfat_update_bh(sb, bhs[n], 0); + exfat_update_bh(bhs[n], 0); n++; blknr++; @@ -358,8 +357,6 @@ int exfat_alloc_cluster(struct inode *inode, unsigned int num_alloc, } } - set_bit(EXFAT_SB_DIRTY, &sbi->s_state); - p_chain->dir = EXFAT_EOF_CLUSTER; while ((new_clu = exfat_find_free_bitmap(sb, hint_clu)) != diff --git a/fs/exfat/misc.c b/fs/exfat/misc.c index 17d41f3d3709..8a3dde59052b 100644 --- a/fs/exfat/misc.c +++ b/fs/exfat/misc.c @@ -163,9 +163,8 @@ u32 exfat_calc_chksum32(void *data, int len, u32 chksum, int type) return chksum; } -void exfat_update_bh(struct super_block *sb, struct buffer_head *bh, int sync) +void exfat_update_bh(struct buffer_head *bh, int sync) { - set_bit(EXFAT_SB_DIRTY, &EXFAT_SB(sb)->s_state); set_buffer_uptodate(bh); mark_buffer_dirty(bh); diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c index edd8023865a0..5eef2217fcf2 100644 --- a/fs/exfat/namei.c +++ b/fs/exfat/namei.c @@ -387,7 +387,7 @@ static int exfat_find_empty_entry(struct inode *inode, ep->dentry.stream.valid_size = cpu_to_le64(size); ep->dentry.stream.size = ep->dentry.stream.valid_size; ep->dentry.stream.flags = p_dir->flags; - exfat_update_bh(sb, bh, IS_DIRSYNC(inode)); + exfat_update_bh(bh, IS_DIRSYNC(inode)); brelse(bh); if (exfat_update_dir_chksum(inode, &(ei->dir), ei->entry)) @@ -1071,7 +1071,7 @@ static int exfat_rename_file(struct inode *inode, struct exfat_chain *p_dir, epnew->dentry.file.attr |= cpu_to_le16(ATTR_ARCHIVE); ei->attr |= ATTR_ARCHIVE; } - exfat_update_bh(sb, new_bh, sync); + exfat_update_bh(new_bh, sync); brelse(old_bh); brelse(new_bh); @@ -1087,7 +1087,7 @@ static int exfat_rename_file(struct inode *inode, struct exfat_chain *p_dir, } memcpy(epnew, epold, DENTRY_SIZE); - exfat_update_bh(sb, new_bh, sync); + exfat_update_bh(new_bh, sync); brelse(old_bh); brelse(new_bh); @@ -1104,7 +1104,7 @@ static int exfat_rename_file(struct inode *inode, struct exfat_chain *p_dir, epold->dentry.file.attr |= cpu_to_le16(ATTR_ARCHIVE); ei->attr |= ATTR_ARCHIVE; } - exfat_update_bh(sb, old_bh, sync); + exfat_update_bh(old_bh, sync); brelse(old_bh); ret = exfat_init_ext_entry(inode, p_dir, oldentry, num_new_entries, p_uniname); @@ -1159,7 +1159,7 @@ static int exfat_move_file(struct inode *inode, struct exfat_chain *p_olddir, epnew->dentry.file.attr |= cpu_to_le16(ATTR_ARCHIVE); ei->attr |= ATTR_ARCHIVE; } - exfat_update_bh(sb, new_bh, IS_DIRSYNC(inode)); + exfat_update_bh(new_bh, IS_DIRSYNC(inode)); brelse(mov_bh); brelse(new_bh); @@ -1175,7 +1175,7 @@ static int exfat_move_file(struct inode *inode, struct exfat_chain *p_olddir, } memcpy(epnew, epmov, DENTRY_SIZE); - exfat_update_bh(sb, new_bh, IS_DIRSYNC(inode)); + exfat_update_bh(new_bh, IS_DIRSYNC(inode)); brelse(mov_bh); brelse(new_bh); diff --git a/fs/exfat/super.c b/fs/exfat/super.c index e650e65536f8..8cb146376d6b 100644 --- a/fs/exfat/super.c +++ b/fs/exfat/super.c @@ -45,9 +45,6 @@ static void exfat_put_super(struct super_block *sb) struct exfat_sb_info *sbi = EXFAT_SB(sb); mutex_lock(&sbi->s_lock); - if (test_and_clear_bit(EXFAT_SB_DIRTY, &sbi->s_state)) - sync_blockdev(sb->s_bdev); - exfat_set_vol_flags(sb, VOL_CLEAN); exfat_free_bitmap(sbi); brelse(sbi->boot_bh); mutex_unlock(&sbi->s_lock); @@ -60,13 +57,14 @@ static int exfat_sync_fs(struct super_block *sb, int wait) struct exfat_sb_info *sbi = EXFAT_SB(sb); int err = 0; + if (!wait) + return 0; + /* If there are some dirty buffers in the bdev inode */ mutex_lock(&sbi->s_lock); - if (test_and_clear_bit(EXFAT_SB_DIRTY, &sbi->s_state)) { - sync_blockdev(sb->s_bdev); - if (exfat_set_vol_flags(sb, VOL_CLEAN)) - err = -EIO; - } + sync_blockdev(sb->s_bdev); + if (exfat_set_vol_flags(sb, VOL_CLEAN)) + err = -EIO; mutex_unlock(&sbi->s_lock); return err; }
remove EXFAT_SB_DIRTY flag and related codes. This flag is set/reset in exfat_put_super()/exfat_sync_fs() to avoid sync_blockdev(). However ... - exfat_put_super(): Before calling this, the VFS has already called sync_filesystem(), so sync is never performed here. - exfat_sync_fs(): After calling this, the VFS calls sync_blockdev(), so, it is meaningless to check EXFAT_SB_DIRTY or to bypass sync_blockdev() here. Not only that, but in some cases can't clear VOL_DIRTY. ex: VOL_DIRTY is set when rmdir starts, but when non-empty-dir is detected, return error without setting EXFAT_SB_DIRTY. If performe 'sync' in this state, VOL_DIRTY will not be cleared. Remove the EXFAT_SB_DIRTY check to ensure synchronization. And, remove the code related to the flag. Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com> --- Changes in v2: - exfat_sync_fs() avoids synchronous processing when wait=0 Changes in v3: - fix return value in exfat_sync_fs() fs/exfat/balloc.c | 4 ++-- fs/exfat/dir.c | 16 ++++++++-------- fs/exfat/exfat_fs.h | 5 +---- fs/exfat/fatent.c | 7 ++----- fs/exfat/misc.c | 3 +-- fs/exfat/namei.c | 12 ++++++------ fs/exfat/super.c | 14 ++++++-------- 7 files changed, 26 insertions(+), 35 deletions(-)