[PATCH 00/13] fixes and cleanups to ext4 resize

[PATCH 00/13] fixes and cleanups to ext4 resize

[Kemeng Shi]

This series contains some random cleanups and a few fixes to correct gdb
backup copy, fix buffer_head leak and so on. More details can be found
in respective log messages.
I run kvm-xfstest with config "ext4/all" and "-g auto" together with
mballoc patchset I sent before.

-------------------- Summary report
KERNEL:    kernel 6.4.0-rc5-xfstests-g90158dd3c06b #23 SMP PREEMPT_DYNAMIC
Wed Jun 28 17:19:32 CST 2023 x86_64
CPUS:      2
MEM:       1975.36

ext4/4k: 531 tests, 31 skipped, 3421 seconds
ext4/1k: 527 tests, 33 skipped, 4638 seconds
ext4/ext3: 523 tests, 119 skipped, 3289 seconds
ext4/encrypt: 509 tests, 3 failures, 138 skipped, 2163 seconds
  Failures: generic/681 generic/682 generic/691
ext4/nojournal: 526 tests, 5 failures, 98 skipped, 3469 seconds
  Failures: ext4/301 ext4/304 generic/455 generic/459 generic/581
ext4/ext3conv: 528 tests, 31 skipped, 3877 seconds
ext4/adv: 528 tests, 4 failures, 38 skipped, 4170 seconds
  Failures: generic/475 generic/477
  Flaky: generic/455: 80% (4/5)   generic/482: 80% (4/5)
ext4/dioread_nolock: 529 tests, 31 skipped, 3825 seconds
ext4/data_journal: 527 tests, 3 failures, 99 skipped, 3307 seconds
  Failures: generic/455 generic/484
  Flaky: generic/068: 40% (2/5)
ext4/bigalloc_4k: 503 tests, 35 skipped, 3286 seconds
ext4/bigalloc_1k: 503 tests, 1 failures, 43 skipped, 4421 seconds
  Failures: shared/298
Totals: 5798 tests, 696 skipped, 75 failures, 0 errors, 39645s

FSTESTVER: blktests 676d42c (Thu, 2 Mar 2023 15:25:44 +0900)
FSTESTVER: e2fsprogs archive/debian/1.47.0-1 (Mon, 6 Feb 2023 22:36:16 -0500)
FSTESTVER: fio  fio-3.31 (Tue, 9 Aug 2022 14:41:25 -0600)
FSTESTVER: fsverity v1.5-6-g5d6f7c4 (Mon, 30 Jan 2023 23:22:45 -0800)
FSTESTVER: ima-evm-utils v1.3.2 (Wed, 28 Oct 2020 13:18:08 -0400)
FSTESTVER: nvme-cli v1.16 (Thu, 11 Nov 2021 13:09:06 -0800)
FSTESTVER: quota  v4.05-53-gd90b7d5 (Tue, 6 Dec 2022 12:59:03 +0100)
FSTESTVER: util-linux v2.38.1 (Thu, 4 Aug 2022 11:06:21 +0200)
FSTESTVER: xfsprogs v6.1.1 (Fri, 13 Jan 2023 19:06:37 +0100)
FSTESTVER: xfstests v2023.02.26-8-g821ef488 (Thu, 2 Mar 2023 10:23:51 -0500)
FSTESTVER: xfstests-bld 35650073 (Mon, 6 Mar 2023 20:48:08 -0500)
FSTESTVER: zz_build-distro bullseye
FSTESTCFG: ext4/all
FSTESTSET: -g auto
FSTESTOPT: aex

There more failures compared with "good" report in reply from Ted [1] as
following:
ext4/nojournal: generic/459 generic/581
ext4/adv: Flaky: generic/455: 80% (4/5)   generic/482: 80% (4/5)
ext4/data_journal: Flaky: generic/068: 40% (2/5)
It seems still a "good" test run according to rules listed in [1].
Please let me know if more tests to run. Thanks!

[1] https://lore.kernel.org/linux-ext4/db478a24-39f5-3cef-8814-89406ce4d2ca@huawei.com/T/#ma9de3f355f0300291d32fe1f0b32c5660c9bd191

Kemeng Shi (13):
  ext4: correct offset of gdb backup in non meta_bg group to
    update_backups
  ext4: add missed brelse in update_backups
  ext4: correct return value of ext4_convert_meta_bg
  ext4: remove gdb backup copy for meta bg in
    setup_new_flex_group_blocks
  ext4: fix typo in setup_new_flex_group_blocks
  ext4: remove redundant check of count
  ext4: remove commented code in reserve_backup_gdb
  ext4: calculate free_clusters_count in cluster unit in
    verify_group_input
  ext4: remove EXT4FS_DEBUG defination in resize.c
  ext4: use saved local variable sbi instead of EXT4_SB(sb)
  ext4: correct gdblock calculation in add_new_gdb_meta_bg to support
    non first group
  ext4: remove unnecessary check for avoiding multiple update_backups in
    ext4_flex_group_add
  ext4: remove unnecessary initialization of count2 in
    set_flexbg_block_bitmap

 fs/ext4/resize.c | 81 +++++++++++++++++++-----------------------------
 1 file changed, 32 insertions(+), 49 deletions(-)

-- 
2.30.0

[PATCH 01/13] ext4: correct offset of gdb backup in non meta_bg group to update_backups

[Kemeng Shi]

Commit 0aeaa2559d6d5 ("ext4: fix corruption when online resizing a 1K
bigalloc fs") found that primary superblock's offset in its group is not
equal to offset of backup superblock in its group when block size is 1K
and bigalloc is enabled. As group descriptor blocks are right after
superblock, we can't pass block number of gdb to update_backups for
the same reason.
The root casue of the issue above is that leading 1K padding block is
count as data block offset for primary block while backup block has
no padding block offset in its group.
Remove padding data block count to fix the issue for gdb backups.

For meta_bg case, update_backups treat blk_off as block number, do no
conversion in this case.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/resize.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index 0361c20910de..87cd5b07a970 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c
@@ -1601,6 +1601,8 @@ static int ext4_flex_group_add(struct super_block *sb,
 		int gdb_num_end = ((group + flex_gd->count - 1) /
 				   EXT4_DESC_PER_BLOCK(sb));
 		int meta_bg = ext4_has_feature_meta_bg(sb);
+		sector_t padding_blocks = meta_bg ? 0 : sbi->s_sbh->b_blocknr -
+					 ext4_group_first_block_no(sb, 0);
 		sector_t old_gdb = 0;
 
 		update_backups(sb, ext4_group_first_block_no(sb, 0),
@@ -1612,8 +1614,8 @@ static int ext4_flex_group_add(struct super_block *sb,
 						     gdb_num);
 			if (old_gdb == gdb_bh->b_blocknr)
 				continue;
-			update_backups(sb, gdb_bh->b_blocknr, gdb_bh->b_data,
-				       gdb_bh->b_size, meta_bg);
+			update_backups(sb, gdb_bh->b_blocknr - padding_blocks,
+				       gdb_bh->b_data, gdb_bh->b_size, meta_bg);
 			old_gdb = gdb_bh->b_blocknr;
 		}
 	}
-- 
2.30.0

[PATCH 02/13] ext4: add missed brelse in update_backups

[Kemeng Shi]

add missed brelse in update_backups

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/resize.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index 87cd5b07a970..7cbc695b7005 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c
@@ -1191,8 +1191,10 @@ static void update_backups(struct super_block *sb, sector_t blk_off, char *data,
 			   ext4_group_first_block_no(sb, group));
 		BUFFER_TRACE(bh, "get_write_access");
 		if ((err = ext4_journal_get_write_access(handle, sb, bh,
-							 EXT4_JTR_NONE)))
+							 EXT4_JTR_NONE))) {
+			brelse(bh);
 			break;
+		}
 		lock_buffer(bh);
 		memcpy(bh->b_data, data, size);
 		if (rest)
-- 
2.30.0

[PATCH 03/13] ext4: correct return value of ext4_convert_meta_bg

[Kemeng Shi]

We return error in "ret", so collect previous error in "ret" instead
of "err" or previous error will be ignored.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/resize.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index 7cbc695b7005..0b3d8c808de1 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c
@@ -1984,8 +1984,8 @@ static int ext4_convert_meta_bg(struct super_block *sb, struct inode *inode)
 
 errout:
 	ret = ext4_journal_stop(handle);
-	if (!err)
-		err = ret;
+	if (!ret)
+		ret = err;
 	return ret;
 
 invalid_resize_inode:
-- 
2.30.0

[PATCH 04/13] ext4: remove gdb backup copy for meta bg in setup_new_flex_group_blocks

[Kemeng Shi]

Wrong check of gdb backup in meta bg as following:
first_group is the first group of meta_bg which contains target group, so
target group is always >= first_group. We check if target group has gdb
backup by comparing first_group with [group + 1] and [group +
EXT4_DESC_PER_BLOCK(sb) - 1]. As group >= first_group, then [group + N] is
> first_group. So no copy of gdb backup in meta bg is done in
setup_new_flex_group_blocks.

