[PATCH 1/3] exfat: add error check when updating dir-entries

[PATCH 1/3] exfat: add error check when updating dir-entries

[Tetsuhiro Kohada]

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 <sj1557.seo@samsung.com>
Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com>
---
 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

[PATCH 2/3] exfat: optimize exfat_zeroed_cluster()

[Tetsuhiro Kohada]

Replace part of exfat_zeroed_cluster() with exfat_update_bhs().
And remove exfat_sync_bhs().

Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com>
---
 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

[PATCH 3/3] exfat: set EXFAT_SB_DIRTY and VOL_DIRTY at the same timing

[Tetsuhiro Kohada]

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 <kohada.t2@gmail.com>
---
 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, &sector);
@@ -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, &sector);
@@ -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

RE: [PATCH 3/3] exfat: set EXFAT_SB_DIRTY and VOL_DIRTY at the same timing

[Namjae Jeon]

> 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 <kohada.t2@gmail.com>
> ---
>  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, &sector); @@ -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, &sector); @@ -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

Re: [PATCH 3/3] exfat: set EXFAT_SB_DIRTY and VOL_DIRTY at the same timing

[Tetsuhiro Kohada]

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

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 <kohada.t2@gmail.com>


> 
> Thanks!
>>
>> Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com>
>> ---
>>   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, &sector); @@ -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, &sector); @@ -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
> 
> 

[-- Attachment #2: exfat__fs_8h_a9aac0f25e9499af92c7ac753f1bcb049_icgraph.png --]
[-- Type: image/png, Size: 345222 bytes --]

Re: [PATCH 3/3] exfat: set EXFAT_SB_DIRTY and VOL_DIRTY at the same timing

[Namjae Jeon]

2020-06-06 18:22 GMT+09:00, Tetsuhiro Kohada <kohada.t2@gmail.com>:
> 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 <kohada.t2@gmail.com>
>
>
>>
>> Thanks!
>>>
>>> Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com>
>>> ---
>>>   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, &sector); @@ -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, &sector); @@ -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
>>
>>
>

RE: [PATCH 3/3] exfat: set EXFAT_SB_DIRTY and VOL_DIRTY at the same timing

[Kohada.Tetsuhiro]

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 <Kohada.Tetsuhiro@dc.MitsubishiElectric.co.jp>

RE: [PATCH 3/3] exfat: set EXFAT_SB_DIRTY and VOL_DIRTY at the same timing

[Namjae Jeon]

> 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 <Kohada.Tetsuhiro@dc.MitsubishiElectric.co.jp>

Re: [PATCH 1/3] exfat: add error check when updating dir-entries

[Namjae Jeon]

2020-06-04 17:44 GMT+09:00, Tetsuhiro Kohada <kohada.t2@gmail.com>:
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 <sj1557.seo@samsung.com>
> Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com>
> ---
>  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
>
>

Re: [PATCH 1/3] exfat: add error check when updating dir-entries

[Tetsuhiro Kohada]

>> 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 <kohada.t2@gmail.com>