| Message ID | 20081127045047.GF14101@mit.edu |
|---|---|
| State | Accepted, archived |
| Headers | show |
Hi Ted, Theodore Tso writes: > 2) You need to set the flag *before* you calculate the block group > checksum, not afterwards. Sorry about this. I forgot to do the quilt refresh (and check that the code I submit is the same that the code I run). > 1) You didn't include a Developer's Certification of Origin (i.e., a > "Signed-off-by" header). Since this is a one line patch, and it seems > pretty clear your intention is to submit this to Linus, This was really an RFC, as you also pointed out. Regarding this patch, the discussion raised the question of whether EXT4_BG_INODE_UNINIT or EXT4_BG_ITABLE_UNINIT would be more coherent than EXT4_BG_INODE_ZEROED wrt. EXT4_BG_INODE_UNINIT and EXT4_BG_BLOCK_UNINIT. This was also used as an example for the discussion about doing the initialization outside of an init thread (which turned up not to be a good idea). This is also the first use of EXT4_BG_INODE_ZEROED in the kernel, so an occasion to revisit the name. I did not look carefully in the progs (EXT2_BG_INODE_ZEROED) to see if it is desirable and easy to change it. cscope indicates that it may be easy (4 instances). > So the corrected patch should look like this.... Thank you, that's settled then,
On Thu, Nov 27, 2008 at 10:30:31AM +0100, Solofo.Ramangalahy@bull.net wrote: > This was really an RFC, as you also pointed out. > Regarding this patch, the discussion raised the question of whether > EXT4_BG_INODE_UNINIT or EXT4_BG_ITABLE_UNINIT would be more coherent > than EXT4_BG_INODE_ZEROED wrt. EXT4_BG_INODE_UNINIT and > EXT4_BG_BLOCK_UNINIT. EXT2_BG_ITABLE_UNINIT (or EXT2_BG_ITABLE_PARTIALLY_UNINIT, to be more correct) would have been better, yes. That way legacy filesystems that didn't enable uninit_bg would have bg_flags == 0, and we would know that inode table was properly initialized. Unfortunately we did it the other way, where EXT2_BG_INODE_ZEROED is set when the inode table is initialized, instead of the other way around. > This is also the first use of EXT4_BG_INODE_ZEROED in the kernel, so > an occasion to revisit the name. Unfortunately, we've been shipping mke2fs in e2fsprogs that sets the EXT4_BG_INODE_ZERO for newly created filesystem, and if the lazy_itable_init configuration parameter is set, it doesn't initialize the inode table and leaves bg_flags set to EXT2_BG_INODE_UNINIT and EXT2_BG_BLOCK_UNINIT. Distributions are already shipping e2fsprogs with this, and there are ext4 filesystems out there in the wild, so it is indeed probably way too late to change this. - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Nov 27, 2008 17:35 -0500, Theodore Ts'o wrote: > Unfortunately, we've been shipping mke2fs in e2fsprogs that sets the > EXT4_BG_INODE_ZERO for newly created filesystem, and if the > lazy_itable_init configuration parameter is set, it doesn't initialize > the inode table and leaves bg_flags set to EXT2_BG_INODE_UNINIT and > EXT2_BG_BLOCK_UNINIT. > > Distributions are already shipping e2fsprogs with this, and there are > ext4 filesystems out there in the wild, so it is indeed probably way > too late to change this. I suppose it would be possible to add a new flag and deprecate the old INODE_ZERO usage after a couple of years. Until we start doing anything with the kernel we've done everything with mke2fs to zero the inode table, so that would be a reasonable assumption for the kernel to make. I agree that having the flag with the opposite meaning would have been better, but hindsight is 20-20. Cheers, Andreas -- Andreas Dilger Sr. Staff Engineer, Lustre Group Sun Microsystems of Canada, Inc. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c index b6ec184..d448eb1 100644 --- a/fs/ext4/resize.c +++ b/fs/ext4/resize.c @@ -864,6 +864,7 @@ int ext4_group_add(struct super_block *sb, struct ext4_new_group_data *input) ext4_inode_table_set(sb, gdp, input->inode_table); /* LV FIXME */ gdp->bg_free_blocks_count = cpu_to_le16(input->free_blocks_count); gdp->bg_free_inodes_count = cpu_to_le16(EXT4_INODES_PER_GROUP(sb)); + gdp->bg_flags |= cpu_to_le16(EXT4_BG_INODE_ZEROED); gdp->bg_checksum = ext4_group_desc_csum(sbi, input->group, gdp); /*