Re: [EXT4 set 2][PATCH 5/5] cleanups: Export jbd2-debug via debugfs
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 that are more that one
directory deep. This causes the entry creation to fail and hence, no proc
file is created.
Instead of fixing this on procfs might as well move the jbd2-debug file to
debugfs which would be the preferred location for this kind of tunable. The
new location is now /sys/kernel/debug/jbd2/jbd2-debug.
Signed-off-by: Jose R. Santos <[EMAIL PROTECTED]>
---
fs/Kconfig | 105 + 5 - 0 !
fs/jbd2/journal.c| 6727 +40 -0 !
include/linux/jbd2.h |21 + 1 - 0 !
3 files changed, 33 insertions(+), 46 deletions(-)
Index: linux-2.6/fs/jbd2/journal.c
===
--- linux-2.6.orig/fs/jbd2/journal.c2007-07-11 09:46:25.0 -0500
+++ linux-2.6/fs/jbd2/journal.c 2007-07-11 11:31:30.0 -0500
@@ -35,6 +35,7 @@
#include
#include
#include
+#include
#include
#include
@@ -1951,64 +1952,50 @@ void jbd2_journal_put_journal_head(struc
}
/*
- * /proc tunables
+ * debugfs tunables
*/
#if defined(CONFIG_JBD2_DEBUG)
-int jbd2_journal_enable_debug;
+u8 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)
-static struct proc_dir_entry *proc_jbd_debug;
+#define JBD2_DEBUG_NAME "jbd2-debug"
-static int read_jbd_debug(char *page, char **start, off_t off,
- int count, int *eof, void *data)
-{
- int ret;
+struct dentry *jbd2_debugfs_dir, *jbd2_debug;
- ret = sprintf(page + off, "%d\n", jbd2_journal_enable_debug);
- *eof = 1;
- return ret;
+static void __init jbd2_create_debugfs_entry(void)
+{
+ jbd2_debugfs_dir = debugfs_create_dir("jbd2", NULL);
+ if (jbd2_debugfs_dir)
+ jbd2_debug = debugfs_create_u8(JBD2_DEBUG_NAME, S_IRUGO,
+ jbd2_debugfs_dir,
+ &jbd2_journal_enable_debug);
}
-static int write_jbd_debug(struct file *file, const char __user *buffer,
- unsigned long count, void *data)
+static void __exit jbd2_remove_debugfs_entry(void)
{
- char buf[32];
-
- if (count > ARRAY_SIZE(buf) - 1)
- count = ARRAY_SIZE(buf) - 1;
- if (copy_from_user(buf, buffer, count))
- return -EFAULT;
- buf[ARRAY_SIZE(buf) - 1] = '\0';
- jbd2_journal_enable_debug = simple_strtoul(buf, NULL, 10);
- return count;
+ if (jbd2_debug)
+ debugfs_remove(jbd2_debug);
+ if (jbd2_debugfs_dir)
+ debugfs_remove(jbd2_debugfs_dir);
}
-#define JBD_PROC_NAME "sys/fs/jbd2-debug"
+#else
-static void __init create_jbd_proc_entry(void)
+static void __init jbd2_create_debugfs_entry(void)
{
- proc_jbd_debug = create_proc_entry(JBD_PROC_NAME, 0644, NULL);
- if (proc_jbd_debug) {
- /* Why is this so hard? */
- proc_jbd_debug->read_proc = read_jbd_debug;
- proc_jbd_debug->write_proc = write_jbd_debug;
- }
+ do {
+ } while (0);
}
-static void __exit jbd2_remove_jbd_proc_entry(void)
+static void __exit jbd2_remove_debugfs_entry(void)
{
- if (proc_jbd_debug)
- remove_proc_entry(JBD_PROC_NAME, NULL);
+ do {
+ } while (0);
}
-#else
-
-#define create_jbd_proc_entry() do {} while (0)
-#define jbd2_remove_jbd_proc_entry() do {} while (0)
-
#endif
struct kmem_cache *jbd2_handle_cache;
@@ -2067,7 +2054,7 @@ static int __init journal_init(void)
ret = journal_init_caches();
if (ret != 0)
jbd2_journal_destroy_caches();
- create_jbd_proc_entry();
+ jbd2_create_debugfs_entry();
return ret;
}
@@ -2078,7 +2065,7 @@ static void __exit journal_exit(void)
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/include/linux/jbd2.h
===
--- linux-2.6.orig/include/linux/jbd2.h 2007-07-11 09:46:25.0 -0500
+++ linux-2.6/include/linux/jbd2.h 2007-07-11 10:37:06.0 -0500
@@ -57,7 +57,7 @@
* CONFIG_JBD2_DEBUG is on.
*/
#define JBD_EXPENSIVE_CHECKING
-extern int jbd2_journal_enable_debug;
+extern u8 jbd2_journal_enable_debug;
#define jbd_debug(n, f, a...) \
do {\
Index: linux-2.6/fs/Kconfig
===
--- linux-2.6.orig/fs/Kconfig 2007-06-22 09:45:51.0 -0500
+++ linux-2.6/fs/Kc
Re: [EXT4 set 2][PATCH 5/5] cleanups: Export jbd2-debug via debugfs
On Wed, 11 Jul 2007 00:38:09 -0500 "Jose R. Santos" <[EMAIL PROTECTED]> wrote: > > > 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? All these changes are logically connected (aren't they?). A single patch is fine. > > 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. :) > I don't think that making it all per-superblock is worth the effort - it's a developer-only thing and developer will have the knowledge to test ext4 on an otherwise-ext3 setup if they're really fussed about the accuracy. So yes, a bare make-it-work patch sounds appropriate. Or remove it, but hey, it might be useful. The timestamping stuff certainly looks useful. - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [EXT4 set 2][PATCH 5/5] cleanups: Export jbd2-debug via debugfs
On Wed, 2007-07-11 at 00:38 -0500, Jose R. Santos wrote:
> On Tue, 10 Jul 2007 16:30:25 -0700
> Andrew Morton <[EMAIL PROTECTED]> wrote:
>
> > On Sun, 01 Jul 2007 03:36:48 -0400
> > Mingming Cao <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]>
> >
> > 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| 6220 +42 -0 !
> > > include/linux/jbd2.h |21 + 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.0 -0700
> > > +++ linux-2.6.22-rc4/fs/jbd2/journal.c2007-06-11 16:36:10.0
> > > -0700
> > > @@ -35,6 +35,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > >
> > > #include
> > > #include
> > > @@ -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. :)
>
Yes, I remember I did, that discovered some inconsistency in ext4 code,
which has already been fixed.
> > >
> > > -#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.h2007-06-11
> > > 16:16:18.0 -0700
> > > +++ linux-2.6.22-rc4/include/linux/jbd2.h 2007-06-11 16:35:25.0
> > > -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
Re: [EXT4 set 2][PATCH 5/5] cleanups: Export jbd2-debug via debugfs
On Tue, 10 Jul 2007 16:30:25 -0700
Andrew Morton <[EMAIL PROTECTED]> wrote:
> On Sun, 01 Jul 2007 03:36:48 -0400
> Mingming Cao <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]>
>
> 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| 6220 +42 -0 !
> > include/linux/jbd2.h |21 + 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.0
> > -0700
> > +++ linux-2.6.22-rc4/fs/jbd2/journal.c 2007-06-11 16:36:10.0
> > -0700
> > @@ -35,6 +35,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > @@ -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.0 -0700
> > +++ linux-2.6.22-rc4/include/linux/jbd2.h 2007-06-11 16:35:25.0
> > -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 "un
Re: [EXT4 set 2][PATCH 5/5] cleanups: Export jbd2-debug via debugfs
On Sun, 01 Jul 2007 03:36:48 -0400
Mingming Cao <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]>
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| 6220 +42 -0 !
> include/linux/jbd2.h |21 + 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.0
> -0700
> +++ linux-2.6.22-rc4/fs/jbd2/journal.c2007-06-11 16:36:10.0
> -0700
> @@ -35,6 +35,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -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.h2007-06-11
> 16:16:18.0 -0700
> +++ linux-2.6.22-rc4/include/linux/jbd2.h 2007-06-11 16:35:25.0
> -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-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
