[PATCH] avoid memory allocations in xfs_fs_vcmn_err

[PATCH] avoid memory allocations in xfs_fs_vcmn_err

[Christoph Hellwig]

xfs_fs_vcmn_err can be called under a spinlock, but does a sleeping memory
allocation to create buffer for it's internal sprintf.  Fortunately it's
the only caller of icmn_err, so we can merge the two and have one single
static buffer and spinlock protecting it.  While we're at it make sure
we proper __attribute__ format annotations so that the compiler can detect
mismatched format strings.


Reported-by: Alexander Beregalov <a.beregalov@gmail.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/support/debug.c
===================================================================
--- xfs.orig/fs/xfs/support/debug.c	2008-12-17 14:59:49.000000000 +0100
+++ xfs/fs/xfs/support/debug.c	2008-12-17 15:10:13.000000000 +0100
@@ -18,6 +18,13 @@
 #include <xfs.h>
 #include "debug.h"
 
+/* xfs_mount.h drags a lot of crap in, sorry.. */
+#include "xfs_sb.h"
+#include "xfs_inum.h"
+#include "xfs_ag.h"
+#include "xfs_dmapi.h"
+#include "xfs_mount.h"
+
 static char		message[1024];	/* keep it off the stack */
 static DEFINE_SPINLOCK(xfs_err_lock);
 
@@ -55,22 +62,42 @@ cmn_err(register int level, char *fmt, .
 }
 
 void
-icmn_err(register int level, char *fmt, va_list ap)
+xfs_fs_vcmn_err(
+	int			level,
+	struct xfs_mount	*mp,
+	char			*fmt,
+	va_list			ap)
 {
-	ulong	flags;
-	int	len;
+	unsigned long		flags;
+	int			len = 0;
 
 	level &= XFS_ERR_MASK;
-	if(level > XFS_MAX_ERR_LEVEL)
+	if (level > XFS_MAX_ERR_LEVEL)
 		level = XFS_MAX_ERR_LEVEL;
+
 	spin_lock_irqsave(&xfs_err_lock,flags);
-	len = vsnprintf(message, sizeof(message), fmt, ap);
+
+	if (mp) {
+		len = sprintf(message, "Filesystem \"%s\": ", mp->m_fsname);
+
+		/*
+		 * Skip the printk if we can't print anything useful
+		 * due to an over-long device name.
+		 */
+		if (len >= sizeof(message))
+			goto out;
+	}
+
+	len = vsnprintf(message + len, sizeof(message) - len, fmt, ap);
 	if (len >= sizeof(message))
 		len = sizeof(message) - 1;
 	if (message[len-1] == '\n')
 		message[len-1] = 0;
+
 	printk("%s%s\n", err_level[level], message);
+ out:
 	spin_unlock_irqrestore(&xfs_err_lock,flags);
+
 	BUG_ON(level == CE_PANIC);
 }
 
Index: xfs/fs/xfs/support/debug.h
===================================================================
--- xfs.orig/fs/xfs/support/debug.h	2008-12-17 15:05:34.000000000 +0100
+++ xfs/fs/xfs/support/debug.h	2008-12-17 15:07:03.000000000 +0100
@@ -27,8 +27,6 @@
 #define CE_ALERT        1               /* alert        */
 #define CE_PANIC        0               /* panic        */
 
-extern void icmn_err(int, char *, va_list)
-	__attribute__ ((format (printf, 2, 0)));
 extern void cmn_err(int, char *, ...)
 	__attribute__ ((format (printf, 2, 3)));
 extern void assfail(char *expr, char *f, int l);
Index: xfs/fs/xfs/xfs_error.c
===================================================================
--- xfs.orig/fs/xfs/xfs_error.c	2008-12-17 14:58:02.000000000 +0100
+++ xfs/fs/xfs/xfs_error.c	2008-12-17 15:12:03.000000000 +0100
@@ -153,21 +153,6 @@ xfs_errortag_clearall(xfs_mount_t *mp, i
 }
 #endif /* DEBUG */
 
-static void
-xfs_fs_vcmn_err(int level, xfs_mount_t *mp, char *fmt, va_list ap)
-{
-	if (mp != NULL) {
-		char	*newfmt;
-		int	len = 16 + mp->m_fsname_len + strlen(fmt);
-
-		newfmt = kmem_alloc(len, KM_SLEEP);
-		sprintf(newfmt, "Filesystem \"%s\": %s", mp->m_fsname, fmt);
-		icmn_err(level, newfmt, ap);
-		kmem_free(newfmt);
-	} else {
-		icmn_err(level, fmt, ap);
-	}
-}
 
 void
 xfs_fs_cmn_err(int level, xfs_mount_t *mp, char *fmt, ...)
