Patchwork [1/9] ext4: Add -DDEBUG to Makefile

login
register
mail settings
Submitter Joe Perches
Date March 16, 2012, 12:07 a.m.
Message ID <7ee60f3b750f4500f9bdcb64f358acbf17987036.1331856300.git.joe@perches.com>
Download mbox | patch
Permalink /patch/147111/
State Rejected
Headers show

Comments

Joe Perches - March 16, 2012, 12:07 a.m.
Add -DDEBUG to enable future use of pr_debug.
No changes to objects as no DEBUG uses currently exist.

Signed-off-by: Joe Perches <joe@perches.com>
---
 fs/ext4/Makefile |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
David Daney - March 16, 2012, 4:03 p.m.
On 03/15/2012 05:07 PM, Joe Perches wrote:
> Add -DDEBUG to enable future use of pr_debug.
> No changes to objects as no DEBUG uses currently exist.
>
> Signed-off-by: Joe Perches<joe@perches.com>
> ---
>   fs/ext4/Makefile |    2 ++
>   1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/fs/ext4/Makefile b/fs/ext4/Makefile
> index 56fd8f86..617a5d8 100644
> --- a/fs/ext4/Makefile
> +++ b/fs/ext4/Makefile
> @@ -2,6 +2,8 @@
>   # Makefile for the linux ext4-filesystem routines.
>   #
>
> +ccflags-$(CONFIG_EXT4_FS) := -DDEBUG

In many other susbsystems/drivers, the definition of DEBUG is gated by a 
separate Kconfig symbol used to select debugging just for that 
susbsystem/driver (see CONFIG_MMC_DEBUG for example).

Why aren't you doing the same here?

David Daney
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joe Perches - March 16, 2012, 4:29 p.m.
On Fri, 2012-03-16 at 09:03 -0700, David Daney wrote:
> On 03/15/2012 05:07 PM, Joe Perches wrote:
> > Add -DDEBUG to enable future use of pr_debug.
> > No changes to objects as no DEBUG uses currently exist.
[]
> > diff --git a/fs/ext4/Makefile b/fs/ext4/Makefile
[]
> > +ccflags-$(CONFIG_EXT4_FS) := -DDEBUG

> In many other susbsystems/drivers, the definition of DEBUG is gated by a 
> separate Kconfig symbol used to select debugging just for that 
> susbsystem/driver (see CONFIG_MMC_DEBUG for example).
> 
> Why aren't you doing the same here?

It can be done later with an additional symbol if
desired.

For now the current code uses printk(KERN_DEBUG and a
straight conversion to pr_debug would eliminate the
KERN_DEBUG the output.

pr_debug without #define DEBUG or dynamic_debug is
compiled away to nothing.


--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o - March 19, 2012, 4:39 a.m.
On Fri, Mar 16, 2012 at 09:03:57AM -0700, David Daney wrote:
> 
> In many other susbsystems/drivers, the definition of DEBUG is gated
> by a separate Kconfig symbol used to select debugging just for that
> susbsystem/driver (see CONFIG_MMC_DEBUG for example).

In ext4 we have many different debugging patches, and in general you'd
never want to enable them all at once (you'd get way too much noise).
The debugging statements are there when a developer is debugging very
specific section of code (i.e., the directory index, or the extent
tree, or the block allocator, etc.).  It's really only ext4 developers
who need to use those debugging statements, and even for them it's
quite rare.

So I've never considered it worthwhile to enable them via a CONFIG_*
menu item; developers who are debugging a specific problem will
generally just drop in the specific #define on an ad hoc basis, and
that works fine.

					- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joe Perches - March 19, 2012, 4:26 p.m.
On Mon, 2012-03-19 at 00:39 -0400, Ted Ts'o wrote:
> So I've never considered it worthwhile to enable them via a CONFIG_*
> menu item; developers who are debugging a specific problem will
> generally just drop in the specific #define on an ad hoc basis, and
> that works fine.

CONFIG_EXT4_DEBUG_MASK=value and/or a module_param(debug) is
possible to add but as Ted said, it's not really too valuable.

dynamic_debugging the always compiled-in current KERN_DEBUG
or eliminating them altogether might have some value though.

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
ddaney.cavm@gmail.com - March 19, 2012, 6:48 p.m.
On 03/18/2012 09:39 PM, Ted Ts'o wrote:
> On Fri, Mar 16, 2012 at 09:03:57AM -0700, David Daney wrote:
>>
>> In many other susbsystems/drivers, the definition of DEBUG is gated
>> by a separate Kconfig symbol used to select debugging just for that
>> susbsystem/driver (see CONFIG_MMC_DEBUG for example).
>
> In ext4 we have many different debugging patches, and in general you'd
> never want to enable them all at once (you'd get way too much noise).
> The debugging statements are there when a developer is debugging very
> specific section of code (i.e., the directory index, or the extent
> tree, or the block allocator, etc.).  It's really only ext4 developers
> who need to use those debugging statements, and even for them it's
> quite rare.
>
> So I've never considered it worthwhile to enable them via a CONFIG_*
> menu item; developers who are debugging a specific problem will
> generally just drop in the specific #define on an ad hoc basis, and
> that works fine.
>

If you want to unconditionally define DEBUG for all of ext4, that is 
fine with me.

I was just noting that it seemed a little odd to me, and different from 
what is done elsewhere.

Thanks,
David Daney
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o - March 20, 2012, 1:05 a.m.
On Mon, Mar 19, 2012 at 11:48:12AM -0700, David Daney wrote:
> 
> If you want to unconditionally define DEBUG for all of ext4, that is
> fine with me.

Since I don't see the value of pr_debug, and don't plan to use it, I
don't plan to unconditionally define DEBUG.  :-)

					- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/ext4/Makefile b/fs/ext4/Makefile
index 56fd8f86..617a5d8 100644
--- a/fs/ext4/Makefile
+++ b/fs/ext4/Makefile
@@ -2,6 +2,8 @@ 
 # Makefile for the linux ext4-filesystem routines.
 #
 
+ccflags-$(CONFIG_EXT4_FS) := -DDEBUG
+
 obj-$(CONFIG_EXT4_FS) += ext4.o
 
 ext4-y	:= balloc.o bitmap.o dir.o file.o fsync.o ialloc.o inode.o page-io.o \