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.