* Re: [PATCH] xfs: Fix oops on IO error during xlog_recover_process_iunlinks()
2012-03-07 1:17 ` Dave Chinner
@ 2012-03-07 11:07 ` Jan Kara
0 siblings, 0 replies; 3+ messages in thread
From: Jan Kara @ 2012-03-07 11:07 UTC (permalink / raw)
To: Dave Chinner; +Cc: Ben Myers, Alex Elder, Jan Kara, xfs
[-- Attachment #1: Type: text/plain, Size: 2805 bytes --]
On Wed 07-03-12 12:17:16, Dave Chinner wrote:
> On Tue, Mar 06, 2012 at 12:00:16PM +0100, Jan Kara wrote:
> > When an IO error happens during inode deletion run from
> > xlog_recover_process_iunlinks() filesystem gets shutdown. Thus any subsequent
> > attempt to read buffers fails. Code in xlog_recover_process_iunlinks() does not
> > count with the fact that read of a buffer which was read a while ago can
> > really fail which results in the oops on
> > agi = XFS_BUF_TO_AGI(agibp);
> >
> > Fix the problem by handling error from xfs_read_agi() in all cases.
> >
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> > fs/xfs/xfs_log_recover.c | 15 ++++++++++++---
> > 1 files changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > index 0ed9ee7..3899264 100644
> > --- a/fs/xfs/xfs_log_recover.c
> > +++ b/fs/xfs/xfs_log_recover.c
> > @@ -3178,11 +3178,17 @@ xlog_recover_process_iunlinks(
> >
> > /*
> > * Reacquire the agibuffer and continue around
> > - * the loop. This should never fail as we know
> > - * the buffer was good earlier on.
> > + * the loop.
> > */
> > error = xfs_read_agi(mp, NULL, agno, &agibp);
> > - ASSERT(error == 0);
> > + /*
> > + * We failed to read a buffer we succeeded
> > + * reading just a while ago. Likely because the
> > + * filesystem is shutdown now. Just try the
> > + * next AG.
> > + */
> > + if (error)
> > + goto next_ag;
> > agi = XFS_BUF_TO_AGI(agibp);
> > }
> > }
>
> That function is full of ugly code. We don't need to continually
> lock and unlock the AGI in the inner loop. Indeed, we probably don't
> even need to lock the buffer to read the AGI bucket entries because
> we aren't going to be racing with anyone here. Hence all we really
> need is an extra hold on the agi buffer to make sure it doesn't go
> away once we've dropped the lock via xfs_buf_relse(). i.e.
>
>
> /*
> * take an extra reference to the buffer and then release it
> * to drop the lock so that it can be acquired in the normal
> * course of the transaction to truncate and free each
> * inode. Because we are not racing with anyone else here
> * for the AGI buffer, we don't even need to hold it locked
> * to read the initial unlinked bucket entries out of the
> * buffer.
> */
> agi = XFS_BUF_TO_AGI(agibp);
> xfs_buf_hold(agibp);
> xfs_buf_relse(agibp);
> for (bucket = 0; bucket < XFS_AGI_UNLINKED_BUCKETS; bucket++) {
> agino = be32_to_cpu(agi->agi_unlinked[bucket]);
> while (agino != NULLAGINO) {
> agino = xlog_recover_process_one_iunlink(mp,
> agno, agino, bucket);
> }
> }
> xfs_buf_rele(agibp)
Thanks for review Dave. So something like attached patch?
Honza
[-- Attachment #2: 0001-xfs-Fix-oops-on-IO-error-during-xlog_recover_process.patch --]
[-- Type: text/x-patch, Size: 2653 bytes --]
>From 276d5ecac71d9e6ec6ac970e594f5a49450d07e2 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Tue, 6 Mar 2012 11:39:48 +0100
Subject: [PATCH v2] xfs: Fix oops on IO error during xlog_recover_process_iunlinks()
When an IO error happens during inode deletion run from
xlog_recover_process_iunlinks() filesystem gets shutdown. Thus any subsequent
attempt to read buffers fails. Code in xlog_recover_process_iunlinks() does not
count with the fact that read of a buffer which was read a while ago can
really fail which results in the oops on
agi = XFS_BUF_TO_AGI(agibp);
Fix the problem by cleaning up the buffer handling in
xlog_recover_process_iunlinks(). We release buffer lock but keep buffer
reference to AG buffer. That is enough for buffer to not go away under us
and we don't have to call xfs_read_agi() all the time.
CC: stable@kernel.org
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/xfs/xfs_log_recover.c | 34 ++++++++++++----------------------
1 files changed, 12 insertions(+), 22 deletions(-)
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 0ed9ee7..0827644 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3161,37 +3161,27 @@ xlog_recover_process_iunlinks(
*/
continue;
}
+ /*
+ * Take an extra reference to the buffer and then release it
+ * to drop the lock so that it can be acquired in the normal
+ * course of the transaction to truncate and free each
+ * inode. Because we are not racing with anyone else here
+ * for the AGI buffer, we don't even need to hold it locked
+ * to read the initial unlinked bucket entries out of the
+ * buffer.
+ */
agi = XFS_BUF_TO_AGI(agibp);
+ xfs_buf_hold(agibp);
+ xfs_buf_relse(agibp);
for (bucket = 0; bucket < XFS_AGI_UNLINKED_BUCKETS; bucket++) {
agino = be32_to_cpu(agi->agi_unlinked[bucket]);
while (agino != NULLAGINO) {
- /*
- * Release the agi buffer so that it can
- * be acquired in the normal course of the
- * transaction to truncate and free the inode.
- */
- xfs_buf_relse(agibp);
-
agino = xlog_recover_process_one_iunlink(mp,
agno, agino, bucket);
-
- /*
- * Reacquire the agibuffer and continue around
- * the loop. This should never fail as we know
- * the buffer was good earlier on.
- */
- error = xfs_read_agi(mp, NULL, agno, &agibp);
- ASSERT(error == 0);
- agi = XFS_BUF_TO_AGI(agibp);
}
}
-
- /*
- * Release the buffer for the current agi so we can
- * go on to the next one.
- */
- xfs_buf_relse(agibp);
+ xfs_buf_rele(agibp);
}
mp->m_dmevmask = mp_dmevmask;
--
1.7.1
[-- Attachment #3: Type: text/plain, Size: 121 bytes --]
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 3+ messages in thread