xfs
[Top] [All Lists]

Re: Re: [PATCH 3/3] XFS: Print error when unable to allocate inodes or

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: Re: [PATCH 3/3] XFS: Print error when unable to allocate inodes or out of free inodes.
From: Raghavendra D Prabhu <raghu.prabhu13@xxxxxxxxx>
Date: Fri, 21 Sep 2012 12:46:44 +0530
Cc: xfs@xxxxxxxxxxx, Ben Myers <bpm@xxxxxxx>, Alex Elder <elder@xxxxxxxxxx>
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-type:content-disposition:in-reply-to :x-operating-system:x-editor:user-agent; bh=XBQaclQLdgsL6573qx4X/vAtRnNTeytE2Sin/1WMCH0=; b=SLdqabwLLo6gbyuZeJsDvGi5FP7WoStcT+ABMsBq0nAEBJ/ZVZthafSFqAw1PDW+Pu u7O9i0pIdTEdvKGPaTQ9e+WM1Uf3Hr3x4AzVUu631XEDmip10F0dxUv0axZzxHpQ4SHD 46gzy+fjlCXAa+LqgxFOeJ/OkXYwNnBaNa7Q2mmG6cHItXfh8yEaYF+JPoU/snrsqMXo J9GJJ6wm5wE5USLjIOco1lRarnDePJunfItPdVYipFixXR5ZtgyKyGa7LAfBFslNJuUa Ete2RqXslGBhAMe7HGHh55lwbFzDJLVBKOj558ZacsccbW04MI2pHwiQA0WB9qakmAzk X28w==
In-reply-to: <20120911232144.GH11511@dastard>
Mail-followup-to: Dave Chinner <david@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx, Ben Myers <bpm@xxxxxxx>, Alex Elder <elder@xxxxxxxxxx>
References: <cover.1347396641.git.rprabhu@xxxxxxxxxxx> <93d9b37ce9ad720e14e2f9311e623a8e3e3139f5.1347396641.git.rprabhu@xxxxxxxxxxx> <20120911232144.GH11511@dastard>
User-agent: Mutt/1.5.21 (2011-07-01)
Hi,


* On Wed, Sep 12, 2012 at 09:21:44AM +1000, Dave Chinner <david@xxxxxxxxxxxxx> 
wrote:
On Wed, Sep 12, 2012 at 03:43:24AM +0530, raghu.prabhu13@xxxxxxxxx wrote:
From: Raghavendra D Prabhu <rprabhu@xxxxxxxxxxx>

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@xxxxxxxxxxx>
---
 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@xxxxxxxxxxxxx





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

Attachment: pgpbyhlsy3rmR.pgp
Description: PGP signature

<Prev in Thread] Current Thread [Next in Thread>