* [PATCH] ext4: Fix missing iput for root inode in case of all failed mount paths. @ 2011-01-16 7:30 Manish Katiyar 2011-01-16 16:20 ` Andreas Dilger 0 siblings, 1 reply; 11+ messages in thread From: Manish Katiyar @ 2011-01-16 7:30 UTC (permalink / raw) To: Theodore Ts'o, ext4; +Cc: mkatiyar Fix missing iput for root inode in case of all failed mount paths. Fixes bug#26752 Signed-off-by: Manish Katiyar <mkatiyar@gmail.com> --- fs/ext4/super.c | 8 +++++++- 1 files changed, 7 insertions(+), 1 deletions(-) diff --git a/fs/ext4/super.c b/fs/ext4/super.c index cb10a06..9570fcc 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -3587,6 +3587,7 @@ no_journal: if (err) { ext4_msg(sb, KERN_ERR, "failed to initialize system " "zone (%d)", err); + iput(root); goto failed_mount4; } @@ -3595,12 +3596,15 @@ no_journal: if (err) { ext4_msg(sb, KERN_ERR, "failed to initialize mballoc (%d)", err); + iput(root); goto failed_mount4; } err = ext4_register_li_request(sb, first_not_zeroed); - if (err) + if (err) { + iput(root); goto failed_mount4; + } sbi->s_kobj.kset = ext4_kset; init_completion(&sbi->s_kobj_unregister); @@ -3609,6 +3613,7 @@ no_journal: if (err) { ext4_mb_release(sb); ext4_ext_release(sb); + iput(root); goto failed_mount4; }; @@ -3648,6 +3653,7 @@ cantfind_ext4: goto failed_mount; failed_mount4: + sb->s_root = NULL; ext4_msg(sb, KERN_ERR, "mount failed"); destroy_workqueue(EXT4_SB(sb)->dio_unwritten_wq); failed_mount_wq: -- 1.7.1 -- Thanks - Manish ================================== [$\*.^ -- I miss being one of them ================================== ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] ext4: Fix missing iput for root inode in case of all failed mount paths. 2011-01-16 7:30 [PATCH] ext4: Fix missing iput for root inode in case of all failed mount paths Manish Katiyar @ 2011-01-16 16:20 ` Andreas Dilger 2011-01-16 18:05 ` Manish Katiyar 0 siblings, 1 reply; 11+ messages in thread From: Andreas Dilger @ 2011-01-16 16:20 UTC (permalink / raw) To: Manish Katiyar; +Cc: Theodore Ts'o, ext4, mkatiyar@gmail.com Why not just put the iput() at failed_mount4() instead of spread around the code? Cheers, Andreas On 2011-01-16, at 0:30, Manish Katiyar <mkatiyar@gmail.com> wrote: > Fix missing iput for root inode in case of all failed mount paths. > Fixes bug#26752 > > Signed-off-by: Manish Katiyar <mkatiyar@gmail.com> > > --- > fs/ext4/super.c | 8 +++++++- > 1 files changed, 7 insertions(+), 1 deletions(-) > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index cb10a06..9570fcc 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -3587,6 +3587,7 @@ no_journal: > if (err) { > ext4_msg(sb, KERN_ERR, "failed to initialize system " > "zone (%d)", err); > + iput(root); > goto failed_mount4; > } > > @@ -3595,12 +3596,15 @@ no_journal: > if (err) { > ext4_msg(sb, KERN_ERR, "failed to initialize mballoc (%d)", > err); > + iput(root); > goto failed_mount4; > } > > err = ext4_register_li_request(sb, first_not_zeroed); > - if (err) > + if (err) { > + iput(root); > goto failed_mount4; > + } > > sbi->s_kobj.kset = ext4_kset; > init_completion(&sbi->s_kobj_unregister); > @@ -3609,6 +3613,7 @@ no_journal: > if (err) { > ext4_mb_release(sb); > ext4_ext_release(sb); > + iput(root); > goto failed_mount4; > }; > > @@ -3648,6 +3653,7 @@ cantfind_ext4: > goto failed_mount; > > failed_mount4: > + sb->s_root = NULL; > ext4_msg(sb, KERN_ERR, "mount failed"); > destroy_workqueue(EXT4_SB(sb)->dio_unwritten_wq); > failed_mount_wq: > -- > 1.7.1 > > > -- > Thanks - > Manish > ================================== > [$\*.^ -- I miss being one of them > ================================== > -- > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ext4: Fix missing iput for root inode in case of all failed mount paths. 2011-01-16 16:20 ` Andreas Dilger @ 2011-01-16 18:05 ` Manish Katiyar 2011-01-30 5:40 ` Manish Katiyar 2011-07-06 22:54 ` Andreas Dilger 0 siblings, 2 replies; 11+ messages in thread From: Manish Katiyar @ 2011-01-16 18:05 UTC (permalink / raw) To: Andreas Dilger; +Cc: Theodore Ts'o, ext4, mkatiyar On Sun, Jan 16, 2011 at 8:20 AM, Andreas Dilger <adilger@dilger.ca> wrote: > Why not just put the iput() at failed_mount4() instead of spread around the code? Thanks Andreas, Here is the updated patch. Signed-off-by: Manish Katiyar <mkatiyar@gmail.com> --- fs/ext4/super.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/fs/ext4/super.c b/fs/ext4/super.c index cb10a06..8fa53e9 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -3522,17 +3522,16 @@ no_journal: if (IS_ERR(root)) { ext4_msg(sb, KERN_ERR, "get root inode failed"); ret = PTR_ERR(root); + root = NULL; goto failed_mount4; } if (!S_ISDIR(root->i_mode) || !root->i_blocks || !root->i_size) { - iput(root); ext4_msg(sb, KERN_ERR, "corrupt root inode, run e2fsck"); goto failed_mount4; } sb->s_root = d_alloc_root(root); if (!sb->s_root) { ext4_msg(sb, KERN_ERR, "get root dentry failed"); - iput(root); ret = -ENOMEM; goto failed_mount4; } @@ -3648,6 +3647,8 @@ cantfind_ext4: goto failed_mount; failed_mount4: + iput(root); + sb->s_root = NULL; ext4_msg(sb, KERN_ERR, "mount failed"); destroy_workqueue(EXT4_SB(sb)->dio_unwritten_wq); failed_mount_wq: -- 1.7.1 > > Cheers, Andreas > > On 2011-01-16, at 0:30, Manish Katiyar <mkatiyar@gmail.com> wrote: > >> Fix missing iput for root inode in case of all failed mount paths. >> Fixes bug#26752 >> >> Signed-off-by: Manish Katiyar <mkatiyar@gmail.com> >> >> --- >> fs/ext4/super.c | 8 +++++++- >> 1 files changed, 7 insertions(+), 1 deletions(-) >> >> diff --git a/fs/ext4/super.c b/fs/ext4/super.c >> index cb10a06..9570fcc 100644 >> --- a/fs/ext4/super.c >> +++ b/fs/ext4/super.c >> @@ -3587,6 +3587,7 @@ no_journal: >> if (err) { >> ext4_msg(sb, KERN_ERR, "failed to initialize system " >> "zone (%d)", err); >> + iput(root); >> goto failed_mount4; >> } >> >> @@ -3595,12 +3596,15 @@ no_journal: >> if (err) { >> ext4_msg(sb, KERN_ERR, "failed to initialize mballoc (%d)", >> err); >> + iput(root); >> goto failed_mount4; >> } >> >> err = ext4_register_li_request(sb, first_not_zeroed); >> - if (err) >> + if (err) { >> + iput(root); >> goto failed_mount4; >> + } >> >> sbi->s_kobj.kset = ext4_kset; >> init_completion(&sbi->s_kobj_unregister); >> @@ -3609,6 +3613,7 @@ no_journal: >> if (err) { >> ext4_mb_release(sb); >> ext4_ext_release(sb); >> + iput(root); >> goto failed_mount4; >> }; >> >> @@ -3648,6 +3653,7 @@ cantfind_ext4: >> goto failed_mount; >> >> failed_mount4: >> + sb->s_root = NULL; >> ext4_msg(sb, KERN_ERR, "mount failed"); >> destroy_workqueue(EXT4_SB(sb)->dio_unwritten_wq); >> failed_mount_wq: >> -- >> 1.7.1 >> >> >> -- >> Thanks - >> Manish >> ================================== >> [$\*.^ -- I miss being one of them >> ================================== >> -- >> 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 > -- Thanks - Manish ================================== [$\*.^ -- I miss being one of them ================================== -- 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] ext4: Fix missing iput for root inode in case of all failed mount paths. 2011-01-16 18:05 ` Manish Katiyar @ 2011-01-30 5:40 ` Manish Katiyar 2011-02-07 23:23 ` Ted Ts'o 2011-07-06 22:54 ` Andreas Dilger 1 sibling, 1 reply; 11+ messages in thread From: Manish Katiyar @ 2011-01-30 5:40 UTC (permalink / raw) To: Andreas Dilger; +Cc: Theodore Ts'o, ext4, mkatiyar On Sun, Jan 16, 2011 at 10:05 AM, Manish Katiyar <mkatiyar@gmail.com> wrote: > On Sun, Jan 16, 2011 at 8:20 AM, Andreas Dilger <adilger@dilger.ca> wrote: >> Why not just put the iput() at failed_mount4() instead of spread around the code? > > Thanks Andreas, Here is the updated patch. > > Signed-off-by: Manish Katiyar <mkatiyar@gmail.com> > --- > fs/ext4/super.c | 5 +++-- > 1 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index cb10a06..8fa53e9 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -3522,17 +3522,16 @@ no_journal: > if (IS_ERR(root)) { > ext4_msg(sb, KERN_ERR, "get root inode failed"); > ret = PTR_ERR(root); > + root = NULL; > goto failed_mount4; > } > if (!S_ISDIR(root->i_mode) || !root->i_blocks || !root->i_size) { > - iput(root); > ext4_msg(sb, KERN_ERR, "corrupt root inode, run e2fsck"); > goto failed_mount4; > } > sb->s_root = d_alloc_root(root); > if (!sb->s_root) { > ext4_msg(sb, KERN_ERR, "get root dentry failed"); > - iput(root); > ret = -ENOMEM; > goto failed_mount4; > } > @@ -3648,6 +3647,8 @@ cantfind_ext4: > goto failed_mount; > > failed_mount4: > + iput(root); > + sb->s_root = NULL; > ext4_msg(sb, KERN_ERR, "mount failed"); > destroy_workqueue(EXT4_SB(sb)->dio_unwritten_wq); > failed_mount_wq: > -- > 1.7.1 > > > >> >> Cheers, Andreas >> >> On 2011-01-16, at 0:30, Manish Katiyar <mkatiyar@gmail.com> wrote: >> >>> Fix missing iput for root inode in case of all failed mount paths. >>> Fixes bug#26752 >>> >>> Signed-off-by: Manish Katiyar <mkatiyar@gmail.com> >>> >>> --- >>> fs/ext4/super.c | 8 +++++++- >>> 1 files changed, 7 insertions(+), 1 deletions(-) >>> >>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c >>> index cb10a06..9570fcc 100644 >>> --- a/fs/ext4/super.c >>> +++ b/fs/ext4/super.c >>> @@ -3587,6 +3587,7 @@ no_journal: >>> if (err) { >>> ext4_msg(sb, KERN_ERR, "failed to initialize system " >>> "zone (%d)", err); >>> + iput(root); >>> goto failed_mount4; >>> } >>> >>> @@ -3595,12 +3596,15 @@ no_journal: >>> if (err) { >>> ext4_msg(sb, KERN_ERR, "failed to initialize mballoc (%d)", >>> err); >>> + iput(root); >>> goto failed_mount4; >>> } >>> >>> err = ext4_register_li_request(sb, first_not_zeroed); >>> - if (err) >>> + if (err) { >>> + iput(root); >>> goto failed_mount4; >>> + } >>> >>> sbi->s_kobj.kset = ext4_kset; >>> init_completion(&sbi->s_kobj_unregister); >>> @@ -3609,6 +3613,7 @@ no_journal: >>> if (err) { >>> ext4_mb_release(sb); >>> ext4_ext_release(sb); >>> + iput(root); >>> goto failed_mount4; >>> }; >>> >>> @@ -3648,6 +3653,7 @@ cantfind_ext4: >>> goto failed_mount; >>> >>> failed_mount4: >>> + sb->s_root = NULL; >>> ext4_msg(sb, KERN_ERR, "mount failed"); >>> destroy_workqueue(EXT4_SB(sb)->dio_unwritten_wq); >>> failed_mount_wq: Hi Ted, Any feedback on this ? -- Thanks - Manish -- 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ext4: Fix missing iput for root inode in case of all failed mount paths. 2011-01-30 5:40 ` Manish Katiyar @ 2011-02-07 23:23 ` Ted Ts'o 2011-02-07 23:31 ` Manish Katiyar 0 siblings, 1 reply; 11+ messages in thread From: Ted Ts'o @ 2011-02-07 23:23 UTC (permalink / raw) To: Manish Katiyar; +Cc: Andreas Dilger, ext4 On Sat, Jan 29, 2011 at 09:40:34PM -0800, Manish Katiyar wrote: > > Any feedback on this ? The patch looks good, but it doesn't fix bug #26752. The root cause of that bug is the fact that ext4_sync_fs() and ext4_put_super() assumes that sbi has been initialized. - Ted ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ext4: Fix missing iput for root inode in case of all failed mount paths. 2011-02-07 23:23 ` Ted Ts'o @ 2011-02-07 23:31 ` Manish Katiyar 2011-02-26 21:35 ` Manish Katiyar 0 siblings, 1 reply; 11+ messages in thread From: Manish Katiyar @ 2011-02-07 23:31 UTC (permalink / raw) To: Ted Ts'o; +Cc: Andreas Dilger, ext4 On Mon, Feb 7, 2011 at 3:23 PM, Ted Ts'o <tytso@mit.edu> wrote: > On Sat, Jan 29, 2011 at 09:40:34PM -0800, Manish Katiyar wrote: >> >> Any feedback on this ? > > The patch looks good, but it doesn't fix bug #26752. > > The root cause of that bug is the fact that ext4_sync_fs() and > ext4_put_super() assumes that sbi has been initialized. Hmm.. Actually I was looking at generic_super_shutdown() which I thought would call ext4_put_super() only if sb->root is set, and since I was setting sb->root explicitly to NULL in my patch in case of failures. -- Thanks - Manish ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ext4: Fix missing iput for root inode in case of all failed mount paths. 2011-02-07 23:31 ` Manish Katiyar @ 2011-02-26 21:35 ` Manish Katiyar 2011-02-28 1:45 ` Ted Ts'o 0 siblings, 1 reply; 11+ messages in thread From: Manish Katiyar @ 2011-02-26 21:35 UTC (permalink / raw) To: Ted Ts'o; +Cc: Andreas Dilger, ext4 On Mon, Feb 7, 2011 at 3:31 PM, Manish Katiyar <mkatiyar@gmail.com> wrote: > On Mon, Feb 7, 2011 at 3:23 PM, Ted Ts'o <tytso@mit.edu> wrote: >> On Sat, Jan 29, 2011 at 09:40:34PM -0800, Manish Katiyar wrote: >>> >>> Any feedback on this ? >> >> The patch looks good, but it doesn't fix bug #26752. >> >> The root cause of that bug is the fact that ext4_sync_fs() and >> ext4_put_super() assumes that sbi has been initialized. Hi Ted, Is there anything else that I need to do to get this merged in your tree ? > > Hmm.. Actually I was looking at generic_super_shutdown() which I > thought would call ext4_put_super() only if sb->root is set, and since > I was setting sb->root explicitly to NULL in my patch in case of > failures. > > -- > Thanks - > Manish > -- Thanks - Manish ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ext4: Fix missing iput for root inode in case of all failed mount paths. 2011-02-26 21:35 ` Manish Katiyar @ 2011-02-28 1:45 ` Ted Ts'o 2011-02-28 4:19 ` Manish Katiyar 0 siblings, 1 reply; 11+ messages in thread From: Ted Ts'o @ 2011-02-28 1:45 UTC (permalink / raw) To: Manish Katiyar; +Cc: Andreas Dilger, ext4 On Sat, Feb 26, 2011 at 01:35:05PM -0800, Manish Katiyar wrote: > > Is there anything else that I need to do to get this merged in your tree ? > I've added the patch with the following explanation: ext4: fix missing iput of root inode for some mount error paths This assures that the root inode is not leaked, and that sb->s_root is NULL, which will prevent generic_shutdown_super() from doing extra work, including call sync_filesystem, which ultimately results in ext4_sync_fs() getting called with an uninitialized struct super, which is the cause of the crash noted in Kernel Bugzilla #26752. https://bugzilla.kernel.org/show_bug.cgi?id=26752 Signed-off-by: Manish Katiyar <mkatiyar@gmail.com> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> Sorry for the delay, but this patch was held up pending my tracingg through the code and understanding why this really fixed BZ #26752. In the future, adding a bit more detail in the commit log will help me process the patch faster, since I won't have to reproduce your analysis. Also, just quoting a BZ number without going into more detail risks my putting it the patch on my "to analyze --- when I have access to network" queue if I happen to come across it while on an airplane. :-) - Ted ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ext4: Fix missing iput for root inode in case of all failed mount paths. 2011-02-28 1:45 ` Ted Ts'o @ 2011-02-28 4:19 ` Manish Katiyar 0 siblings, 0 replies; 11+ messages in thread From: Manish Katiyar @ 2011-02-28 4:19 UTC (permalink / raw) To: Ted Ts'o; +Cc: Andreas Dilger, ext4 On Sun, Feb 27, 2011 at 5:45 PM, Ted Ts'o <tytso@mit.edu> wrote: > On Sat, Feb 26, 2011 at 01:35:05PM -0800, Manish Katiyar wrote: >> >> Is there anything else that I need to do to get this merged in your tree ? >> > > I've added the patch with the following explanation: > > ext4: fix missing iput of root inode for some mount error paths > > This assures that the root inode is not leaked, and that sb->s_root is > NULL, which will prevent generic_shutdown_super() from doing extra > work, including call sync_filesystem, which ultimately results in > ext4_sync_fs() getting called with an uninitialized struct super, > which is the cause of the crash noted in Kernel Bugzilla #26752. > > https://bugzilla.kernel.org/show_bug.cgi?id=26752 > > Signed-off-by: Manish Katiyar <mkatiyar@gmail.com> > Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> > > > Sorry for the delay, but this patch was held up pending my tracingg > through the code and understanding why this really fixed BZ #26752. > In the future, adding a bit more detail in the commit log will help me > process the patch faster, since I won't have to reproduce your > analysis. > > Also, just quoting a BZ number without going into more detail risks my > putting it the patch on my "to analyze --- when I have access to > network" queue if I happen to come across it while on an airplane. :-) Hi Ted, Thanks a lot. That was my first bugfix :-), will keep that in mind in future. -- Thanks - Manish -- 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ext4: Fix missing iput for root inode in case of all failed mount paths. 2011-01-16 18:05 ` Manish Katiyar 2011-01-30 5:40 ` Manish Katiyar @ 2011-07-06 22:54 ` Andreas Dilger 2011-07-10 19:11 ` Manish Katiyar 1 sibling, 1 reply; 11+ messages in thread From: Andreas Dilger @ 2011-07-06 22:54 UTC (permalink / raw) To: Manish Katiyar; +Cc: Theodore Ts'o, ext4, Yu Jian On 2011-01-16, at 11:05 AM, Manish Katiyar wrote: > On Sun, Jan 16, 2011 at 8:20 AM, Andreas Dilger <adilger@dilger.ca> wrote: >> Why not just put the iput() at failed_mount4() instead of spread around the code? > > Thanks Andreas, Here is the updated patch. We are hitting this same problem due to ENOMEM on allocating some large filesystem structures for 128TB filesystems. However, when we were going to add this patch to our patch series (until vendor kernels include it), Yu Jian (one of our developers) noticed a problem with the patch. In the error path, the patch is doing: failed_mount4: iput(root); sb->s_root = NULL; but in fact sb->s_root is a dentry allocated by d_alloc_root(), so the above code is freeing the inode, but still leaking the dentry. This is of course a lot better than before (no oops), but still isn't correct. The original problem appears to have been inadvertently fixed with commit 8aefcd557d26d0023a36f9ec5afbf55e59f8f26b, because ext4_clear_inode() now checks "if (EXT4_I(inode)->jinode)" before deferencing EXT4_SB() and the now-NULL s_fs_info. jinode should be NULL during mount, because it has never been opened. I haven't confirmed this theory yet, however. Manish, can you please give this a try with your fault-injection testing? It looks like we could revert 32a9bb57d7c1fd04ae0f72b8f671501f000a0e9f (this patch, leaving the two explicit iput() in place in case of a bad root inode or dentry) and leave the VFS to clean up the root dentry. > Signed-off-by: Manish Katiyar <mkatiyar@gmail.com> > --- > fs/ext4/super.c | 5 +++-- > 1 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index cb10a06..8fa53e9 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -3522,17 +3522,16 @@ no_journal: > if (IS_ERR(root)) { > ext4_msg(sb, KERN_ERR, "get root inode failed"); > ret = PTR_ERR(root); > + root = NULL; > goto failed_mount4; > } > if (!S_ISDIR(root->i_mode) || !root->i_blocks || !root->i_size) { > - iput(root); > ext4_msg(sb, KERN_ERR, "corrupt root inode, run e2fsck"); > goto failed_mount4; > } > sb->s_root = d_alloc_root(root); > if (!sb->s_root) { > ext4_msg(sb, KERN_ERR, "get root dentry failed"); > - iput(root); > ret = -ENOMEM; > goto failed_mount4; > } > @@ -3648,6 +3647,8 @@ cantfind_ext4: > goto failed_mount; > > failed_mount4: > + iput(root); > + sb->s_root = NULL; > ext4_msg(sb, KERN_ERR, "mount failed"); > destroy_workqueue(EXT4_SB(sb)->dio_unwritten_wq); > failed_mount_wq: > -- > 1.7.1 > > > >> >> Cheers, Andreas >> >> On 2011-01-16, at 0:30, Manish Katiyar <mkatiyar@gmail.com> wrote: >> >>> Fix missing iput for root inode in case of all failed mount paths. >>> Fixes bug#26752 >>> >>> Signed-off-by: Manish Katiyar <mkatiyar@gmail.com> >>> >>> --- >>> fs/ext4/super.c | 8 +++++++- >>> 1 files changed, 7 insertions(+), 1 deletions(-) >>> >>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c >>> index cb10a06..9570fcc 100644 >>> --- a/fs/ext4/super.c >>> +++ b/fs/ext4/super.c >>> @@ -3587,6 +3587,7 @@ no_journal: >>> if (err) { >>> ext4_msg(sb, KERN_ERR, "failed to initialize system " >>> "zone (%d)", err); >>> + iput(root); >>> goto failed_mount4; >>> } >>> >>> @@ -3595,12 +3596,15 @@ no_journal: >>> if (err) { >>> ext4_msg(sb, KERN_ERR, "failed to initialize mballoc (%d)", >>> err); >>> + iput(root); >>> goto failed_mount4; >>> } >>> >>> err = ext4_register_li_request(sb, first_not_zeroed); >>> - if (err) >>> + if (err) { >>> + iput(root); >>> goto failed_mount4; >>> + } >>> >>> sbi->s_kobj.kset = ext4_kset; >>> init_completion(&sbi->s_kobj_unregister); >>> @@ -3609,6 +3613,7 @@ no_journal: >>> if (err) { >>> ext4_mb_release(sb); >>> ext4_ext_release(sb); >>> + iput(root); >>> goto failed_mount4; >>> }; >>> >>> @@ -3648,6 +3653,7 @@ cantfind_ext4: >>> goto failed_mount; >>> >>> failed_mount4: >>> + sb->s_root = NULL; >>> ext4_msg(sb, KERN_ERR, "mount failed"); >>> destroy_workqueue(EXT4_SB(sb)->dio_unwritten_wq); >>> failed_mount_wq: >>> -- >>> 1.7.1 >>> >>> >>> -- >>> Thanks - >>> Manish >>> ================================== >>> [$\*.^ -- I miss being one of them >>> ================================== >>> -- >>> 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 >> > > > > -- > Thanks - > Manish > ================================== > [$\*.^ -- I miss being one of them > ================================== Cheers, Andreas ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ext4: Fix missing iput for root inode in case of all failed mount paths. 2011-07-06 22:54 ` Andreas Dilger @ 2011-07-10 19:11 ` Manish Katiyar 0 siblings, 0 replies; 11+ messages in thread From: Manish Katiyar @ 2011-07-10 19:11 UTC (permalink / raw) To: Andreas Dilger; +Cc: Theodore Ts'o, ext4, Yu Jian On Wed, Jul 6, 2011 at 3:54 PM, Andreas Dilger <adilger@dilger.ca> wrote: > On 2011-01-16, at 11:05 AM, Manish Katiyar wrote: >> On Sun, Jan 16, 2011 at 8:20 AM, Andreas Dilger <adilger@dilger.ca> wrote: >>> Why not just put the iput() at failed_mount4() instead of spread around the code? >> >> Thanks Andreas, Here is the updated patch. > > We are hitting this same problem due to ENOMEM on allocating some large > filesystem structures for 128TB filesystems. However, when we were going > to add this patch to our patch series (until vendor kernels include it), > Yu Jian (one of our developers) noticed a problem with the patch. > > In the error path, the patch is doing: > > failed_mount4: > iput(root); > sb->s_root = NULL; > > but in fact sb->s_root is a dentry allocated by d_alloc_root(), so the > above code is freeing the inode, but still leaking the dentry. This > is of course a lot better than before (no oops), but still isn't correct. > > The original problem appears to have been inadvertently fixed with commit > 8aefcd557d26d0023a36f9ec5afbf55e59f8f26b, because ext4_clear_inode() now > checks "if (EXT4_I(inode)->jinode)" before deferencing EXT4_SB() and the > now-NULL s_fs_info. jinode should be NULL during mount, because it has > never been opened. I haven't confirmed this theory yet, however. Manish, > can you please give this a try with your fault-injection testing? > > It looks like we could revert 32a9bb57d7c1fd04ae0f72b8f671501f000a0e9f > (this patch, leaving the two explicit iput() in place in case of a bad > root inode or dentry) and leave the VFS to clean up the root dentry. Hi Andreas, I reverted my original patch in latest Ted's tree and retried. ext4_clear_inode() is fixed, but we still panic in #14 <signal handler called> #15 ext4_sync_fs (sb=0x17d6ee00, wait=0) at fs/ext4/super.c:4191 #16 0x080d5214 in __sync_filesystem (sb=0x17d6ee00, wait=0) at fs/sync.c:49 #17 0x080d5277 in sync_filesystem (sb=0x17d6ee00) at fs/sync.c:74 #18 0x080bc188 in generic_shutdown_super (sb=0x17d6ee00) at fs/super.c:282 #19 0x080bc236 in kill_block_super (sb=0x17d6ee00) at fs/super.c:856 #20 0x080bc3fe in deactivate_locked_super (s=0x17d6ee00) at fs/super.c:183 #21 0x080bc96b in mount_bdev (fs_type=0x8264974, flags=0, dev_name=0x17d74ed0 "/dev/loop0", data=0x0, fill_super=0x81236f1 <ext4_fill_super>) at fs/super.c:831 #22 0x0811fc36 in ext4_mount (fs_type=0x8264974, flags=0, dev_name=0x17d74ed0 "/dev/loop0", data=0x0) at fs/ext4/super.c:4820 (gdb) f 15 #15 ext4_sync_fs (sb=0x17d6ee00, wait=0) at fs/ext4/super.c:4191 4191 flush_workqueue(sbi->dio_unwritten_wq); (gdb) p sbi $1 = (struct ext4_sb_info *) 0x0 -- Thanks - Manish -- 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2011-07-10 19:12 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-01-16 7:30 [PATCH] ext4: Fix missing iput for root inode in case of all failed mount paths Manish Katiyar 2011-01-16 16:20 ` Andreas Dilger 2011-01-16 18:05 ` Manish Katiyar 2011-01-30 5:40 ` Manish Katiyar 2011-02-07 23:23 ` Ted Ts'o 2011-02-07 23:31 ` Manish Katiyar 2011-02-26 21:35 ` Manish Katiyar 2011-02-28 1:45 ` Ted Ts'o 2011-02-28 4:19 ` Manish Katiyar 2011-07-06 22:54 ` Andreas Dilger 2011-07-10 19:11 ` Manish Katiyar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).