| Message ID | 1415689169-19393-3-git-send-email-wangxg.fnst@cn.fujitsu.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, Nov 11, 2014 at 02:59:28PM +0800, Xiaoguang Wang wrote: > When we use tune2fs -I new_ino_size to change inode size, if everything is OK, > the corresponding ext4_group_desc.bg_free_blocks_count will be decreased, so > obviously, we need to re-compute the group descriptor checksums, fix this. If > not doing this, mount operation will fail. > > Meanwhile, the patch will trigger an existing memory write overflow, which will > casue segfault, please see the next patch. > > Signed-off-by: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com> > --- > misc/tune2fs.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/misc/tune2fs.c b/misc/tune2fs.c > index 065b483..91dc7c1 100644 > --- a/misc/tune2fs.c > +++ b/misc/tune2fs.c > @@ -2908,8 +2908,7 @@ retry_open: > EXT4_FEATURE_RO_COMPAT_METADATA_CSUM)) > rewrite_checksums = 1; > } > - if (rewrite_checksums) > - rewrite_metadata_checksums(fs); > + > if (I_flag) { > if (mount_flags & EXT2_MF_MOUNTED) { > fputs(_("The inode size may only be " > @@ -2935,6 +2934,7 @@ retry_open: > if (resize_inode(fs, new_inode_size) == 0) { > printf(_("Setting inode size %lu\n"), > new_inode_size); > + rewrite_checksums = 1; rewrite_metadata_checksums() was designed to rewrite checksums on every metadata object in the filesystem (extents, ACLs, directory blocks), which I think does more work than necessary for your use case (if I'm reading this correctly) of simply rewriting the block group descriptor checksums. Would it suffice to call ext2fs_group_desc_csum_set() when setting bg_free_blocks_count? I think this could be done in ext2fs_calculate_summary_stats(). --D > } else { > printf("%s", _("Failed to change inode size\n")); > rc = 1; > @@ -2942,6 +2942,9 @@ retry_open: > } > } > > + if (rewrite_checksums) > + rewrite_metadata_checksums(fs); > + > if (l_flag) > list_super(sb); > if (stride_set) { > -- > 1.8.2.1 > > -- > 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 -- 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
Hi, On 11/11/2014 04:33 PM, Darrick J. Wong wrote: > On Tue, Nov 11, 2014 at 02:59:28PM +0800, Xiaoguang Wang wrote: >> When we use tune2fs -I new_ino_size to change inode size, if everything is OK, >> the corresponding ext4_group_desc.bg_free_blocks_count will be decreased, so >> obviously, we need to re-compute the group descriptor checksums, fix this. If >> not doing this, mount operation will fail. >> >> Meanwhile, the patch will trigger an existing memory write overflow, which will >> casue segfault, please see the next patch. >> >> Signed-off-by: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com> >> --- >> misc/tune2fs.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/misc/tune2fs.c b/misc/tune2fs.c >> index 065b483..91dc7c1 100644 >> --- a/misc/tune2fs.c >> +++ b/misc/tune2fs.c >> @@ -2908,8 +2908,7 @@ retry_open: >> EXT4_FEATURE_RO_COMPAT_METADATA_CSUM)) >> rewrite_checksums = 1; >> } >> - if (rewrite_checksums) >> - rewrite_metadata_checksums(fs); >> + >> if (I_flag) { >> if (mount_flags & EXT2_MF_MOUNTED) { >> fputs(_("The inode size may only be " >> @@ -2935,6 +2934,7 @@ retry_open: >> if (resize_inode(fs, new_inode_size) == 0) { >> printf(_("Setting inode size %lu\n"), >> new_inode_size); >> + rewrite_checksums = 1; > > rewrite_metadata_checksums() was designed to rewrite checksums on every > metadata object in the filesystem (extents, ACLs, directory blocks), which I > think does more work than necessary for your use case (if I'm reading this > correctly) of simply rewriting the block group descriptor checksums. Would it > suffice to call ext2fs_group_desc_csum_set() when setting bg_free_blocks_count? > I think this could be done in ext2fs_calculate_summary_stats(). For a new created file system, I think ext2fs_group_desc_csum_set() would be sufficient, because the needed blocks(used for new inode table, they must be continuous) will be free. But for a file system, if it already has many directories and files, these blocks may be in use, then we need to replace these blocks with other blocks, so the ACLs and extent tree may also be modified, see inode_scan_and_fix(), so I think here we need a rewrite_metadata_checksums(), in other words, it's the safest :) I'm still new to ext4(learning the code now), so I may also miss something, thanks! Regards, Xiaoguang Wang > > --D > >> } else { >> printf("%s", _("Failed to change inode size\n")); >> rc = 1; >> @@ -2942,6 +2942,9 @@ retry_open: >> } >> } >> >> + if (rewrite_checksums) >> + rewrite_metadata_checksums(fs); >> + >> if (l_flag) >> list_super(sb); >> if (stride_set) { >> -- >> 1.8.2.1 >> >> -- >> 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 > . > -- 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 Tue, Nov 11, 2014 at 05:08:42PM +0800, Xiaoguang Wang wrote: > Hi, > > On 11/11/2014 04:33 PM, Darrick J. Wong wrote: > > On Tue, Nov 11, 2014 at 02:59:28PM +0800, Xiaoguang Wang wrote: > >> When we use tune2fs -I new_ino_size to change inode size, if everything is OK, > >> the corresponding ext4_group_desc.bg_free_blocks_count will be decreased, so > >> obviously, we need to re-compute the group descriptor checksums, fix this. If > >> not doing this, mount operation will fail. > >> > >> Meanwhile, the patch will trigger an existing memory write overflow, which will > >> casue segfault, please see the next patch. > >> > >> Signed-off-by: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com> > >> --- > >> misc/tune2fs.c | 7 +++++-- > >> 1 file changed, 5 insertions(+), 2 deletions(-) > >> > >> diff --git a/misc/tune2fs.c b/misc/tune2fs.c > >> index 065b483..91dc7c1 100644 > >> --- a/misc/tune2fs.c > >> +++ b/misc/tune2fs.c > >> @@ -2908,8 +2908,7 @@ retry_open: > >> EXT4_FEATURE_RO_COMPAT_METADATA_CSUM)) > >> rewrite_checksums = 1; > >> } > >> - if (rewrite_checksums) > >> - rewrite_metadata_checksums(fs); > >> + > >> if (I_flag) { > >> if (mount_flags & EXT2_MF_MOUNTED) { > >> fputs(_("The inode size may only be " > >> @@ -2935,6 +2934,7 @@ retry_open: > >> if (resize_inode(fs, new_inode_size) == 0) { > >> printf(_("Setting inode size %lu\n"), > >> new_inode_size); > >> + rewrite_checksums = 1; > > > > rewrite_metadata_checksums() was designed to rewrite checksums on every > > metadata object in the filesystem (extents, ACLs, directory blocks), which > > I think does more work than necessary for your use case (if I'm reading > > this correctly) of simply rewriting the block group descriptor checksums. > > Would it suffice to call ext2fs_group_desc_csum_set() when setting > > bg_free_blocks_count? I think this could be done in > > ext2fs_calculate_summary_stats(). > > For a new created file system, I think ext2fs_group_desc_csum_set() would be > sufficient, because the needed blocks(used for new inode table, they must be > continuous) will be free. But for a file system, if it already has many > directories and files, these blocks may be in use, then we need to replace > these blocks with other blocks, so the ACLs and extent tree may also be > modified, see inode_scan_and_fix(), so I think here we need a > rewrite_metadata_checksums(), in other words, it's the safest :) I'm still > new to ext4(learning the code now), so I may also miss something, thanks! The checksums for extent blocks, ACLs, and directories don't care about the LBA of the block, which means that they can be moved around the FS without having to rewrite the checksum. I still think you can get away with just: for (dgrp_t i = 0; i < fs->group_desc_count; i++) ext2fs_group_desc_csum_set(fs, i); to fix your problem. That said, do you have a test case you could send me? I'm having trouble reproducing the symptoms on the -next branch. 1.42.12 can reproduce it reliably... but the funny thing is that 1.42.* doesn't know about metadata_csum at all. --D > > Regards, > Xiaoguang Wang > > > > > > > --D > > > >> } else { > >> printf("%s", _("Failed to change inode size\n")); > >> rc = 1; > >> @@ -2942,6 +2942,9 @@ retry_open: > >> } > >> } > >> > >> + if (rewrite_checksums) > >> + rewrite_metadata_checksums(fs); > >> + > >> if (l_flag) > >> list_super(sb); > >> if (stride_set) { > >> -- > >> 1.8.2.1 > >> > >> -- > >> 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 > > . > > > > -- > 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 -- 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/misc/tune2fs.c b/misc/tune2fs.c index 065b483..91dc7c1 100644 --- a/misc/tune2fs.c +++ b/misc/tune2fs.c @@ -2908,8 +2908,7 @@ retry_open: EXT4_FEATURE_RO_COMPAT_METADATA_CSUM)) rewrite_checksums = 1; } - if (rewrite_checksums) - rewrite_metadata_checksums(fs); + if (I_flag) { if (mount_flags & EXT2_MF_MOUNTED) { fputs(_("The inode size may only be " @@ -2935,6 +2934,7 @@ retry_open: if (resize_inode(fs, new_inode_size) == 0) { printf(_("Setting inode size %lu\n"), new_inode_size); + rewrite_checksums = 1; } else { printf("%s", _("Failed to change inode size\n")); rc = 1; @@ -2942,6 +2942,9 @@ retry_open: } } + if (rewrite_checksums) + rewrite_metadata_checksums(fs); + if (l_flag) list_super(sb); if (stride_set) {
When we use tune2fs -I new_ino_size to change inode size, if everything is OK, the corresponding ext4_group_desc.bg_free_blocks_count will be decreased, so obviously, we need to re-compute the group descriptor checksums, fix this. If not doing this, mount operation will fail. Meanwhile, the patch will trigger an existing memory write overflow, which will casue segfault, please see the next patch. Signed-off-by: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com> --- misc/tune2fs.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)