[resend,2/5] ext4: wrap error handling and corrupted bitmaps setting

Message ID 1527594317-9214-2-git-send-email-wshilong1991@gmail.com
State New
Headers show
Series
  • [1/5] ext4: fix race with setting free_inode/clusters_counter
Related show

Commit Message

Wang Shilong May 29, 2018, 11:45 a.m.
From: Wang Shilong <wshilong@ddn.com>

Cleanup to handle ext4 error codes together with
bitmaps corrupted bits setting.

Signed-off-by: Wang Shilong <wshilong@ddn.com>
Reviewed-by: Andreas Dilger <adilger@dilger.ca>
---
 fs/ext4/balloc.c  | 28 +++++++++++++++-------------
 fs/ext4/ext4.h    | 35 ++++++++++++++++++++++++-----------
 fs/ext4/ialloc.c  | 37 +++++++++++++++++++------------------
 fs/ext4/mballoc.c | 20 ++++++++------------
 fs/ext4/super.c   | 11 +++++++----
 5 files changed, 73 insertions(+), 58 deletions(-)

Comments

Theodore Y. Ts'o July 30, 2018, 1:34 a.m. | #1
On Tue, May 29, 2018 at 08:45:14PM +0900, Wang Shilong wrote:
> From: Wang Shilong <wshilong@ddn.com>
> 
> Cleanup to handle ext4 error codes together with
> bitmaps corrupted bits setting.
> 
> Signed-off-by: Wang Shilong <wshilong@ddn.com>
> Reviewed-by: Andreas Dilger <adilger@dilger.ca>
> ---
>  fs/ext4/balloc.c  | 28 +++++++++++++++-------------
>  fs/ext4/ext4.h    | 35 ++++++++++++++++++++++++-----------
>  fs/ext4/ialloc.c  | 37 +++++++++++++++++++------------------
>  fs/ext4/mballoc.c | 20 ++++++++------------
>  fs/ext4/super.c   | 11 +++++++----
>  5 files changed, 73 insertions(+), 58 deletions(-)

This change makes the code longer and it's **really** not clear why it
is a "cleanup".  This is an example of explaining *why* is often far
more important than the *what*.

					- Ted

Patch

diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
index b00481c..e3feeae 100644
--- a/fs/ext4/balloc.c
+++ b/fs/ext4/balloc.c
@@ -191,7 +191,7 @@  static int ext4_init_block_bitmap(struct super_block *sb,
 	/* If checksum is bad mark all blocks used to prevent allocation
 	 * essentially implementing a per-group read-only flag. */
 	if (!ext4_group_desc_csum_verify(sb, block_group, gdp)) {
-		ext4_mark_group_bitmap_corrupted(sb, block_group,
+		__ext4_mark_group_bitmap_corrupted(sb, block_group,
 					EXT4_GROUP_INFO_BBITMAP_CORRUPT |
 					EXT4_GROUP_INFO_IBITMAP_CORRUPT);
 		return -EFSBADCRC;
@@ -375,18 +375,19 @@  static int ext4_validate_block_bitmap(struct super_block *sb,
 	if (unlikely(!ext4_block_bitmap_csum_verify(sb, block_group,
 			desc, bh))) {
 		ext4_unlock_group(sb, block_group);
-		ext4_error(sb, "bg %u: bad block bitmap checksum", block_group);
 		ext4_mark_group_bitmap_corrupted(sb, block_group,
-					EXT4_GROUP_INFO_BBITMAP_CORRUPT);
+					EXT4_GROUP_INFO_BBITMAP_CORRUPT,
+					"bg %u: bad block bitmap checksum",
+					block_group);
 		return -EFSBADCRC;
 	}
 	blk = ext4_valid_block_bitmap(sb, desc, block_group, bh);
 	if (unlikely(blk != 0)) {
 		ext4_unlock_group(sb, block_group);
-		ext4_error(sb, "bg %u: block %llu: invalid block bitmap",
-			   block_group, blk);
 		ext4_mark_group_bitmap_corrupted(sb, block_group,
-					EXT4_GROUP_INFO_BBITMAP_CORRUPT);
+				EXT4_GROUP_INFO_BBITMAP_CORRUPT,
+				"bg %u: block %llu: invalid block bitmap",
+				block_group, blk);
 		return -EFSCORRUPTED;
 	}
 	set_buffer_verified(bh);
@@ -419,10 +420,10 @@  struct buffer_head *
 	bitmap_blk = ext4_block_bitmap(sb, desc);
 	if ((bitmap_blk <= le32_to_cpu(sbi->s_es->s_first_data_block)) ||
 	    (bitmap_blk >= ext4_blocks_count(sbi->s_es))) {
-		ext4_error(sb, "Invalid block bitmap block %llu in "
-			   "block_group %u", bitmap_blk, block_group);
 		ext4_mark_group_bitmap_corrupted(sb, block_group,
-					EXT4_GROUP_INFO_BBITMAP_CORRUPT);
+				EXT4_GROUP_INFO_BBITMAP_CORRUPT,
+				"Invalid block bitmap block %llu in "
+				"block_group %u", bitmap_blk, block_group);
 		return ERR_PTR(-EFSCORRUPTED);
 	}
 	bh = sb_getblk(sb, bitmap_blk);
@@ -498,11 +499,12 @@  int ext4_wait_block_bitmap(struct super_block *sb, ext4_group_t block_group,
 		return -EFSCORRUPTED;
 	wait_on_buffer(bh);
 	if (!buffer_uptodate(bh)) {
-		ext4_error(sb, "Cannot read block bitmap - "
-			   "block_group = %u, block_bitmap = %llu",
-			   block_group, (unsigned long long) bh->b_blocknr);
 		ext4_mark_group_bitmap_corrupted(sb, block_group,
-					EXT4_GROUP_INFO_BBITMAP_CORRUPT);
+				EXT4_GROUP_INFO_BBITMAP_CORRUPT,
+				"Cannot read block bitmap - "
+				"block_group = %u, block_bitmap = %llu",
+				block_group,
+				(unsigned long long)bh->b_blocknr);
 		return -EIO;
 	}
 	clear_buffer_new(bh);
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index fa52b7d..ed5b988 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2530,9 +2530,9 @@  extern int ext4_alloc_flex_bg_array(struct super_block *sb,
 				    ext4_group_t ngroup);
 extern const char *ext4_decode_error(struct super_block *sb, int errno,
 				     char nbuf[16]);
-extern void ext4_mark_group_bitmap_corrupted(struct super_block *sb,
-					     ext4_group_t block_group,
-					     unsigned int flags);
+extern void __ext4_mark_group_bitmap_corrupted(struct super_block *sb,
+					       ext4_group_t block_group,
+					       unsigned int flags);
 
 extern __printf(4, 5)
 void __ext4_error(struct super_block *, const char *, unsigned int,
@@ -2558,10 +2558,10 @@  extern __printf(3, 4)
 void __ext4_msg(struct super_block *, const char *, const char *, ...);
 extern void __dump_mmp_msg(struct super_block *, struct mmp_struct *mmp,
 			   const char *, unsigned int, const char *);
-extern __printf(7, 8)
+extern __printf(8, 9)
 void __ext4_grp_locked_error(const char *, unsigned int,
 			     struct super_block *, ext4_group_t,
-			     unsigned long, ext4_fsblk_t,
+			     unsigned long, ext4_fsblk_t, unsigned int,
 			     const char *, ...);
 
 #define EXT4_ERROR_INODE(inode, fmt, a...) \
@@ -2591,9 +2591,16 @@  void __ext4_grp_locked_error(const char *, unsigned int,
 	__ext4_msg(sb, level, fmt, ##__VA_ARGS__)
 #define dump_mmp_msg(sb, mmp, msg)					\
 	__dump_mmp_msg(sb, mmp, __func__, __LINE__, msg)
-#define ext4_grp_locked_error(sb, grp, ino, block, fmt, ...)		\
-	__ext4_grp_locked_error(__func__, __LINE__, sb, grp, ino, block, \
-				fmt, ##__VA_ARGS__)
+#define ext4_grp_locked_error(sb, grp, ino, block, flags, fmt, ...)	\
+	__ext4_grp_locked_error(__func__, __LINE__, sb, grp, ino, block,\
+				flags, fmt, ##__VA_ARGS__)
+#define ext4_mark_group_bitmap_corrupted(sb, group, flags, fmt, ...)	\
+	do {								\
+		__ext4_error(sb, __func__, __LINE__,			\
+			     fmt, ##__VA_ARGS__);			\
+		__ext4_mark_group_bitmap_corrupted(sb, group, flags);	\
+	} while (0)
+
 
 #else
 
@@ -2634,11 +2641,17 @@  void __ext4_grp_locked_error(const char *, unsigned int,
 } while (0)
 #define dump_mmp_msg(sb, mmp, msg)					\
 	__dump_mmp_msg(sb, mmp, "", 0, "")
-#define ext4_grp_locked_error(sb, grp, ino, block, fmt, ...)		\
+#define ext4_grp_locked_error(sb, grp, ino, block, flags, fmt, ...)	\
 do {									\
-	no_printk(fmt, ##__VA_ARGS__);				\
-	__ext4_grp_locked_error("", 0, sb, grp, ino, block, " ");	\
+	no_printk(fmt, ##__VA_ARGS__);					\
+	__ext4_grp_locked_error("", 0, sb, grp, ino, block, flags, " ");\
 } while (0)
+#define ext4_mark_group_bitmap_corrupted(sb, group, flags, fmt, ...)	\
+	do {								\
+		no_printk(fmt, ##__VA_ARGS__);				\
+		__ext4_error(sb, "", 0, " ");				\
+		__ext4_mark_group_bitmap_corrupted(sb, group, flags);	\
+	} while (0)
 
 #endif
 
diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index 4d6e007..b7f7299 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -94,10 +94,10 @@  static int ext4_validate_inode_bitmap(struct super_block *sb,
 	if (!ext4_inode_bitmap_csum_verify(sb, block_group, desc, bh,
 					   EXT4_INODES_PER_GROUP(sb) / 8)) {
 		ext4_unlock_group(sb, block_group);
-		ext4_error(sb, "Corrupt inode bitmap - block_group = %u, "
-			   "inode_bitmap = %llu", block_group, blk);
 		ext4_mark_group_bitmap_corrupted(sb, block_group,
-					EXT4_GROUP_INFO_IBITMAP_CORRUPT);
+				EXT4_GROUP_INFO_IBITMAP_CORRUPT,
+				"Corrupt inode bitmap - block_group = %u, "
+				"inode_bitmap = %llu", block_group, blk);
 		return -EFSBADCRC;
 	}
 	set_buffer_verified(bh);
@@ -127,10 +127,10 @@  static int ext4_validate_inode_bitmap(struct super_block *sb,
 	bitmap_blk = ext4_inode_bitmap(sb, desc);
 	if ((bitmap_blk <= le32_to_cpu(sbi->s_es->s_first_data_block)) ||
 	    (bitmap_blk >= ext4_blocks_count(sbi->s_es))) {
-		ext4_error(sb, "Invalid inode bitmap blk %llu in "
-			   "block_group %u", bitmap_blk, block_group);
 		ext4_mark_group_bitmap_corrupted(sb, block_group,
-					EXT4_GROUP_INFO_IBITMAP_CORRUPT);
+				EXT4_GROUP_INFO_IBITMAP_CORRUPT,
+				"Invalid inode bitmap blk %llu in "
+				"block_group %u", bitmap_blk, block_group);
 		return ERR_PTR(-EFSCORRUPTED);
 	}
 	bh = sb_getblk(sb, bitmap_blk);
@@ -182,11 +182,11 @@  static int ext4_validate_inode_bitmap(struct super_block *sb,
 	wait_on_buffer(bh);
 	if (!buffer_uptodate(bh)) {
 		put_bh(bh);
-		ext4_error(sb, "Cannot read inode bitmap - "
-			   "block_group = %u, inode_bitmap = %llu",
-			   block_group, bitmap_blk);
 		ext4_mark_group_bitmap_corrupted(sb, block_group,
-				EXT4_GROUP_INFO_IBITMAP_CORRUPT);
+				EXT4_GROUP_INFO_IBITMAP_CORRUPT,
+				"Cannot read inode bitmap - "
+				"block_group = %u, inode_bitmap = %llu",
+				block_group, bitmap_blk);
 		return ERR_PTR(-EIO);
 	}
 
@@ -333,9 +333,10 @@  void ext4_free_inode(handle_t *handle, struct inode *inode)
 		if (!fatal)
 			fatal = err;
 	} else {
-		ext4_error(sb, "bit already cleared for inode %lu", ino);
 		ext4_mark_group_bitmap_corrupted(sb, block_group,
-					EXT4_GROUP_INFO_IBITMAP_CORRUPT);
+				EXT4_GROUP_INFO_IBITMAP_CORRUPT,
+				"bit already cleared for inode %lu",
+				ino);
 	}
 
 error_return:
@@ -904,10 +905,10 @@  struct inode *__ext4_new_inode(handle_t *handle, struct inode *dir,
 			goto next_group;
 
 		if (group == 0 && (ino + 1) < EXT4_FIRST_INO(sb)) {
-			ext4_error(sb, "reserved inode found cleared - "
-				   "inode=%lu", ino + 1);
 			ext4_mark_group_bitmap_corrupted(sb, group,
-					EXT4_GROUP_INFO_IBITMAP_CORRUPT);
+				EXT4_GROUP_INFO_IBITMAP_CORRUPT,
+				"reserved inode found cleared - "
+				"inode=%lu", ino + 1);
 			goto next_group;
 		}
 
@@ -1097,10 +1098,10 @@  struct inode *__ext4_new_inode(handle_t *handle, struct inode *dir,
 		 * twice.
 		 */
 		err = -EIO;
-		ext4_error(sb, "failed to insert inode %lu: doubly allocated?",
-			   inode->i_ino);
 		ext4_mark_group_bitmap_corrupted(sb, group,
-					EXT4_GROUP_INFO_IBITMAP_CORRUPT);
+				EXT4_GROUP_INFO_IBITMAP_CORRUPT,
+				"failed to insert inode %lu: doubly allocated?",
+				inode->i_ino);
 		goto out;
 	}
 	inode->i_generation = prandom_u32();
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 243c42f..3680623 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -467,11 +467,10 @@  static void mb_free_blocks_double(struct inode *inode, struct ext4_buddy *e4b,
 			ext4_grp_locked_error(sb, e4b->bd_group,
 					      inode ? inode->i_ino : 0,
 					      blocknr,
+					      EXT4_GROUP_INFO_BBITMAP_CORRUPT,
 					      "freeing block already freed "
 					      "(bit %u)",
 					      first + i);
-			ext4_mark_group_bitmap_corrupted(sb, e4b->bd_group,
-					EXT4_GROUP_INFO_BBITMAP_CORRUPT);
 		}
 		mb_clear_bit(first + i, e4b->bd_info->bb_bitmap);
 	}
@@ -741,6 +740,7 @@  void ext4_mb_generate_buddy(struct super_block *sb,
 
 	if (free != grp->bb_free) {
 		ext4_grp_locked_error(sb, group, 0, 0,
+				      EXT4_GROUP_INFO_BBITMAP_CORRUPT,
 				      "block bitmap and bg descriptor "
 				      "inconsistent: %u vs %u free clusters",
 				      free, grp->bb_free);
@@ -749,8 +749,6 @@  void ext4_mb_generate_buddy(struct super_block *sb,
 		 * corrupt and update bb_free using bitmap value
 		 */
 		grp->bb_free = free;
-		ext4_mark_group_bitmap_corrupted(sb, group,
-					EXT4_GROUP_INFO_BBITMAP_CORRUPT);
 	}
 	mb_set_largest_free_order(sb, grp);
 
@@ -1451,11 +1449,10 @@  static void mb_free_blocks(struct inode *inode, struct ext4_buddy *e4b,
 		ext4_grp_locked_error(sb, e4b->bd_group,
 				      inode ? inode->i_ino : 0,
 				      blocknr,
+				      EXT4_GROUP_INFO_BBITMAP_CORRUPT,
 				      "freeing already freed block "
 				      "(bit %u); block bitmap corrupt.",
 				      block);
-		ext4_mark_group_bitmap_corrupted(sb, e4b->bd_group,
-				EXT4_GROUP_INFO_BBITMAP_CORRUPT);
 		mb_regenerate_buddy(e4b);
 		goto done;
 	}
@@ -1949,11 +1946,10 @@  void ext4_mb_complex_scan_group(struct ext4_allocation_context *ac,
 			 * we have free blocks
 			 */
 			ext4_grp_locked_error(sb, e4b->bd_group, 0, 0,
+					EXT4_GROUP_INFO_BBITMAP_CORRUPT,
 					"%d free clusters as per "
 					"group info. But bitmap says 0",
 					free);
-			ext4_mark_group_bitmap_corrupted(sb, e4b->bd_group,
-					EXT4_GROUP_INFO_BBITMAP_CORRUPT);
 			break;
 		}
 
@@ -1961,11 +1957,10 @@  void ext4_mb_complex_scan_group(struct ext4_allocation_context *ac,
 		BUG_ON(ex.fe_len <= 0);
 		if (free < ex.fe_len) {
 			ext4_grp_locked_error(sb, e4b->bd_group, 0, 0,
+					EXT4_GROUP_INFO_BBITMAP_CORRUPT,
 					"%d free clusters as per "
 					"group info. But got %d blocks",
 					free, ex.fe_len);
-			ext4_mark_group_bitmap_corrupted(sb, e4b->bd_group,
-					EXT4_GROUP_INFO_BBITMAP_CORRUPT);
 			/*
 			 * The number of free blocks differs. This mostly
 			 * indicate that the bitmap is corrupt. So exit
@@ -3850,7 +3845,8 @@  static int ext4_mb_new_preallocation(struct ext4_allocation_context *ac)
 			 pa, (unsigned long) pa->pa_lstart,
 			 (unsigned long) pa->pa_pstart,
 			 (unsigned long) pa->pa_len);
-		ext4_grp_locked_error(sb, group, 0, 0, "free %u, pa_free %u",
+		ext4_grp_locked_error(sb, group, 0, 0, 0,
+					"free %u, pa_free %u",
 					free, pa->pa_free);
 		/*
 		 * pa is already deleted so we use the value obtained
@@ -4678,7 +4674,7 @@  static void ext4_try_merge_freed_extent(struct ext4_sb_info *sbi,
 		else {
 			ext4_grp_locked_error(sb, group, 0,
 				ext4_group_first_block_no(sb, group) +
-				EXT4_C2B(sbi, cluster),
+				EXT4_C2B(sbi, cluster), 0,
 				"Block already on to-be-freed list");
 			return 0;
 		}
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index d6fa6cf..554bceb 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -709,7 +709,7 @@  void __ext4_warning_inode(const struct inode *inode, const char *function,
 void __ext4_grp_locked_error(const char *function, unsigned int line,
 			     struct super_block *sb, ext4_group_t grp,
 			     unsigned long ino, ext4_fsblk_t block,
-			     const char *fmt, ...)
+			     unsigned int flags, const char *fmt, ...)
 __releases(bitlock)
 __acquires(bitlock)
 {
@@ -740,6 +740,9 @@  void __ext4_grp_locked_error(const char *function, unsigned int line,
 		va_end(args);
 	}
 
+	if (flags)
+		__ext4_mark_group_bitmap_corrupted(sb, grp, flags);
+
 	if (test_opt(sb, ERRORS_CONT)) {
 		ext4_commit_super(sb, 0);
 		return;
@@ -763,9 +766,9 @@  void __ext4_grp_locked_error(const char *function, unsigned int line,
 	return;
 }
 
-void ext4_mark_group_bitmap_corrupted(struct super_block *sb,
-				     ext4_group_t group,
-				     unsigned int flags)
+void __ext4_mark_group_bitmap_corrupted(struct super_block *sb,
+					ext4_group_t group,
+					unsigned int flags)
 {
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 	struct ext4_group_info *grp = ext4_get_group_info(sb, group);