public inbox for linux-xfs@vger.kernel.org
 help / color / mirror / Atom feed
From: Dave Chinner <david@fromorbit.com>
To: stable@kernel.org
Cc: xfs@oss.sgi.com
Subject: [PATCH 04/19] xfs: Wrapped journal record corruption on read at recovery
Date: Fri, 12 Mar 2010 09:42:02 +1100	[thread overview]
Message-ID: <1268347337-7160-5-git-send-email-david@fromorbit.com> (raw)
In-Reply-To: <1268347337-7160-1-git-send-email-david@fromorbit.com>

From: Andy Poling <andy@realbig.com>

>From fc5bc4c85c45f0bf854404e5736aa8b65720a18d
Date: Tue, 3 Nov 2009 17:26:47 +0000
Summary of problem:

If a journal record wraps at the physical end of the journal, it has to be
read in two parts in xlog_do_recovery_pass(): a read at the physical end and a
read at the physical beginning.  If xlog_bread() has to re-align the first
read, the second read request does not take that re-alignment into account.
If the first read was re-aligned, the second read over-writes the end of the
data from the first read, effectively corrupting it.  This can happen either
when reading the record header or reading the record data.

The first sanity check in xlog_recover_process_data() is to check for a valid
clientid, so that is the error reported.

Summary of fix:

If there was a first read at the physical end, XFS_BUF_PTR() returns where the
data was requested to begin.  Conversely, because it is the result of
xlog_align(), offset indicates where the requested data for the first read
actually begins - whether or not xlog_bread() has re-aligned it.

Using offset as the base for the calculation of where to place the second read
data ensures that it will be correctly placed immediately following the data
from the first read instead of sometimes over-writing the end of it.

The attached patch has resolved the reported problem of occasional inability
to recover the journal (reporting "bad clientid").

