On Tue, 10 Jul 2007 16:30:25 -0700
Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Sun, 01 Jul 2007 03:36:48 -0400
> Mingming Cao <cmm@xxxxxxxxxx> 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@xxxxxxxxxx>
>
> 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.
Will fix.
> Alternatively (and preferably) do this via an update to
> Documentation/filesystems/ext4.txt.
Seems like I also need to update the doc on Kconfig as well. Do you
prefer this in separate patches? (current patch, kconfig patch, ext4
doc update patch?
> > 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.
Will fix.
>
> > 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?
I think I did, but honestly don't remember. Will check with the new
patch. :)
> >
> > -#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.
OK.
> > #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.
OK.
> Shoudln't all this debug info be a per-superblock thing rather than
> kernel-wide?
I don't think it is worth pursuing this feature since this seems to
have been broken for a while now (its been there since the first git
revission in ext3) and nobody has noticed it until now. It could be
address on a later patch though, since the initial purpose of the patch
was to fix the broken JBD2_DEBUG option. Of course, this may not be
clearly express in the changelog. :)
-JRS
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
