Skip to contentSkip to conversation

ext4 fixups for 2.6.23-rc8-mm2/ext4 git tree

PATCH In review [email protected] 4 participants 17 messages Oct 9, 2007 – Oct 11, 2007 ext4 file system

lore
Share & tools
lore

[email protected]

author
#0 lore raw

cover ext4 fixups for 2.6.23-rc8-mm2/ext4 git tree

Signature · 17 lines
I've been maintaining a containers patchset that sits on top of the -mm tree and in testing the patches I maintain, I've found a bug in the ext4 code in 2.6.23-rc8-mm2 that causes the following build error:

linux/fs/ext4/mballoc.c: In function 'init_ext4_proc':
linux/fs/ext4/mballoc.c:2923: error: 'proc_root_fs' undeclared (first use in this function)
linux/fs/ext4/mballoc.c:2923: error: (Each undeclared identifier is reported only once
linux/fs/ext4/mballoc.c:2923: error: for each function it appears in.)

This same build error occurs in the ext4 git tree and so the patch that addresses this is the first patch.

The second patch addresses a problem that commit bfbca121e6ffc9f97439d541bf1a847accb3aed4 (mballoc core patch) introduced, whereby the added call to init_ext4_proc() means that init_ext4_fs() doesn't fully clean up after itself on fail.

These patches were originally for 2.6.23-rc8-mm2 but I've refreshed them so they apply cleanly to the ext4 git tree (refreshed just because lines were offset).

Hopefully these patches help.

Thanks,
Mark Nelson.
author
#1 lore raw

PATCH 1/2 add init_ext4_proc() stub for when CONFIG_PROC_FS is not set

init_ext4_fs() calls init_ext4_proc() so we need a stub init_ext4_proc() for the case that CONFIG_PROC_FS is not set.
Without the stub we get a build error:

fs/ext4/mballoc.c: In function 'init_ext4_proc':
fs/ext4/mballoc.c:2837: error: 'proc_root_fs' undeclared (first use in this function) fs/ext4/mballoc.c:2837: error: (Each undeclared identifier is reported only once fs/ext4/mballoc.c:2837: error: for each function it appears in.)

Add a stub init_ext4_proc() function that does nothing but return 0

Signed-off-by Mark Nelson <[email protected]>
---
 fs/ext4/mballoc.c |    7 +++++++
 1 file changed, 7 insertions(+)
fs/ext4/mballoc.c +7 -0
Index: ext4/fs/ext4/mballoc.c
===================================================================
@@ -2825,6 +2825,7 @@ static int ext4_mb_destroy_per_dev_proc(
 	return 0;
 }
 
+#ifdef CONFIG_PROC_FS
 int __init init_ext4_proc(void)
 {
 	ext4_pspace_cachep =

@@ -2840,6 +2841,12 @@ int __init init_ext4_proc(void)
 
 	return 0;
 }
