linux-ext4.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/8] ext4: use ASSERT() to replace J_ASSERT()
@ 2020-10-19  9:02 Chunguang Xu
  2020-10-19  9:02 ` [PATCH v2 2/8] ext4: remove redundant mb_regenerate_buddy() Chunguang Xu
                   ` (7 more replies)
  0 siblings, 8 replies; 10+ messages in thread
From: Chunguang Xu @ 2020-10-19  9:02 UTC (permalink / raw)
  To: tytso, adilger.kernel; +Cc: linux-ext4

There are currently multiple forms of assertion, such as J_ASSERT().
J_ASEERT is provided for the jbd module, which is a public module.
Maybe we should use custom ASSERT() like other file systems, such as
xfs, which would be better.

Signed-off-by: Chunguang Xu <brookxu@tencent.com>
---
 fs/ext4/balloc.c   |  2 +-
 fs/ext4/ext4.h     | 10 ++++++++++
 fs/ext4/fsync.c    |  2 +-
 fs/ext4/indirect.c |  4 ++--
 fs/ext4/inode.c    |  6 +++---
 fs/ext4/namei.c    | 12 ++++--------
 fs/ext4/super.c    |  2 +-
 7 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
index dea738b..386cfc3 100644
--- a/fs/ext4/balloc.c
+++ b/fs/ext4/balloc.c
@@ -185,7 +185,7 @@ static int ext4_init_block_bitmap(struct super_block *sb,
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 	ext4_fsblk_t start, tmp;
 
-	J_ASSERT_BH(bh, buffer_locked(bh));
+	ASSERT(buffer_locked(bh));
 
 	/* If checksum is bad mark all blocks used to prevent allocation
 	 * essentially implementing a per-group read-only flag. */
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 250e905..512f833 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -98,6 +98,16 @@
 #define ext_debug(ino, fmt, ...)	no_printk(fmt, ##__VA_ARGS__)
 #endif
 
+#define ASSERT(assert)							\
+do {									\
+	if (unlikely(!(assert))) {					\
+		printk(KERN_EMERG					\
+		       "Assertion failure in %s() at %s:%d: \"%s\"\n",	\
+		       __func__, __FILE__, __LINE__, #assert);		\
+		BUG();							\
+	}								\
+} while (0)
+
 /* data type for block offset of block group */
 typedef int ext4_grpblk_t;
 
diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c
index 6476994..f9e33c7 100644
--- a/fs/ext4/fsync.c
+++ b/fs/ext4/fsync.c
@@ -136,7 +136,7 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 	if (unlikely(ext4_forced_shutdown(sbi)))
 		return -EIO;
 
-	J_ASSERT(ext4_journal_current_handle() == NULL);
+	ASSERT(ext4_journal_current_handle() == NULL);
 
 	trace_ext4_sync_file_enter(file, datasync);
 
diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c
index 05efa682..1223a18 100644
--- a/fs/ext4/indirect.c
+++ b/fs/ext4/indirect.c
@@ -534,8 +534,8 @@ int ext4_ind_map_blocks(handle_t *handle, struct inode *inode,
 	ext4_fsblk_t first_block = 0;
 
 	trace_ext4_ind_map_blocks_enter(inode, map->m_lblk, map->m_len, flags);
-	J_ASSERT(!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)));
-	J_ASSERT(handle != NULL || (flags & EXT4_GET_BLOCKS_CREATE) == 0);
+	ASSERT(!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)));
+	ASSERT(handle != NULL || (flags & EXT4_GET_BLOCKS_CREATE) == 0);
 	depth = ext4_block_to_path(inode, map->m_lblk, offsets,
 				   &blocks_to_boundary);
 
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 09096fe..8113898 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -825,7 +825,7 @@ struct buffer_head *ext4_getblk(handle_t *handle, struct inode *inode,
 	int create = map_flags & EXT4_GET_BLOCKS_CREATE;
 	int err;
 
-	J_ASSERT(handle != NULL || create == 0);
+	ASSERT(handle != NULL || create == 0);
 
 	map.m_lblk = block;
 	map.m_len = 1;
@@ -840,8 +840,8 @@ struct buffer_head *ext4_getblk(handle_t *handle, struct inode *inode,
 	if (unlikely(!bh))
 		return ERR_PTR(-ENOMEM);
 	if (map.m_flags & EXT4_MAP_NEW) {
-		J_ASSERT(create != 0);
-		J_ASSERT(handle != NULL);
+		ASSERT(create != 0);
+		ASSERT(handle != NULL);
 
 		/*
 		 * Now that we do not always journal data, we should
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index b735372..1cb9424 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -182,10 +182,6 @@ static struct buffer_head *__ext4_read_dirblock(struct inode *inode,
 	return bh;
 }
 
-#ifndef assert
-#define assert(test) J_ASSERT(test)
-#endif
-
 #ifdef DX_DEBUG
 #define dxtrace(command) command
 #else
@@ -849,7 +845,7 @@ struct stats dx_show_entries(struct dx_hash_info *hinfo, struct inode *dir,
 					break;
 				}
 			}
-			assert (at == p - 1);
+			ASSERT(at == p - 1);
 		}
 
 		at = p - 1;
@@ -1265,8 +1261,8 @@ static void dx_insert_block(struct dx_frame *frame, u32 hash, ext4_lblk_t block)
 	struct dx_entry *old = frame->at, *new = old + 1;
 	int count = dx_get_count(entries);
 
-	assert(count < dx_get_limit(entries));
-	assert(old < entries + count);
+	ASSERT(count < dx_get_limit(entries));
+	ASSERT(old < entries + count);
 	memmove(new + 1, new, (char *)(entries + count) - (char *)(new));
 	dx_set_hash(new, hash);
 	dx_set_block(new, block);
@@ -2959,7 +2955,7 @@ int ext4_orphan_add(handle_t *handle, struct inode *inode)
 	 * hold i_mutex, or the inode can not be referenced from outside,
 	 * so i_nlink should not be bumped due to race
 	 */
-	J_ASSERT((S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
+	ASSERT((S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
 		  S_ISLNK(inode->i_mode)) || inode->i_nlink == 0);
 
 	BUFFER_TRACE(sbi->s_sbh, "get_write_access");
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 9d01318..8aa163a 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1251,7 +1251,7 @@ static void ext4_put_super(struct super_block *sb)
 	 * in-memory list had better be clean by this point. */
 	if (!list_empty(&sbi->s_orphan))
 		dump_orphan_list(sb, sbi);
-	J_ASSERT(list_empty(&sbi->s_orphan));
+	ASSERT(list_empty(&sbi->s_orphan));
 
 	sync_blockdev(sb->s_bdev);
 	invalidate_bdev(sb->s_bdev);
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/8] ext4: remove redundant mb_regenerate_buddy()
  2020-10-19  9:02 [PATCH v2 1/8] ext4: use ASSERT() to replace J_ASSERT() Chunguang Xu
@ 2020-10-19  9:02 ` Chunguang Xu
  2020-10-19  9:02 ` [PATCH v2 3/8] ext4: simplify the code of mb_find_order_for_block Chunguang Xu
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Chunguang Xu @ 2020-10-19  9:02 UTC (permalink / raw)
  To: tytso, adilger.kernel; +Cc: linux-ext4

After this patch (163a203), if an abnormal bitmap is detected, we
will mark the group as corrupt, and we will not use this group in
the future. Therefore, it should be meaningless to regenerate the
buddy bitmap of this group, It might be better to delete it.

Signed-off-by: Chunguang Xu <brookxu@tencent.com>
---
 fs/ext4/mballoc.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 74a48d6..03337c8 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -822,24 +822,6 @@ void ext4_mb_generate_buddy(struct super_block *sb,
 	spin_unlock(&sbi->s_bal_lock);
 }
 
-static void mb_regenerate_buddy(struct ext4_buddy *e4b)
-{
-	int count;
-	int order = 1;
-	void *buddy;
-
-	while ((buddy = mb_find_buddy(e4b, order++, &count))) {
-		ext4_set_bits(buddy, 0, count);
-	}
-	e4b->bd_info->bb_fragments = 0;
-	memset(e4b->bd_info->bb_counters, 0,
-		sizeof(*e4b->bd_info->bb_counters) *
-		(e4b->bd_sb->s_blocksize_bits + 2));
-
-	ext4_mb_generate_buddy(e4b->bd_sb, e4b->bd_buddy,
-		e4b->bd_bitmap, e4b->bd_group);
-}
-
 /* The buddy information is attached the buddy cache inode
  * for convenience. The information regarding each group
  * is loaded via ext4_mb_load_buddy. The information involve
@@ -1510,7 +1492,6 @@ static void mb_free_blocks(struct inode *inode, struct ext4_buddy *e4b,
 				      block);
 		ext4_mark_group_bitmap_corrupted(sb, e4b->bd_group,
 				EXT4_GROUP_INFO_BBITMAP_CORRUPT);
-		mb_regenerate_buddy(e4b);
 		goto done;
 	}
 
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 3/8] ext4: simplify the code of mb_find_order_for_block
  2020-10-19  9:02 [PATCH v2 1/8] ext4: use ASSERT() to replace J_ASSERT() Chunguang Xu
  2020-10-19  9:02 ` [PATCH v2 2/8] ext4: remove redundant mb_regenerate_buddy() Chunguang Xu
@ 2020-10-19  9:02 ` Chunguang Xu
  2020-10-19  9:02 ` [PATCH v2 4/8] ext4: add the gdt block of meta_bg to system_zone Chunguang Xu
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Chunguang Xu @ 2020-10-19  9:02 UTC (permalink / raw)
  To: tytso, adilger.kernel; +Cc: linux-ext4

The code of mb_find_order_for_block is a bit obscure, but we can
simplify it with mb_find_buddy(), make the code more concise.

Signed-off-by: Chunguang Xu <brookxu@tencent.com>
---
 fs/ext4/mballoc.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 03337c8..56075ce 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -1289,22 +1289,18 @@ static void ext4_mb_unload_buddy(struct ext4_buddy *e4b)
 
 static int mb_find_order_for_block(struct ext4_buddy *e4b, int block)
 {
-	int order = 1;
-	int bb_incr = 1 << (e4b->bd_blkbits - 1);
+	int order = 1, max;
 	void *bb;
 
 	BUG_ON(e4b->bd_bitmap == e4b->bd_buddy);
 	BUG_ON(block >= (1 << (e4b->bd_blkbits + 3)));
 
-	bb = e4b->bd_buddy;
 	while (order <= e4b->bd_blkbits + 1) {
-		block = block >> 1;
-		if (!mb_test_bit(block, bb)) {
+		bb = mb_find_buddy(e4b, order, &max);
+		if (!mb_test_bit(block >> order, bb)) {
 			/* this block is part of buddy of order 'order' */
 			return order;
 		}
-		bb += bb_incr;
-		bb_incr >>= 1;
 		order++;
 	}
 	return 0;
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 4/8] ext4: add the gdt block of meta_bg to system_zone
  2020-10-19  9:02 [PATCH v2 1/8] ext4: use ASSERT() to replace J_ASSERT() Chunguang Xu
  2020-10-19  9:02 ` [PATCH v2 2/8] ext4: remove redundant mb_regenerate_buddy() Chunguang Xu
  2020-10-19  9:02 ` [PATCH v2 3/8] ext4: simplify the code of mb_find_order_for_block Chunguang Xu
@ 2020-10-19  9:02 ` Chunguang Xu
  2020-10-19  9:02 ` [PATCH v2 5/8] ext4: update ext4_data_block_valid related comments Chunguang Xu
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Chunguang Xu @ 2020-10-19  9:02 UTC (permalink / raw)
  To: tytso, adilger.kernel; +Cc: linux-ext4

In order to avoid poor search efficiency of system_zone, the
system only adds metadata of some sparse group to system_zone.
In the meta_bg scenario, the non-sparse group may contain gdt
blocks. Perhaps we should add these blocks to system_zone to
improve fault tolerance without significantly reducing system
performance.

Signed-off-by: Chunguang Xu <brookxu@tencent.com>
---
 fs/ext4/block_validity.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/fs/ext4/block_validity.c b/fs/ext4/block_validity.c
index 8e6ca23..37025e3 100644
--- a/fs/ext4/block_validity.c
+++ b/fs/ext4/block_validity.c
@@ -218,6 +218,7 @@ int ext4_setup_system_zone(struct super_block *sb)
 	struct ext4_group_desc *gdp;
 	ext4_group_t i;
 	int flex_size = ext4_flex_bg_size(sbi);
+	int gd_blks;
 	int ret;
 
 	system_blks = kzalloc(sizeof(*system_blks), GFP_KERNEL);
@@ -226,13 +227,16 @@ int ext4_setup_system_zone(struct super_block *sb)
 
 	for (i=0; i < ngroups; i++) {
 		cond_resched();
-		if (ext4_bg_has_super(sb, i) &&
-		    ((i < 5) || ((i % flex_size) == 0))) {
-			ret = add_system_zone(system_blks,
-					ext4_group_first_block_no(sb, i),
-					ext4_bg_num_gdb(sb, i) + 1, 0);
-			if (ret)
-				goto err;
+		if ((i < 5) || ((i % flex_size) == 0)) {
+			gd_blks = ext4_bg_has_super(sb, i) +
+				ext4_bg_num_gdb(sb, i);
+			if (gd_blks) {
+				ret = add_system_zone(system_blks,
+						ext4_group_first_block_no(sb, i),
+						gd_blks, 0);
+				if (ret)
+					goto err;
+			}
 		}
 		gdp = ext4_get_group_desc(sb, i, NULL);
 		ret = add_system_zone(system_blks,
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 5/8] ext4: update ext4_data_block_valid related comments
  2020-10-19  9:02 [PATCH v2 1/8] ext4: use ASSERT() to replace J_ASSERT() Chunguang Xu
                   ` (2 preceding siblings ...)
  2020-10-19  9:02 ` [PATCH v2 4/8] ext4: add the gdt block of meta_bg to system_zone Chunguang Xu
@ 2020-10-19  9:02 ` Chunguang Xu
  2020-10-19  9:02 ` [PATCH v2 6/8] ext4: add a helper function to validate metadata block Chunguang Xu
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Chunguang Xu @ 2020-10-19  9:02 UTC (permalink / raw)
  To: tytso, adilger.kernel; +Cc: linux-ext4

From: Chunguang Xu <brookxu@tencent.com>

Since ext4_data_block_valid() has been renamed to ext4_inode_block_valid(),
the related comments need to be updated.

Signed-off-by: Chunguang Xu <brookxu@tencent.com>
---
 fs/ext4/block_validity.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/block_validity.c b/fs/ext4/block_validity.c
index 37025e3..07e9dc3 100644
--- a/fs/ext4/block_validity.c
+++ b/fs/ext4/block_validity.c
@@ -206,7 +206,7 @@ static void ext4_destroy_system_zone(struct rcu_head *rcu)
  *
  * The update of system_blks pointer in this function is protected by
  * sb->s_umount semaphore. However we have to be careful as we can be
- * racing with ext4_data_block_valid() calls reading system_blks rbtree
+ * racing with ext4_inode_block_valid() calls reading system_blks rbtree
  * protected only by RCU. That's why we first build the rbtree and then
  * swap it in place.
  */
@@ -262,7 +262,7 @@ int ext4_setup_system_zone(struct super_block *sb)
 
 	/*
 	 * System blks rbtree complete, announce it once to prevent racing
-	 * with ext4_data_block_valid() accessing the rbtree at the same
+	 * with ext4_inode_block_valid() accessing the rbtree at the same
 	 * time.
 	 */
 	rcu_assign_pointer(sbi->s_system_blks, system_blks);
@@ -282,7 +282,7 @@ int ext4_setup_system_zone(struct super_block *sb)
  *
  * The update of system_blks pointer in this function is protected by
  * sb->s_umount semaphore. However we have to be careful as we can be
- * racing with ext4_data_block_valid() calls reading system_blks rbtree
+ * racing with ext4_inode_block_valid() calls reading system_blks rbtree
  * protected only by RCU. So we first clear the system_blks pointer and
  * then free the rbtree only after RCU grace period expires.
  */
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 6/8] ext4: add a helper function to validate metadata block
  2020-10-19  9:02 [PATCH v2 1/8] ext4: use ASSERT() to replace J_ASSERT() Chunguang Xu
                   ` (3 preceding siblings ...)
  2020-10-19  9:02 ` [PATCH v2 5/8] ext4: update ext4_data_block_valid related comments Chunguang Xu
@ 2020-10-19  9:02 ` Chunguang Xu
  2020-10-19  9:02 ` [PATCH v2 7/8] ext4: delete invalid code inside ext4_xattr_block_set() Chunguang Xu
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Chunguang Xu @ 2020-10-19  9:02 UTC (permalink / raw)
  To: tytso, adilger.kernel; +Cc: linux-ext4

There is a need to check whether a block or a segment overlaps
with metadata, since information of system_zone is incomplete,
we need a more accurate function. Now we check whether it
overlaps with block bitmap, inode bitmap, and inode table.
Perhaps it is better to add a check of super_block and block
group descriptor and provide a helper function.

Signed-off-by: Chunguang Xu <brookxu@tencent.com>
---
 fs/ext4/ext4.h    |  5 ++++-
 fs/ext4/mballoc.c | 57 +++++++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 49 insertions(+), 13 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 512f833..1ea700d 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2701,7 +2701,10 @@ extern ext4_group_t ext4_mb_prefetch(struct super_block *sb,
 				     unsigned int nr, int *cnt);
 extern void ext4_mb_prefetch_fini(struct super_block *sb, ext4_group_t group,
 				  unsigned int nr);
-
+extern int ext4_metadata_block_overlaps(struct super_block *sb,
+					ext4_group_t block_group,
+					ext4_fsblk_t block,
+					unsigned long count);
 extern void ext4_free_blocks(handle_t *handle, struct inode *inode,
 			     struct buffer_head *bh, ext4_fsblk_t block,
 			     unsigned long count, int flags);
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 56075ce..e0a4265 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -5044,6 +5044,49 @@ static void ext4_try_merge_freed_extent(struct ext4_sb_info *sbi,
 	return 0;
 }
 
+/*
+ * Returns 1 if the passed-in block region (block, block+count)
+ * overlaps with some other filesystem metadata blocks. Others,
+ * return 0.
+ */
+int ext4_metadata_block_overlaps(struct super_block *sb,
+				 ext4_group_t block_group,
+				 ext4_fsblk_t block,
+				 unsigned long count)
+{
+	struct ext4_sb_info *sbi = EXT4_SB(sb);
+	struct ext4_group_desc *gdp;
+	int gd_first = ext4_group_first_block_no(sb, block_group);
+	int itable, gd_blk;
+	int ret = 0;
+
+	gdp = ext4_get_group_desc(sb, block_group, NULL);
+	// check block bitmap and inode bitmap
+	if (in_range(ext4_block_bitmap(sb, gdp), block, count) ||
+	    in_range(ext4_inode_bitmap(sb, gdp), block, count))
+		ret = 1;
+
+	// check inode table
+	itable = ext4_inode_table(sb, gdp);
+	if (!(block >= itable + sbi->s_itb_per_group ||
+	      block + count - 1  < itable))
+		ret = 1;
+
+	/* check super_block and block group descriptor table, the
+	 * reserved space of the block group descriptor is managed
+	 * by resize_inode, it may not be processed now due to
+	 * performance.
+	 */
+	gd_blk = ext4_bg_has_super(sb, block_group) +
+		ext4_bg_num_gdb(sb, block_group);
+	if (gd_blk) {
+		if (!(block >= gd_first + gd_blk ||
+		      block + count - 1 < gd_first))
+			ret = 1;
+	}
+	return ret;
+}
+
 /**
  * ext4_free_blocks() -- Free given blocks and update quota
  * @handle:		handle for this transaction
@@ -5170,13 +5213,7 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode,
 		goto error_return;
 	}
 
-	if (in_range(ext4_block_bitmap(sb, gdp), block, count) ||
-	    in_range(ext4_inode_bitmap(sb, gdp), block, count) ||
-	    in_range(block, ext4_inode_table(sb, gdp),
-		     sbi->s_itb_per_group) ||
-	    in_range(block + count - 1, ext4_inode_table(sb, gdp),
-		     sbi->s_itb_per_group)) {
-
+	if (ext4_metadata_block_overlaps(sb, block_group, block, count)) {
 		ext4_error(sb, "Freeing blocks in system zone - "
 			   "Block = %llu, count = %lu", block, count);
 		/* err = 0. ext4_std_error should be a no op */
@@ -5362,11 +5399,7 @@ int ext4_group_add_blocks(handle_t *handle, struct super_block *sb,
 		goto error_return;
 	}
 
-	if (in_range(ext4_block_bitmap(sb, desc), block, count) ||
-	    in_range(ext4_inode_bitmap(sb, desc), block, count) ||
-	    in_range(block, ext4_inode_table(sb, desc), sbi->s_itb_per_group) ||
-	    in_range(block + count - 1, ext4_inode_table(sb, desc),
-		     sbi->s_itb_per_group)) {
+	if (ext4_metadata_block_overlaps(sb, block_group, block, count)) {
 		ext4_error(sb, "Adding blocks in system zones - "
 			   "Block = %llu, count = %lu",
 			   block, count);
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 7/8] ext4: delete invalid code inside ext4_xattr_block_set()
  2020-10-19  9:02 [PATCH v2 1/8] ext4: use ASSERT() to replace J_ASSERT() Chunguang Xu
                   ` (4 preceding siblings ...)
  2020-10-19  9:02 ` [PATCH v2 6/8] ext4: add a helper function to validate metadata block Chunguang Xu
@ 2020-10-19  9:02 ` Chunguang Xu
  2020-10-19  9:02 ` [PATCH v2 8/8] ext4: fix a memory leak of ext4_free_data Chunguang Xu
  2020-10-20  3:37 ` [PATCH v2 1/8] ext4: use ASSERT() to replace J_ASSERT() Andreas Dilger
  7 siblings, 0 replies; 10+ messages in thread
From: Chunguang Xu @ 2020-10-19  9:02 UTC (permalink / raw)
  To: tytso, adilger.kernel; +Cc: linux-ext4

Delete invalid code inside ext4_xattr_block_set().

Signed-off-by: Chunguang Xu <brookxu@tencent.com>
---
 fs/ext4/xattr.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index cba4b87..56728f3 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -1927,7 +1927,6 @@ struct ext4_xattr_block_find {
 	} else {
 		/* Allocate a buffer where we construct the new block. */
 		s->base = kzalloc(sb->s_blocksize, GFP_NOFS);
-		/* assert(header == s->base) */
 		error = -ENOMEM;
 		if (s->base == NULL)
 			goto cleanup;
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 8/8] ext4: fix a memory leak of ext4_free_data
  2020-10-19  9:02 [PATCH v2 1/8] ext4: use ASSERT() to replace J_ASSERT() Chunguang Xu
                   ` (5 preceding siblings ...)
  2020-10-19  9:02 ` [PATCH v2 7/8] ext4: delete invalid code inside ext4_xattr_block_set() Chunguang Xu
@ 2020-10-19  9:02 ` Chunguang Xu
  2020-10-20  3:37 ` [PATCH v2 1/8] ext4: use ASSERT() to replace J_ASSERT() Andreas Dilger
  7 siblings, 0 replies; 10+ messages in thread
From: Chunguang Xu @ 2020-10-19  9:02 UTC (permalink / raw)
  To: tytso, adilger.kernel; +Cc: linux-ext4

From: Chunguang Xu <brookxu@tencent.com>

When freeing metadata, we will create an ext4_free_data and
insert it into the pending free list. After the current
transaction is committed, the object will be freed.

ext4_mb_free_metadata() will check whether the area to be
freed overlaps with the pending free list. If true, return
directly. At this time, ext4_free_data is leaked. Fortunately,
the probability of this problem is relatively small, maybe we
should fix this problem.

Signed-off-by: Chunguang Xu <brookxu@tencent.com>
---
 fs/ext4/mballoc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index e0a4265..aa6732a 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -5015,6 +5015,7 @@ static void ext4_try_merge_freed_extent(struct ext4_sb_info *sbi,
 				ext4_group_first_block_no(sb, group) +
 				EXT4_C2B(sbi, cluster),
 				"Block already on to-be-freed list");
+			kmem_cache_free(ext4_free_data_cachep, new_entry);
 			return 0;
 		}
 	}
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/8] ext4: use ASSERT() to replace J_ASSERT()
  2020-10-19  9:02 [PATCH v2 1/8] ext4: use ASSERT() to replace J_ASSERT() Chunguang Xu
                   ` (6 preceding siblings ...)
  2020-10-19  9:02 ` [PATCH v2 8/8] ext4: fix a memory leak of ext4_free_data Chunguang Xu
@ 2020-10-20  3:37 ` Andreas Dilger
  2020-10-20  5:03   ` brookxu
  7 siblings, 1 reply; 10+ messages in thread
From: Andreas Dilger @ 2020-10-20  3:37 UTC (permalink / raw)
  To: Chunguang Xu; +Cc: Ted Tso, Ext4 Developers List

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

On Oct 19, 2020, at 3:02 AM, Chunguang Xu <brookxu.cn@gmail.com> wrote:
> 
> There are currently multiple forms of assertion, such as J_ASSERT().
> J_ASEERT is provided for the jbd module, which is a public module.

(typo) "J_ASSERT()"

> Maybe we should use custom ASSERT() like other file systems, such as
> xfs, which would be better.

My one minor complaint is that "ASSERT()" is a very generic name and is
likely to cause conflicts with ASSERT in other headers.  That said, I
also see many other filesystems using their own ASSERT() macro, so I
guess they are all in private headers only?

Some minor comments/questions below, but not worth changing the patch
unless you think they are important...

You can add:

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

> @@ -185,7 +185,7 @@ static int ext4_init_block_bitmap(struct super_block *sb,
> 	struct ext4_sb_info *sbi = EXT4_SB(sb);
> 	ext4_fsblk_t start, tmp;
> 
> -	J_ASSERT_BH(bh, buffer_locked(bh));
> +	ASSERT(buffer_locked(bh));

I thought J_ASSERT_BH() did something useful, but J_ASSERT_BH() just maps
to J_ASSERT() internally anyway.

> +#define ASSERT(assert)							\
> +do {									\
> +	if (unlikely(!(assert))) {					\
> +		printk(KERN_EMERG					\
> +		       "Assertion failure in %s() at %s:%d: \"%s\"\n",	\

(style) better to use single quotes '%s' to avoid the need to escape \".

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/8] ext4: use ASSERT() to replace J_ASSERT()
  2020-10-20  3:37 ` [PATCH v2 1/8] ext4: use ASSERT() to replace J_ASSERT() Andreas Dilger
@ 2020-10-20  5:03   ` brookxu
  0 siblings, 0 replies; 10+ messages in thread
From: brookxu @ 2020-10-20  5:03 UTC (permalink / raw)
  To: Andreas Dilger; +Cc: Ted Tso, Ext4 Developers List

Thanks for your reply.

Andreas Dilger wrote on 2020/10/20 11:37:
> On Oct 19, 2020, at 3:02 AM, Chunguang Xu <brookxu.cn@gmail.com> wrote:
>>
>> There are currently multiple forms of assertion, such as J_ASSERT().
>> J_ASEERT is provided for the jbd module, which is a public module.
> 
> (typo) "J_ASSERT()"
Thanks, I  will Fixed that.

>> Maybe we should use custom ASSERT() like other file systems, such as
>> xfs, which would be better.
> 
> My one minor complaint is that "ASSERT()" is a very generic name and is
> likely to cause conflicts with ASSERT in other headers.  That said, I
> also see many other filesystems using their own ASSERT() macro, so I
> guess they are all in private headers only?
I also thought about this before, but even if we define it in a private
header file, because we still include several header files in a certain
file, it seems that the conflict cannot be resolved. However, maybe it
is safest to use a name with ext4 prefix. I will try to fix it in next
version. Thanks.

> Some minor comments/questions below, but not worth changing the patch
> unless you think they are important...
> 
> You can add:
> 
> Reviewed-by: Andreas Dilger <adilger@dilger.ca>
> 
>> @@ -185,7 +185,7 @@ static int ext4_init_block_bitmap(struct super_block *sb,
>> 	struct ext4_sb_info *sbi = EXT4_SB(sb);
>> 	ext4_fsblk_t start, tmp;
>>
>> -	J_ASSERT_BH(bh, buffer_locked(bh));
>> +	ASSERT(buffer_locked(bh));
> 
> I thought J_ASSERT_BH() did something useful, but J_ASSERT_BH() just maps
> to J_ASSERT() internally anyway.
> 
>> +#define ASSERT(assert)							\
>> +do {									\
>> +	if (unlikely(!(assert))) {					\
>> +		printk(KERN_EMERG					\
>> +		       "Assertion failure in %s() at %s:%d: \"%s\"\n",	\
> 
> (style) better to use single quotes '%s' to avoid the need to escape \".
Thanks, this is a good suggestion, I will fix it next version.

> Cheers, Andreas
> 
> 
> 
> 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-10-20  5:03 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-19  9:02 [PATCH v2 1/8] ext4: use ASSERT() to replace J_ASSERT() Chunguang Xu
2020-10-19  9:02 ` [PATCH v2 2/8] ext4: remove redundant mb_regenerate_buddy() Chunguang Xu
2020-10-19  9:02 ` [PATCH v2 3/8] ext4: simplify the code of mb_find_order_for_block Chunguang Xu
2020-10-19  9:02 ` [PATCH v2 4/8] ext4: add the gdt block of meta_bg to system_zone Chunguang Xu
2020-10-19  9:02 ` [PATCH v2 5/8] ext4: update ext4_data_block_valid related comments Chunguang Xu
2020-10-19  9:02 ` [PATCH v2 6/8] ext4: add a helper function to validate metadata block Chunguang Xu
2020-10-19  9:02 ` [PATCH v2 7/8] ext4: delete invalid code inside ext4_xattr_block_set() Chunguang Xu
2020-10-19  9:02 ` [PATCH v2 8/8] ext4: fix a memory leak of ext4_free_data Chunguang Xu
2020-10-20  3:37 ` [PATCH v2 1/8] ext4: use ASSERT() to replace J_ASSERT() Andreas Dilger
2020-10-20  5:03   ` brookxu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).