Index: xfs/fs/xfs/xfs_error.h
===================================================================
--- xfs.orig/fs/xfs/xfs_error.h	2008-12-17 15:05:46.000000000 +0100
+++ xfs/fs/xfs/xfs_error.h	2008-12-17 15:12:01.000000000 +0100
@@ -159,11 +159,15 @@ extern int xfs_errortag_clearall(xfs_mou
 #define		XFS_PTAG_FSBLOCK_ZERO		0x00000080
 
 struct xfs_mount;
-/* PRINTFLIKE4 */
+
+extern void xfs_fs_vcmn_err(int level, struct xfs_mount *mp,
+		char *fmt, va_list ap)
+	__attribute__ ((format (printf, 3, 0)));
 extern void xfs_cmn_err(int panic_tag, int level, struct xfs_mount *mp,
-			char *fmt, ...);
-/* PRINTFLIKE3 */
-extern void xfs_fs_cmn_err(int level, struct xfs_mount *mp, char *fmt, ...);
+			char *fmt, ...)
+	__attribute__ ((format (printf, 4, 5)));
+extern void xfs_fs_cmn_err(int level, struct xfs_mount *mp, char *fmt, ...)
+	__attribute__ ((format (printf, 3, 4)));
 
 extern void xfs_hex_dump(void *p, int length);
 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] avoid memory allocations in xfs_fs_vcmn_err

[Eric Sandeen]

Christoph Hellwig wrote:

> +	if (mp) {
> +		len = sprintf(message, "Filesystem \"%s\": ", mp->m_fsname);
> +
> +		/*
> +		 * Skip the printk if we can't print anything useful
> +		 * due to an over-long device name.
> +		 */
> +		if (len >= sizeof(message))
> +			goto out;


Do we really want to drop the whole message if the fs name can't fit?
Maybe drop the fsname altogether and print the error anyway?  Being
completely silent doesn't sound great...

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] avoid memory allocations in xfs_fs_vcmn_err

[Christoph Hellwig]

On Wed, Dec 17, 2008 at 11:46:13AM -0600, Eric Sandeen wrote:
> Do we really want to drop the whole message if the fs name can't fit?
> Maybe drop the fsname altogether and print the error anyway?  Being
> completely silent doesn't sound great...

That's what we effecitvely do now, except that we still call snprintf.
I think it's an unlikely enough case that we don't need to worry about
it.  And if long enough device names become common we can just increase
the buffer size.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] avoid memory allocations in xfs_fs_vcmn_err

[Eric Sandeen]

Christoph Hellwig wrote:
> xfs_fs_vcmn_err can be called under a spinlock, but does a sleeping memory
> allocation to create buffer for it's internal sprintf.  Fortunately it's
> the only caller of icmn_err, so we can merge the two and have one single
> static buffer and spinlock protecting it.  While we're at it make sure
> we proper __attribute__ format annotations so that the compiler can detect
> mismatched format strings.
> 
> 
> Reported-by: Alexander Beregalov <a.beregalov@gmail.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Eric Sandeen <sandeen@sandeen.net>