Signed-off-by: Andy Poling <andy@realbig.com>
Reviewed-by: Alex Elder <aelder@sgi.com>
Signed-off-by: Alex Elder <aelder@sgi.com>
---
 fs/xfs/xfs_log_recover.c |   24 +++++++-----------------
 1 files changed, 7 insertions(+), 17 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index fb17f82..b5b0d80 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3517,7 +3517,7 @@ xlog_do_recovery_pass(
 {
 	xlog_rec_header_t	*rhead;
 	xfs_daddr_t		blk_no;
-	xfs_caddr_t		bufaddr, offset;
+	xfs_caddr_t		offset;
 	xfs_buf_t		*hbp, *dbp;
 	int			error = 0, h_size;
 	int			bblks, split_bblks;
@@ -3610,7 +3610,7 @@ xlog_do_recovery_pass(
 			/*
 			 * Check for header wrapping around physical end-of-log
 			 */
-			offset = NULL;
+			offset = XFS_BUF_PTR(hbp);
 			split_hblks = 0;
 			wrapped_hblks = 0;
 			if (blk_no + hblks <= log->l_logBBsize) {
@@ -3646,9 +3646,8 @@ xlog_do_recovery_pass(
 				 *   - order is important.
 				 */
 				wrapped_hblks = hblks - split_hblks;
-				bufaddr = XFS_BUF_PTR(hbp);
 				error = XFS_BUF_SET_PTR(hbp,
-						bufaddr + BBTOB(split_hblks),
+						offset + BBTOB(split_hblks),
 						BBTOB(hblks - split_hblks));
 				if (error)
 					goto bread_err2;
@@ -3658,14 +3657,10 @@ xlog_do_recovery_pass(
 				if (error)
 					goto bread_err2;
 
-				error = XFS_BUF_SET_PTR(hbp, bufaddr,
+				error = XFS_BUF_SET_PTR(hbp, offset,
 							BBTOB(hblks));
 				if (error)
 					goto bread_err2;
-
-				if (!offset)
-					offset = xlog_align(log, 0,
-							wrapped_hblks, hbp);
 			}
 			rhead = (xlog_rec_header_t *)offset;
 			error = xlog_valid_rec_header(log, rhead,
@@ -3685,7 +3680,7 @@ xlog_do_recovery_pass(
 			} else {
 				/* This log record is split across the
 				 * physical end of log */
-				offset = NULL;
+				offset = XFS_BUF_PTR(dbp);
 				split_bblks = 0;
 				if (blk_no != log->l_logBBsize) {
 					/* some data is before the physical
@@ -3714,9 +3709,8 @@ xlog_do_recovery_pass(
 				 *   _first_, then the log start (LR header end)
 				 *   - order is important.
 				 */
-				bufaddr = XFS_BUF_PTR(dbp);
 				error = XFS_BUF_SET_PTR(dbp,
-						bufaddr + BBTOB(split_bblks),
+						offset + BBTOB(split_bblks),
 						BBTOB(bblks - split_bblks));
 				if (error)
 					goto bread_err2;
@@ -3727,13 +3721,9 @@ xlog_do_recovery_pass(
 				if (error)
 					goto bread_err2;
 
-				error = XFS_BUF_SET_PTR(dbp, bufaddr, h_size);
+				error = XFS_BUF_SET_PTR(dbp, offset, h_size);
 				if (error)
 					goto bread_err2;
-
-				if (!offset)
-					offset = xlog_align(log, wrapped_hblks,
-						bblks - split_bblks, dbp);
 			}
 			xlog_unpack_data(rhead, offset, log);
 			if ((error = xlog_recover_process_data(log, rhash,
-- 
1.6.5

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

  parent reply	other threads:[~2010-03-11 22:41 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <1268347337-7160-1-git-send-email-david@fromorbit.com>
2010-03-11 22:41 ` [PATCH 01/19] xfs: simplify inode teardown Dave Chinner
2010-03-11 22:42 ` [PATCH 02/19] xfs: fix mmap_sem/iolock inversion in xfs_free_eofblocks Dave Chinner
2010-03-11 22:42 ` [PATCH 03/19] xfs: I/O completion handlers must use NOFS allocations Dave Chinner
2010-03-11 22:42 ` Dave Chinner [this message]
2010-03-11 22:42 ` [PATCH 05/19] xfs: Fix error return for fallocate() on XFS Dave Chinner
2010-03-11 22:42 ` [PATCH 06/19] xfs: check for not fully initialized inodes in xfs_ireclaim Dave Chinner
2010-03-11 22:42 ` [PATCH 07/19] xfs: fix timestamp handling in xfs_setattr Dave Chinner
2010-03-11 22:42 ` [PATCH 08/19] xfs: Don't flush stale inodes Dave Chinner
2010-03-11 22:42 ` [PATCH 09/19] xfs: Ensure we force all busy extents in range to disk Dave Chinner
2010-03-11 22:42 ` [PATCH 10/19] xfs: reclaim inodes under a write lock Dave Chinner
2010-03-11 22:42 ` [PATCH 11/19] xfs: Avoid inodes in reclaim when flushing from inode cache Dave Chinner
2010-03-11 22:42 ` [PATCH 12/19] xfs: reclaim all inodes by background tree walks Dave Chinner
2010-03-11 22:42 ` [PATCH 13/19] xfs: fix stale inode flush avoidance Dave Chinner
2010-03-11 22:42 ` [PATCH 14/19] xfs: xfs_swap_extents needs to handle dynamic fork offsets Dave Chinner
2010-03-11 22:42 ` [PATCH 15/19] xfs: quota limit statvfs available blocks Dave Chinner
2010-03-11 22:42 ` [PATCH 16/19] xfs: don't hold onto reserved blocks on remount,ro Dave Chinner
2010-03-11 22:42 ` [PATCH 17/19] xfs: remove invalid barrier optimization from xfs_fsync Dave Chinner
2010-03-11 22:42 ` [PATCH 18/19] xfs: Non-blocking inode locking in IO completion Dave Chinner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1268347337-7160-5-git-send-email-david@fromorbit.com \
    --to=david@fromorbit.com \
    --cc=stable@kernel.org \
    --cc=xfs@oss.sgi.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox