cifs: Rename cERROR and cifserror to cifs_vfs_err

Commit Message

Joe Perches March 13, 2013, 12:40 p.m. UTC

On Wed, 2013-03-13 at 07:51 -0400, Jeff Layton wrote:
> On Wed, 13 Mar 2013 04:36:54 -0700
> Joe Perches <[e-mail]> wrote:
> > Perhaps a better idea than this patch is to
> > change both the cERROR and cFYI macros to
> > a new use of cifs_dbg(type, fmt, ...)
> > 
> > cERROR(set, fmt, ...)   -> cifs_dbg(VFS, fmt, ...)
> > cFYI(set, fmt, ...)     -> cifs_dbg(FYI, fmt, ...)
> > 
> > This conversion would mark both these macros
> > as debug stataments as they are only enabled
> > with CONFIG_CIFS_DEBUG.
[]
> > This would also enable an easier conversion to
> > dynamic debugging of these debug macros.
> > 
> > I'd prefer to move the newline from the macro
> > to the format as that is more consistent with
> > the rest of the kernel.
[]
> I like this change overall, but the size of the patch is pretty
> daunting.

I'd characterize it as more mechanically dull than
daunting, but it is awfully large (~300 KB).

> If you could change the code that underlies cERROR() and
> cFYI() without needing to touch all of their call sites, it might be
> a simpler initial step.

I think that would not help.

> OTOH, I would also prefer to move the newline into the format and
> that's impossible without touching most of these call sites.

Well, all but the ones that already have a defective
newline.  I think there are 4 or 5.

The .ko size increases a few hundred bytes when the
newlines are moved to the format, though it's still
about a 1% overall size reduction.

There would be:

264 uses of cifs_dbg(VFS, ...)
622 uses of cifs_dbg(FYI, ...)
 15 uses of cifs_dbg(NOISY, ...)

to verify. Using strings on the .ko simplifies that.

$ size fs/cifs/cifs.ko*
   text	   data	    bss	    dec	    hex	filename
 265891	   2525	    132	 268548	  41904	fs/cifs/cifs.ko.new
 268359	   2525	    132	 271016	  422a8	fs/cifs/cifs.ko.old

The core of it is to cifs_debug.h
---
 fs/cifs/cifs_debug.h | 70 +++++++++++++++++-----------------------------------
 1 file changed, 23 insertions(+), 47 deletions(-)



--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Jeff Layton March 15, 2013, 6:56 p.m. UTC | #1

On Thu, 14 Mar 2013 12:24:37 -0700
Joe Perches <[e-mail]> wrote:

> It's not obvious from reading the macro names that these macros
> are for debugging.  Convert the names to a single more typical
> kernel style cifs_dbg macro.
> 
> 	cERROR(1, ...)   -> cifs_dbg(VFS, ...)
> 	cFYI(1, ...)     -> cifs_dbg(FYI, ...)
> 	cFYI(DBG2, ...)  -> cifs_dbg(NOISY, ...)
> 
> Move the terminating format newline from the macro to the call site.
> 
> Add CONFIG_CIFS_DEBUG function cifs_vfs_err to emit the
> "CIFS VFS: " prefix for VFS messages.
> 
> Size is reduced ~ 1% when CONFIG_CIFS_DEBUG is set (default y)
> 
> $ size fs/cifs/cifs.ko*
>    text    data     bss     dec     hex filename
>  265245	   2525	    132	 267902	  4167e	fs/cifs/cifs.ko.new
>  268359    2525     132  271016   422a8 fs/cifs/cifs.ko.old
> 


This all looks like good stuff. I am a bit concerned about mashing all
of these cleanups into the same patch though.
> Other miscellaneous changes around these conversions:
> 
> o Miscellaneous typo fixes
> o Add terminating \n's to almost all formats and remove them
>   from the macros to be more kernel style like.  A few formats
>   previously had defective \n's
> o Remove unnecessary OOM messages as kmalloc() calls dump_stack
> o Coalesce formats to make grep easier,
>   added missing spaces when coalescing formats
> o Use %s, __func__ instead of embedded function name
> o Removed unnecessary "cifs: " prefixes
> o Convert kzalloc with multiply to kcalloc
> o Remove unused cifswarn macro
>