No need to do gdb backup copy in meta bg from setup_new_flex_group_blocks
as we always copy updated gdb block to backups at end of
ext4_flex_group_add as following:

ext4_flex_group_add
  /* no gdb backup copy for meta bg any more */
  setup_new_flex_group_blocks

  /* update current group number */
  ext4_update_super
    sbi->s_groups_count += flex_gd->count;

  /*
   * if group in meta bg contains backup is added, the primary gdb block
   * of the meta bg will be copy to backup in new added group here.
   */
  for (; gdb_num <= gdb_num_end; gdb_num++)
    update_backups(...)

In summary, we can remove wrong gdb backup copy code in
setup_new_flex_group_blocks.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/resize.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index 0b3d8c808de1..0e83b3f91545 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c
@@ -560,13 +560,8 @@ static int setup_new_flex_group_blocks(struct super_block *sb,
 		if (meta_bg == 0 && !ext4_bg_has_super(sb, group))
 			goto handle_itb;
 
-		if (meta_bg == 1) {
-			ext4_group_t first_group;
-			first_group = ext4_meta_bg_first_group(sb, group);
-			if (first_group != group + 1 &&
-			    first_group != group + EXT4_DESC_PER_BLOCK(sb) - 1)
-				goto handle_itb;
-		}
+		if (meta_bg == 1)
+			goto handle_itb;
 
 		block = start + ext4_bg_has_super(sb, group);
 		/* Copy all of the GDT blocks into the backup in this group */
-- 
2.30.0

[PATCH 05/13] ext4: fix typo in setup_new_flex_group_blocks

[Kemeng Shi]

grop -> group

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/resize.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index 0e83b3f91545..f2a8c24a6fbb 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c
@@ -609,7 +609,7 @@ static int setup_new_flex_group_blocks(struct super_block *sb,
 		}
 
 handle_itb:
-		/* Initialize group tables of the grop @group */
+		/* Initialize group tables of the group @group */
 		if (!(bg_flags[i] & EXT4_BG_INODE_ZEROED))
 			goto handle_bb;
 
-- 
2.30.0

[PATCH 06/13] ext4: remove redundant check of count

[Kemeng Shi]

Remove zero check of count which is always non-zero.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/resize.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index f2a8c24a6fbb..c14b09d48679 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c
@@ -699,16 +699,14 @@ static int setup_new_flex_group_blocks(struct super_block *sb,
 			block = start;
 		}
 
-		if (count) {
-			err = set_flexbg_block_bitmap(sb, handle,
-						      flex_gd,
-						      EXT4_B2C(sbi, start),
-						      EXT4_B2C(sbi,
-							       start + count
-							       - 1));
-			if (err)
-				goto out;
-		}
+		err = set_flexbg_block_bitmap(sb, handle,
+				flex_gd,
+				EXT4_B2C(sbi, start),
+				EXT4_B2C(sbi,
+					start + count
+					- 1));
+		if (err)
+			goto out;
 	}
 
 out:
-- 
2.30.0

[PATCH 07/13] ext4: remove commented code in reserve_backup_gdb

[Kemeng Shi]

