| Message ID | 20230629120044.1261968-12-shikemeng@huaweicloud.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fixes and cleanups to ext4 resize | expand |
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
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 >
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
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!
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
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 >
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
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 >
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);
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(-)