Jeff Layton March 15, 2013, 7:01 p.m. UTC | #2

On Thu, 14 Mar 2013 12:24:37 -0700
Joe Perches <[e-mail]> wrote:

> It's not obvious from reading the macro names that these macros
> are for debugging.  Convert the names to a single more typical
> kernel style cifs_dbg macro.
> 
> 	cERROR(1, ...)   -> cifs_dbg(VFS, ...)
> 	cFYI(1, ...)     -> cifs_dbg(FYI, ...)
> 	cFYI(DBG2, ...)  -> cifs_dbg(NOISY, ...)
> 
> Move the terminating format newline from the macro to the call site.
> 
> Add CONFIG_CIFS_DEBUG function cifs_vfs_err to emit the
> "CIFS VFS: " prefix for VFS messages.
> 
> Size is reduced ~ 1% when CONFIG_CIFS_DEBUG is set (default y)
> 
> $ size fs/cifs/cifs.ko*
>    text    data     bss     dec     hex filename
>  265245	   2525	    132	 267902	  4167e	fs/cifs/cifs.ko.new
>  268359    2525     132  271016   422a8 fs/cifs/cifs.ko.old
> 

(my apologies -- my MUA has a mind of its own sometimes)

This all looks like good stuff. I am a bit concerned about mashing all
of these cleanups into the same patch though.

> Other miscellaneous changes around these conversions:
> 
> o Miscellaneous typo fixes
> o Add terminating \n's to almost all formats and remove them
>   from the macros to be more kernel style like.  A few formats
>   previously had defective \n's
> o Remove unnecessary OOM messages as kmalloc() calls dump_stack
> o Coalesce formats to make grep easier,
>   added missing spaces when coalescing formats
> o Use %s, __func__ instead of embedded function name
> o Removed unnecessary "cifs: " prefixes

> o Convert kzalloc with multiply to kcalloc
	^^^
Things like this really ought to be a separate patch, even though it is
a trivial change. That's a minor nit though...
 
> o Remove unused cifswarn macro
> 

I think we ought to go ahead and take this for 3.10. I do have some
minor concern about having to deal with backports of later patches to
kernels that don't have these changes, but hey, that's the price of
dealing with old kernels.

The sooner Steve merges this into his for-next tree, the better. This
bound to give us all sorts of merge conflicts for the 3.10 window, so
we want to make sure that people know what to base their work on.

Acked-by: Jeff Layton <[e-mail]>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Joe Perches March 15, 2013, 8:43 p.m. UTC | #3

On Fri, 2013-03-15 at 15:16 -0500, Steve French wrote:

> I would like to merge the three we have (one still has to be put in
> for-next - the one from Jeff) for for-next in the next three or fouir
> days - but I can create another temporary branch for the ones for 3.10
> - but ... I would like to simplify things by merging the other cleanup
> patches we already have first.  I agree that we are going to have to
> let people know to rebase everything for cifs for 3.10 on this big
> cleanup patch.

Another possibility is to extend the
cifs_dbg(type, fmt. ...) types from

VFS, FYI, NOISY

so that the messages could be
filtered/classified better.

Also, perhaps it'd be better to use
pr_debug instead of KERN_ERR for
the VFS messages.


--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Steve French March 15, 2013, 9:46 p.m. UTC | #4

