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
===================================================================
--- 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)
{
--
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
> ===================================================================
> --- 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
On Tue, 2007-10-09 at 09:31 -0700, Badari Pulavarty wrote:
> 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
> > ===================================================================
> > --- 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
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, [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
> > > ===================================================================
> > > --- 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
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
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
}
Badari Pulavarty wrote:
> 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
>> ===================================================================
>> --- 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
>
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
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
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