Remarks regarding sparse_super2 feature

Hello,

I was looking to sparse_super2 feature and there is some points I do not understand in the way it is handled on fs initialisation and resize.

In ext2fs_initialize (initialize.c), the backup superblocks field is initialised at line 437, but they were used previously when checking for overhead (at line 407) when the second value is still ~0. Could not this lead to wrong overhead computation in some cases?
This is certainly very unlikely because of the 50 margin taken on this overhead. As this is some chicken/egg problem, solution is not obvious. A way is perhaps to have ext2fs_bg_has_super accepting ~0 as a group always having a backup superblock (unless extending the number of groups to 64 bits, if such a number of groups is reached, it will obviously be the latest so be the candidate for the backup bg).

In case there is some other solution, there is same kind of problem in adjust_fs_info of resize2fs.c (line 724 check for backup super block and line 839 updates the value).

Concerning adjust_fs_info I do not understand the logic of some tests concerning update of these values:
at line 844:

			if (old_fs->group_desc_count == 1)
				fs->super->s_backup_bgs[0] = 1;
			if (old_fs->group_desc_count == 1 &&
			    fs->super->s_backup_bgs[0])
				fs->super->s_backup_bgs[0] = last_bg;
			else if (fs->super->s_backup_bgs[1])
				fs->super->s_backup_bgs[1] = last_bg;

couldn't the 2 first "if" be collapsed: if first one is true, it leads to second one be true and if first one is false, second is also false. Or perhaps it means it is the wrong value that is checked or initialised in second if?

For the other case (shrinking the fs) at line 856:

			if (last_bg > 1 &&
			    old_fs->super->s_backup_bgs[1] == old_last_bg)
				fs->super->s_backup_bgs[1] = last_bg;

what ensures the location where the new super block backup will be set is a free block?

Regards,

Damien

Good point. You're right that in most cases the margin should hide the problem, but to be correct, I've changed this to be:

	has_bg = 0;
	if (ext2fs_has_feature_sparse_super2(super)) {
		/*
		 * We have to do this manually since
		 * super->s_backup_bgs hasn't been set up yet.
		 */
		if (fs->group_desc_count == 2)
			has_bg = param->s_backup_bgs[0] != 0;
		else
			has_bg = param->s_backup_bgs[1] != 0;
	} else
		has_bg = ext2fs_bg_has_super(fs, fs->group_desc_count - 1);
	if (has_bg)
		overhead += 1 + fs->desc_blocks + super->s_reserved_gdt_blocks;

You're right, the resize2fs handling for sparse_super2 was pretty badly broken.

Thanks for asking these questions. I found some rotten code when I started looking. I'll make sure these get fixed before e2fsprogs 1.43 is released. (It looks like the worst of the bugs only lead to the summary block group statistics being screwed up, and in some cases, backup block group descriptors not getting established when growing the file system, so the while the code was buggy, the impact of the bugs was relatively small.)

This is handled by reserve_sparse2_last_group(). We have to deal this sort of thing whenever we need to do things like grow the group descriptors, and we will relocate data blocks as necessary to make room for blocks that have to be at specific locations. (This means we have allocate new blocks for the blocks we are moving, copy the data blocks, and then update the inode(s) to point the new block locations.)

Cheers,

- Ted

[...]

Hello,

Thanks for the feedback.
For the last point, you're right, I did not look deep enough in the remaining part of the code (and it looks obvious that when shrinking the fs, there is some need to reallocate blocks).

Regards,

Damien