+#else
+int __init init_ext4_proc(void)
+{
+	return 0;
+}
+#endif
 
 void exit_ext4_proc(void)
 {
#2 lore raw

Re: [patch 1/2] add init_ext4_proc() stub for when CONFIG_PROC_FS is not set

Quoted 32 lines · On Tue, 2007-10-09 at 15:50 +1000, [email protected] wrote:

plain text document attachment (ext4-add-init_ext4_proc-stub.patch) init_ext4_fs() calls init_ext4_proc() so we need a stub init_ext4_proc() for the case that CONFIG_PROC_FS is not set.
Without the stub we get a build error:

fs/ext4/mballoc.c: In function 'init_ext4_proc':
fs/ext4/mballoc.c:2837: error: 'proc_root_fs' undeclared (first use in this function) fs/ext4/mballoc.c:2837: error: (Each undeclared identifier is reported only once fs/ext4/mballoc.c:2837: error: for each function it appears in.)

Add a stub init_ext4_proc() function that does nothing but return 0

Signed-off-by Mark Nelson <[email protected]>
---
 fs/ext4/mballoc.c |    7 +++++++
 1 file changed, 7 insertions(+)

Index: ext4/fs/ext4/mballoc.c
===================================================================

fs/ext4/mballoc.c +1 -0
@@ -2825,6 +2825,7 @@ static int ext4_mb_destroy_per_dev_proc(
 	return 0;
 }
 
+#ifdef CONFIG_PROC_FS
 int __init init_ext4_proc(void)
 {
 	ext4_pspace_cachep =

@@ -2840,6 +2841,12 @@ int __init init_ext4_proc(void)
 
 	return 0;

fs/ext4/mballoc.c int __init init_ext4_proc(void) · line 2852

Quoted 10 lines
fs/ext4/mballoc.c +6 -0
 }
+#else
+int __init init_ext4_proc(void)
+{
+	return 0;
+}
+#endif
 
 void exit_ext4_proc(void)
 {

Nope. I don't think we can do this :(

For example, we need to create ext4_pspace_cachep kmem cache for the pre-allocation to work. We can't ifdef it out.

Mingming/Amit, can you take a look at this ? It looks like we NEED procfs support to make mballoc work. If so, we need to add it to the dependency.

Thanks,
Badari

#3 lore raw

Re: [patch 1/2] add init_ext4_proc() stub for when CONFIG_PROC_FS is not set

Quoted 46 lines · Badari Pulavarty
Quoted 32 lines · On Tue, 2007-10-09 at 15:50 +1000, [email protected] wrote:

plain text document attachment (ext4-add-init_ext4_proc-stub.patch) init_ext4_fs() calls init_ext4_proc() so we need a stub init_ext4_proc() for the case that CONFIG_PROC_FS is not set.
Without the stub we get a build error:

fs/ext4/mballoc.c: In function 'init_ext4_proc':
fs/ext4/mballoc.c:2837: error: 'proc_root_fs' undeclared (first use in this function) fs/ext4/mballoc.c:2837: error: (Each undeclared identifier is reported only once fs/ext4/mballoc.c:2837: error: for each function it appears in.)

Add a stub init_ext4_proc() function that does nothing but return 0

Signed-off-by Mark Nelson <[email protected]>
---
 fs/ext4/mballoc.c |    7 +++++++
 1 file changed, 7 insertions(+)

Index: ext4/fs/ext4/mballoc.c
===================================================================

fs/ext4/mballoc.c +1 -0
@@ -2825,6 +2825,7 @@ static int ext4_mb_destroy_per_dev_proc(
 	return 0;
 }
 
+#ifdef CONFIG_PROC_FS
 int __init init_ext4_proc(void)
 {
 	ext4_pspace_cachep =

@@ -2840,6 +2841,12 @@ int __init init_ext4_proc(void)
 
 	return 0;

fs/ext4/mballoc.c int __init init_ext4_proc(void) · line 2852

Quoted 10 lines
fs/ext4/mballoc.c +6 -0
 }
+#else
+int __init init_ext4_proc(void)
+{
+	return 0;
+}
+#endif
 
 void exit_ext4_proc(void)
 {

Nope. I don't think we can do this :(

Quoted 8 lines

For example, we need to create ext4_pspace_cachep kmem cache for the pre-allocation to work. We can't ifdef it out.

Mingming/Amit, can you take a look at this ? It looks like we NEED procfs support to make mballoc work. If so, we need to add it to the dependency.

I guess our testing did not catch this up because we have CONFIG_PROC_FS enabled all the time. mballoc needs procfs for exporting some stats info and tunable paramenters to optimize/customize multiple allocation.

We could select CONFIG_PROC_FS at kconfig when ext4dev is enabled.

Mingming

#4 lore raw

Re: [patch 1/2] add init_ext4_proc() stub for when CONFIG_PROC_FS is not set

Quoted 56 lines · Mingming Cao
Quoted 46 lines · Badari Pulavarty
Quoted 32 lines · On Tue, 2007-10-09 at 15:50 +1000, [email protected] wrote:

plain text document attachment (ext4-add-init_ext4_proc-stub.patch) init_ext4_fs() calls init_ext4_proc() so we need a stub init_ext4_proc() for the case that CONFIG_PROC_FS is not set.
Without the stub we get a build error:

fs/ext4/mballoc.c: In function 'init_ext4_proc':
fs/ext4/mballoc.c:2837: error: 'proc_root_fs' undeclared (first use in this function) fs/ext4/mballoc.c:2837: error: (Each undeclared identifier is reported only once fs/ext4/mballoc.c:2837: error: for each function it appears in.)

Add a stub init_ext4_proc() function that does nothing but return 0

Signed-off-by Mark Nelson <[email protected]>
---
 fs/ext4/mballoc.c |    7 +++++++
 1 file changed, 7 insertions(+)

Index: ext4/fs/ext4/mballoc.c
===================================================================

fs/ext4/mballoc.c +1 -0
@@ -2825,6 +2825,7 @@ static int ext4_mb_destroy_per_dev_proc(
 	return 0;
 }
 
+#ifdef CONFIG_PROC_FS
 int __init init_ext4_proc(void)
 {
 	ext4_pspace_cachep =

@@ -2840,6 +2841,12 @@ int __init init_ext4_proc(void)
 
 	return 0;

fs/ext4/mballoc.c int __init init_ext4_proc(void) · line 2852

Quoted 10 lines
fs/ext4/mballoc.c +6 -0
 }
+#else
+int __init init_ext4_proc(void)
+{
+	return 0;
+}
+#endif
 
 void exit_ext4_proc(void)
 {

Nope. I don't think we can do this :(

Quoted 8 lines

For example, we need to create ext4_pspace_cachep kmem cache for the pre-allocation to work. We can't ifdef it out.

Mingming/Amit, can you take a look at this ? It looks like we NEED procfs support to make mballoc work. If so, we need to add it to the dependency.

Quoted 5 lines

I guess our testing did not catch this up because we have CONFIG_PROC_FS enabled all the time. mballoc needs procfs for exporting some stats info and tunable paramenters to optimize/customize multiple allocation.

We could select CONFIG_PROC_FS at kconfig when ext4dev is enabled.

For testing it may be okay, but in the long run we need to make sure that ext4 doesn't depend on PROC_FS support for operation :(

It would be nice to make it work without PROC_FS dependency, if you configure procfs we should get more info/tunables..

ext4_pspace_cachep creation has to be moved to somewhere else ?

Thanks,
Badari

maintainer
#5 lore raw

Re: [patch 1/2] add init_ext4_proc() stub for when CONFIG_PROC_FS is not set

In reply to Mingming Cao
Quoted 5 lines · Mingming Cao

I guess our testing did not catch this up because we have CONFIG_PROC_FS enabled all the time. mballoc needs procfs for exporting some stats info and tunable paramenters to optimize/customize multiple allocation.

We could select CONFIG_PROC_FS at kconfig when ext4dev is enabled.

We definitely should be able to compile without CONFIG_PROC_FS; it's a major flaw in the mballoc-core.patch that it doesn't work without it.

I'm not sure why ext4_pspace_cachep is initialized in init_ext4_proc(), since it looks like that is being used as part of the core mballoc infrastructure, and just for proc work. It's definitely very unfortunate that the proc support is intertwined with the rest of the mballoc code, since the it means that adding the straight-forward #ifdef's will make the code quite ugly.

- Ted

maintainer
#6 lore raw

Re: [patch 1/2] add init_ext4_proc() stub for when CONFIG_PROC_FS is not set

Quoted 9 lines · Theodore Tso
Quoted 5 lines · Mingming Cao

I guess our testing did not catch this up because we have CONFIG_PROC_FS enabled all the time. mballoc needs procfs for exporting some stats info and tunable paramenters to optimize/customize multiple allocation.

We could select CONFIG_PROC_FS at kconfig when ext4dev is enabled.

We definitely should be able to compile without CONFIG_PROC_FS; it's a major flaw in the mballoc-core.patch that it doesn't work without it.

I will fold the following into mballoc-core.patch. It's sufficient to allow us to compile w/o CONFIG_PROC_FS defined.

- Ted

fs/ext4/mballoc.c +4 -0
index 4409c0c..2305af4 100644
@@ -2854,9 +2854,11 @@ int __init init_ext4_proc(void)
 	if (ext4_pspace_cachep == NULL)
 		return -ENOMEM;
 
+#ifdef CONFIG_PROC_FS
 	proc_root_ext4 = proc_mkdir(EXT4_ROOT, proc_root_fs);
 	if (proc_root_ext4 == NULL)
 		printk(KERN_ERR "EXT4-fs: Unable to create %s\n", EXT4_ROOT);
+#endif
 
 	return 0;
 }

@@ -2865,7 +2867,9 @@ void exit_ext4_proc(void)
 {
 	/* XXX: synchronize_rcu(); */
 	kmem_cache_destroy(ext4_pspace_cachep);
+#ifdef CONFIG_PROC_FS
 	remove_proc_entry(EXT4_ROOT, proc_root_fs);
+#endif
 }
#7 lore raw

Re: [patch 1/2] add init_ext4_proc() stub for when CONFIG_PROC_FS is not set

Quoted 33 lines · Theodore Tso
Quoted 9 lines · Theodore Tso
Quoted 5 lines · Mingming Cao

I guess our testing did not catch this up because we have CONFIG_PROC_FS enabled all the time. mballoc needs procfs for exporting some stats info and tunable paramenters to optimize/customize multiple allocation.

We could select CONFIG_PROC_FS at kconfig when ext4dev is enabled.

We definitely should be able to compile without CONFIG_PROC_FS; it's a major flaw in the mballoc-core.patch that it doesn't work without it.

I will fold the following into mballoc-core.patch. It's sufficient to allow us to compile w/o CONFIG_PROC_FS defined.

- Ted

fs/ext4/mballoc.c +2 -0
index 4409c0c..2305af4 100644
@@ -2854,9 +2854,11 @@ int __init init_ext4_proc(void)
 	if (ext4_pspace_cachep == NULL)
 		return -ENOMEM;
 
+#ifdef CONFIG_PROC_FS
 	proc_root_ext4 = proc_mkdir(EXT4_ROOT, proc_root_fs);
 	if (proc_root_ext4 == NULL)
 		printk(KERN_ERR "EXT4-fs: Unable to create %s\n", EXT4_ROOT);
+#endif
 
 	return 0;
 }

fs/ext4/mballoc.c void exit_ext4_proc(void) · line 2874

Quoted 8 lines
fs/ext4/mballoc.c +2 -0
@@ -2865,7 +2867,9 @@ void exit_ext4_proc(void)
 {
 	/* XXX: synchronize_rcu(); */
 	kmem_cache_destroy(ext4_pspace_cachep);
+#ifdef CONFIG_PROC_FS
 	remove_proc_entry(EXT4_ROOT, proc_root_fs);
+#endif
 }

Its a good start. I think there are lots of proc handling routines that can be move into #ifdef CONFIG_PROC_FS also.

All the code around ext4_mb_read_prealloc_table(), ext4_mb_write_prealloc_table(), MB_PROC_VALUE_READ(stats), MB_PROC_VALUE_WRITE(stats), .. can be ifdefed out.

Thanks,
Badari

maintainer
#8 lore raw

Re: [patch 1/2] add init_ext4_proc() stub for when CONFIG_PROC_FS is not set

Quoted 6 lines · Badari Pulavarty

Its a good start. I think there are lots of proc handling routines that can be move into #ifdef CONFIG_PROC_FS also.

All the code around ext4_mb_read_prealloc_table(), ext4_mb_write_prealloc_table(), MB_PROC_VALUE_READ(stats), MB_PROC_VALUE_WRITE(stats), .. can be ifdefed out.

There's no need to ifdef them out; in include/proc_fs.h there are the following convenience #define's if CONFIG_PROC_FS is not defined:

#define remove_proc_entry(name, parent) do {} while (0) static inline struct proc_dir_entry *proc_symlink(const char *name,

		struct proc_dir_entry *parent,const char *dest) {return NULL;}
static inline struct proc_dir_entry *proc_mkdir(const char *name,
	struct proc_dir_entry *parent) {return NULL;}

static inline struct proc_dir_entry *create_proc_read_entry(const char *name,

	mode_t mode, struct proc_dir_entry *base, 
	read_proc_t *read_proc, void * data) { return NULL; }
static inline struct proc_dir_entry *create_proc_info_entry(const char *name,
	mode_t mode, struct proc_dir_entry *base, get_info_t *get_info)
	{ return NULL; }

Adding the #ifdef's just makes the code look ugly, and for no purpose, since the compiler will take care of removing the code for us.

Regards,

- Ted

#9 lore raw

Re: [patch 1/2] add init_ext4_proc() stub for when CONFIG_PROC_FS is not set

Quoted 10 lines · Theodore Tso
Quoted 6 lines · Badari Pulavarty

Its a good start. I think there are lots of proc handling routines that can be move into #ifdef CONFIG_PROC_FS also.

All the code around ext4_mb_read_prealloc_table(), ext4_mb_write_prealloc_table(), MB_PROC_VALUE_READ(stats), MB_PROC_VALUE_WRITE(stats), .. can be ifdefed out.

There's no need to ifdef them out; in include/proc_fs.h there are the following convenience #define's if CONFIG_PROC_FS is not defined:

Sorry. If I wasn't clear.. What I meant to say was the above routines are NOT needed if we don't define CONFIG_PROC_FS. They are supporting read/write to /proc files. We can #ifdef them out to reduce the text size.

Thanks,
Badari

author
#10 lore raw

Re: [patch 1/2] add init_ext4_proc() stub for when CONFIG_PROC_FS is not set

In reply to Badari Pulavarty
Quoted 41 lines · Badari Pulavarty
Quoted 40 lines · On Tue, 2007-10-09 at 15:50 +1000, [email protected] wrote:

plain text document attachment (ext4-add-init_ext4_proc-stub.patch) init_ext4_fs() calls init_ext4_proc() so we need a stub init_ext4_proc() for the case that CONFIG_PROC_FS is not set.
Without the stub we get a build error:

fs/ext4/mballoc.c: In function 'init_ext4_proc':
fs/ext4/mballoc.c:2837: error: 'proc_root_fs' undeclared (first use in this function) fs/ext4/mballoc.c:2837: error: (Each undeclared identifier is reported only once fs/ext4/mballoc.c:2837: error: for each function it appears in.)

Add a stub init_ext4_proc() function that does nothing but return 0

Signed-off-by Mark Nelson <[email protected]>
---
 fs/ext4/mballoc.c |    7 +++++++
 1 file changed, 7 insertions(+)

Index: ext4/fs/ext4/mballoc.c
===================================================================

fs/ext4/mballoc.c +7 -0
@@ -2825,6 +2825,7 @@ static int ext4_mb_destroy_per_dev_proc(
 	return 0;
 }
 
+#ifdef CONFIG_PROC_FS
 int __init init_ext4_proc(void)
 {
 	ext4_pspace_cachep =

@@ -2840,6 +2841,12 @@ int __init init_ext4_proc(void)
 
 	return 0;
 }
+#else
+int __init init_ext4_proc(void)
+{
+	return 0;
+}
+#endif
Quoted 7 lines

fs/ext4/mballoc.c int __init init_ext4_proc(void) · line 2852

Quoted 2 lines
fs/ext4/mballoc.c +0 -0
 void exit_ext4_proc(void)
 {

Nope. I don't think we can do this :(

For example, we need to create ext4_pspace_cachep kmem cache for the pre-allocation to work. We can't ifdef it out.

This is definitely something I'll leave to the experts then :) At least you guys know about this now I guess.

Sorry again Andrew.

Thanks all!

Mark.

Quoted 9 lines

Mingming/Amit, can you take a look at this ? It looks like we NEED procfs support to make mballoc work. If so, we need to add it to the dependency.

Thanks,
Badari

author
#11 lore raw

PATCH 2/2 move init_ext4_proc() last and add cleanup call exit_ext4_proc()

In reply to [email protected]

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(-)
fs/ext4/super.c +5 -4
Index: 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:
#12 lore raw

Re: [patch 2/2] move init_ext4_proc() last and add cleanup call exit_ext4_proc()

Quoted 31 lines · 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
===================================================================

fs/ext4/super.c +0 -4
@@ -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)

fs/ext4/super.c static int __init init_ext4_fs(void) · line 3019

Quoted 10 lines
fs/ext4/super.c +5 -0
 		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

author
#13 lore raw

Re: [patch 2/2] move init_ext4_proc() last and add cleanup call exit_ext4_proc()

Quoted 38 lines · Badari Pulavarty
Quoted 37 lines · 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
===================================================================

fs/ext4/super.c +4 -4
@@ -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:
Quoted 9 lines

fs/ext4/super.c static int __init init_ext4_fs(void) · line 3019

Quoted 4 lines
fs/ext4/super.c +1 -0
+	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(-)
fs/ext4/super.c +5 -4
Index: 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 :)

