public inbox for linux-xfs@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH V2 0/3] Print when ENOSPC due to lack of inodes in non-inode64 mount.
@ 2012-09-11 22:13 raghu.prabhu13
  2012-09-11 22:13 ` [PATCH 1/3] Add ratelimited printk for different alert levels raghu.prabhu13
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: raghu.prabhu13 @ 2012-09-11 22:13 UTC (permalink / raw)
  To: xfs; +Cc: Raghavendra D Prabhu

From: Raghavendra D Prabhu <rprabhu@wnohang.net>

Currently, when there are no free inodes left / free space to allocate them (usually
without inode64), there is no indication anywhere of this case, making it harder
to diagnose this case. 

Hence, this series prints the causes/reasons to kernel log in a ratelimited
manner, when such a situation arises.

----- 
Version 1: Initially sent with pr_err_once to print the error only once.

Version 2: Added ratelimited functions to xfs_message.h instead of printing once
with pr_err_once on suggestion of Dave Chinner.

Raghavendra D Prabhu (3):
  Add ratelimited printk for different alert levels
  XFS: Print error when xfs_ialloc_ag_select fails to find continuous
    free space.
  XFS: Print error when unable to allocate inodes or out of free
    inodes.


 fs/xfs/xfs_ialloc.c  | 24 +++++++++++++++++++-----
 fs/xfs/xfs_message.h | 28 ++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 5 deletions(-)

-- 
1.7.12

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/3] Add ratelimited printk for different alert levels
  2012-09-11 22:13 [PATCH V2 0/3] Print when ENOSPC due to lack of inodes in non-inode64 mount raghu.prabhu13
@ 2012-09-11 22:13 ` raghu.prabhu13
  2012-09-11 22:43   ` Dave Chinner
  2012-09-12  3:22   ` Joe Perches
  2012-09-11 22:13 ` [PATCH 2/3] XFS: Print error when xfs_ialloc_ag_select fails to find continuous free space raghu.prabhu13
  2012-09-11 22:13 ` [PATCH 3/3] XFS: Print error when unable to allocate inodes or out of free inodes raghu.prabhu13
  2 siblings, 2 replies; 11+ messages in thread
From: raghu.prabhu13 @ 2012-09-11 22:13 UTC (permalink / raw)
  To: xfs; +Cc: Raghavendra D Prabhu, Alex Elder, Ben Myers, open list

From: Raghavendra D Prabhu <rprabhu@wnohang.net>

Ratelimited printk will be useful in printing xfs messages which are otherwise
not required to be printed always due to their high rate (to prevent kernel ring
buffer from overflowing), while at the same time required to be printed.

Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
---
 fs/xfs/xfs_message.h | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/fs/xfs/xfs_message.h b/fs/xfs/xfs_message.h
index 56dc0c1..87999a5 100644
--- a/fs/xfs/xfs_message.h
+++ b/fs/xfs/xfs_message.h
@@ -1,6 +1,8 @@
 #ifndef __XFS_MESSAGE_H
 #define __XFS_MESSAGE_H 1
 
+#include <linux/ratelimit.h>
+
 struct xfs_mount;
 
 extern __printf(2, 3)
@@ -30,6 +32,32 @@ void xfs_debug(const struct xfs_mount *mp, const char *fmt, ...)
 }
 #endif
 