On Fri, Mar 15, 2013 at 2:01 PM, Jeff Layton <[e-mail]> wrote:
>
> On Thu, 14 Mar 2013 12:24:37 -0700
> Joe Perches <[e-mail]> wrote:
>
> > It's not obvious from reading the macro names that these macros
> > are for debugging.  Convert the names to a single more typical
> > kernel style cifs_dbg macro.
> >
> >       cERROR(1, ...)   -> cifs_dbg(VFS, ...)
> >       cFYI(1, ...)     -> cifs_dbg(FYI, ...)
> >       cFYI(DBG2, ...)  -> cifs_dbg(NOISY, ...)
> >
> > Move the terminating format newline from the macro to the call site.
> >
> > Add CONFIG_CIFS_DEBUG function cifs_vfs_err to emit the
> > "CIFS VFS: " prefix for VFS messages.
> >
> > Size is reduced ~ 1% when CONFIG_CIFS_DEBUG is set (default y)
> >
> > $ size fs/cifs/cifs.ko*
> >    text    data     bss     dec     hex filename
> >  265245          2525     132  267902   4167e fs/cifs/cifs.ko.new
> >  268359    2525     132  271016   422a8 fs/cifs/cifs.ko.old
> >
>
> (my apologies -- my MUA has a mind of its own sometimes)
>
> This all looks like good stuff. I am a bit concerned about mashing all
> of these cleanups into the same patch though.
>
> > Other miscellaneous changes around these conversions:
> >
> > o Miscellaneous typo fixes
> > o Add terminating \n's to almost all formats and remove them
> >   from the macros to be more kernel style like.  A few formats
> >   previously had defective \n's
> > o Remove unnecessary OOM messages as kmalloc() calls dump_stack
> > o Coalesce formats to make grep easier,
> >   added missing spaces when coalescing formats
> > o Use %s, __func__ instead of embedded function name
> > o Removed unnecessary "cifs: " prefixes
>
> > o Convert kzalloc with multiply to kcalloc
>         ^^^
> Things like this really ought to be a separate patch, even though it is
> a trivial change. That's a minor nit though...
>
> > o Remove unused cifswarn macro
> >
>
> I think we ought to go ahead and take this for 3.10. I do have some
> minor concern about having to deal with backports of later patches to
> kernels that don't have these changes, but hey, that's the price of
> dealing with old kernels.
>
> The sooner Steve merges this into his for-next tree, the better. This
> bound to give us all sorts of merge conflicts for the 3.10 window, so
> we want to make sure that people know what to base their work on.

I would like to merge the three we have (one still has to be put in for-next
- the one from Jeff) for for-next in the next three or fouir days - but I
can create another temporary branch for the ones for 3.10 - but ... I would
like to simplify things by merging the other cleanup patches we already have
first.  I agree that we are going to have to let people know to rebase
everything for cifs for 3.10 on this big cleanup patch.

--
Thanks,

Steve



--
Thanks,

Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Steve French March 25, 2013, 4:44 a.m. UTC | #5

On Fri, Mar 15, 2013 at 3:43 PM, Joe Perches <[e-mail]> wrote:
> On Fri, 2013-03-15 at 15:16 -0500, Steve French wrote:
>
>> I would like to merge the three we have (one still has to be put in
>> for-next - the one from Jeff) for for-next in the next three or fouir
>> days - but I can create another temporary branch for the ones for 3.10
>> - but ... I would like to simplify things by merging the other cleanup
>> patches we already have first.  I agree that we are going to have to
>> let people know to rebase everything for cifs for 3.10 on this big
>> cleanup patch.
>
> Another possibility is to extend the
> cifs_dbg(type, fmt. ...) types from
>
> VFS, FYI, NOISY
>
> so that the messages could be
> filtered/classified better.
>
> Also, perhaps it'd be better to use
> pr_debug instead of KERN_ERR for
> the VFS messages.

Although it required hand merging, this large patch is now merged into
cifs-2.6.git (for-next branch, destined for 3.10) but ... I only
merged three obvious other patches (destined for 3.10) before it

4521ba9ca4138f424769f17ae7166ff5ccf08805 fs: cifs: use kmemdup instead
of kmalloc + memcpy
41057e5a5d673015fc0d0e587e993d26f1818b1c cifs: replaced kmalloc +
memset with kzalloc
8f43465d17982767dd5b3f5d48216ef5c100c274 cifs: ignore the unc= and
prefixpath= mount options

and I merged one so far after it that Jeff had already rebased ontop
of Joe's large patch

457d2157ec2da6f9fa755e16ab740979f581e328 cifs: remove ENOSPC handling
in smb_sendv

So ... going forward for 3.10 make sure to fixup subsequent new cifs
patches ontop Joe's large "cifs: Rename cERROR and cFYI to cifs_dbg"
patch or else it will make merging a mess since Joe's patch changes
just about very cifs file due to the macro renaming.