Quoted 7 lines

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

maintainer
#14 lore raw

Re: [patch 2/2] move init_ext4_proc() last and add cleanup call exit_ext4_proc()

In reply to Badari Pulavarty
Quoted 3 lines · Badari Pulavarty

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

maintainer
#15 lore raw

Re: [patch 2/2] move init_ext4_proc() last and add cleanup call exit_ext4_proc()

In reply to [email protected]
Quoted 5 lines · 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

fs/ext4/mballoc.c +2 -2
index 2305af4..b247ca0 100644
@@ -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);
fs/ext4/super.c +5 -3
index 30e2b64..5882165 100644
@@ -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");
include/linux/ext4_fs.h +2 -2
index 6a0909f..783c6f3 100644
@@ -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 *);
author
#16 lore raw

Re: [patch 2/2] move init_ext4_proc() last and add cleanup call exit_ext4_proc()

Quoted 7 lines · Theodore Tso
Quoted 5 lines · 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().

Quoted 6 lines

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

Quoted 80 lines

I will fold this into the mballoc-core.patch.

- Ted

fs/ext4/mballoc.c +2 -2
index 2305af4..b247ca0 100644
@@ -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);
fs/ext4/super.c +5 -3
index 30e2b64..5882165 100644
@@ -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");
include/linux/ext4_fs.h +2 -2
index 6a0909f..783c6f3 100644
@@ -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 *);