Re: [EXT4 set 2][PATCH 5/5] cleanups: Export jbd2-debug via debugfs

[Andrew Morton <akpm@linux-foundation.org>]

On Sun, 01 Jul 2007 03:36:48 -0400
Mingming Cao <cmm@us.ibm.com> wrote:

> > On Jun 07, 2007  23:45 -0500, Jose R. Santos wrote:
> > > The jbd2-debug file used to be located in /proc/sys/fs/jbd2-debug, but
> > > create_proc_entry() does not do lookups on file names with more that one
> > > directory deep.  This causes the entry creation to fail and hence, no proc
> > > file is created.  This patch moves the file to /proc/jbd2-degug.
> > > 
> > > The file could be move to /proc/fs/jbd2/jbd2-debug, but it would require
> > > some minor alterations to the jbd-stats patch.
> > 
> > I don't think we really want to be adding top-level files in /proc.
> > What about using the "debugfs" filesystem (not to be confused with
> > the e2fsprogs 'debugfs' command)?
> 
> How about this then?  Moved the file to use debugfs as well as having
> the nice effect of removing more lines than what it adds.
> 
> Signed-off-by: Jose R. Santos <jrs@us.ibm.com>

Please clean up the changelog.

The changelog should include information about the location and the content
of these debugfs files.  it should provide any instructions which users
will need to be able to create and use those files.

Alternatively (and preferably) do this via an update to
Documentation/filesystems/ext4.txt.

>  fs/jbd2/journal.c    |   62    20 +    42 -    0 !
>  include/linux/jbd2.h |    2    1 +     1 -     0 !
>  2 files changed, 21 insertions(+), 43 deletions(-)

Again, this patch isn't in Ted's kernel.org directory and hasn't been in -mm.

Apart from the lack of testing and review which this causes, it means I
can't just do `pushpatch name-of-this-patch' and look at it in tkdiff.  So
I squint at the diff, but that's harder when the diff wasn't prepared with
`diff -p'.  Oh well.


> Index: linux-2.6.22-rc4/fs/jbd2/journal.c
> ===================================================================
> --- linux-2.6.22-rc4.orig/fs/jbd2/journal.c	2007-06-11 16:16:18.000000000 -0700
> +++ linux-2.6.22-rc4/fs/jbd2/journal.c	2007-06-11 16:36:10.000000000 -0700
> @@ -35,6 +35,7 @@
>  #include <linux/kthread.h>
>  #include <linux/poison.h>
>  #include <linux/proc_fs.h>
> +#include <linux/debugfs.h>
>  
>  #include <asm/uaccess.h>
>  #include <asm/page.h>
> @@ -1954,60 +1955,37 @@
>   * /proc tunables
>   */
>  #if defined(CONFIG_JBD2_DEBUG)
> -int jbd2_journal_enable_debug;
> +u16 jbd2_journal_enable_debug;
>  EXPORT_SYMBOL(jbd2_journal_enable_debug);
>  #endif
>  
> -#if defined(CONFIG_JBD2_DEBUG) && defined(CONFIG_PROC_FS)
> +#if defined(CONFIG_JBD2_DEBUG) && defined(CONFIG_DEBUG_FS)

Has this been compile-tested with CONFIG_DEBUGFS=n?

>  
> -#define create_jbd_proc_entry() do {} while (0)
> -#define jbd2_remove_jbd_proc_entry() do {} while (0)
> +#define jbd2_create_debugfs_entry() do {} while (0)
> +#define jbd2_remove_debugfs_entry() do {} while (0)

I suggest that these be converted to (preferable) inline functions while
you're there.

>  #endif
>  
> @@ -2067,7 +2045,7 @@
>  	ret = journal_init_caches();
>  	if (ret != 0)
>  		jbd2_journal_destroy_caches();
> -	create_jbd_proc_entry();
> +	jbd2_create_debugfs_entry();
>  	return ret;
>  }
>  
> @@ -2078,7 +2056,7 @@
>  	if (n)
>  		printk(KERN_EMERG "JBD: leaked %d journal_heads!\n", n);
>  #endif
> -	jbd2_remove_jbd_proc_entry();
> +	jbd2_remove_debugfs_entry();
>  	jbd2_journal_destroy_caches();
>  }
>  
> Index: linux-2.6.22-rc4/include/linux/jbd2.h
> ===================================================================
> --- linux-2.6.22-rc4.orig/include/linux/jbd2.h	2007-06-11 16:16:18.000000000 -0700
> +++ linux-2.6.22-rc4/include/linux/jbd2.h	2007-06-11 16:35:25.000000000 -0700
> @@ -57,7 +57,7 @@
>   * CONFIG_JBD2_DEBUG is on.
>   */
>  #define JBD_EXPENSIVE_CHECKING

JBD2?

> -extern int jbd2_journal_enable_debug;
> +extern u16 jbd2_journal_enable_debug;

Why was this made 16-bit?  To save 2 bytes?  Could have saved 3 if we're
going to do that.


Shoudln't all this debug info be a per-superblock thing rather than
kernel-wide?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/