The first problem that is addressed is that the addition of init_ext4_proc()
means that the code doesn't clean up after itself on a failure of
init_ext4_xattr(), and the second is that usually init_*_proc() should be
the last thing called, so we move it to the end and add the cleanup call to
exit_ext4_proc().
Signed-off-by: Mark Nelson <[email protected]>
---
fs/ext4/super.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
Index: ext4/fs/ext4/super.c
===================================================================
--- ext4.orig/fs/ext4/super.c
+++ ext4/fs/ext4/super.c
@@ -2999,10 +2999,6 @@ static int __init init_ext4_fs(void)
{
int err;
- err = init_ext4_proc();
- if (err)
- return err;
-
err = init_ext4_xattr();
if (err)
return err;
@@ -3012,7 +3008,12 @@ static int __init init_ext4_fs(void)
err = register_filesystem(&ext4dev_fs_type);
if (err)
goto out;
+ err = init_ext4_proc();
+ if (err)
+ goto out_proc;
return 0;
+out_proc:
+ exit_ext4_proc();
out:
destroy_inodecache();
out1:
--
On Tue, 2007-10-09 at 15:50 +1000, [email protected] wrote:
> plain text document attachment (ext4-move-init_ext4_proc-add-
> cleanup.patch)
> The first problem that is addressed is that the addition of init_ext4_proc()
> means that the code doesn't clean up after itself on a failure of
> init_ext4_xattr(), and the second is that usually init_*_proc() should be
> the last thing called, so we move it to the end and add the cleanup call to
> exit_ext4_proc().
>
> Signed-off-by: Mark Nelson <[email protected]>
> ---
> fs/ext4/super.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> Index: ext4/fs/ext4/super.c
> ===================================================================
> --- ext4.orig/fs/ext4/super.c
> +++ ext4/fs/ext4/super.c
> @@ -2999,10 +2999,6 @@ static int __init init_ext4_fs(void)
> {
> int err;
>
> - err = init_ext4_proc();
> - if (err)
> - return err;
> -
> err = init_ext4_xattr();
> if (err)
> return err;
> @@ -3012,7 +3008,12 @@ static int __init init_ext4_fs(void)
> err = register_filesystem(&ext4dev_fs_type);
> if (err)
> goto out;
> + err = init_ext4_proc();
> + if (err)
> + goto out_proc;
> return 0;
> +out_proc:
> + exit_ext4_proc();
> out:
> destroy_inodecache();
> out1:
Nope. You can not call exit_ext4_proc() if there is a failure
in init_ext4_proc(). If the kmem_cache_create() for ext4_pspace_cachep
fails, you would end up calling kmem_cache_destory(NULL).
The best way is to make init_ext4_proc() cleanup itself, in case
of an error. Hmm.. we are not handling proc_mkdir(EXT4_ROOT)
failures.
Thanks,
Badari
Badari Pulavarty wrote:
> On Tue, 2007-10-09 at 15:50 +1000, [email protected] wrote:
>> plain text document attachment (ext4-move-init_ext4_proc-add-
>> cleanup.patch)
>> The first problem that is addressed is that the addition of init_ext4_proc()
>> means that the code doesn't clean up after itself on a failure of
>> init_ext4_xattr(), and the second is that usually init_*_proc() should be
>> the last thing called, so we move it to the end and add the cleanup call to
>> exit_ext4_proc().
>>
>> Signed-off-by: Mark Nelson <[email protected]>
>> ---
>> fs/ext4/super.c | 9 +++++----
>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> Index: ext4/fs/ext4/super.c
>> ===================================================================
>> --- ext4.orig/fs/ext4/super.c
>> +++ ext4/fs/ext4/super.c
>> @@ -2999,10 +2999,6 @@ static int __init init_ext4_fs(void)
>> {
>> int err;
>>
>> - err = init_ext4_proc();
>> - if (err)
>> - return err;
>> -
>> err = init_ext4_xattr();
>> if (err)
>> return err;
>> @@ -3012,7 +3008,12 @@ static int __init init_ext4_fs(void)
>> err = register_filesystem(&ext4dev_fs_type);
>> if (err)
>> goto out;
>> + err = init_ext4_proc();
>> + if (err)
>> + goto out_proc;
>> return 0;
>> +out_proc:
>> + exit_ext4_proc();
>> out:
>> destroy_inodecache();
>> out1:
>
> Nope. You can not call exit_ext4_proc() if there is a failure
> in init_ext4_proc(). If the kmem_cache_create() for ext4_pspace_cachep
> fails, you would end up calling kmem_cache_destory(NULL).
>
Oh yes, you're exactly right. Sorry, I meant to call unregister_filesystem.
(connection between brain and hands temporarily lost while typing...)
What I should have had was the following:
Subject: move init_ext4_proc() last and add unregister_filesystem() cleanup
The addition of init_ext4_proc() means that the code doesn't clean up
after itself on a failure of init_ext4_xattr(), so move the call to
init_ext4_proc() last and then add cleanup call to unregister_filesystem().
Signed-off-by: Mark Nelson <[email protected]>
---
fs/ext4/super.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
Index: ext4/fs/ext4/super.c
===================================================================
--- ext4.orig/fs/ext4/super.c
+++ ext4/fs/ext4/super.c
@@ -2999,10 +2999,6 @@ static int __init init_ext4_fs(void)
{
int err;
- err = init_ext4_proc();
- if (err)
- return err;
-
err = init_ext4_xattr();
if (err)
return err;
@@ -3012,7 +3008,12 @@ static int __init init_ext4_fs(void)
err = register_filesystem(&ext4dev_fs_type);
if (err)
goto out;
+ err = init_ext4_proc();
+ if (err)
+ goto out_filesystem;
return 0;
+out_filesystem:
+ unregister_filesystem(&ext4dev_fs_type);
out:
destroy_inodecache();
out1:
Sorry about the patch noise Andrew...
Thanks Badari!
Mark.
If helping turns to hindering, let me know and I'll leave you ext4 experts in peace :)
> The best way is to make init_ext4_proc() cleanup itself, in case
> of an error. Hmm.. we are not handling proc_mkdir(EXT4_ROOT)
> failures.
>
> Thanks,
> Badari
>
On Tue, Oct 09, 2007 at 09:22:02AM -0700, Badari Pulavarty wrote:
> The best way is to make init_ext4_proc() cleanup itself, in case
> of an error. Hmm.. we are not handling proc_mkdir(EXT4_ROOT)
> failures.
That's actually OK; if proc_root_ext4 is NULL, the procfs routines
should deal just fine.
- Ted
On Tue, Oct 09, 2007 at 03:50:35PM +1000, [email protected] wrote:
> The first problem that is addressed is that the addition of init_ext4_proc()
> means that the code doesn't clean up after itself on a failure of
> init_ext4_xattr(), and the second is that usually init_*_proc() should be
> the last thing called, so we move it to the end and add the cleanup call to
> exit_ext4_proc().
I've fixed the first simply by just doing this. The order really
doesn't matter much here, and in fact it would probably be better to
change {init,exit}_ext4_proc() to be {init,exit}_ext4_mballoc() since
that's actually more accurate, and you need the call to
init_ext4_proc() even if you aren't using procfs.
I will fold this into the mballoc-core.patch.
- Ted
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 2305af4..b247ca0 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2845,7 +2845,7 @@ static int ext4_mb_destroy_per_dev_proc(struct super_block *sb)
return 0;
}
-int __init init_ext4_proc(void)
+int __init init_ext4_mballoc(void)
{
ext4_pspace_cachep =
kmem_cache_create("ext4_prealloc_space",
@@ -2863,7 +2863,7 @@ int __init init_ext4_proc(void)
return 0;
}
-void exit_ext4_proc(void)
+void exit_ext4_mballoc(void)
{
/* XXX: synchronize_rcu(); */
kmem_cache_destroy(ext4_pspace_cachep);
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 30e2b64..5882165 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -2994,13 +2994,13 @@ static int __init init_ext4_fs(void)
{
int err;
- err = init_ext4_proc();
+ err = init_ext4_mballoc();
if (err)
return err;
err = init_ext4_xattr();
if (err)
- return err;
+ goto out2;
err = init_inodecache();
if (err)
goto out1;
@@ -3012,6 +3012,8 @@ out:
destroy_inodecache();
out1:
exit_ext4_xattr();
+out2:
+ exit_ext4_mballoc();
return err;
}
@@ -3020,7 +3022,7 @@ static void __exit exit_ext4_fs(void)
unregister_filesystem(&ext4dev_fs_type);
destroy_inodecache();
exit_ext4_xattr();
- exit_ext4_proc();
+ exit_ext4_mballoc();
}
MODULE_AUTHOR("Remy Card, Stephen Tweedie, Andrew Morton, Andreas Dilger, Theodore Ts'o and others");
diff --git a/include/linux/ext4_fs.h b/include/linux/ext4_fs.h
index 6a0909f..783c6f3 100644
--- a/include/linux/ext4_fs.h
+++ b/include/linux/ext4_fs.h
@@ -954,8 +954,8 @@ extern int ext4_mb_release(struct super_block *);
extern ext4_fsblk_t ext4_mb_new_blocks(handle_t *, struct ext4_allocation_request *, int *);
extern int ext4_mb_reserve_blocks(struct super_block *, int);
extern void ext4_mb_discard_inode_preallocations(struct inode *);
-extern int __init init_ext4_proc(void);
-extern void exit_ext4_proc(void);
+extern int __init init_ext4_mballoc(void);
+extern void exit_ext4_mballoc(void);
extern void ext4_mb_free_blocks(handle_t *, struct inode *,
unsigned long, unsigned long, int, unsigned long *);
Theodore Tso wrote:
> On Tue, Oct 09, 2007 at 03:50:35PM +1000, [email protected] wrote:
>> The first problem that is addressed is that the addition of init_ext4_proc()
>> means that the code doesn't clean up after itself on a failure of
>> init_ext4_xattr(), and the second is that usually init_*_proc() should be
>> the last thing called, so we move it to the end and add the cleanup call to
>> exit_ext4_proc().
>
> I've fixed the first simply by just doing this. The order really
> doesn't matter much here, and in fact it would probably be better to
> change {init,exit}_ext4_proc() to be {init,exit}_ext4_mballoc() since
> that's actually more accurate, and you need the call to
> init_ext4_proc() even if you aren't using procfs.
>
That makes sense. Forget my last patch then.
Next time I'll just report the build error :)
Thanks!
Mark
> I will fold this into the mballoc-core.patch.
>
> - Ted
>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 2305af4..b247ca0 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2845,7 +2845,7 @@ static int ext4_mb_destroy_per_dev_proc(struct super_block *sb)
> return 0;
> }
>
> -int __init init_ext4_proc(void)
> +int __init init_ext4_mballoc(void)
> {
> ext4_pspace_cachep =
> kmem_cache_create("ext4_prealloc_space",
> @@ -2863,7 +2863,7 @@ int __init init_ext4_proc(void)
> return 0;
> }
>
> -void exit_ext4_proc(void)
> +void exit_ext4_mballoc(void)
> {
> /* XXX: synchronize_rcu(); */
> kmem_cache_destroy(ext4_pspace_cachep);
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 30e2b64..5882165 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -2994,13 +2994,13 @@ static int __init init_ext4_fs(void)
> {
> int err;
>
> - err = init_ext4_proc();
> + err = init_ext4_mballoc();
> if (err)
> return err;
>
> err = init_ext4_xattr();
> if (err)
> - return err;
> + goto out2;
> err = init_inodecache();
> if (err)
> goto out1;
> @@ -3012,6 +3012,8 @@ out:
> destroy_inodecache();
> out1:
> exit_ext4_xattr();
> +out2:
> + exit_ext4_mballoc();
> return err;
> }
>
> @@ -3020,7 +3022,7 @@ static void __exit exit_ext4_fs(void)
> unregister_filesystem(&ext4dev_fs_type);
> destroy_inodecache();
> exit_ext4_xattr();
> - exit_ext4_proc();
> + exit_ext4_mballoc();
> }
>
> MODULE_AUTHOR("Remy Card, Stephen Tweedie, Andrew Morton, Andreas Dilger, Theodore Ts'o and others");
> diff --git a/include/linux/ext4_fs.h b/include/linux/ext4_fs.h
> index 6a0909f..783c6f3 100644
> --- a/include/linux/ext4_fs.h
> +++ b/include/linux/ext4_fs.h
> @@ -954,8 +954,8 @@ extern int ext4_mb_release(struct super_block *);
> extern ext4_fsblk_t ext4_mb_new_blocks(handle_t *, struct ext4_allocation_request *, int *);
> extern int ext4_mb_reserve_blocks(struct super_block *, int);
> extern void ext4_mb_discard_inode_preallocations(struct inode *);
> -extern int __init init_ext4_proc(void);
> -extern void exit_ext4_proc(void);
> +extern int __init init_ext4_mballoc(void);
> +extern void exit_ext4_mballoc(void);
> extern void ext4_mb_free_blocks(handle_t *, struct inode *,
> unsigned long, unsigned long, int, unsigned long *);
>
>