Message ID | 1313569222-5079-1-git-send-email-tm@tao.ma |
---|---|
State | Rejected, archived |
Headers | show |
On 2011-08-17, at 2:20 AM, Tao Ma <tm@tao.ma> wrote: > In 85fe4025, Christoph removed get_next_ino() from new_inode() and > do s_buddy_cache->i_ino = get_next_ino(); > And then 48e6061b gives EXT4_BAD_INO to this inode. But actually > s_buddy_cache is used directly and we never get it from an inode number. > So it should be safe for us to not set i_ino at all and I guess that > is the case Christoph described in his commit log of 85fe4025 > "For a few more filesystems we can avoid assigning any inode number given > that they aren't user visible". I don't see that setting i_ino is harmful here (only done once per filesystem mount) and the comment clearly indicates that it is for precautionary reasons only. If this inode happens to appear in a crashdump for some reason, I'd prefer it has an inode number that can be identified easily rather than some random value left over from a previously allocated and freed inode, because inode_init_always() does not zero out i_ino. > Cc: Yu Jian <yujian@whamcloud.com> > Cc: Andreas Dilger <adilger@whamcloud.com> > Cc: "Theodore Ts'o" <tytso@mit.edu> > Signed-off-by: Tao Ma <boyu.mt@taobao.com> > --- > fs/ext4/mballoc.c | 5 ----- > 1 files changed, 0 insertions(+), 5 deletions(-) > > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c > index 17a5a57..982f783 100644 > --- a/fs/ext4/mballoc.c > +++ b/fs/ext4/mballoc.c > @@ -2342,11 +2342,6 @@ static int ext4_mb_init_backend(struct super_block *sb) > ext4_msg(sb, KERN_ERR, "can't get new inode"); > goto err_freesgi; > } > - /* To avoid potentially colliding with an valid on-disk inode number, > - * use EXT4_BAD_INO for the buddy cache inode number. This inode is > - * not in the inode hash, so it should never be found by iget(), but > - * this will avoid confusion if it ever shows up during debugging. */ > - sbi->s_buddy_cache->i_ino = EXT4_BAD_INO; > EXT4_I(sbi->s_buddy_cache)->i_disksize = 0; > for (i = 0; i < ngroups; i++) { > desc = ext4_get_group_desc(sb, i, NULL); > -- > 1.7.0.4 > > -- > 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 08/17/2011 11:31 PM, Andreas Dilger wrote: > On 2011-08-17, at 2:20 AM, Tao Ma <tm@tao.ma> wrote: >> In 85fe4025, Christoph removed get_next_ino() from new_inode() and >> do s_buddy_cache->i_ino = get_next_ino(); >> And then 48e6061b gives EXT4_BAD_INO to this inode. But actually >> s_buddy_cache is used directly and we never get it from an inode number. >> So it should be safe for us to not set i_ino at all and I guess that >> is the case Christoph described in his commit log of 85fe4025 >> "For a few more filesystems we can avoid assigning any inode number given >> that they aren't user visible". > > I don't see that setting i_ino is harmful here (only done once per filesystem mount) and the comment clearly indicates that it is for precautionary reasons only. > > If this inode happens to appear in a crashdump for some reason, I'd prefer it has an inode number that can be identified easily rather than some random value left over from a previously allocated and freed inode, because inode_init_always() does not zero out i_ino. Fair enough. Thanks for the explanation. Thanks Tao > >> Cc: Yu Jian <yujian@whamcloud.com> >> Cc: Andreas Dilger <adilger@whamcloud.com> >> Cc: "Theodore Ts'o" <tytso@mit.edu> >> Signed-off-by: Tao Ma <boyu.mt@taobao.com> >> --- >> fs/ext4/mballoc.c | 5 ----- >> 1 files changed, 0 insertions(+), 5 deletions(-) >> >> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c >> index 17a5a57..982f783 100644 >> --- a/fs/ext4/mballoc.c >> +++ b/fs/ext4/mballoc.c >> @@ -2342,11 +2342,6 @@ static int ext4_mb_init_backend(struct super_block *sb) >> ext4_msg(sb, KERN_ERR, "can't get new inode"); >> goto err_freesgi; >> } >> - /* To avoid potentially colliding with an valid on-disk inode number, >> - * use EXT4_BAD_INO for the buddy cache inode number. This inode is >> - * not in the inode hash, so it should never be found by iget(), but >> - * this will avoid confusion if it ever shows up during debugging. */ >> - sbi->s_buddy_cache->i_ino = EXT4_BAD_INO; >> EXT4_I(sbi->s_buddy_cache)->i_disksize = 0; >> for (i = 0; i < ngroups; i++) { >> desc = ext4_get_group_desc(sb, i, NULL); >> -- >> 1.7.0.4 >> >> -- >> 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/fs/ext4/mballoc.c b/fs/ext4/mballoc.c index 17a5a57..982f783 100644 --- a/fs/ext4/mballoc.c +++ b/fs/ext4/mballoc.c @@ -2342,11 +2342,6 @@ static int ext4_mb_init_backend(struct super_block *sb) ext4_msg(sb, KERN_ERR, "can't get new inode"); goto err_freesgi; } - /* To avoid potentially colliding with an valid on-disk inode number, - * use EXT4_BAD_INO for the buddy cache inode number. This inode is - * not in the inode hash, so it should never be found by iget(), but - * this will avoid confusion if it ever shows up during debugging. */ - sbi->s_buddy_cache->i_ino = EXT4_BAD_INO; EXT4_I(sbi->s_buddy_cache)->i_disksize = 0; for (i = 0; i < ngroups; i++) { desc = ext4_get_group_desc(sb, i, NULL);