> Index: xfs/fs/xfs/support/debug.c
> ===================================================================
> --- xfs.orig/fs/xfs/support/debug.c	2008-12-17 14:59:49.000000000 +0100
> +++ xfs/fs/xfs/support/debug.c	2008-12-17 15:10:13.000000000 +0100
> @@ -18,6 +18,13 @@
>  #include <xfs.h>
>  #include "debug.h"
>  
> +/* xfs_mount.h drags a lot of crap in, sorry.. */
> +#include "xfs_sb.h"
> +#include "xfs_inum.h"
> +#include "xfs_ag.h"
> +#include "xfs_dmapi.h"
> +#include "xfs_mount.h"
> +
>  static char		message[1024];	/* keep it off the stack */
>  static DEFINE_SPINLOCK(xfs_err_lock);
>  
> @@ -55,22 +62,42 @@ cmn_err(register int level, char *fmt, .
>  }
>  
>  void
> -icmn_err(register int level, char *fmt, va_list ap)
> +xfs_fs_vcmn_err(
> +	int			level,
> +	struct xfs_mount	*mp,
> +	char			*fmt,
> +	va_list			ap)
>  {
> -	ulong	flags;
> -	int	len;
> +	unsigned long		flags;
> +	int			len = 0;
>  
>  	level &= XFS_ERR_MASK;
> -	if(level > XFS_MAX_ERR_LEVEL)
> +	if (level > XFS_MAX_ERR_LEVEL)
>  		level = XFS_MAX_ERR_LEVEL;
> +
>  	spin_lock_irqsave(&xfs_err_lock,flags);
> -	len = vsnprintf(message, sizeof(message), fmt, ap);
> +
> +	if (mp) {
> +		len = sprintf(message, "Filesystem \"%s\": ", mp->m_fsname);
> +
> +		/*
> +		 * Skip the printk if we can't print anything useful
> +		 * due to an over-long device name.
> +		 */
> +		if (len >= sizeof(message))
> +			goto out;
> +	}
> +
> +	len = vsnprintf(message + len, sizeof(message) - len, fmt, ap);
>  	if (len >= sizeof(message))
>  		len = sizeof(message) - 1;
>  	if (message[len-1] == '\n')
>  		message[len-1] = 0;
> +
>  	printk("%s%s\n", err_level[level], message);
> + out:
>  	spin_unlock_irqrestore(&xfs_err_lock,flags);
> +
>  	BUG_ON(level == CE_PANIC);
>  }
>  
> Index: xfs/fs/xfs/support/debug.h
> ===================================================================
> --- xfs.orig/fs/xfs/support/debug.h	2008-12-17 15:05:34.000000000 +0100
> +++ xfs/fs/xfs/support/debug.h	2008-12-17 15:07:03.000000000 +0100
> @@ -27,8 +27,6 @@
>  #define CE_ALERT        1               /* alert        */
>  #define CE_PANIC        0               /* panic        */
>  
> -extern void icmn_err(int, char *, va_list)
> -	__attribute__ ((format (printf, 2, 0)));
>  extern void cmn_err(int, char *, ...)
>  	__attribute__ ((format (printf, 2, 3)));
>  extern void assfail(char *expr, char *f, int l);
> Index: xfs/fs/xfs/xfs_error.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_error.c	2008-12-17 14:58:02.000000000 +0100
> +++ xfs/fs/xfs/xfs_error.c	2008-12-17 15:12:03.000000000 +0100
> @@ -153,21 +153,6 @@ xfs_errortag_clearall(xfs_mount_t *mp, i
>  }
>  #endif /* DEBUG */
>  
> -static void
> -xfs_fs_vcmn_err(int level, xfs_mount_t *mp, char *fmt, va_list ap)
> -{
> -	if (mp != NULL) {
> -		char	*newfmt;
> -		int	len = 16 + mp->m_fsname_len + strlen(fmt);
> -
> -		newfmt = kmem_alloc(len, KM_SLEEP);
> -		sprintf(newfmt, "Filesystem \"%s\": %s", mp->m_fsname, fmt);
> -		icmn_err(level, newfmt, ap);
> -		kmem_free(newfmt);
> -	} else {
> -		icmn_err(level, fmt, ap);
> -	}
> -}
>  
>  void
>  xfs_fs_cmn_err(int level, xfs_mount_t *mp, char *fmt, ...)
> Index: xfs/fs/xfs/xfs_error.h
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_error.h	2008-12-17 15:05:46.000000000 +0100
> +++ xfs/fs/xfs/xfs_error.h	2008-12-17 15:12:01.000000000 +0100
> @@ -159,11 +159,15 @@ extern int xfs_errortag_clearall(xfs_mou
>  #define		XFS_PTAG_FSBLOCK_ZERO		0x00000080
>  
>  struct xfs_mount;
> -/* PRINTFLIKE4 */
> +
> +extern void xfs_fs_vcmn_err(int level, struct xfs_mount *mp,
> +		char *fmt, va_list ap)
> +	__attribute__ ((format (printf, 3, 0)));
>  extern void xfs_cmn_err(int panic_tag, int level, struct xfs_mount *mp,
> -			char *fmt, ...);
> -/* PRINTFLIKE3 */
> -extern void xfs_fs_cmn_err(int level, struct xfs_mount *mp, char *fmt, ...);
> +			char *fmt, ...)
> +	__attribute__ ((format (printf, 4, 5)));
> +extern void xfs_fs_cmn_err(int level, struct xfs_mount *mp, char *fmt, ...)
> +	__attribute__ ((format (printf, 3, 4)));
>  
>  extern void xfs_hex_dump(void *p, int length);
>  
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs