[patch 0/2] ext4 fixups for 2.6.23-rc8-mm2/ext4 git tree

[patch 0/2] ext4 fixups for 2.6.23-rc8-mm2/ext4 git tree

[markn]

-- 
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.

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

[markn]

[-- Attachment #1: ext4-add-init_ext4_proc-stub.patch --]
[-- Type: text/plain, Size: 1122 bytes --]

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 <markn@au1.ibm.com>
---
 fs/ext4/mballoc.c |    7 +++++++
 1 file changed, 7 insertions(+)

Index: ext4/fs/ext4/mballoc.c
===================================================================
--- ext4.orig/fs/ext4/mballoc.c
+++ 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)
 {

-- 

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

[Badari Pulavarty]

On Tue, 2007-10-09 at 15:50 +1000, markn@au1.ibm.com 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 <markn@au1.ibm.com>
> ---
>  fs/ext4/mballoc.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> Index: ext4/fs/ext4/mballoc.c
> ===================================================================
> --- ext4.orig/fs/ext4/mballoc.c
> +++ 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)
>  {

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

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

[Mingming Cao]

On Tue, 2007-10-09 at 09:31 -0700, Badari Pulavarty wrote:
> On Tue, 2007-10-09 at 15:50 +1000, markn@au1.ibm.com 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 <markn@au1.ibm.com>
> > ---
> >  fs/ext4/mballoc.c |    7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > Index: ext4/fs/ext4/mballoc.c
> > ===================================================================
> > --- ext4.orig/fs/ext4/mballoc.c
> > +++ 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)
> >  {
> 
> 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.
> 
> 

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

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

[Badari Pulavarty]

On Tue, 2007-10-09 at 10:03 -0700, Mingming Cao wrote:
> On Tue, 2007-10-09 at 09:31 -0700, Badari Pulavarty wrote:
> > On Tue, 2007-10-09 at 15:50 +1000, markn@au1.ibm.com 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 <markn@au1.ibm.com>
> > > ---
> > >  fs/ext4/mballoc.c |    7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > Index: ext4/fs/ext4/mballoc.c
> > > ===================================================================
> > > --- ext4.orig/fs/ext4/mballoc.c
> > > +++ 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)
> > >  {
> > 
> > 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.
> > 
> > 
> 
> 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

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

[Theodore Tso]

On Tue, Oct 09, 2007 at 10:03:06AM -0700, Mingming Cao wrote:
> 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

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

[Theodore Tso]

On Tue, Oct 09, 2007 at 01:40:12PM -0400, Theodore Tso wrote:
> On Tue, Oct 09, 2007 at 10:03:06AM -0700, Mingming Cao wrote:
> > 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

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 4409c0c..2305af4 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -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
 }
 
 

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

[Badari Pulavarty]

On Tue, 2007-10-09 at 20:22 -0400, Theodore Tso wrote:
> On Tue, Oct 09, 2007 at 01:40:12PM -0400, Theodore Tso wrote:
> > On Tue, Oct 09, 2007 at 10:03:06AM -0700, Mingming Cao wrote:
> > > 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
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 4409c0c..2305af4 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -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
>  }
>  

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

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

[Theodore Tso]

On Wed, Oct 10, 2007 at 08:30:10AM -0700, Badari Pulavarty wrote:
> 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

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

[Badari Pulavarty]

On Wed, 2007-10-10 at 15:00 -0400, Theodore Tso wrote:
> On Wed, Oct 10, 2007 at 08:30:10AM -0700, Badari Pulavarty wrote:
> > 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

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

[Mark Nelson]

Badari Pulavarty wrote:
> On Tue, 2007-10-09 at 15:50 +1000, markn@au1.ibm.com 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 <markn@au1.ibm.com>
>> ---
>>  fs/ext4/mballoc.c |    7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> Index: ext4/fs/ext4/mballoc.c
>> ===================================================================
>> --- ext4.orig/fs/ext4/mballoc.c
>> +++ 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)
>>  {
> 
> 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.

> 
> 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
> 

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

[markn]

[-- Attachment #1: ext4-move-init_ext4_proc-add-cleanup.patch --]
[-- Type: text/plain, Size: 1052 bytes --]

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 <markn@au1.ibm.com>
---
 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:

-- 

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

[Badari Pulavarty]

On Tue, 2007-10-09 at 15:50 +1000, markn@au1.ibm.com 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 <markn@au1.ibm.com>
> ---
>  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

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

[Mark Nelson]

Badari Pulavarty wrote:
> On Tue, 2007-10-09 at 15:50 +1000, markn@au1.ibm.com 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 <markn@au1.ibm.com>
>> ---
>>  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 <markn@au1.ibm.com>
---
 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
> 

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

[Theodore Tso]

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

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

[Theodore Tso]

On Tue, Oct 09, 2007 at 03:50:35PM +1000, markn@au1.ibm.com 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 *);
 

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

[Mark Nelson]

Theodore Tso wrote:
> On Tue, Oct 09, 2007 at 03:50:35PM +1000, markn@au1.ibm.com 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 *);
>  
>