Add error check when synchronously updating dir-entries.
Furthermore, add exfat_update_bhs(). It wait for write completion once
instead of sector by sector.
Suggested-by: Sungjong Seo <[email protected]>
Signed-off-by: Tetsuhiro Kohada <[email protected]>
---
fs/exfat/dir.c | 15 +++++++++------
fs/exfat/exfat_fs.h | 3 ++-
fs/exfat/file.c | 5 ++++-
fs/exfat/inode.c | 8 +++++---
fs/exfat/misc.c | 19 +++++++++++++++++++
5 files changed, 39 insertions(+), 11 deletions(-)
diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c
index de43534aa299..3eb8386fb5f2 100644
--- a/fs/exfat/dir.c
+++ b/fs/exfat/dir.c
@@ -602,16 +602,19 @@ void exfat_update_dir_chksum_with_entry_set(struct exfat_entry_set_cache *es)
es->modified = true;
}
-void exfat_free_dentry_set(struct exfat_entry_set_cache *es, int sync)
+int exfat_free_dentry_set(struct exfat_entry_set_cache *es, int sync)
{
- int i;
+ int i, err = 0;
- for (i = 0; i < es->num_bh; i++) {
- if (es->modified)
- exfat_update_bh(es->sb, es->bh[i], sync);
- brelse(es->bh[i]);
+ if (es->modified) {
+ set_bit(EXFAT_SB_DIRTY, &EXFAT_SB(es->sb)->s_state);
+ err = exfat_update_bhs(es->bh, es->num_bh, sync);
}
+
+ for (i = 0; i < es->num_bh; i++)
+ err ? bforget(es->bh[i]):brelse(es->bh[i]);
kfree(es);
+ return err;
}
static int exfat_walk_fat_chain(struct super_block *sb,
diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
index 595f3117f492..f4fa0e833486 100644
--- a/fs/exfat/exfat_fs.h
+++ b/fs/exfat/exfat_fs.h
@@ -462,7 +462,7 @@ struct exfat_dentry *exfat_get_dentry_cached(struct exfat_entry_set_cache *es,
int num);
struct exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb,
struct exfat_chain *p_dir, int entry, unsigned int type);
-void exfat_free_dentry_set(struct exfat_entry_set_cache *es, int sync);
+int exfat_free_dentry_set(struct exfat_entry_set_cache *es, int sync);
int exfat_count_dir_entries(struct super_block *sb, struct exfat_chain *p_dir);
/* inode.c */
@@ -515,6 +515,7 @@ void exfat_set_entry_time(struct exfat_sb_info *sbi, struct timespec64 *ts,
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);
+int exfat_update_bhs(struct buffer_head **bhs, int nr_bhs, 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/file.c b/fs/exfat/file.c
index fce03f318787..37c8f04c1f8a 100644
--- a/fs/exfat/file.c
+++ b/fs/exfat/file.c
@@ -153,6 +153,7 @@ int __exfat_truncate(struct inode *inode, loff_t new_size)
struct timespec64 ts;
struct exfat_dentry *ep, *ep2;
struct exfat_entry_set_cache *es;
+ int err;
es = exfat_get_dentry_set(sb, &(ei->dir), ei->entry,
ES_ALL_ENTRIES);
@@ -187,7 +188,9 @@ int __exfat_truncate(struct inode *inode, loff_t new_size)
}
exfat_update_dir_chksum_with_entry_set(es);
- exfat_free_dentry_set(es, inode_needs_sync(inode));
+ err = exfat_free_dentry_set(es, inode_needs_sync(inode));
+ if (err)
+ return err;
}
/* cut off from the FAT chain */
diff --git a/fs/exfat/inode.c b/fs/exfat/inode.c
index ef7cf7a6d187..c0bfd1a586aa 100644
--- a/fs/exfat/inode.c
+++ b/fs/exfat/inode.c
@@ -77,8 +77,7 @@ static int __exfat_write_inode(struct inode *inode, int sync)
ep2->dentry.stream.size = ep2->dentry.stream.valid_size;
exfat_update_dir_chksum_with_entry_set(es);
- exfat_free_dentry_set(es, sync);
- return 0;
+ return exfat_free_dentry_set(es, sync);
}
int exfat_write_inode(struct inode *inode, struct writeback_control *wbc)
@@ -222,6 +221,7 @@ static int exfat_map_cluster(struct inode *inode, unsigned int clu_offset,
if (ei->dir.dir != DIR_DELETED && modified) {
struct exfat_dentry *ep;
struct exfat_entry_set_cache *es;
+ int err;
es = exfat_get_dentry_set(sb, &(ei->dir), ei->entry,
ES_ALL_ENTRIES);
@@ -240,7 +240,9 @@ static int exfat_map_cluster(struct inode *inode, unsigned int clu_offset,
ep->dentry.stream.valid_size;
exfat_update_dir_chksum_with_entry_set(es);
- exfat_free_dentry_set(es, inode_needs_sync(inode));
+ err = exfat_free_dentry_set(es, inode_needs_sync(inode));
+ if (err)
+ return err;
} /* end of if != DIR_DELETED */
diff --git a/fs/exfat/misc.c b/fs/exfat/misc.c
index 17d41f3d3709..dc34968e99d3 100644
--- a/fs/exfat/misc.c
+++ b/fs/exfat/misc.c
@@ -173,6 +173,25 @@ void exfat_update_bh(struct super_block *sb, struct buffer_head *bh, int sync)
sync_dirty_buffer(bh);
}
+int exfat_update_bhs(struct buffer_head **bhs, int nr_bhs, int sync)
+{
+ int i, err = 0;
+
+ for (i = 0; i < nr_bhs; i++) {
+ set_buffer_uptodate(bhs[i]);
+ mark_buffer_dirty(bhs[i]);
+ if (sync)
+ write_dirty_buffer(bhs[i], 0);
+ }
+
+ for (i = 0; i < nr_bhs && sync; i++) {
+ wait_on_buffer(bhs[i]);
+ if (!buffer_uptodate(bhs[i]))
+ err = -EIO;
+ }
+ return err;
+}
+
void exfat_chain_set(struct exfat_chain *ec, unsigned int dir,
unsigned int size, unsigned char flags)
{
--
2.25.1
Set EXFAT_SB_DIRTY flag in exfat_put_super().
In some cases, can't clear VOL_DIRTY with 'sync'.
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.
Signed-off-by: Tetsuhiro Kohada <[email protected]>
---
fs/exfat/balloc.c | 4 ++--
fs/exfat/dir.c | 18 ++++++++----------
fs/exfat/exfat_fs.h | 2 +-
fs/exfat/fatent.c | 6 +-----
fs/exfat/misc.c | 3 +--
fs/exfat/namei.c | 12 ++++++------
fs/exfat/super.c | 3 +++
7 files changed, 22 insertions(+), 26 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 3eb8386fb5f2..96c9a817d928 100644
--- a/fs/exfat/dir.c
+++ b/fs/exfat/dir.c
@@ -468,7 +468,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);
@@ -478,7 +478,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;
@@ -514,7 +514,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;
@@ -536,7 +536,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);
@@ -545,7 +545,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++) {
@@ -554,7 +554,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;
}
@@ -578,7 +578,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);
}
@@ -606,10 +606,8 @@ int exfat_free_dentry_set(struct exfat_entry_set_cache *es, int sync)
{
int i, err = 0;
- if (es->modified) {
- set_bit(EXFAT_SB_DIRTY, &EXFAT_SB(es->sb)->s_state);
+ if (es->modified)
err = exfat_update_bhs(es->bh, es->num_bh, sync);
- }
for (i = 0; i < es->num_bh; i++)
err ? bforget(es->bh[i]):brelse(es->bh[i]);
diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
index f4fa0e833486..0e094d186612 100644
--- a/fs/exfat/exfat_fs.h
+++ b/fs/exfat/exfat_fs.h
@@ -514,7 +514,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);
int exfat_update_bhs(struct buffer_head **bhs, int nr_bhs, int sync);
void exfat_chain_set(struct exfat_chain *ec, unsigned int dir,
unsigned int size, unsigned char flags);
diff --git a/fs/exfat/fatent.c b/fs/exfat/fatent.c
index 5d11bc2f1b68..f8171183b4c1 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) {
@@ -261,7 +260,6 @@ int exfat_zeroed_cluster(struct inode *dir, unsigned int clu)
memset(bhs[n]->b_data, 0, sb->s_blocksize);
}
- set_bit(EXFAT_SB_DIRTY, &sbi->s_state);
err = exfat_update_bhs(bhs, n, IS_DIRSYNC(dir));
if (err)
goto release_bhs;
@@ -326,8 +324,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 dc34968e99d3..564718747fb2 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 5b0f35329d63..e36c9fc4a5d6 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);
@@ -1083,7 +1083,7 @@ static int exfat_rename_file(struct inode *inode, struct exfat_chain *p_dir,
return -EIO;
memcpy(epnew, epold, DENTRY_SIZE);
- exfat_update_bh(sb, new_bh, sync);
+ exfat_update_bh(new_bh, sync);
brelse(old_bh);
brelse(new_bh);
@@ -1100,7 +1100,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);
@@ -1155,7 +1155,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);
@@ -1167,7 +1167,7 @@ static int exfat_move_file(struct inode *inode, struct exfat_chain *p_olddir,
return -EIO;
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..199a1e78f9e5 100644
--- a/fs/exfat/super.c
+++ b/fs/exfat/super.c
@@ -104,6 +104,9 @@ int exfat_set_vol_flags(struct super_block *sb, unsigned short new_flag)
struct boot_sector *p_boot = (struct boot_sector *)sbi->boot_bh->b_data;
bool sync;
+ if (new_flag == VOL_DIRTY)
+ set_bit(EXFAT_SB_DIRTY, &sbi->s_state);
+
/* flags are not changed */
if (sbi->vol_flag == new_flag)
return 0;
--
2.25.1
Replace part of exfat_zeroed_cluster() with exfat_update_bhs().
And remove exfat_sync_bhs().
Signed-off-by: Tetsuhiro Kohada <[email protected]>
---
fs/exfat/fatent.c | 54 ++++++++++-------------------------------------
1 file changed, 11 insertions(+), 43 deletions(-)
diff --git a/fs/exfat/fatent.c b/fs/exfat/fatent.c
index 267e5e09eb13..5d11bc2f1b68 100644
--- a/fs/exfat/fatent.c
+++ b/fs/exfat/fatent.c
@@ -230,21 +230,6 @@ int exfat_find_last_cluster(struct super_block *sb, struct exfat_chain *p_chain,
return 0;
}
-static inline int exfat_sync_bhs(struct buffer_head **bhs, int nr_bhs)
-{
- int i, err = 0;
-
- for (i = 0; i < nr_bhs; i++)
- write_dirty_buffer(bhs[i], 0);
-
- for (i = 0; i < nr_bhs; i++) {
- wait_on_buffer(bhs[i]);
- if (!err && !buffer_uptodate(bhs[i]))
- err = -EIO;
- }
- return err;
-}
-
int exfat_zeroed_cluster(struct inode *dir, unsigned int clu)
{
struct super_block *sb = dir->i_sb;
@@ -266,41 +251,24 @@ int exfat_zeroed_cluster(struct inode *dir, unsigned int clu)
}
/* Zeroing the unused blocks on this cluster */
- n = 0;
while (blknr < last_blknr) {
- bhs[n] = sb_getblk(sb, blknr);
- if (!bhs[n]) {
- err = -ENOMEM;
- goto release_bhs;
- }
- memset(bhs[n]->b_data, 0, sb->s_blocksize);
- exfat_update_bh(sb, bhs[n], 0);
-
- n++;
- blknr++;
-
- if (n == nr_bhs) {
- if (IS_DIRSYNC(dir)) {
- err = exfat_sync_bhs(bhs, n);
- if (err)
- goto release_bhs;
+ for (n = 0; n < nr_bhs && blknr < last_blknr; n++, blknr++) {
+ bhs[n] = sb_getblk(sb, blknr);
+ if (!bhs[n]) {
+ err = -ENOMEM;
+ goto release_bhs;
}
-
- for (i = 0; i < n; i++)
- brelse(bhs[i]);
- n = 0;
+ memset(bhs[n]->b_data, 0, sb->s_blocksize);
}
- }
- if (IS_DIRSYNC(dir)) {
- err = exfat_sync_bhs(bhs, n);
+ set_bit(EXFAT_SB_DIRTY, &sbi->s_state);
+ err = exfat_update_bhs(bhs, n, IS_DIRSYNC(dir));
if (err)
goto release_bhs;
- }
-
- for (i = 0; i < n; i++)
- brelse(bhs[i]);
+ for (i = 0; i < n; i++)
+ brelse(bhs[i]);
+ }
return 0;
release_bhs:
--
2.25.1
> Set EXFAT_SB_DIRTY flag in exfat_put_super().
>
> In some cases, can't clear VOL_DIRTY with 'sync'.
> 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.
Good catch.
Can you split this patch into two? (Don't set VOL_DIRTY on -ENOTEMPTY and Setting EXFAT_SB_DIRTY is
merged into exfat_set_vol_flag). I need to check the second one more.
Thanks!
>
> Signed-off-by: Tetsuhiro Kohada <[email protected]>
> ---
> fs/exfat/balloc.c | 4 ++--
> fs/exfat/dir.c | 18 ++++++++----------
> fs/exfat/exfat_fs.h | 2 +-
> fs/exfat/fatent.c | 6 +-----
> fs/exfat/misc.c | 3 +--
> fs/exfat/namei.c | 12 ++++++------
> fs/exfat/super.c | 3 +++
> 7 files changed, 22 insertions(+), 26 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 3eb8386fb5f2..96c9a817d928 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -468,7 +468,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); @@ -478,7 +478,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;
> @@ -514,7 +514,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;
> @@ -536,7 +536,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); @@ -545,7 +545,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++) { @@ -554,7 +554,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;
> }
> @@ -578,7 +578,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);
> }
>
> @@ -606,10 +606,8 @@ int exfat_free_dentry_set(struct exfat_entry_set_cache *es, int sync) {
> int i, err = 0;
>
> - if (es->modified) {
> - set_bit(EXFAT_SB_DIRTY, &EXFAT_SB(es->sb)->s_state);
> + if (es->modified)
> err = exfat_update_bhs(es->bh, es->num_bh, sync);
> - }
>
> for (i = 0; i < es->num_bh; i++)
> err ? bforget(es->bh[i]):brelse(es->bh[i]);
> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h index f4fa0e833486..0e094d186612 100644
> --- a/fs/exfat/exfat_fs.h
> +++ b/fs/exfat/exfat_fs.h
> @@ -514,7 +514,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);
> int exfat_update_bhs(struct buffer_head **bhs, int nr_bhs, int sync); void exfat_chain_set(struct
> exfat_chain *ec, unsigned int dir,
> unsigned int size, unsigned char flags); diff --git a/fs/exfat/fatent.c
> b/fs/exfat/fatent.c index 5d11bc2f1b68..f8171183b4c1 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) { @@ -261,7 +260,6 @@ int exfat_zeroed_cluster(struct
> inode *dir, unsigned int clu)
> memset(bhs[n]->b_data, 0, sb->s_blocksize);
> }
>
> - set_bit(EXFAT_SB_DIRTY, &sbi->s_state);
> err = exfat_update_bhs(bhs, n, IS_DIRSYNC(dir));
> if (err)
> goto release_bhs;
> @@ -326,8 +324,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 dc34968e99d3..564718747fb2 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 5b0f35329d63..e36c9fc4a5d6 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);
>
> @@ -1083,7 +1083,7 @@ static int exfat_rename_file(struct inode *inode, struct exfat_chain *p_dir,
> return -EIO;
>
> memcpy(epnew, epold, DENTRY_SIZE);
> - exfat_update_bh(sb, new_bh, sync);
> + exfat_update_bh(new_bh, sync);
> brelse(old_bh);
> brelse(new_bh);
>
> @@ -1100,7 +1100,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);
> @@ -1155,7 +1155,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);
>
> @@ -1167,7 +1167,7 @@ static int exfat_move_file(struct inode *inode, struct exfat_chain *p_olddir,
> return -EIO;
>
> 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..199a1e78f9e5 100644
> --- a/fs/exfat/super.c
> +++ b/fs/exfat/super.c
> @@ -104,6 +104,9 @@ int exfat_set_vol_flags(struct super_block *sb, unsigned short new_flag)
> struct boot_sector *p_boot = (struct boot_sector *)sbi->boot_bh->b_data;
> bool sync;
>
> + if (new_flag == VOL_DIRTY)
> + set_bit(EXFAT_SB_DIRTY, &sbi->s_state);
> +
> /* flags are not changed */
> if (sbi->vol_flag == new_flag)
> return 0;
> --
> 2.25.1
On 2020/06/05 16:32, Namjae Jeon wrote:
>> Set EXFAT_SB_DIRTY flag in exfat_put_super().
>>
>> In some cases, can't clear VOL_DIRTY with 'sync'.
>> 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.
> Good catch.
>
> Can you split this patch into two? (Don't set VOL_DIRTY on -ENOTEMPTY and Setting EXFAT_SB_DIRTY is
> merged into exfat_set_vol_flag). I need to check the second one more.
Can't do that.
exfat_set_vol_flag() is called when rmdir processing begins. When Not-empty is detected,
VOL_DIRTY has already been written and synced to the media.
This sequence is same as other write functions.
<begin writing function>
set VOL_DIRTY (write to the media)
preparation/state analysis
update bh & set SB_DIRTY
clear VOL_DIRTY (cached)
<end>
SB_DIRTY is set when updating bh.
However, in some cases SB_DIRTY is not set.
If SB_DIRTY is not set, exfat_sync_fs() does nothing.
I thought there was a problem with separating VOL_DIRTY and SB_DIRTY.
So I investigated the timing when these are set. Attach the caller graph.
The green box is a function that sets SB_DIRTY directly.
The red box is a function that calls exfat_set_vol_flag() and sets VOL_DIRTY.
VOL_DIRTY is set on all paths before setting SB_DIRTY.
I think VOL_DIRTY and SB_DIRTY should be set at the same time.
That way, It is not necessary to set SB_DIRTY when updating bh.
<begin writing function>
set VOL_DIRTY (actually write on media), and set SB_DIRTY
preparation/state analysis
update data
clear VOL_DIRTY (cached)
<end>
By doing this, sync is guaranteed if VOL_DIRTY is set by calling exfat_set_vol_flag.
This change may still have problems, but it's little better than before, I think.
BR
---
Tetsuhiro Kohada <[email protected]>
>
> Thanks!
>>
>> Signed-off-by: Tetsuhiro Kohada <[email protected]>
>> ---
>> fs/exfat/balloc.c | 4 ++--
>> fs/exfat/dir.c | 18 ++++++++----------
>> fs/exfat/exfat_fs.h | 2 +-
>> fs/exfat/fatent.c | 6 +-----
>> fs/exfat/misc.c | 3 +--
>> fs/exfat/namei.c | 12 ++++++------
>> fs/exfat/super.c | 3 +++
>> 7 files changed, 22 insertions(+), 26 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 3eb8386fb5f2..96c9a817d928 100644
>> --- a/fs/exfat/dir.c
>> +++ b/fs/exfat/dir.c
>> @@ -468,7 +468,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); @@ -478,7 +478,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;
>> @@ -514,7 +514,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;
>> @@ -536,7 +536,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); @@ -545,7 +545,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++) { @@ -554,7 +554,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;
>> }
>> @@ -578,7 +578,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);
>> }
>>
>> @@ -606,10 +606,8 @@ int exfat_free_dentry_set(struct exfat_entry_set_cache *es, int sync) {
>> int i, err = 0;
>>
>> - if (es->modified) {
>> - set_bit(EXFAT_SB_DIRTY, &EXFAT_SB(es->sb)->s_state);
>> + if (es->modified)
>> err = exfat_update_bhs(es->bh, es->num_bh, sync);
>> - }
>>
>> for (i = 0; i < es->num_bh; i++)
>> err ? bforget(es->bh[i]):brelse(es->bh[i]);
>> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h index f4fa0e833486..0e094d186612 100644
>> --- a/fs/exfat/exfat_fs.h
>> +++ b/fs/exfat/exfat_fs.h
>> @@ -514,7 +514,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);
>> int exfat_update_bhs(struct buffer_head **bhs, int nr_bhs, int sync); void exfat_chain_set(struct
>> exfat_chain *ec, unsigned int dir,
>> unsigned int size, unsigned char flags); diff --git a/fs/exfat/fatent.c
>> b/fs/exfat/fatent.c index 5d11bc2f1b68..f8171183b4c1 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) { @@ -261,7 +260,6 @@ int exfat_zeroed_cluster(struct
>> inode *dir, unsigned int clu)
>> memset(bhs[n]->b_data, 0, sb->s_blocksize);
>> }
>>
>> - set_bit(EXFAT_SB_DIRTY, &sbi->s_state);
>> err = exfat_update_bhs(bhs, n, IS_DIRSYNC(dir));
>> if (err)
>> goto release_bhs;
>> @@ -326,8 +324,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 dc34968e99d3..564718747fb2 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 5b0f35329d63..e36c9fc4a5d6 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);
>>
>> @@ -1083,7 +1083,7 @@ static int exfat_rename_file(struct inode *inode, struct exfat_chain *p_dir,
>> return -EIO;
>>
>> memcpy(epnew, epold, DENTRY_SIZE);
>> - exfat_update_bh(sb, new_bh, sync);
>> + exfat_update_bh(new_bh, sync);
>> brelse(old_bh);
>> brelse(new_bh);
>>
>> @@ -1100,7 +1100,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);
>> @@ -1155,7 +1155,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);
>>
>> @@ -1167,7 +1167,7 @@ static int exfat_move_file(struct inode *inode, struct exfat_chain *p_olddir,
>> return -EIO;
>>
>> 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..199a1e78f9e5 100644
>> --- a/fs/exfat/super.c
>> +++ b/fs/exfat/super.c
>> @@ -104,6 +104,9 @@ int exfat_set_vol_flags(struct super_block *sb, unsigned short new_flag)
>> struct boot_sector *p_boot = (struct boot_sector *)sbi->boot_bh->b_data;
>> bool sync;
>>
>> + if (new_flag == VOL_DIRTY)
>> + set_bit(EXFAT_SB_DIRTY, &sbi->s_state);
>> +
>> /* flags are not changed */
>> if (sbi->vol_flag == new_flag)
>> return 0;
>> --
>> 2.25.1
>
>
2020-06-06 18:22 GMT+09:00, Tetsuhiro Kohada <[email protected]>:
> On 2020/06/05 16:32, Namjae Jeon wrote:
>>> Set EXFAT_SB_DIRTY flag in exfat_put_super().
>>>
>>> In some cases, can't clear VOL_DIRTY with 'sync'.
>>> 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.
>> Good catch.
>>
>> Can you split this patch into two? (Don't set VOL_DIRTY on -ENOTEMPTY and
>> Setting EXFAT_SB_DIRTY is
>> merged into exfat_set_vol_flag). I need to check the second one more.
>
> Can't do that.
>
> exfat_set_vol_flag() is called when rmdir processing begins. When Not-empty
> is detected,
> VOL_DIRTY has already been written and synced to the media.
You can move it before calling exfat_remove_entries().
>
> This sequence is same as other write functions.
> <begin writing function>
> set VOL_DIRTY (write to the media)
> preparation/state analysis
> update bh & set SB_DIRTY
> clear VOL_DIRTY (cached)
> <end>
>
> SB_DIRTY is set when updating bh.
> However, in some cases SB_DIRTY is not set.
> If SB_DIRTY is not set, exfat_sync_fs() does nothing.
>
> I thought there was a problem with separating VOL_DIRTY and SB_DIRTY.
> So I investigated the timing when these are set. Attach the caller graph.
> The green box is a function that sets SB_DIRTY directly.
> The red box is a function that calls exfat_set_vol_flag() and sets
> VOL_DIRTY.
> VOL_DIRTY is set on all paths before setting SB_DIRTY.
>
> I think VOL_DIRTY and SB_DIRTY should be set at the same time.
> That way, It is not necessary to set SB_DIRTY when updating bh.
>
> <begin writing function>
> set VOL_DIRTY (actually write on media), and set SB_DIRTY
> preparation/state analysis
> update data
> clear VOL_DIRTY (cached)
> <end>
>
> By doing this, sync is guaranteed if VOL_DIRTY is set by calling
> exfat_set_vol_flag.
>
> This change may still have problems, but it's little better than before, I
> think.
I need to check more if it is the best or there is more better way.
Thanks!
>
> BR
> ---
> Tetsuhiro Kohada <[email protected]>
>
>
>>
>> Thanks!
>>>
>>> Signed-off-by: Tetsuhiro Kohada <[email protected]>
>>> ---
>>> fs/exfat/balloc.c | 4 ++--
>>> fs/exfat/dir.c | 18 ++++++++----------
>>> fs/exfat/exfat_fs.h | 2 +-
>>> fs/exfat/fatent.c | 6 +-----
>>> fs/exfat/misc.c | 3 +--
>>> fs/exfat/namei.c | 12 ++++++------
>>> fs/exfat/super.c | 3 +++
>>> 7 files changed, 22 insertions(+), 26 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
>>> 3eb8386fb5f2..96c9a817d928 100644
>>> --- a/fs/exfat/dir.c
>>> +++ b/fs/exfat/dir.c
>>> @@ -468,7 +468,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); @@ -478,7
>>> +478,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;
>>> @@ -514,7 +514,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;
>>> @@ -536,7 +536,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); @@ -545,7
>>> +545,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++) { @@ -554,7 +554,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;
>>> }
>>> @@ -578,7 +578,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);
>>> }
>>>
>>> @@ -606,10 +606,8 @@ int exfat_free_dentry_set(struct
>>> exfat_entry_set_cache *es, int sync) {
>>> int i, err = 0;
>>>
>>> - if (es->modified) {
>>> - set_bit(EXFAT_SB_DIRTY, &EXFAT_SB(es->sb)->s_state);
>>> + if (es->modified)
>>> err = exfat_update_bhs(es->bh, es->num_bh, sync);
>>> - }
>>>
>>> for (i = 0; i < es->num_bh; i++)
>>> err ? bforget(es->bh[i]):brelse(es->bh[i]);
>>> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h index
>>> f4fa0e833486..0e094d186612 100644
>>> --- a/fs/exfat/exfat_fs.h
>>> +++ b/fs/exfat/exfat_fs.h
>>> @@ -514,7 +514,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);
>>> int exfat_update_bhs(struct buffer_head **bhs, int nr_bhs, int sync);
>>> void exfat_chain_set(struct
>>> exfat_chain *ec, unsigned int dir,
>>> unsigned int size, unsigned char flags); diff --git
>>> a/fs/exfat/fatent.c
>>> b/fs/exfat/fatent.c index 5d11bc2f1b68..f8171183b4c1 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) { @@ -261,7 +260,6 @@ int
>>> exfat_zeroed_cluster(struct
>>> inode *dir, unsigned int clu)
>>> memset(bhs[n]->b_data, 0, sb->s_blocksize);
>>> }
>>>
>>> - set_bit(EXFAT_SB_DIRTY, &sbi->s_state);
>>> err = exfat_update_bhs(bhs, n, IS_DIRSYNC(dir));
>>> if (err)
>>> goto release_bhs;
>>> @@ -326,8 +324,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 dc34968e99d3..564718747fb2 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
>>> 5b0f35329d63..e36c9fc4a5d6 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);
>>>
>>> @@ -1083,7 +1083,7 @@ static int exfat_rename_file(struct inode *inode,
>>> struct exfat_chain *p_dir,
>>> return -EIO;
>>>
>>> memcpy(epnew, epold, DENTRY_SIZE);
>>> - exfat_update_bh(sb, new_bh, sync);
>>> + exfat_update_bh(new_bh, sync);
>>> brelse(old_bh);
>>> brelse(new_bh);
>>>
>>> @@ -1100,7 +1100,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);
>>> @@ -1155,7 +1155,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);
>>>
>>> @@ -1167,7 +1167,7 @@ static int exfat_move_file(struct inode *inode,
>>> struct exfat_chain *p_olddir,
>>> return -EIO;
>>>
>>> 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..199a1e78f9e5 100644
>>> --- a/fs/exfat/super.c
>>> +++ b/fs/exfat/super.c
>>> @@ -104,6 +104,9 @@ int exfat_set_vol_flags(struct super_block *sb,
>>> unsigned short new_flag)
>>> struct boot_sector *p_boot = (struct boot_sector
>>> *)sbi->boot_bh->b_data;
>>> bool sync;
>>>
>>> + if (new_flag == VOL_DIRTY)
>>> + set_bit(EXFAT_SB_DIRTY, &sbi->s_state);
>>> +
>>> /* flags are not changed */
>>> if (sbi->vol_flag == new_flag)
>>> return 0;
>>> --
>>> 2.25.1
>>
>>
>
2020-06-04 17:44 GMT+09:00, Tetsuhiro Kohada <[email protected]>:
Hi Tetsuhiro,
> Add error check when synchronously updating dir-entries.
> Furthermore, add exfat_update_bhs(). It wait for write completion once
> instead of sector by sector.
This patch can be split into two also ?
>
> Suggested-by: Sungjong Seo <[email protected]>
> Signed-off-by: Tetsuhiro Kohada <[email protected]>
> ---
> fs/exfat/dir.c | 15 +++++++++------
> fs/exfat/exfat_fs.h | 3 ++-
> fs/exfat/file.c | 5 ++++-
> fs/exfat/inode.c | 8 +++++---
> fs/exfat/misc.c | 19 +++++++++++++++++++
> 5 files changed, 39 insertions(+), 11 deletions(-)
>
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c
> index de43534aa299..3eb8386fb5f2 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -602,16 +602,19 @@ void exfat_update_dir_chksum_with_entry_set(struct
> exfat_entry_set_cache *es)
> es->modified = true;
> }
>
> -void exfat_free_dentry_set(struct exfat_entry_set_cache *es, int sync)
> +int exfat_free_dentry_set(struct exfat_entry_set_cache *es, int sync)
> {
> - int i;
> + int i, err = 0;
>
> - for (i = 0; i < es->num_bh; i++) {
> - if (es->modified)
> - exfat_update_bh(es->sb, es->bh[i], sync);
> - brelse(es->bh[i]);
> + if (es->modified) {
> + set_bit(EXFAT_SB_DIRTY, &EXFAT_SB(es->sb)->s_state);
EXFAT_SB_DIRTY set can be merged into exfat_update_bhs() ?
> + err = exfat_update_bhs(es->bh, es->num_bh, sync);
> }
> +
> + for (i = 0; i < es->num_bh; i++)
> + err ? bforget(es->bh[i]):brelse(es->bh[i]);
> kfree(es);
> + return err;
> }
>
> static int exfat_walk_fat_chain(struct super_block *sb,
> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
> index 595f3117f492..f4fa0e833486 100644
> --- a/fs/exfat/exfat_fs.h
> +++ b/fs/exfat/exfat_fs.h
> @@ -462,7 +462,7 @@ struct exfat_dentry *exfat_get_dentry_cached(struct
> exfat_entry_set_cache *es,
> int num);
> struct exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb,
> struct exfat_chain *p_dir, int entry, unsigned int type);
> -void exfat_free_dentry_set(struct exfat_entry_set_cache *es, int sync);
> +int exfat_free_dentry_set(struct exfat_entry_set_cache *es, int sync);
> int exfat_count_dir_entries(struct super_block *sb, struct exfat_chain
> *p_dir);
>
> /* inode.c */
> @@ -515,6 +515,7 @@ void exfat_set_entry_time(struct exfat_sb_info *sbi,
> struct timespec64 *ts,
> 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);
> +int exfat_update_bhs(struct buffer_head **bhs, int nr_bhs, 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/file.c b/fs/exfat/file.c
> index fce03f318787..37c8f04c1f8a 100644
> --- a/fs/exfat/file.c
> +++ b/fs/exfat/file.c
> @@ -153,6 +153,7 @@ int __exfat_truncate(struct inode *inode, loff_t
> new_size)
> struct timespec64 ts;
> struct exfat_dentry *ep, *ep2;
> struct exfat_entry_set_cache *es;
> + int err;
>
> es = exfat_get_dentry_set(sb, &(ei->dir), ei->entry,
> ES_ALL_ENTRIES);
> @@ -187,7 +188,9 @@ int __exfat_truncate(struct inode *inode, loff_t
> new_size)
> }
>
> exfat_update_dir_chksum_with_entry_set(es);
> - exfat_free_dentry_set(es, inode_needs_sync(inode));
> + err = exfat_free_dentry_set(es, inode_needs_sync(inode));
> + if (err)
> + return err;
> }
>
> /* cut off from the FAT chain */
> diff --git a/fs/exfat/inode.c b/fs/exfat/inode.c
> index ef7cf7a6d187..c0bfd1a586aa 100644
> --- a/fs/exfat/inode.c
> +++ b/fs/exfat/inode.c
> @@ -77,8 +77,7 @@ static int __exfat_write_inode(struct inode *inode, int
> sync)
> ep2->dentry.stream.size = ep2->dentry.stream.valid_size;
>
> exfat_update_dir_chksum_with_entry_set(es);
> - exfat_free_dentry_set(es, sync);
> - return 0;
> + return exfat_free_dentry_set(es, sync);
> }
>
> int exfat_write_inode(struct inode *inode, struct writeback_control *wbc)
> @@ -222,6 +221,7 @@ static int exfat_map_cluster(struct inode *inode,
> unsigned int clu_offset,
> if (ei->dir.dir != DIR_DELETED && modified) {
> struct exfat_dentry *ep;
> struct exfat_entry_set_cache *es;
> + int err;
>
> es = exfat_get_dentry_set(sb, &(ei->dir), ei->entry,
> ES_ALL_ENTRIES);
> @@ -240,7 +240,9 @@ static int exfat_map_cluster(struct inode *inode,
> unsigned int clu_offset,
> ep->dentry.stream.valid_size;
>
> exfat_update_dir_chksum_with_entry_set(es);
> - exfat_free_dentry_set(es, inode_needs_sync(inode));
> + err = exfat_free_dentry_set(es, inode_needs_sync(inode));
> + if (err)
> + return err;
>
> } /* end of if != DIR_DELETED */
>
> diff --git a/fs/exfat/misc.c b/fs/exfat/misc.c
> index 17d41f3d3709..dc34968e99d3 100644
> --- a/fs/exfat/misc.c
> +++ b/fs/exfat/misc.c
> @@ -173,6 +173,25 @@ void exfat_update_bh(struct super_block *sb, struct
> buffer_head *bh, int sync)
> sync_dirty_buffer(bh);
> }
>
> +int exfat_update_bhs(struct buffer_head **bhs, int nr_bhs, int sync)
> +{
> + int i, err = 0;
> +
> + for (i = 0; i < nr_bhs; i++) {
> + set_buffer_uptodate(bhs[i]);
> + mark_buffer_dirty(bhs[i]);
> + if (sync)
> + write_dirty_buffer(bhs[i], 0);
> + }
> +
> + for (i = 0; i < nr_bhs && sync; i++) {
> + wait_on_buffer(bhs[i]);
> + if (!buffer_uptodate(bhs[i]))
> + err = -EIO;
> + }
> + return err;
> +}
> +
> void exfat_chain_set(struct exfat_chain *ec, unsigned int dir,
> unsigned int size, unsigned char flags)
> {
> --
> 2.25.1
>
>
Thank you for your comment.
> >> Can you split this patch into two? (Don't set VOL_DIRTY on -ENOTEMPTY and
> >> Setting EXFAT_SB_DIRTY is
> >> merged into exfat_set_vol_flag). I need to check the second one more.
> >
> > Can't do that.
> >
> > exfat_set_vol_flag() is called when rmdir processing begins. When Not-empty
> > is detected,
> > VOL_DIRTY has already been written and synced to the media.
> You can move it before calling exfat_remove_entries().
Can be moved, but that doesn't solve the problem.
It causes the similar problem as before.
exfat_remove_entries() calls exfat_get_dentry().
If exfat_get_dentry() fails, update bh and set SB_DIRTY will not be executed.
As a result, SB_DIRTY is not set and sync does not work.
Similar problems occur with other writing functions.
Similar problems occur when pre-write checks are added in the future.
If you don't set VOL_DIRTY at the beginning, you should delay to set VOL_DIRTY until update-bh & set SB_DIRTY.
This avoids unnecessary changes to VOL_DIRTY/VOL_CLEAN.
I think this method is smart, but it is difficult to decide when to set VOL_CLEAN.
(I tried to implement it, but gave up)
> > By doing this, sync is guaranteed if VOL_DIRTY is set by calling
> > exfat_set_vol_flag.
> >
> > This change may still have problems, but it's little better than before, I
> > think.
> I need to check more if it is the best or there is more better way.
I think the sync-problems still exist.
Let's improve little by little. :-)
BR
---
Kohada Tetsuhiro <[email protected]>
> Thank you for your comment.
>
> > >> Can you split this patch into two? (Don't set VOL_DIRTY on
> > >> -ENOTEMPTY and Setting EXFAT_SB_DIRTY is merged into
> > >> exfat_set_vol_flag). I need to check the second one more.
> > >
> > > Can't do that.
> > >
> > > exfat_set_vol_flag() is called when rmdir processing begins. When
> > > Not-empty is detected, VOL_DIRTY has already been written and synced
> > > to the media.
> > You can move it before calling exfat_remove_entries().
>
> Can be moved, but that doesn't solve the problem.
> It causes the similar problem as before.
>
> exfat_remove_entries() calls exfat_get_dentry().
> If exfat_get_dentry() fails, update bh and set SB_DIRTY will not be executed.
> As a result, SB_DIRTY is not set and sync does not work.
> Similar problems occur with other writing functions.
> Similar problems occur when pre-write checks are added in the future.
>
> If you don't set VOL_DIRTY at the beginning, you should delay to set VOL_DIRTY until update-bh & set
> SB_DIRTY.
> This avoids unnecessary changes to VOL_DIRTY/VOL_CLEAN.
Right, That's what I am going to point out.
> I think this method is smart, but it is difficult to decide when to set VOL_CLEAN.
> (I tried to implement it, but gave up)
Okay, I'm a little busy now, but I'll give you feedback soon.
Thanks for your work!
>
> > > By doing this, sync is guaranteed if VOL_DIRTY is set by calling
> > > exfat_set_vol_flag.
> > >
> > > This change may still have problems, but it's little better than
> > > before, I think.
> > I need to check more if it is the best or there is more better way.
>
> I think the sync-problems still exist.
> Let's improve little by little. :-)
>
> BR
> ---
> Kohada Tetsuhiro <[email protected]>
>> Add error check when synchronously updating dir-entries.
>> Furthermore, add exfat_update_bhs(). It wait for write completion once
>> instead of sector by sector.
> This patch can be split into two also ?
I sent a patch split into 'write multiple sectors at once'
and 'add error check when updating dir-entries'.
The other two patches(2nd & 3rd) are no-changed, so have not been sent.
If you need the other two patches, I will send them.
In that case, please tell me how to write the subject and change-log.
BR
---
Tetsuhiro Kohada <[email protected]>