xfs
[Top] [All Lists]

Re: [PATCH] xfs: shutdown filesystem if xfs_perag_get fails

To: sekharan@xxxxxxxxxx
Subject: Re: [PATCH] xfs: shutdown filesystem if xfs_perag_get fails
From: Mark Tinguely <tinguely@xxxxxxx>
Date: Fri, 26 Apr 2013 10:32:34 -0500
Cc: Dave Chinner <david@xxxxxxxxxxxxx>, Eric Sandeen <sandeen@xxxxxxxxxxx>, xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1366929706.4098.6.camel@xxxxxxxxxxxxxxxxxx>
References: <20130419204102.736961610@xxxxxxx> <20130421174107.007313126@xxxxxxx> <5174603A.8030208@xxxxxxxxxxx> <51753EDE.6000301@xxxxxxx> <51754A13.5000808@xxxxxxxxxxx> <5175532B.3050509@xxxxxxx> <20130422233033.GK30622@dastard> <51769111.3050103@xxxxxxx> <1366732475.3762.32402.camel@xxxxxxxxxxxxxxxxxx> <20130423204956.GM10481@dastard> <1366929706.4098.6.camel@xxxxxxxxxxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:9.0) Gecko/20120122 Thunderbird/9.0
On 04/25/13 17:41, Chandra Seetharaman wrote:
In which case something along the lines of

---
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 3806088..3fb2fa6 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -203,7 +203,13 @@ xfs_perag_get(struct xfs_mount *mp, xfs_agnumber_t
agno)
         if (pag) {
                 ASSERT(atomic_read(&pag->pag_ref)>= 0);
                 ref = atomic_inc_return(&pag->pag_ref);
-       }
+       } else
+               /*
+                * xfs_perag_get() is called with invalid agno,
+                * which cannot happen. This indicates a problem
+                * in the calling code.
+                */
+               BUG();
         rcu_read_unlock();
         trace_xfs_perag_get(mp, agno, ref, _RET_IP_);
         return pag;
--------

would be useful ?. Since we have a NULL pag, we will trip somewhere
else. At least with this, there is a pointer to the debugger/sysadmin
about where/what to look for (may be with more valuable/correct comment
than above).


We will have to make sure the callers of xfs_perag_get() handle the NULL
before dereferencing it. Sometimes the NULL is normal and just means the
perag structure has not been initialize yet.

Properly handling the NULL from xfs_perag_get() in the caller will also
mean that the callers of the callers of xfs_perag_get() have to handle
the NULL returned to them. I will come back to this once the CRC stuff
has been put to rest.

--Mark.

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