diff mbox series

libxfs: dirty buffers should be marked uptodate too

Message ID 20240812223836.GB6051@frogsfrogsfrogs (mailing list archive)
State Accepted, archived
Headers show
Series libxfs: dirty buffers should be marked uptodate too | expand

Commit Message

Darrick J. Wong Aug. 12, 2024, 10:38 p.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

I started fuzz-testing the realtime rmap feature with a very large
number of realtime allocation groups.  There were so many rt groups that
repair had to rebuild /realtime in the metadata directory tree, and that
directory was big enough to spur the creation of a block format
directory.

Unfortunately, repair then walks both directory trees to look for
unconnceted files.  This part of phase 6 emits CRC errors on the newly
created buffers for the /realtime directory, declares the directory to
be garbage, and moves all the rt rmap inodes to /lost+found, resulting
in a corrupt fs.

Poking around in gdb, I noticed that the buffer contents were indeed
zero, and that UPTODATE was not set.  This was very strange, until I
added a watch on bp->b_flags to watch for accesses.  It turns out that
xfs_repair's prefetch code will _get a buffer and zero the contents if
UPTODATE is not set.

The directory tree code in libxfs will also _get a buffer, initialize
it, and log it to the coordinating transaction, which in this case is
the transactions used to reconnect the rmap btree inodes to /realtime.
At no point does any of that code ever set UPTODATE on the buffer, which
is why prefetch zaps the contents.

Hence change both buffer dirtying functions to set UPTODATE, since a
dirty buffer is by definition at least as recent as whatever's on disk.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 libxfs/rdwr.c  |    2 +-
 libxfs/trans.c |    1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

Comments

Christoph Hellwig Aug. 13, 2024, 6:01 a.m. UTC | #1 
On Mon, Aug 12, 2024 at 03:38:36PM -0700, Darrick J. Wong wrote:
> Hence change both buffer dirtying functions to set UPTODATE, since a
> dirty buffer is by definition at least as recent as whatever's on disk.

This also matches what the kernel does in xfs_trans_dirty_buf (
except that it weirdly spells the uptodate flag done..) and other
page / buffer caches:

Reviewed-by: Christoph Hellwig <hch@lst.de>
diff mbox series

Patch

diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
index bd773a7375bc..35be785c435a 100644
--- a/libxfs/rdwr.c
+++ b/libxfs/rdwr.c
@@ -919,7 +919,7 @@  libxfs_buf_mark_dirty(
 	 */
 	bp->b_error = 0;
 	bp->b_flags &= ~LIBXFS_B_STALE;
-	bp->b_flags |= LIBXFS_B_DIRTY;
+	bp->b_flags |= LIBXFS_B_DIRTY | LIBXFS_B_UPTODATE;
 }
 
 /* Prepare a buffer to be sent to the MRU list. */
diff --git a/libxfs/trans.c b/libxfs/trans.c
index b5f6081a16cb..5c896ba1661b 100644
--- a/libxfs/trans.c
+++ b/libxfs/trans.c
@@ -716,6 +716,7 @@  libxfs_trans_dirty_buf(
 	ASSERT(bp->b_transp == tp);
 	ASSERT(bip != NULL);
 
+	bp->b_flags |= LIBXFS_B_UPTODATE;
 	tp->t_flags |= XFS_TRANS_DIRTY;
 	set_bit(XFS_LI_DIRTY, &bip->bli_item.li_flags);
 }