Remove commented code in reserve_backup_gdb

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/resize.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index c14b09d48679..07828903b818 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c
@@ -1080,9 +1080,6 @@ static int reserve_backup_gdb(handle_t *handle, struct inode *inode,
 	for (i = 0; i < reserved_gdb; i++) {
 		int err2;
 		data = (__le32 *)primary[i]->b_data;
-		/* printk("reserving backup %lu[%u] = %lu\n",
-		       primary[i]->b_blocknr, gdbackups,
-		       blk + primary[i]->b_blocknr); */
 		data[gdbackups] = cpu_to_le32(blk + primary[i]->b_blocknr);
 		err2 = ext4_handle_dirty_metadata(handle, NULL, primary[i]);
 		if (!err)
-- 
2.30.0

[PATCH 08/13] ext4: calculate free_clusters_count in cluster unit in verify_group_input

[Kemeng Shi]

We treat free_clusters_count in cluster unit while free_blocks_count is
in block unit. Convert free_blocks_count to cluster unit to match the
unit.
Currently, verify_group_input is only called from ext4_ioctl_group_add
which does not support bigalloc yet. The dismatch is easily ingored
when we try to support bigalloc in ext4_ioctl_group_add (ext4_resize_fs
already supports resize with bigalloc enabled). Just fix this in
advance.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/resize.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index 07828903b818..c532bb613043 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c
@@ -154,8 +154,9 @@ static int verify_group_input(struct super_block *sb,
 
 	overhead = ext4_group_overhead_blocks(sb, group);
 	metaend = start + overhead;
-	input->free_clusters_count = free_blocks_count =
-		input->blocks_count - 2 - overhead - sbi->s_itb_per_group;
+	free_blocks_count = input->blocks_count - 2 - overhead -
+			    sbi->s_itb_per_group;
+	input->free_clusters_count = EXT4_B2C(sbi, free_blocks_count);
 
 	if (test_opt(sb, DEBUG))
 		printk(KERN_DEBUG "EXT4-fs: adding %s group %u: %u blocks "
-- 
2.30.0

[PATCH 09/13] ext4: remove EXT4FS_DEBUG defination in resize.c

[Kemeng Shi]

Remove EXT4FS_DEBUG defination in resize.c for following reasons:
1. EXT4FS_DEBUG will enable debug messages, it should only be defined
when debugging.
2. ext4.h included from ext4_jbd2.h after EXT4FS_DEBUG defination will
"#undef EXT4FS_DEBUG", then EXT4FS_DEBUG defination in resize.c can't
actually turn on ext4_debug messages.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/resize.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index c532bb613043..686eee65c118 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c
@@ -10,8 +10,6 @@
  */
 
 
-#define EXT4FS_DEBUG
-
 #include <linux/errno.h>
 #include <linux/slab.h>
 #include <linux/jiffies.h>
-- 
2.30.0

[PATCH 10/13] ext4: use saved local variable sbi instead of EXT4_SB(sb)

[Kemeng Shi]

We save EXT4_SB(sb) to local variable sbi at beginning of function
ext4_resize_begin. Use sbi directly instead of EXT4_SB(sb) to
remove unnecessary pointer dereference.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/resize.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index 686eee65c118..b9507e432496 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c
@@ -55,7 +55,7 @@ int ext4_resize_begin(struct super_block *sb)
 	 * If the reserved GDT blocks is non-zero, the resize_inode feature
 	 * should always be set.
 	 */
-	if (EXT4_SB(sb)->s_es->s_reserved_gdt_blocks &&
+	if (sbi->s_es->s_reserved_gdt_blocks &&
 	    !ext4_has_feature_resize_inode(sb)) {
 		ext4_error(sb, "resize_inode disabled but reserved GDT blocks non-zero");
 		return -EFSCORRUPTED;
@@ -67,9 +67,9 @@ int ext4_resize_begin(struct super_block *sb)
          * bad time to do it anyways.
          */
 	if (EXT4_B2C(sbi, sbi->s_sbh->b_blocknr) !=
-	    le32_to_cpu(EXT4_SB(sb)->s_es->s_first_data_block)) {
+	    le32_to_cpu(sbi->s_es->s_first_data_block)) {
 		ext4_warning(sb, "won't resize using backup superblock at %llu",
-			(unsigned long long)EXT4_SB(sb)->s_sbh->b_blocknr);
+			(unsigned long long)sbi->s_sbh->b_blocknr);
 		return -EPERM;
 	}
 
@@ -77,7 +77,7 @@ int ext4_resize_begin(struct super_block *sb)
 	 * We are not allowed to do online-resizing on a filesystem mounted
 	 * with error, because it can destroy the filesystem easily.
 	 */
-	if (EXT4_SB(sb)->s_mount_state & EXT4_ERROR_FS) {
+	if (sbi->s_mount_state & EXT4_ERROR_FS) {
 		ext4_warning(sb, "There are errors in the filesystem, "
 			     "so online resizing is not allowed");
 		return -EPERM;
@@ -89,7 +89,7 @@ int ext4_resize_begin(struct super_block *sb)
 	}
 
 	if (test_and_set_bit_lock(EXT4_FLAGS_RESIZING,
-				  &EXT4_SB(sb)->s_ext4_flags))
+				  &sbi->s_ext4_flags))
 		ret = -EBUSY;
 
 	return ret;
-- 
2.30.0

[PATCH 11/13] ext4: correct gdblock calculation in add_new_gdb_meta_bg to support non first group

[Kemeng Shi]

In add_new_gdb_meta_bg, we assume that group could be non first
group in meta block group as we call ext4_meta_bg_first_block_no
to get first block of meta block group rather than call
ext4_group_first_block_no for passed group directly. Then ext4_bg_has_super
should be called with first group in meta group rather than new added
group. Or we can call ext4_group_first_block_no instead of
ext4_meta_bg_first_block_no to assume only first group of
meta group will be passed.
Either way, ext4_meta_bg_first_block_no will be useless and
could be removed.

This patch do it in first way to make add_new_gdb_meta_bg support non
first group in meta block group.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/resize.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index b9507e432496..da832466ce74 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c
@@ -110,12 +110,6 @@ static ext4_group_t ext4_meta_bg_first_group(struct super_block *sb,
 	       EXT4_DESC_PER_BLOCK_BITS(sb);
 }
 
-static ext4_fsblk_t ext4_meta_bg_first_block_no(struct super_block *sb,
-					     ext4_group_t group) {
-	group = ext4_meta_bg_first_group(sb, group);
-	return ext4_group_first_block_no(sb, group);
-}
-
 static ext4_grpblk_t ext4_group_overhead_blocks(struct super_block *sb,
 						ext4_group_t group) {
 	ext4_grpblk_t overhead;
@@ -954,8 +948,9 @@ static int add_new_gdb_meta_bg(struct super_block *sb,
 	unsigned long gdb_num = group / EXT4_DESC_PER_BLOCK(sb);
 	int err;
 
-	gdblock = ext4_meta_bg_first_block_no(sb, group) +
-		   ext4_bg_has_super(sb, group);
+	group = ext4_meta_bg_first_group(sb, group);
+	gdblock = ext4_group_first_block_no(sb, group) +
+		  ext4_bg_has_super(sb, group);
 	gdb_bh = ext4_sb_bread(sb, gdblock, 0);
 	if (IS_ERR(gdb_bh))
 		return PTR_ERR(gdb_bh);
-- 
2.30.0

[PATCH 12/13] ext4: remove unnecessary check for avoiding multiple update_backups in ext4_flex_group_add

[Kemeng Shi]

Commit 0acdb8876fead ("ext4: don't call update_backups() multiple times
for the same bg") add check in ext4_flex_group_add to avoid call
update_backups multiple times for block group descriptors in the same
group descriptor block. However, we have already call update_backups in
block step, so the added check is unnecessary.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/resize.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index da832466ce74..d2b3ee50af31 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c
@@ -1589,7 +1589,6 @@ static int ext4_flex_group_add(struct super_block *sb,
 		int meta_bg = ext4_has_feature_meta_bg(sb);
 		sector_t padding_blocks = meta_bg ? 0 : sbi->s_sbh->b_blocknr -
 					 ext4_group_first_block_no(sb, 0);
-		sector_t old_gdb = 0;
 
 		update_backups(sb, ext4_group_first_block_no(sb, 0),
 			       (char *)es, sizeof(struct ext4_super_block), 0);
@@ -1598,11 +1597,8 @@ static int ext4_flex_group_add(struct super_block *sb,
 
 			gdb_bh = sbi_array_rcu_deref(sbi, s_group_desc,
 						     gdb_num);
-			if (old_gdb == gdb_bh->b_blocknr)
-				continue;
 			update_backups(sb, gdb_bh->b_blocknr - padding_blocks,
 				       gdb_bh->b_data, gdb_bh->b_size, meta_bg);
-			old_gdb = gdb_bh->b_blocknr;
 		}
 	}
 exit:
-- 
2.30.0

[PATCH 13/13] ext4: remove unnecessary initialization of count2 in set_flexbg_block_bitmap

[Kemeng Shi]

We always overwrite count2 to "EXT4_CLUSTERS_PER_GROUP(sb) -
(first_cluster - start)" after its initialization in for loop
initialization statement .
Just remove unnecessary initialization of count2.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/resize.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index d2b3ee50af31..327475c2e2e7 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c
@@ -453,8 +453,7 @@ static int set_flexbg_block_bitmap(struct super_block *sb, handle_t *handle,
 
 	ext4_debug("mark clusters [%llu-%llu] used\n", first_cluster,
 		   last_cluster);
-	for (count2 = count; count > 0;
-	     count -= count2, first_cluster += count2) {
+	for (; count > 0; count -= count2, first_cluster += count2) {
 		ext4_fsblk_t start;
 		struct buffer_head *bh;
 		ext4_group_t group;
-- 
2.30.0

Re: [PATCH 03/13] ext4: correct return value of ext4_convert_meta_bg

[Theodore Ts'o]

On Thu, Jun 29, 2023 at 08:00:34PM +0800, Kemeng Shi wrote:
> We return error in "ret", so collect previous error in "ret" instead
> of "err" or previous error will be ignored.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> ---
>  fs/ext4/resize.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
> index 7cbc695b7005..0b3d8c808de1 100644
> --- a/fs/ext4/resize.c
> +++ b/fs/ext4/resize.c
> @@ -1984,8 +1984,8 @@ static int ext4_convert_meta_bg(struct super_block *sb, struct inode *inode)
>  
>  errout:
>  	ret = ext4_journal_stop(handle);
> -	if (!err)
> -		err = ret;
> +	if (!ret)
> +		ret = err;
>  	return ret;

If there is a previous error in "err", I think we would want to
prioritize returning that error, as opposed to the potential error
from ext4_journal_stop().  So how about this instead?

errout:
	ret = ext4_journal_stop(handle);
	if (!err)
		ret = err;
	return ret;


						- Ted

Re: [PATCH 01/13] ext4: correct offset of gdb backup in non meta_bg group to update_backups

[Theodore Ts'o]

On Thu, Jun 29, 2023 at 08:00:32PM +0800, Kemeng Shi wrote:
> Commit 0aeaa2559d6d5 ("ext4: fix corruption when online resizing a 1K
> bigalloc fs") found that primary superblock's offset in its group is not
> equal to offset of backup superblock in its group when block size is 1K
> and bigalloc is enabled. As group descriptor blocks are right after
> superblock, we can't pass block number of gdb to update_backups for
> the same reason.
> The root casue of the issue above is that leading 1K padding block is
> count as data block offset for primary block while backup block has
> no padding block offset in its group.
> Remove padding data block count to fix the issue for gdb backups.
> 
> For meta_bg case, update_backups treat blk_off as block number, do no
> conversion in this case.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Reviewed-by: Theodore Ts'o <tytso@mit.edu>

Re: [PATCH 02/13] ext4: add missed brelse in update_backups

[Theodore Ts'o]

On Thu, Jun 29, 2023 at 08:00:33PM +0800, Kemeng Shi wrote:
> add missed brelse in update_backups
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Reviewed-by: Theodore Ts'o <tytso@mit.edu>

Re: [PATCH 04/13] ext4: remove gdb backup copy for meta bg in setup_new_flex_group_blocks

[Theodore Ts'o]

On Thu, Jun 29, 2023 at 08:00:35PM +0800, Kemeng Shi wrote:
> Wrong check of gdb backup in meta bg as following:
> first_group is the first group of meta_bg which contains target group, so
> target group is always >= first_group. We check if target group has gdb
> backup by comparing first_group with [group + 1] and [group +
> EXT4_DESC_PER_BLOCK(sb) - 1]. As group >= first_group, then [group + N] is
> > first_group. So no copy of gdb backup in meta bg is done in
> setup_new_flex_group_blocks.
> 
> No need to do gdb backup copy in meta bg from setup_new_flex_group_blocks
> as we always copy updated gdb block to backups at end of
> ext4_flex_group_add as following:
> 
> ext4_flex_group_add
>   /* no gdb backup copy for meta bg any more */
>   setup_new_flex_group_blocks
> 
>   /* update current group number */
>   ext4_update_super
>     sbi->s_groups_count += flex_gd->count;
> 
>   /*
>    * if group in meta bg contains backup is added, the primary gdb block
>    * of the meta bg will be copy to backup in new added group here.
>    */
>   for (; gdb_num <= gdb_num_end; gdb_num++)
>     update_backups(...)
> 
> In summary, we can remove wrong gdb backup copy code in
> setup_new_flex_group_blocks.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Reviewed-by: Theodore Ts'o <tytso@mit.edu>

Re: [PATCH 05/13] ext4: fix typo in setup_new_flex_group_blocks

[Theodore Ts'o]

On Thu, Jun 29, 2023 at 08:00:36PM +0800, Kemeng Shi wrote:
> grop -> group
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Reviewed-by: Theodore Ts'o <tytso@mit.edu>

Re: [PATCH 06/13] ext4: remove redundant check of count

[Theodore Ts'o]

On Thu, Jun 29, 2023 at 08:00:37PM +0800, Kemeng Shi wrote:
> Remove zero check of count which is always non-zero.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Reviewed-by: Theodore Ts'o <tytso@mit.edu>

Re: [PATCH 07/13] ext4: remove commented code in reserve_backup_gdb

[Theodore Ts'o]

On Thu, Jun 29, 2023 at 08:00:38PM +0800, Kemeng Shi wrote:
> Remove commented code in reserve_backup_gdb
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Reviewed-by: Theodore Ts'o <tytso@mit.edu>

Re: [PATCH 08/13] ext4: calculate free_clusters_count in cluster unit in verify_group_input

[Theodore Ts'o]

On Thu, Jun 29, 2023 at 08:00:39PM +0800, Kemeng Shi wrote:
> We treat free_clusters_count in cluster unit while free_blocks_count is
> in block unit. Convert free_blocks_count to cluster unit to match the
> unit.
> Currently, verify_group_input is only called from ext4_ioctl_group_add
> which does not support bigalloc yet. The dismatch is easily ingored
> when we try to support bigalloc in ext4_ioctl_group_add (ext4_resize_fs
> already supports resize with bigalloc enabled). Just fix this in
> advance.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

I'd rewrite the commit description a bit:

The field free_cluster_count in struct ext4_new_group_data should be
in units of clusters.  In verify_group_input() this field is being
filled in units of blocks.  Fortunately, we don't support online
resizing of bigalloc file systems, and for non-bigalloc file systems,
the cluster size == block size.  But fix this in case we do support
online resizing of bigalloc file systems in the future.

Other than that:

Reviewed-by: Theodore Ts'o <tytso@mit.edu>

						- Ted

Re: [PATCH 09/13] ext4: remove EXT4FS_DEBUG defination in resize.c

[Theodore Ts'o]

On Thu, Jun 29, 2023 at 08:00:40PM +0800, Kemeng Shi wrote:
> Remove EXT4FS_DEBUG defination in resize.c for following reasons:
> 1. EXT4FS_DEBUG will enable debug messages, it should only be defined
> when debugging.
> 2. ext4.h included from ext4_jbd2.h after EXT4FS_DEBUG defination will
> "#undef EXT4FS_DEBUG", then EXT4FS_DEBUG defination in resize.c can't
> actually turn on ext4_debug messages.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Reviewed-by: Theodore Ts'o <tytso@mit.edu>

Re: [PATCH 10/13] ext4: use saved local variable sbi instead of EXT4_SB(sb)

[Theodore Ts'o]

On Thu, Jun 29, 2023 at 08:00:41PM +0800, Kemeng Shi wrote:
> We save EXT4_SB(sb) to local variable sbi at beginning of function
> ext4_resize_begin. Use sbi directly instead of EXT4_SB(sb) to
> remove unnecessary pointer dereference.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Reviewed-by: Theodore Ts'o <tytso@mit.edu>

Re: [PATCH 11/13] ext4: correct gdblock calculation in add_new_gdb_meta_bg to support non first group

[Theodore Ts'o]

On Thu, Jun 29, 2023 at 08:00:42PM +0800, Kemeng Shi wrote:
> In add_new_gdb_meta_bg, we assume that group could be non first
> group in meta block group as we call ext4_meta_bg_first_block_no
> to get first block of meta block group rather than call
> ext4_group_first_block_no for passed group directly. Then ext4_bg_has_super
> should be called with first group in meta group rather than new added
> group. Or we can call ext4_group_first_block_no instead of
> ext4_meta_bg_first_block_no to assume only first group of
> meta group will be passed.
> Either way, ext4_meta_bg_first_block_no will be useless and
> could be removed.

Unfortunately, I spent more time trying to understand the commit
description than the C code.  Perhaps this might be a better way of
describing the situation?

The ext4_new descs() function calls ext4_meta_bg_first_block_no() with
the group paramter when the group is the first group of a meta_bg
(e.g., when (group % EXT4_DESC_PER_BLOCK) is zero.  So we can simplify
things a bit by removing ext4_meta_bg_first_block_no() and an open
coding its logic.

Does this make more sense to tou?

					- Ted

Re: [PATCH 12/13] ext4: remove unnecessary check for avoiding multiple update_backups in ext4_flex_group_add

[Theodore Ts'o]

On Thu, Jun 29, 2023 at 08:00:43PM +0800, Kemeng Shi wrote:
> Commit 0acdb8876fead ("ext4: don't call update_backups() multiple times
> for the same bg") add check in ext4_flex_group_add to avoid call
> update_backups multiple times for block group descriptors in the same
> group descriptor block. However, we have already call update_backups in
> block step, so the added check is unnecessary.

I'm having trouble understaind this comit.  What do you mean by the
"block step" in the last paragraph?

					- Ted

Re: [PATCH 13/13] ext4: remove unnecessary initialization of count2 in set_flexbg_block_bitmap

[Theodore Ts'o]

On Thu, Jun 29, 2023 at 08:00:44PM +0800, Kemeng Shi wrote:
> We always overwrite count2 to "EXT4_CLUSTERS_PER_GROUP(sb) -
> (first_cluster - start)" after its initialization in for loop
> initialization statement .
> Just remove unnecessary initialization of count2.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>

Reviewed-by: Theodore Ts'o <tytso@mit.edu>

Re: [PATCH 03/13] ext4: correct return value of ext4_convert_meta_bg

[Kemeng Shi]

on 8/16/2023 11:00 AM, Theodore Ts'o wrote:
> On Thu, Jun 29, 2023 at 08:00:34PM +0800, Kemeng Shi wrote:
>> We return error in "ret", so collect previous error in "ret" instead
>> of "err" or previous error will be ignored.
>>
>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
>> ---
>>  fs/ext4/resize.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
>> index 7cbc695b7005..0b3d8c808de1 100644
>> --- a/fs/ext4/resize.c
>> +++ b/fs/ext4/resize.c
>> @@ -1984,8 +1984,8 @@ static int ext4_convert_meta_bg(struct super_block *sb, struct inode *inode)
>>  
>>  errout:
>>  	ret = ext4_journal_stop(handle);
>> -	if (!err)
>> -		err = ret;
>> +	if (!ret)
>> +		ret = err;
>>  	return ret;
> 
> If there is a previous error in "err", I think we would want to
> prioritize returning that error, as opposed to the potential error
> from ext4_journal_stop().  So how about this instead?
> 
> errout:
> 	ret = ext4_journal_stop(handle);
> 	if (!err)
> 		ret = err;
> 	return ret;
> 
I think you mean:
errout:
	ret = ext4_journal_stop(handle);
	if (*err*)
		ret = err;
	return ret;
And I will fix in this way if I don't get wrong.
> 
> 						- Ted
> 

Re: [PATCH 08/13] ext4: calculate free_clusters_count in cluster unit in verify_group_input

[Kemeng Shi]

on 8/16/2023 11:22 AM, Theodore Ts'o wrote:
> On Thu, Jun 29, 2023 at 08:00:39PM +0800, Kemeng Shi wrote:
>> We treat free_clusters_count in cluster unit while free_blocks_count is
>> in block unit. Convert free_blocks_count to cluster unit to match the
>> unit.
>> Currently, verify_group_input is only called from ext4_ioctl_group_add
>> which does not support bigalloc yet. The dismatch is easily ingored
>> when we try to support bigalloc in ext4_ioctl_group_add (ext4_resize_fs
>> already supports resize with bigalloc enabled). Just fix this in
>> advance.
>>
>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> 
> I'd rewrite the commit description a bit:
> 
> The field free_cluster_count in struct ext4_new_group_data should be
> in units of clusters.  In verify_group_input() this field is being
> filled in units of blocks.  Fortunately, we don't support online
> resizing of bigalloc file systems, and for non-bigalloc file systems,
> the cluster size == block size.  But fix this in case we do support
> online resizing of bigalloc file systems in the future.
> 
Sorry for my poor language and thanks a lot for the rewrite. I will
fill rewriten description in next version!
> Other than that:
> 
> Reviewed-by: Theodore Ts'o <tytso@mit.edu>
> 
> 						- Ted
> 

Re: [PATCH 11/13] ext4: correct gdblock calculation in add_new_gdb_meta_bg to support non first group

[Kemeng Shi]

on 8/16/2023 11:45 AM, Theodore Ts'o wrote:
> On Thu, Jun 29, 2023 at 08:00:42PM +0800, Kemeng Shi wrote:
>> In add_new_gdb_meta_bg, we assume that group could be non first
>> group in meta block group as we call ext4_meta_bg_first_block_no
>> to get first block of meta block group rather than call
>> ext4_group_first_block_no for passed group directly. Then ext4_bg_has_super
>> should be called with first group in meta group rather than new added
>> group. Or we can call ext4_group_first_block_no instead of
>> ext4_meta_bg_first_block_no to assume only first group of
>> meta group will be passed.
>> Either way, ext4_meta_bg_first_block_no will be useless and
>> could be removed.
> 
> Unfortunately, I spent more time trying to understand the commit
> description than the C code.  Perhaps this might be a better way of
> describing the situation?
> 
Sorry for my poor language again, :( I will try to improve this.
> The ext4_new descs() function calls ext4_meta_bg_first_block_no() with
> the group paramter when the group is the first group of a meta_bg
> (e.g., when (group % EXT4_DESC_PER_BLOCK) is zero.  So we can simplify
> things a bit by removing ext4_meta_bg_first_block_no() and an open
> coding its logic.
> 
> Does this make more sense to tou?
> 
This patch tries to correct gdbblock calculation in add_new_gdb_meta_bg
in case group from caller is not the first group of meta_bg which is
supposed to be handled by add_new_gdb_meta_bg.
We should call ext4_bg_has_super with first group in meta_bg instead
of group which could be non first group in meta_bg to calculate gdb
of meta_bg.
Fortunately, the only caller ext4_add_new_descs always call
add_new_gdb_meta_bg with first group of meta_bg and no real issue
will happen.


> 					- Ted
> 

Re: [PATCH 12/13] ext4: remove unnecessary check for avoiding multiple update_backups in ext4_flex_group_add

[Kemeng Shi]

on 8/16/2023 11:47 AM, Theodore Ts'o wrote:
> On Thu, Jun 29, 2023 at 08:00:43PM +0800, Kemeng Shi wrote:
>> Commit 0acdb8876fead ("ext4: don't call update_backups() multiple times
>> for the same bg") add check in ext4_flex_group_add to avoid call
>> update_backups multiple times for block group descriptors in the same
>> group descriptor block. However, we have already call update_backups in
>> block step, so the added check is unnecessary.
> 
> I'm having trouble understaind this comit.  What do you mean by the
> "block step" in the last paragraph?
> 
Sorry for the confusing stuff. I mean we foreach in group descriptor block
step instead of foreach in group descriptor step to update backup.
So there is no chance to call update_backups for different descriptors
in the same bg.

> 					- Ted
> 

Re: [PATCH 03/13] ext4: correct return value of ext4_convert_meta_bg

[Theodore Ts'o]

On Thu, Aug 17, 2023 at 10:48:51AM +0800, Kemeng Shi wrote:
> 
> > errout:
> > 	ret = ext4_journal_stop(handle);
> > 	if (!err)
> > 		ret = err;
> > 	return ret;
> > 
> I think you mean:
> errout:
> 	ret = ext4_journal_stop(handle);
> 	if (*err*)
> 		ret = err;
> 	return ret;
> And I will fix in this way if I don't get wrong.

Yes, that's what I meant.  Another way of putting it might be:

	ret = ext4_journal_stop(handle);
	return (err ? err : ret);

				- Ted

Re: [PATCH 11/13] ext4: correct gdblock calculation in add_new_gdb_meta_bg to support non first group

[Theodore Ts'o]

On Thu, Aug 17, 2023 at 11:38:34AM +0800, Kemeng Shi wrote:
> 
> 
> on 8/16/2023 11:45 AM, Theodore Ts'o wrote:
> > On Thu, Jun 29, 2023 at 08:00:42PM +0800, Kemeng Shi wrote:
> >> In add_new_gdb_meta_bg, we assume that group could be non first
> >> group in meta block group as we call ext4_meta_bg_first_block_no
> >> to get first block of meta block group rather than call
> >> ext4_group_first_block_no for passed group directly. Then ext4_bg_has_super
> >> should be called with first group in meta group rather than new added
> >> group. Or we can call ext4_group_first_block_no instead of
> >> ext4_meta_bg_first_block_no to assume only first group of
> >> meta group will be passed.
> >> Either way, ext4_meta_bg_first_block_no will be useless and
> >> could be removed.
> > 
> > Unfortunately, I spent more time trying to understand the commit
> > description than the C code.  Perhaps this might be a better way of
> > describing the situation?
> > 
> Sorry for my poor language again, :( I will try to improve this.
> > The ext4_new descs() function calls ext4_meta_bg_first_block_no() with
> > the group paramter when the group is the first group of a meta_bg
> > (e.g., when (group % EXT4_DESC_PER_BLOCK) is zero.  So we can simplify
> > things a bit by removing ext4_meta_bg_first_block_no() and an open
> > coding its logic.
> > 
> > Does this make more sense to tou?
> > 
> This patch tries to correct gdbblock calculation in add_new_gdb_meta_bg
> in case group from caller is not the first group of meta_bg which is
> supposed to be handled by add_new_gdb_meta_bg.
> We should call ext4_bg_has_super with first group in meta_bg instead
> of group which could be non first group in meta_bg to calculate gdb
> of meta_bg.
> Fortunately, the only caller ext4_add_new_descs always call
> add_new_gdb_meta_bg with first group of meta_bg and no real issue
> will happen.

To be clear, this doesn't have a functional change given how the code
is going to be used, right?  It's really more of a cleanup with a goal
of making the code easier to understand.  If so, we should make this
explicit at the beginning of the commit description, as opposed to
putting it at the end.

In journalism this is referred to as "burying the lede"[1], where the
"lede" the most important/key piece of information.  In general, it is
desirable not to "bury the lede".  That is, the most important
information, including why people should care, and what this is doing,
at the beginning of the commit description (or article in the case of
journalsm).

[1] https://www.masterclass.com/articles/bury-the-lede-explained

					- Ted

Re: [PATCH 12/13] ext4: remove unnecessary check for avoiding multiple update_backups in ext4_flex_group_add

[Theodore Ts'o]

On Thu, Aug 17, 2023 at 11:53:52AM +0800, Kemeng Shi wrote:
> 
> 
> on 8/16/2023 11:47 AM, Theodore Ts'o wrote:
> > On Thu, Jun 29, 2023 at 08:00:43PM +0800, Kemeng Shi wrote:
> >> Commit 0acdb8876fead ("ext4: don't call update_backups() multiple times
> >> for the same bg") add check in ext4_flex_group_add to avoid call
> >> update_backups multiple times for block group descriptors in the same
> >> group descriptor block. However, we have already call update_backups in
> >> block step, so the added check is unnecessary.
> > 
> > I'm having trouble understaind this comit.  What do you mean by the
> > "block step" in the last paragraph?
> > 
> Sorry for the confusing stuff. I mean we foreach in group descriptor block
> step instead of foreach in group descriptor step to update backup.
> So there is no chance to call update_backups for different descriptors
> in the same bg.

I'm still confused.  This commit is not so much removing an
unnecessary check as much as forcing update_backups() to be called for
every single block group.  Right?

So either we're doing extra work, or (b) we're fixing a problem
because we actually *need* to call update_backups() for every single
block group.

More generally, this whole patch series is making it clear that the
online resize code is hard to understand, because it's super
complicated, leading to potential bugs, and very clearly code which is
very hard to maintain.  So this may mean we need better comments to
make it clear *when* the backup block groups are going to be
accomplished, given the various different cases (e.g., no flex_bg or
meta_bg, with flex_bg, or with meat_bg).

							- Ted

Re: [PATCH 11/13] ext4: correct gdblock calculation in add_new_gdb_meta_bg to support non first group

[Kemeng Shi]

on 8/17/2023 10:03 PM, Theodore Ts'o wrote:
> On Thu, Aug 17, 2023 at 11:38:34AM +0800, Kemeng Shi wrote:
>>
>>
>> on 8/16/2023 11:45 AM, Theodore Ts'o wrote:
>>> On Thu, Jun 29, 2023 at 08:00:42PM +0800, Kemeng Shi wrote:
>>>> In add_new_gdb_meta_bg, we assume that group could be non first
>>>> group in meta block group as we call ext4_meta_bg_first_block_no
>>>> to get first block of meta block group rather than call
>>>> ext4_group_first_block_no for passed group directly. Then ext4_bg_has_super
>>>> should be called with first group in meta group rather than new added
>>>> group. Or we can call ext4_group_first_block_no instead of
>>>> ext4_meta_bg_first_block_no to assume only first group of
>>>> meta group will be passed.
>>>> Either way, ext4_meta_bg_first_block_no will be useless and
>>>> could be removed.
>>>
>>> Unfortunately, I spent more time trying to understand the commit
>>> description than the C code.  Perhaps this might be a better way of
>>> describing the situation?
>>>
>> Sorry for my poor language again, :( I will try to improve this.
>>> The ext4_new descs() function calls ext4_meta_bg_first_block_no() with
>>> the group paramter when the group is the first group of a meta_bg
>>> (e.g., when (group % EXT4_DESC_PER_BLOCK) is zero.  So we can simplify
>>> things a bit by removing ext4_meta_bg_first_block_no() and an open
>>> coding its logic.
>>>
>>> Does this make more sense to tou?
>>>
>> This patch tries to correct gdbblock calculation in add_new_gdb_meta_bg
>> in case group from caller is not the first group of meta_bg which is
>> supposed to be handled by add_new_gdb_meta_bg.
>> We should call ext4_bg_has_super with first group in meta_bg instead
>> of group which could be non first group in meta_bg to calculate gdb
>> of meta_bg.
>> Fortunately, the only caller ext4_add_new_descs always call
>> add_new_gdb_meta_bg with first group of meta_bg and no real issue
>> will happen.
> 
> To be clear, this doesn't have a functional change given how the code
> is going to be used, right?  It's really more of a cleanup with a goal
> of making the code easier to understand.  If so, we should make this
> explicit at the beginning of the commit description, as opposed to
> putting it at the end.
> 
Actually, there seems a functional change to add_new_gdb_meta_bg.
Assume 'group' is the new added group, 'first_group' is the first group
of meta_bg which contains 'group',
Original way to calculate gdbblock:
gdbblock = group_first_block('first_group') + bg_has_super(*'group'*)
New ay to calculate gdbblock
gdbblock = group_first_block('first_group') + bg_has_super(*'first_group'*)
If new added group is not the first group of meta_bg, add_new_gdb_meta_bg
get a wrong gdbblock.
Maybe it's more of a bugfix to as add_new_gdb_meta_bg treats group
as any group of meta_bg instead of first group of meta_bg. And it's
more of a cleanup as only first group is passed from caller.
> In journalism this is referred to as "burying the lede"[1], where the
> "lede" the most important/key piece of information.  In general, it is
> desirable not to "bury the lede".  That is, the most important
> information, including why people should care, and what this is doing,
> at the beginning of the commit description (or article in the case of
> journalsm).
> 
> [1] https://www.masterclass.com/articles/bury-the-lede-explained
> 
> 					- Ted
> 
Thanks for this information. But I'm little confused weather a cleanup
or a bugfix I should mention at the beginning. Any more advise is
appreciated!

Re: [PATCH 12/13] ext4: remove unnecessary check for avoiding multiple update_backups in ext4_flex_group_add

[Kemeng Shi]

on 8/17/2023 10:11 PM, Theodore Ts'o wrote:
> On Thu, Aug 17, 2023 at 11:53:52AM +0800, Kemeng Shi wrote:
>>
>>
>> on 8/16/2023 11:47 AM, Theodore Ts'o wrote:
>>> On Thu, Jun 29, 2023 at 08:00:43PM +0800, Kemeng Shi wrote:
>>>> Commit 0acdb8876fead ("ext4: don't call update_backups() multiple times
>>>> for the same bg") add check in ext4_flex_group_add to avoid call
>>>> update_backups multiple times for block group descriptors in the same
>>>> group descriptor block. However, we have already call update_backups in
>>>> block step, so the added check is unnecessary.
>>>
>>> I'm having trouble understaind this comit.  What do you mean by the
>>> "block step" in the last paragraph?
>>>
>> Sorry for the confusing stuff. I mean we foreach in group descriptor block
>> step instead of foreach in group descriptor step to update backup.
>> So there is no chance to call update_backups for different descriptors
>> in the same bg.
> 
> I'm still confused.  This commit is not so much removing an
> unnecessary check as much as forcing update_backups() to be called for
> every single block group.  Right?
> 
Ah, I guess here is the thing I missed that make this confusing:
sbi->s_group_desc contains only primary block of each group. i.e.
sbi->s_group_desc['i'] is the primary gdb block of group 'i'. It's clear
that primary gdb blocks of different groups will not be the same. Then
all blocks in s_group_desc[*] should be different and the removed check
is unnecessary.
From add_new_gdb and add_new_gdb_meta_bg, we can find that we always
add primary gdb block of new group to s_group_desc. To be more specific:
add_new_gdb
	gdblock = EXT4_SB(sb)->s_sbh->b_blocknr + 1 + gdb_num;
	gdb_bh = ext4_sb_bread(sb, gdblock, 0);
	n_group_desc[gdb_num] = gdb_bh;

add_new_gdb_meta_bg
	gdblock = ext4_meta_bg_first_block_no(sb, group) +
		  ext4_bg_has_super(sb, group);
	gdb_bh = ext4_sb_bread(sb, gdblock, 0);
	n_group_desc[gdb_num] = gdb_bh;
	
> So either we're doing extra work, or (b) we're fixing a problem
> because we actually *need* to call update_backups() for every single
> block group.
> 
> More generally, this whole patch series is making it clear that the
> online resize code is hard to understand, because it's super
> complicated, leading to potential bugs, and very clearly code which is
> very hard to maintain.  So this may mean we need better comments to
> make it clear *when* the backup block groups are going to be
> accomplished, given the various different cases (e.g., no flex_bg or
> meta_bg, with flex_bg, or with meat_bg).
> 
Yes, I agree with this. I wonder if a series to add comments of some
common rules is good to you. Like the information mentioned above
that s_group_desc contains primary gdb block of each group.

> 							- Ted
> 

Re: [PATCH 11/13] ext4: correct gdblock calculation in add_new_gdb_meta_bg to support non first group

[Theodore Ts'o]

On Fri, Aug 18, 2023 at 10:29:52AM +0800, Kemeng Shi wrote:
> Actually, there seems a functional change to add_new_gdb_meta_bg.
> Assume 'group' is the new added group, 'first_group' is the first group
> of meta_bg which contains 'group',
> Original way to calculate gdbblock:
> gdbblock = group_first_block('first_group') + bg_has_super(*'group'*)
> New ay to calculate gdbblock
> gdbblock = group_first_block('first_group') + bg_has_super(*'first_group'*)
> If new added group is not the first group of meta_bg, add_new_gdb_meta_bg
> get a wrong gdbblock.

If you look at the ext4_add_new_descs() function,
add_new_gdb_meta_bg() is only called when the group is a multiple of
EXT4_DESC_PER_BLOCK --- that is, when group % EXT4_DESC_PER_BLOCK == 0.

As such, it is only called when with group is the first group in the
meta_bg.  So there is no bug here.  The code is bit confusing, I agree
--- I myself got confused because it's been years since I last looked
at the code, and it's not particularly commented well, which is my fault.

This also makes the commit description "... to support non-first
group" incorrect, since it never gets called as with a "non-first
group".

The patch makes things a little simpler, but the commit description
would confuse anyone who looked at it while doing code archeology.
The change is fine, although at this point, given how we both
misunderstood how the code worked without doing some deep mind-melds
with the C code in question, it's clear that we need some better
comments in the code.

For example, the comment "add_new_gdb_meta_bg is the sister of
add_new_gdb" is clearly insufficient.

						- Ted

Re: [PATCH 12/13] ext4: remove unnecessary check for avoiding multiple update_backups in ext4_flex_group_add

[Theodore Ts'o]

On Fri, Aug 18, 2023 at 11:16:35AM +0800, Kemeng Shi wrote:
> Ah, I guess here is the thing I missed that make this confusing:
> sbi->s_group_desc contains only primary block of each group. i.e.
> sbi->s_group_desc['i'] is the primary gdb block of group 'i'.

Correct.  In fact, when we need to modify a block group descriptor for
a group, we call ext4_get_group_desc(), and it references
sbi->s_group_desc to find the appropriate buffer_head for the bg
descriptor that we want to modify.

I'm not sure "only" is the right adjective to use here, since the
whole *point* of s_group_desc[] is to keep the buffer_heads for the
block group descriptor blocks in memory, so we can modify them when we
allocate or free blocks, inodes, etc.  And we only modify the primary
block group descriptors.

The secondary, or backup block group descriptors are only by used
e2fsck when the primary block group descriptor has been overwritten,
so we can find the inode table, allocation bitmaps, and so on.  So we
do *not* modify them in the course of normal operations, and that's by
design.  The only time the kernel will update those block group
descriptors is when we are doing an online resize, and we need make
sure the backup descriptors are updated, so that if the primary
descriptors get completely smashed, we can still get critical
information such as the location of that block group's inode table.

> From add_new_gdb and add_new_gdb_meta_bg, we can find that we always
> add primary gdb block of new group to s_group_desc. To be more specific:
> add_new_gdb
> 	gdblock = EXT4_SB(sb)->s_sbh->b_blocknr + 1 + gdb_num;
> 	gdb_bh = ext4_sb_bread(sb, gdblock, 0);
> 	n_group_desc[gdb_num] = gdb_bh;
> 
> add_new_gdb_meta_bg
> 	gdblock = ext4_meta_bg_first_block_no(sb, group) +
> 		  ext4_bg_has_super(sb, group);
> 	gdb_bh = ext4_sb_bread(sb, gdblock, 0);
> 	n_group_desc[gdb_num] = gdb_bh;

Put another way, there are EXT4_DESC_PER_BLOCK(sb) bg descriptors in a
block.  For a file system with the 64-bit feature enabled, the size of
the block group descriptor is 128.  If the block size is 4096, then we
can fit 32 block group descriptors in a block.

When we add a new block group such that its block group descriptor
will spill into a new block, then we need to expand s_group_desc[]
array, and initialize the new block group descriptor block.  And
that's the job of add_new_gdb() and add_new_gdb_meta_bg().

> > More generally, this whole patch series is making it clear that the
> > online resize code is hard to understand, because it's super
> > complicated, leading to potential bugs, and very clearly code which is
> > very hard to maintain.  So this may mean we need better comments to
> > make it clear *when* the backup block groups are going to be
> > accomplished, given the various different cases (e.g., no flex_bg or
> > meta_bg, with flex_bg, or with meat_bg).
> > 
> Yes, I agree with this. I wonder if a series to add comments of some
> common rules is good to you. Like the information mentioned above
> that s_group_desc contains primary gdb block of each group.

Well, the meaning of s_group_desc[] was obvious to me, but that's why
it's useful to have somone with "new eyes" take a look at code, since
what may be obvious to old hands might not be obvious to someone
looking at the code for the first time --- and so yes, it's probably
worth documenting.  The question is where is the best place to put it,
since the primary place where s_group_desc[] is *not* online resize.

s_group_desc[] is initialized in ext4_group_desc_init() in
fs/ext4/super.c, and it is used in fs/ext4/balloc.c, and of course, it
is defined in fs/ext4.h.  

						- Ted

Re: [PATCH 11/13] ext4: correct gdblock calculation in add_new_gdb_meta_bg to support non first group

[Kemeng Shi]

on 8/18/2023 12:07 PM, Theodore Ts'o wrote:
> On Fri, Aug 18, 2023 at 10:29:52AM +0800, Kemeng Shi wrote:
>> Actually, there seems a functional change to add_new_gdb_meta_bg.
>> Assume 'group' is the new added group, 'first_group' is the first group
>> of meta_bg which contains 'group',
>> Original way to calculate gdbblock:
>> gdbblock = group_first_block('first_group') + bg_has_super(*'group'*)
>> New ay to calculate gdbblock
>> gdbblock = group_first_block('first_group') + bg_has_super(*'first_group'*)
>> If new added group is not the first group of meta_bg, add_new_gdb_meta_bg
>> get a wrong gdbblock.
> 
> If you look at the ext4_add_new_descs() function,
> add_new_gdb_meta_bg() is only called when the group is a multiple of
> EXT4_DESC_PER_BLOCK --- that is, when group % EXT4_DESC_PER_BLOCK == 0.
>
> As such, it is only called when with group is the first group in the
> meta_bg.  So there is no bug here.  The code is bit confusing, I agree
> --- I myself got confused because it's been years since I last looked
> at the code, and it's not particularly commented well, which is my fault.
> 
Yes, add_new_gdb_meta_bg is only called with first group of mebg and no real
bug here.
> This also makes the commit description "... to support non-first
> group" incorrect, since it never gets called as with a "non-first
> group".
> 
Ah, what I want to say is "support non-frist group in fulture". And if there
is definely no need in fulture, it's more intuitive just treat group from caller
as first group in meta_bg.
> The patch makes things a little simpler, but the commit description
> would confuse anyone who looked at it while doing code archeology.
> The change is fine, although at this point, given how we both
> misunderstood how the code worked without doing some deep mind-melds
> with the C code in question, it's clear that we need some better
> comments in the code.
> 
> For example, the comment "add_new_gdb_meta_bg is the sister of
> add_new_gdb" is clearly insufficient.
> Is following comment looks good to you:
When all reserved primary blocks are consumed, we create meta_bg group and
allocate new primary block at first block or block after backup superblock
(if exsiting) in first group of meta_bg group.
This function is only called when first group of meta_bg is added.
> 						- Ted
> 

Re: [PATCH 12/13] ext4: remove unnecessary check for avoiding multiple update_backups in ext4_flex_group_add

[Kemeng Shi]

on 8/18/2023 1:00 PM, Theodore Ts'o wrote:
> On Fri, Aug 18, 2023 at 11:16:35AM +0800, Kemeng Shi wrote:
>> Ah, I guess here is the thing I missed that make this confusing:
>> sbi->s_group_desc contains only primary block of each group. i.e.
>> sbi->s_group_desc['i'] is the primary gdb block of group 'i'.
> 
> Correct.  In fact, when we need to modify a block group descriptor for
> a group, we call ext4_get_group_desc(), and it references
> sbi->s_group_desc to find the appropriate buffer_head for the bg
> descriptor that we want to modify.
> 
> I'm not sure "only" is the right adjective to use here, since the
> whole *point* of s_group_desc[] is to keep the buffer_heads for the
> block group descriptor blocks in memory, so we can modify them when we
> allocate or free blocks, inodes, etc.  And we only modify the primary
> block group descriptors.
> 
> The secondary, or backup block group descriptors are only by used
> e2fsck when the primary block group descriptor has been overwritten,
> so we can find the inode table, allocation bitmaps, and so on.  So we
> do *not* modify them in the course of normal operations, and that's by
> design.  The only time the kernel will update those block group
> descriptors is when we are doing an online resize, and we need make
> sure the backup descriptors are updated, so that if the primary
> descriptors get completely smashed, we can still get critical
> information such as the location of that block group's inode table.
> >> From add_new_gdb and add_new_gdb_meta_bg, we can find that we always
>> add primary gdb block of new group to s_group_desc. To be more specific:
>> add_new_gdb
>> 	gdblock = EXT4_SB(sb)->s_sbh->b_blocknr + 1 + gdb_num;
>> 	gdb_bh = ext4_sb_bread(sb, gdblock, 0);
>> 	n_group_desc[gdb_num] = gdb_bh;
>>
>> add_new_gdb_meta_bg
>> 	gdblock = ext4_meta_bg_first_block_no(sb, group) +
>> 		  ext4_bg_has_super(sb, group);
>> 	gdb_bh = ext4_sb_bread(sb, gdblock, 0);
>> 	n_group_desc[gdb_num] = gdb_bh;
> 
> Put another way, there are EXT4_DESC_PER_BLOCK(sb) bg descriptors in a
> block.  For a file system with the 64-bit feature enabled, the size of
> the block group descriptor is 128.  If the block size is 4096, then we
> can fit 32 block group descriptors in a block.
> 
> When we add a new block group such that its block group descriptor
> will spill into a new block, then we need to expand s_group_desc[]
> array, and initialize the new block group descriptor block.  And
> that's the job of add_new_gdb() and add_new_gdb_meta_bg().
> 
Hi Ted, thanks for the explain. Here are more updates from what I found:
I find that descriptor_loc show layout of gdb blocks in s_group_desc[]
which is: block of s_group_desc[i] will be superblock + i + 1 for
non-meta_bg and 'first block of meta_bg' + has_super for meta_bg.
Although descriptor_loc is called to initialize s_group_desc[], the
expanded gdb block to s_group_desc from add_new_gdb obeys the same
layout.
Back to the original purpose of this patch which is to remove
unnecessary of equality check of s_group_desc[gdb_num - 1].b_blocknr and
s_group_desc[gdb_num].b_blocknr, we can see each s_group_desc has it's
own block from layout above and the check should be unnecessary.
But no insistant on this if you still have concern about it.
>>> More generally, this whole patch series is making it clear that the
>>> online resize code is hard to understand, because it's super
>>> complicated, leading to potential bugs, and very clearly code which is
>>> very hard to maintain.  So this may mean we need better comments to
>>> make it clear *when* the backup block groups are going to be
>>> accomplished, given the various different cases (e.g., no flex_bg or
>>> meta_bg, with flex_bg, or with meat_bg).
>>>
>> Yes, I agree with this. I wonder if a series to add comments of some
>> common rules is good to you. Like the information mentioned above
>> that s_group_desc contains primary gdb block of each group.
> 
> Well, the meaning of s_group_desc[] was obvious to me, but that's why
> it's useful to have somone with "new eyes" take a look at code, since
> what may be obvious to old hands might not be obvious to someone
> looking at the code for the first time --- and so yes, it's probably
Yes. this is just for anyone starting to read this code.
> worth documenting.  The question is where is the best place to put it,
> since the primary place where s_group_desc[] is *not* online resize.
> 
> s_group_desc[] is initialized in ext4_group_desc_init() in
> fs/ext4/super.c, and it is used in fs/ext4/balloc.c, and of course, it
> is defined in fs/ext4.h.  
I plan to add comment in fs/ext4.h as following:
struct ext4_sb_info {
	...
	struct buffer_head * __rcu *s_group_desc; /* Primary gdb blocks of online groups */
But I'm not sure it's proper now as you menthioned s_group_desc[] is to
keep the buffer_heads for the block group descriptor blocks in memory
and it contains primary gdb block is a coincidence that we only modify
primary block in kernel.

Besides, I plan to go through the resize code again in fulture and
add some comments to make it easy for anyone starting read this
or make it easy to maintain. Please let me if you disklike it.
> 
> 						- Ted
> 

Re: [PATCH 12/13] ext4: remove unnecessary check for avoiding multiple update_backups in ext4_flex_group_add

[Theodore Ts'o]

On Fri, Aug 18, 2023 at 04:42:31PM +0800, Kemeng Shi wrote:
> > s_group_desc[] is initialized in ext4_group_desc_init() in
> > fs/ext4/super.c, and it is used in fs/ext4/balloc.c, and of course, it
> > is defined in fs/ext4.h.  
> I plan to add comment in fs/ext4.h as following:
> struct ext4_sb_info {
> 	...
> 	struct buffer_head * __rcu *s_group_desc; /* Primary gdb blocks of online groups */
> But I'm not sure it's proper now as you menthioned s_group_desc[] is to
> keep the buffer_heads for the block group descriptor blocks in memory
> and it contains primary gdb block is a coincidence that we only modify
> primary block in kernel.

In general, the terminology that ext4 developers have used is "block
group descriptors" and "backup block group descriptors".  The kernel
never *uses* the backup block group users; and with the sparse_super2
feature, the "backup superblocks" and "backup block group descriptors"
are optional.

They are used by e2fsck if we need to recover a trashed superblock and
block group descriptors, which is why code that is resizing the file
system, or updating the label or the UUID need to update the backup
superblocks and/or backup block group descriptors so that we can
better recover disaster.

So I'd just suggest changing the comment above to "array of bh's for
the block group descriptors".

Cheers,

							- Ted

> Besides, I plan to go through the resize code again in fulture and
> add some comments to make it easy for anyone starting read this
> or make it easy to maintain. Please let me if you disklike it.

P.S.

BTW, a useful test program to add is one that checks to make sure that
the "static" parts of the superblock and block group descriptors
(i.e., the parts that don't get changed under normal operation while
the file system is mounted when the kernel *isn't* trying to do a
resize or change the label, UUID, or in the future, the new ioctl's to
update the parts of the superblock that can get modified by tune2fs),
and to make sure that all of the backup superblock and block group
descriptors have gotten updated.

Some of the bugs that you found may have resulted in some of the
backup bg descriptors not getting updataed, which we wouldn't
necessarily notice unless we had a test program that explicitly
checked for them.

And truth to tell, the only backup superblock and block group
descriptor that actually gets used to recover the file system is the
first one, since that's the one e2fsck will fall back to
automatically.  An expert might try to use one of the other backup
block groups as a desperate measure, and there might be some automated
programs that might be smart enough to use the backup block groups
when trying to recover the location of the partition table when the
file system and partition table is very badly damaged --- so that's
one of the reasons why with sparse_super2, the number of backup block
group descriptors can be limited to (for example) one located in the
first block group, and one located in the very last block group.

Re: [PATCH 11/13] ext4: correct gdblock calculation in add_new_gdb_meta_bg to support non first group

[Theodore Ts'o]

On Fri, Aug 18, 2023 at 03:09:35PM +0800, Kemeng Shi wrote:
> > Is following comment looks good to you:
>
> When all reserved primary blocks are consumed, we create meta_bg group and
> allocate new primary block at first block or block after backup superblock
> (if exsiting) in first group of meta_bg group.
> This function is only called when first group of meta_bg is added.

Well, it's possible to create a file system where all of the block
group descriptors use meta_bg, and there are no "traditional" block
group descriptors.  And so what happens is if there is no available
space in the existing block group descriptors for the new block group,
and there are no reserved block group descriptors (I'd remove
"primary" as that's not something that we've used traditionally), then
what happens is that the meta_bg feature will get enabled, and
es->s_first_meta_bg will get set to the first block group that is
managed using meta_bg.  s_first_meta_bg must be a multiple of
EXT4_DESC_PER_BLOCK(sb).

Some of this is documented in Documentation/filesystems/ext4/blockgroup.rst
already.

Cheers,

						- Ted

Re: [PATCH 12/13] ext4: remove unnecessary check for avoiding multiple update_backups in ext4_flex_group_add

[Kemeng Shi]

on 8/19/2023 12:00 AM, Theodore Ts'o wrote:
> On Fri, Aug 18, 2023 at 04:42:31PM +0800, Kemeng Shi wrote:
>>> s_group_desc[] is initialized in ext4_group_desc_init() in
>>> fs/ext4/super.c, and it is used in fs/ext4/balloc.c, and of course, it
>>> is defined in fs/ext4.h.  
>> I plan to add comment in fs/ext4.h as following:
>> struct ext4_sb_info {
>> 	...
>> 	struct buffer_head * __rcu *s_group_desc; /* Primary gdb blocks of online groups */
>> But I'm not sure it's proper now as you menthioned s_group_desc[] is to
>> keep the buffer_heads for the block group descriptor blocks in memory
>> and it contains primary gdb block is a coincidence that we only modify
>> primary block in kernel.
> 
> In general, the terminology that ext4 developers have used is "block
> group descriptors" and "backup block group descriptors".  The kernel
> never *uses* the backup block group users; and with the sparse_super2
> feature, the "backup superblocks" and "backup block group descriptors"
> are optional.
> 
Sure.
> They are used by e2fsck if we need to recover a trashed superblock and
> block group descriptors, which is why code that is resizing the file
> system, or updating the label or the UUID need to update the backup
> superblocks and/or backup block group descriptors so that we can
> better recover disaster.
OK, kernel updates backup blocks and userspace tool recovers from backup
in case that blocks used by kernel is corrupted.
> 
> So I'd just suggest changing the comment above to "array of bh's for
> the block group descriptors".
> 
Sure, I will do this in next version.

Besides, what about the check this patch tries to remove. Would you
prefer to keep it or remove it with better git description. Both
ways are acceptable to me. Thanks.

> Cheers,
> 
> 							- Ted
> 
>> Besides, I plan to go through the resize code again in fulture and
>> add some comments to make it easy for anyone starting read this
>> or make it easy to maintain. Please let me if you disklike it.
> 
> P.S.
> 
> BTW, a useful test program to add is one that checks to make sure that
> the "static" parts of the superblock and block group descriptors
> (i.e., the parts that don't get changed under normal operation while
> the file system is mounted when the kernel *isn't* trying to do a
> resize or change the label, UUID, or in the future, the new ioctl's to
> update the parts of the superblock that can get modified by tune2fs),
> and to make sure that all of the backup superblock and block group
> descriptors have gotten updated>
> Some of the bugs that you found may have resulted in some of the
> backup bg descriptors not getting updataed, which we wouldn't
> necessarily notice unless we had a test program that explicitly
> checked for them.
> 
> And truth to tell, the only backup superblock and block group
> descriptor that actually gets used to recover the file system is the
> first one, since that's the one e2fsck will fall back to
> automatically.  An expert might try to use one of the other backup
> block groups as a desperate measure, and there might be some automated
> programs that might be smart enough to use the backup block groups
> when trying to recover the location of the partition table when the
> file system and partition table is very badly damaged --- so that's
> one of the reasons why with sparse_super2, the number of backup block
> group descriptors can be limited to (for example) one located in the
> first block group, and one located in the very last block group.
> 
> Thanks for let me know and I do have no knowldge about how backup is used
in usersapce.

Re: [PATCH 11/13] ext4: correct gdblock calculation in add_new_gdb_meta_bg to support non first group

[Kemeng Shi]

on 8/19/2023 12:54 AM, Theodore Ts'o wrote:
> On Fri, Aug 18, 2023 at 03:09:35PM +0800, Kemeng Shi wrote:
>>> Is following comment looks good to you:
>>
>> When all reserved primary blocks are consumed, we create meta_bg group and
>> allocate new primary block at first block or block after backup superblock
>> (if exsiting) in first group of meta_bg group.
>> This function is only called when first group of meta_bg is added.
> 
> Well, it's possible to create a file system where all of the block
> group descriptors use meta_bg, and there are no "traditional" block
> group descriptors.  And so what happens is if there is no available
> space in the existing block group descriptors for the new block group,
> and there are no reserved block group descriptors (I'd remove
> "primary" as that's not something that we've used traditionally), then
> what happens is that the meta_bg feature will get enabled, and 
> es->s_first_meta_bg will get set to the first block group that is
> managed using meta_bg.  s_first_meta_bg must be a multiple of
> EXT4_DESC_PER_BLOCK(sb).
> 
> Some of this is documented in Documentation/filesystems/ext4/blockgroup.rst
> already.
>
As these information into comment of add_new_gdb_meta_bg could help to some
dgree. I summary the information to:

If there is no available space in the existing block group descriptors for
the new block group and there are no reserved block group descriptors, then
the meta_bg feature will get enabled, and es->s_first_meta_bg will get set
to the first block group that is managed using meta_bg and s_first_meta_bg
must be a multiple of EXT4_DESC_PER_BLOCK(sb).
This function will be called when first group of meta_bg is added to bring
new group descriptors block of new added meta_bg.

Or I will leave comments unchange in next version if it's redundant to you.
Thanks!
> Cheers,
> 
> 						- Ted
>