Patch

diff --git a/fs/cifs/cifs_debug.h b/fs/cifs/cifs_debug.h
index 69ae3d3..c99b40f 100644
--- a/fs/cifs/cifs_debug.h
+++ b/fs/cifs/cifs_debug.h
@@ -25,18 +25,20 @@ 
 void cifs_dump_mem(char *label, void *data, int length);
 void cifs_dump_detail(void *);
 void cifs_dump_mids(struct TCP_Server_Info *);
-#ifdef CONFIG_CIFS_DEBUG2
-#define DBG2 2
-#else
-#define DBG2 0
-#endif
 extern int traceSMB;		/* flag which enables the function below */
 void dump_smb(void *, int);
 #define CIFS_INFO	0x01
 #define CIFS_RC		0x02
 #define CIFS_TIMER	0x04
 
+#define VFS 1
+#define FYI 2
 extern int cifsFYI;
+#ifdef CONFIG_CIFS_DEBUG2
+#define NOISY 4
+#else
+#define NOISY 0
+#endif
 
 /*
  *	debug ON
@@ -44,31 +46,21 @@  extern int cifsFYI;
  */
 #ifdef CONFIG_CIFS_DEBUG
 
-/* information message: e.g., configuration, major event */
-#define cifsfyi(fmt, ...)						\
-do {									\
-	if (cifsFYI & CIFS_INFO)					\
-		printk(KERN_DEBUG "%s: " fmt "\n",			\
-		       __FILE__, ##__VA_ARGS__);			\
-} while (0)
-
-#define cFYI(set, fmt, ...)						\
-do {									\
-	if (set)							\
-		cifsfyi(fmt, ##__VA_ARGS__);				\
-} while (0)
+__printf(1, 2) void cifs_vfs_err(const char *fmt, ...);
 
-#define cifswarn(fmt, ...)						\
-	printk(KERN_WARNING fmt "\n", ##__VA_ARGS__)
-
-/* error event message: e.g., i/o error */
-#define cifserror(fmt, ...)						\
-	printk(KERN_ERR "CIFS VFS: " fmt "\n", ##__VA_ARGS__);		\
-
-#define cERROR(set, fmt, ...)						\
+/* information message: e.g., configuration, major event */
+#define cifs_dbg(type, fmt, ...)					\
 do {									\
-	if (set)							\
-		cifserror(fmt, ##__VA_ARGS__);				\
+	if (type == FYI) {						\
+		if (cifsFYI & CIFS_INFO) {				\
+			printk(KERN_DEBUG "%s: " fmt,			\
+			       __FILE__, ##__VA_ARGS__);		\
+		}							\
+	} else if (type == VFS) {					\
+		cifs_vfs_err(fmt, ##__VA_ARGS__);			\
+	} else if (type == NOISY && type != 0) {			\
+		printk(KERN_DEBUG fmt, ##__VA_ARGS__);			\
+	}								\
 } while (0)
 
 /*
@@ -76,27 +68,11 @@  do {									\
  *	---------
  */
 #else		/* _CIFS_DEBUG */
-#define cifsfyi(fmt, ...)						\
+#define cifs_dbg(type, fmt, ...)					\
 do {									\
 	if (0)								\
-		printk(KERN_DEBUG "%s: " fmt "\n",			\
-		       __FILE__, ##__VA_ARGS__);			\
+		printk(KERN_DEBUG fmt, ##__VA_ARGS__);			\
 } while (0)
-#define cFYI(set, fmt, ...)						\
-do {									\
-	if (0 && set)							\
-		cifsfyi(fmt, ##__VA_ARGS__);				\
-} while (0)
-#define cifserror(fmt, ...)						\
-do {									\
-	if (0)								\
-		printk(KERN_ERR "CIFS VFS: " fmt "\n", ##__VA_ARGS__);	\
-} while (0)
-#define cERROR(set, fmt, ...)						\
-do {									\
-	if (0 && set)							\
-		cifserror(fmt, ##__VA_ARGS__);				\
-} while (0)
-#endif		/* _CIFS_DEBUG */
+#endif
 
 #endif				/* _H_CIFS_DEBUG */