+#define xfs_printk_ratelimited(xfs_printk, dev, fmt, ...)		\
+do {									\
+	static DEFINE_RATELIMIT_STATE(_rs,				\
+				      DEFAULT_RATELIMIT_INTERVAL,	\
+				      DEFAULT_RATELIMIT_BURST);		\
+	if (__ratelimit(&_rs))						\
+		xfs_printk(dev, fmt, ##__VA_ARGS__);			\
+} while (0)
+
+#define xfs_emerg_ratelimited(dev, fmt, ...)				\
+	xfs_printk_ratelimited(xfs_emerg, dev, fmt, ##__VA_ARGS__)
+#define xfs_alert_ratelimited(dev, fmt, ...)				\
+	xfs_printk_ratelimited(xfs_alert, dev, fmt, ##__VA_ARGS__)
+#define xfs_crit_ratelimited(dev, fmt, ...)				\
+	xfs_printk_ratelimited(xfs_crit, dev, fmt, ##__VA_ARGS__)
+#define xfs_err_ratelimited(dev, fmt, ...)				\
+	xfs_printk_ratelimited(xfs_err, dev, fmt, ##__VA_ARGS__)
+#define xfs_warn_ratelimited(dev, fmt, ...)				\
+	xfs_printk_ratelimited(xfs_warn, dev, fmt, ##__VA_ARGS__)
+#define xfs_notice_ratelimited(dev, fmt, ...)				\
+	xfs_printk_ratelimited(xfs_notice, dev, fmt, ##__VA_ARGS__)
+#define xfs_info_ratelimited(dev, fmt, ...)				\
+	xfs_printk_ratelimited(xfs_info, dev, fmt, ##__VA_ARGS__)
+#define xfs_dbg_ratelimited(dev, fmt, ...)				\
+	xfs_printk_ratelimited(xfs_dbg, dev, fmt, ##__VA_ARGS__)
+
 extern void assfail(char *expr, char *f, int l);
 
 extern void xfs_hex_dump(void *p, int length);
-- 
1.7.12

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/3] XFS: Print error when xfs_ialloc_ag_select fails to find continuous free space.
  2012-09-11 22:13 [PATCH V2 0/3] Print when ENOSPC due to lack of inodes in non-inode64 mount raghu.prabhu13
  2012-09-11 22:13 ` [PATCH 1/3] Add ratelimited printk for different alert levels raghu.prabhu13
@ 2012-09-11 22:13 ` raghu.prabhu13
  2012-09-11 22:48   ` Dave Chinner
  2012-09-11 22:13 ` [PATCH 3/3] XFS: Print error when unable to allocate inodes or out of free inodes raghu.prabhu13
  2 siblings, 1 reply; 11+ messages in thread
From: raghu.prabhu13 @ 2012-09-11 22:13 UTC (permalink / raw)
  To: xfs; +Cc: Raghavendra D Prabhu, Alex Elder, Ben Myers, open list

From: Raghavendra D Prabhu <rprabhu@wnohang.net>

When xfs_ialloc_ag_select fails to find any AG with continuous free blocks
required for inode allocation, printk the error in ratelimited manner.

Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
---
 fs/xfs/xfs_ialloc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_ialloc.c b/fs/xfs/xfs_ialloc.c
index 5aceb3f..e75a39d 100644
--- a/fs/xfs/xfs_ialloc.c
+++ b/fs/xfs/xfs_ialloc.c
@@ -539,8 +539,11 @@ nextag:
 		if (agno >= agcount)
 			agno = 0;
 		if (agno == pagno) {
-			if (flags == 0)
+			if (flags == 0) {
+				xfs_err_ratelimited(mp,
+					"Out of continuous free blocks for inode allocation");
 				return NULLAGNUMBER;
+			}
 			flags = 0;
 		}
 	}
-- 
1.7.12

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/3] XFS: Print error when unable to allocate inodes or out of free inodes.
  2012-09-11 22:13 [PATCH V2 0/3] Print when ENOSPC due to lack of inodes in non-inode64 mount raghu.prabhu13
  2012-09-11 22:13 ` [PATCH 1/3] Add ratelimited printk for different alert levels raghu.prabhu13
  2012-09-11 22:13 ` [PATCH 2/3] XFS: Print error when xfs_ialloc_ag_select fails to find continuous free space raghu.prabhu13
@ 2012-09-11 22:13 ` raghu.prabhu13
  2012-09-11 23:21   ` Dave Chinner
  2 siblings, 1 reply; 11+ messages in thread
From: raghu.prabhu13 @ 2012-09-11 22:13 UTC (permalink / raw)
  To: xfs; +Cc: Raghavendra D Prabhu, Alex Elder, Ben Myers, open list

From: Raghavendra D Prabhu <rprabhu@wnohang.net>

When xfs_dialloc is unable to allocate required number of inodes or there are no
AGs with free inodes, printk the error in ratelimited manner.

Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
---
 fs/xfs/xfs_ialloc.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_ialloc.c b/fs/xfs/xfs_ialloc.c
index e75a39d..034131b 100644
--- a/fs/xfs/xfs_ialloc.c
+++ b/fs/xfs/xfs_ialloc.c
@@ -990,8 +990,11 @@ xfs_dialloc(
 				goto out_error;
 
 			xfs_perag_put(pag);
-			*inop = NULLFSINO;
-			return 0;
+
+			xfs_err_ratelimited(mp,
+				"Unable to allocate inodes in AG %d: Required %d, Current %llu, Maximum %llu",
+				agno, XFS_IALLOC_INODES(mp), mp->m_sb.sb_icount, mp->m_maxicount);
+			goto out_spc;
 		}
 
 		if (ialloced) {
@@ -1016,11 +1019,19 @@ nextag:
 		if (++agno == mp->m_sb.sb_agcount)
 			agno = 0;
 		if (agno == start_agno) {
-			*inop = NULLFSINO;
-			return noroom ? ENOSPC : 0;
+			if (noroom) {
+				xfs_err_ratelimited(mp,
+					"Out of AGs with free inodes: Required %d, Current %llu, Maximum %llu",
+					 XFS_IALLOC_INODES(mp), mp->m_sb.sb_icount, mp->m_maxicount);
+				goto out_spc;
+			}
+			return 0;
 		}
 	}
 
+out_spc:
+	*inop = NULLFSINO;
+	return ENOSPC;
 out_alloc:
 	*IO_agbp = NULL;
 	return xfs_dialloc_ag(tp, agbp, parent, inop);
-- 
1.7.12

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] Add ratelimited printk for different alert levels
  2012-09-11 22:13 ` [PATCH 1/3] Add ratelimited printk for different alert levels raghu.prabhu13
@ 2012-09-11 22:43   ` Dave Chinner
  2012-09-12  3:22   ` Joe Perches
  1 sibling, 0 replies; 11+ messages in thread
From: Dave Chinner @ 2012-09-11 22:43 UTC (permalink / raw)
  To: raghu.prabhu13
  Cc: open list, Raghavendra D Prabhu, Alex Elder, Ben Myers, xfs

On Wed, Sep 12, 2012 at 03:43:22AM +0530, raghu.prabhu13@gmail.com wrote:
> From: Raghavendra D Prabhu <rprabhu@wnohang.net>
> 
> Ratelimited printk will be useful in printing xfs messages which are otherwise
> not required to be printed always due to their high rate (to prevent kernel ring
> buffer from overflowing), while at the same time required to be printed.
> 
> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
> ---
>  fs/xfs/xfs_message.h | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/fs/xfs/xfs_message.h b/fs/xfs/xfs_message.h
> index 56dc0c1..87999a5 100644
> --- a/fs/xfs/xfs_message.h
> +++ b/fs/xfs/xfs_message.h
> @@ -1,6 +1,8 @@
>  #ifndef __XFS_MESSAGE_H
>  #define __XFS_MESSAGE_H 1
>  
> +#include <linux/ratelimit.h>
> +

Include this in xfs_linux.h rather than here.

>  struct xfs_mount;
>  
>  extern __printf(2, 3)
> @@ -30,6 +32,32 @@ void xfs_debug(const struct xfs_mount *mp, const char *fmt, ...)
>  }
>  #endif
>  
> +#define xfs_printk_ratelimited(xfs_printk, dev, fmt, ...)		\
> +do {									\
> +	static DEFINE_RATELIMIT_STATE(_rs,				\
> +				      DEFAULT_RATELIMIT_INTERVAL,	\
> +				      DEFAULT_RATELIMIT_BURST);		\
> +	if (__ratelimit(&_rs))						\
> +		xfs_printk(dev, fmt, ##__VA_ARGS__);			\
> +} while (0)

Use "func" not xfs_printk here. xfs_printk looks too much like a
real function name (indeed, we already have __xfs_printk) rather
than a macro parameter.

> +#define xfs_emerg_ratelimited(dev, fmt, ...)				\
> +	xfs_printk_ratelimited(xfs_emerg, dev, fmt, ##__VA_ARGS__)
> +#define xfs_alert_ratelimited(dev, fmt, ...)				\
> +	xfs_printk_ratelimited(xfs_alert, dev, fmt, ##__VA_ARGS__)
> +#define xfs_crit_ratelimited(dev, fmt, ...)				\
> +	xfs_printk_ratelimited(xfs_crit, dev, fmt, ##__VA_ARGS__)
> +#define xfs_err_ratelimited(dev, fmt, ...)				\
> +	xfs_printk_ratelimited(xfs_err, dev, fmt, ##__VA_ARGS__)
> +#define xfs_warn_ratelimited(dev, fmt, ...)				\
> +	xfs_printk_ratelimited(xfs_warn, dev, fmt, ##__VA_ARGS__)
> +#define xfs_notice_ratelimited(dev, fmt, ...)				\
> +	xfs_printk_ratelimited(xfs_notice, dev, fmt, ##__VA_ARGS__)
> +#define xfs_info_ratelimited(dev, fmt, ...)				\
> +	xfs_printk_ratelimited(xfs_info, dev, fmt, ##__VA_ARGS__)
> +#define xfs_dbg_ratelimited(dev, fmt, ...)				\
> +	xfs_printk_ratelimited(xfs_dbg, dev, fmt, ##__VA_ARGS__)

Here's the problem with adding macros that aren't used. xfs_dbg
does not exist - the function is xfs_debug(). The compiler won't
catch that until the macro is used, so only add the macros which are
needed for this patch series.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] XFS: Print error when xfs_ialloc_ag_select fails to find continuous free space.
  2012-09-11 22:13 ` [PATCH 2/3] XFS: Print error when xfs_ialloc_ag_select fails to find continuous free space raghu.prabhu13
@ 2012-09-11 22:48   ` Dave Chinner
  0 siblings, 0 replies; 11+ messages in thread
From: Dave Chinner @ 2012-09-11 22:48 UTC (permalink / raw)
  To: raghu.prabhu13
  Cc: open list, Raghavendra D Prabhu, Alex Elder, Ben Myers, xfs

On Wed, Sep 12, 2012 at 03:43:23AM +0530, raghu.prabhu13@gmail.com wrote:
> From: Raghavendra D Prabhu <rprabhu@wnohang.net>
> 
> When xfs_ialloc_ag_select fails to find any AG with continuous free blocks
> required for inode allocation, printk the error in ratelimited manner.
> 
> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
> ---
>  fs/xfs/xfs_ialloc.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_ialloc.c b/fs/xfs/xfs_ialloc.c
> index 5aceb3f..e75a39d 100644
> --- a/fs/xfs/xfs_ialloc.c
> +++ b/fs/xfs/xfs_ialloc.c
> @@ -539,8 +539,11 @@ nextag:
>  		if (agno >= agcount)
>  			agno = 0;
>  		if (agno == pagno) {
> -			if (flags == 0)
> +			if (flags == 0) {
> +				xfs_err_ratelimited(mp,
> +					"Out of continuous free blocks for inode allocation");

http://oss.sgi.com/archives/xfs/2012-06/msg00041.html

<quote>
Couple of things for all 3 patches. Firstly - 80 columns. We tend
to keep the pformat string on a single line so it is easy to grep
for like so:

				pr_err_once(mp,
		"Insufficient contiguous free space for inode allocation");
</quote>

So, you need to change the error message to the one suggested, and
follow 80-character width limits like the rest of the code.

Also, I think the error message is better at the caller site, not in
the function itself. i.e. if we get a NULLAGNUMBER returned, the
caller decided whether to emit an error message or not.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] XFS: Print error when unable to allocate inodes or out of free inodes.
  2012-09-11 22:13 ` [PATCH 3/3] XFS: Print error when unable to allocate inodes or out of free inodes raghu.prabhu13
@ 2012-09-11 23:21   ` Dave Chinner
  2012-09-21  7:16     ` Raghavendra D Prabhu
  0 siblings, 1 reply; 11+ messages in thread
From: Dave Chinner @ 2012-09-11 23:21 UTC (permalink / raw)
  To: raghu.prabhu13
  Cc: open list, Raghavendra D Prabhu, Alex Elder, Ben Myers, xfs

On Wed, Sep 12, 2012 at 03:43:24AM +0530, raghu.prabhu13@gmail.com wrote:
> From: Raghavendra D Prabhu <rprabhu@wnohang.net>
> 
> When xfs_dialloc is unable to allocate required number of inodes or there are no
> AGs with free inodes, printk the error in ratelimited manner.
> 
> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
> ---
>  fs/xfs/xfs_ialloc.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ialloc.c b/fs/xfs/xfs_ialloc.c
> index e75a39d..034131b 100644
> --- a/fs/xfs/xfs_ialloc.c
> +++ b/fs/xfs/xfs_ialloc.c
> @@ -990,8 +990,11 @@ xfs_dialloc(
>  				goto out_error;
>  
>  			xfs_perag_put(pag);
> -			*inop = NULLFSINO;
> -			return 0;
> +
> +			xfs_err_ratelimited(mp,
> +				"Unable to allocate inodes in AG %d: Required %d, Current %llu, Maximum %llu",
> +				agno, XFS_IALLOC_INODES(mp), mp->m_sb.sb_icount, mp->m_maxicount);
> +			goto out_spc;

This changes the error to be returned from 0 to ENOSPC. Adding error
messages shouldn't change the logic of the code.

Also, you might want tolook at how ENOSPC is returned from
xfs_ialloc_ag_alloc(). it only occurs when:

	if (mp->m_maxicount &&
            mp->m_sb.sb_icount + newlen > mp->m_maxicount) {

i.e. it is exactly the same error case as the "noroom" error below.
It has nothing to do with being unable to allocate inodes in the
specific AG - the global inode count is too high. IOWs, the error
message is not correct.

Also, 80 columns.

>  		}
>  
>  		if (ialloced) {
> @@ -1016,11 +1019,19 @@ nextag:
>  		if (++agno == mp->m_sb.sb_agcount)
>  			agno = 0;
>  		if (agno == start_agno) {
> -			*inop = NULLFSINO;
> -			return noroom ? ENOSPC : 0;
> +			if (noroom) {
> +				xfs_err_ratelimited(mp,
> +					"Out of AGs with free inodes: Required %d, Current %llu, Maximum %llu",
> +					 XFS_IALLOC_INODES(mp), mp->m_sb.sb_icount, mp->m_maxicount);

The error message here is misleading - the error is that we've
exceeded the maximum inode count for the filesystem (same as the
above error message case), so no further allocations are allowed.

What about the !noroom case? Isn't that a real ENOSPC condition?
i.e. we've tried to allocate inodes in every AG and yet we've failed
in all of them because there is no aligned, contiguous free space in
any of the AGs. Shouldn't that emit an appropriate warning?

> +				goto out_spc;
> +			}
> +			return 0;
>  		}
>  	}
>  
> +out_spc:
> +	*inop = NULLFSINO;
> +	return ENOSPC;
>  out_alloc:
>  	*IO_agbp = NULL;
>  	return xfs_dialloc_ag(tp, agbp, parent, inop);

Default behaviour on a loop break is to allocate inodes, not return
ENOSPC.

BTW, there's no need to cc LKML for XFS specific patches. LKML is
noisy enough as it is without unnecessary cross-posts....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] Add ratelimited printk for different alert levels
  2012-09-11 22:13 ` [PATCH 1/3] Add ratelimited printk for different alert levels raghu.prabhu13
  2012-09-11 22:43   ` Dave Chinner
@ 2012-09-12  3:22   ` Joe Perches
  2012-09-13  0:51     ` Dave Chinner
  1 sibling, 1 reply; 11+ messages in thread
From: Joe Perches @ 2012-09-12  3:22 UTC (permalink / raw)
  To: raghu.prabhu13
  Cc: Alex Elder, Ben Myers, open list, xfs, Raghavendra D Prabhu

On Wed, 2012-09-12 at 03:43 +0530, raghu.prabhu13@gmail.com wrote:
> Ratelimited printk will be useful in printing xfs messages which are otherwise
> not required to be printed always due to their high rate (to prevent kernel ring
> buffer from overflowing), while at the same time required to be printed.
[]
> diff --git a/fs/xfs/xfs_message.h b/fs/xfs/xfs_message.h
[]
> @@ -30,6 +32,32 @@ void xfs_debug(const struct xfs_mount *mp, const char *fmt, ...)
>  }
>  #endif
>  
> +#define xfs_printk_ratelimited(xfs_printk, dev, fmt, ...)		\
> +do {									\
> +	static DEFINE_RATELIMIT_STATE(_rs,				\
> +				      DEFAULT_RATELIMIT_INTERVAL,	\
> +				      DEFAULT_RATELIMIT_BURST);		\
> +	if (__ratelimit(&_rs))						\
> +		xfs_printk(dev, fmt, ##__VA_ARGS__);			\
> +} while (0)

It might be better to use an xfs singleton RATELIMIT_STATE

DEFINE_RATELIMIT_STATE(xfs_rs);
...
#define xfs_printk_ratelimited(xfs_printk, dev, fmt, ...)		\
do {									\
	if (__ratelimit(&xfs_rs))					\
		xfs_printk(dev, fmt, ##__VA_ARGS__);			\
} while (0)


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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] Add ratelimited printk for different alert levels
  2012-09-12  3:22   ` Joe Perches
@ 2012-09-13  0:51     ` Dave Chinner
  0 siblings, 0 replies; 11+ messages in thread
From: Dave Chinner @ 2012-09-13  0:51 UTC (permalink / raw)
  To: Joe Perches
  Cc: Alex Elder, Ben Myers, open list, xfs, Raghavendra D Prabhu,
	raghu.prabhu13

On Tue, Sep 11, 2012 at 08:22:39PM -0700, Joe Perches wrote:
> On Wed, 2012-09-12 at 03:43 +0530, raghu.prabhu13@gmail.com wrote:
> > Ratelimited printk will be useful in printing xfs messages which are otherwise
> > not required to be printed always due to their high rate (to prevent kernel ring
> > buffer from overflowing), while at the same time required to be printed.
> []
> > diff --git a/fs/xfs/xfs_message.h b/fs/xfs/xfs_message.h
> []
> > @@ -30,6 +32,32 @@ void xfs_debug(const struct xfs_mount *mp, const char *fmt, ...)
> >  }
> >  #endif
> >  
> > +#define xfs_printk_ratelimited(xfs_printk, dev, fmt, ...)		\
> > +do {									\
> > +	static DEFINE_RATELIMIT_STATE(_rs,				\
> > +				      DEFAULT_RATELIMIT_INTERVAL,	\
> > +				      DEFAULT_RATELIMIT_BURST);		\
> > +	if (__ratelimit(&_rs))						\
> > +		xfs_printk(dev, fmt, ##__VA_ARGS__);			\
> > +} while (0)
> 
> It might be better to use an xfs singleton RATELIMIT_STATE
> 
> DEFINE_RATELIMIT_STATE(xfs_rs);
> ...
> #define xfs_printk_ratelimited(xfs_printk, dev, fmt, ...)		\
> do {									\
> 	if (__ratelimit(&xfs_rs))					\
> 		xfs_printk(dev, fmt, ##__VA_ARGS__);			\
> } while (0)

Which would then result in ratelimiting dropping potentially
important, unique messages. I think it's much better to guarantee
ratelimited messages get emitted at least once, especially as there
is the potential for multiple filesystems to emit messages
simultaneously.

I think per-location rate limiting is fine for the current usage -
ratelimiting is not widespread so there isn't a massive increase in
size as a result of this. If we do start to use ratelimiting in lots
of places in XFS, then we might have to revisit this, but it's OK
for now.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Re: [PATCH 3/3] XFS: Print error when unable to allocate inodes or out of free inodes.
  2012-09-11 23:21   ` Dave Chinner
@ 2012-09-21  7:16     ` Raghavendra D Prabhu
  2012-09-26  6:14       ` Raghavendra Prabhu
  0 siblings, 1 reply; 11+ messages in thread
From: Raghavendra D Prabhu @ 2012-09-21  7:16 UTC (permalink / raw)
  To: Dave Chinner; +Cc: Ben Myers, Alex Elder, xfs


[-- Attachment #1.1: Type: text/plain, Size: 4477 bytes --]

Hi,


* On Wed, Sep 12, 2012 at 09:21:44AM +1000, Dave Chinner <david@fromorbit.com> wrote:
>On Wed, Sep 12, 2012 at 03:43:24AM +0530, raghu.prabhu13@gmail.com wrote:
>> From: Raghavendra D Prabhu <rprabhu@wnohang.net>
>>
>> When xfs_dialloc is unable to allocate required number of inodes or there are no
>> AGs with free inodes, printk the error in ratelimited manner.
>>
>> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
>> ---
>>  fs/xfs/xfs_ialloc.c | 19 +++++++++++++++----
>>  1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_ialloc.c b/fs/xfs/xfs_ialloc.c
>> index e75a39d..034131b 100644
>> --- a/fs/xfs/xfs_ialloc.c
>> +++ b/fs/xfs/xfs_ialloc.c
>> @@ -990,8 +990,11 @@ xfs_dialloc(
>>  				goto out_error;
>>
>>  			xfs_perag_put(pag);
>> -			*inop = NULLFSINO;
>> -			return 0;
>> +
>> +			xfs_err_ratelimited(mp,
>> +				"Unable to allocate inodes in AG %d: Required %d, Current %llu, Maximum %llu",
>> +				agno, XFS_IALLOC_INODES(mp), mp->m_sb.sb_icount, mp->m_maxicount);
>> +			goto out_spc;
>
>This changes the error to be returned from 0 to ENOSPC. Adding error
>messages shouldn't change the logic of the code.

1) Yes, there is a confusion regarding that.  

>
>Also, you might want tolook at how ENOSPC is returned from
>xfs_ialloc_ag_alloc(). it only occurs when:
>
>	if (mp->m_maxicount &&
>            mp->m_sb.sb_icount + newlen > mp->m_maxicount) {
>
>i.e. it is exactly the same error case as the "noroom" error below.
>It has nothing to do with being unable to allocate inodes in the
>specific AG - the global inode count is too high. IOWs, the error
>message is not correct.

Now, in xfs_dialloc 


	if (mp->m_maxicount &&
	    mp->m_sb.sb_icount + XFS_IALLOC_INODES(mp) > mp->m_maxicount) {
		noroom = 1;
		okalloc = 0;
	}

why does it not make sense to bail out with ENOSPC then itself? I 
mean, what is the point of the loop when there is no room 
(noroom=1) and no allocations are allowed (okalloc = 0), also 
since the xfs_ialloc_ag_alloc in the loop uses same global logic 
to return.



>
>Also, 80 columns.
>
>>  		}
>>
>>  		if (ialloced) {
>> @@ -1016,11 +1019,19 @@ nextag:
>>  		if (++agno == mp->m_sb.sb_agcount)
>>  			agno = 0;
>>  		if (agno == start_agno) {
>> -			*inop = NULLFSINO;
>> -			return noroom ? ENOSPC : 0;
>> +			if (noroom) {
>> +				xfs_err_ratelimited(mp,
>> +					"Out of AGs with free inodes: Required %d, Current %llu, Maximum %llu",
>> +					 XFS_IALLOC_INODES(mp), mp->m_sb.sb_icount, mp->m_maxicount);
>
>The error message here is misleading - the error is that we've
>exceeded the maximum inode count for the filesystem (same as the
>above error message case), so no further allocations are allowed.
>
>What about the !noroom case? Isn't that a real ENOSPC condition?
>i.e. we've tried to allocate inodes in every AG and yet we've failed
>in all of them because there is no aligned, contiguous free space in
>any of the AGs. Shouldn't that emit an appropriate warning?

The warning is already emitted in call to xfs_ialloc_ag_select. 

Now, what does inop = NULLFSINO, noroom = 0 and return value of 0 mean, 

from the call chain of xfs_dir_ialloc -> xfs_ialloc -> 
xfs_dialloc 

I see that, it is a true ENOSPC only if both the buffer pointed 
by ialloc_context and the inode are NULL or there is an error 
returned, in former case (noroom=0)  xfs_dir_ialloc retries the 
allocation (ie.  when AGI buffer is non-NULL).

Now, in case of global inode exhaustion, it is hard error which 
can be fixed only by remounting with inode64 and nothing else 
will do, hence, I think ENOSPC must be returned as error instead 
of 0. (also in case of point #1 above)

So, are the assumptions made above correct?

>
>> +				goto out_spc;
>> +			}
>> +			return 0;
>>  		}
>>  	}
>>
>> +out_spc:
>> +	*inop = NULLFSINO;
>> +	return ENOSPC;
>>  out_alloc:
>>  	*IO_agbp = NULL;
>>  	return xfs_dialloc_ag(tp, agbp, parent, inop);
>
>Default behaviour on a loop break is to allocate inodes, not return
>ENOSPC.
>
>BTW, there's no need to cc LKML for XFS specific patches. LKML is
>noisy enough as it is without unnecessary cross-posts....
>
>Cheers,
>
>Dave.
>-- 
>Dave Chinner
>david@fromorbit.com
>




Regards,
-- 
Raghavendra Prabhu
GPG Id : 0xD72BE977
Fingerprint: B93F EBCB 8E05 7039 CD3C A4B8 A616 DCA1 D72B E977
www: wnohang.net

[-- Attachment #1.2: Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 121 bytes --]

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Re: [PATCH 3/3] XFS: Print error when unable to allocate inodes or out of free inodes.
  2012-09-21  7:16     ` Raghavendra D Prabhu
@ 2012-09-26  6:14       ` Raghavendra Prabhu
  0 siblings, 0 replies; 11+ messages in thread
From: Raghavendra Prabhu @ 2012-09-26  6:14 UTC (permalink / raw)
  To: Dave Chinner; +Cc: Ben Myers, Alex Elder, xfs

Hi,

On Fri, Sep 21, 2012 at 12:46 PM, Raghavendra D Prabhu
<raghu.prabhu13@gmail.com> wrote:
> Hi,
>
>
>>
>>> +                               goto out_spc;
>>> +                       }
>>> +                       return 0;
>>>                 }
>>>         }
>>>
>>> +out_spc:
>>> +       *inop = NULLFSINO;
>>> +       return ENOSPC;
>>>  out_alloc:
>>>         *IO_agbp = NULL;
>>>         return xfs_dialloc_ag(tp, agbp, parent, inop);
>>
>>
>> Default behaviour on a loop break is to allocate inodes, not return
>> ENOSPC.
>>
>> BTW, there's no need to cc LKML for XFS specific patches. LKML is
>> noisy enough as it is without unnecessary cross-posts....
>>
>> Cheers,
>>
>> Dave.
>> --
>> Dave Chinner
>> david@fromorbit.com
>>
>

Ignore the previous. Resending the patch with fixes mentioned.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2012-09-26  6:12 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-11 22:13 [PATCH V2 0/3] Print when ENOSPC due to lack of inodes in non-inode64 mount raghu.prabhu13
2012-09-11 22:13 ` [PATCH 1/3] Add ratelimited printk for different alert levels raghu.prabhu13
2012-09-11 22:43   ` Dave Chinner
2012-09-12  3:22   ` Joe Perches
2012-09-13  0:51     ` Dave Chinner
2012-09-11 22:13 ` [PATCH 2/3] XFS: Print error when xfs_ialloc_ag_select fails to find continuous free space raghu.prabhu13
2012-09-11 22:48   ` Dave Chinner
2012-09-11 22:13 ` [PATCH 3/3] XFS: Print error when unable to allocate inodes or out of free inodes raghu.prabhu13
2012-09-11 23:21   ` Dave Chinner
2012-09-21  7:16     ` Raghavendra D Prabhu
2012-09-26  6:14       ` Raghavendra Prabhu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox