Hi Darrick,
while reviewing your online repair series I've noticed that the new
xfs_buf_delwri_queue_here helper sets XBF_DONE in addition to waiting
for the buffer to go off a delwri list, and that reminded me off an
assert I saw during my allocator experiments, where
xfs_trans_read_buf_map or xfs_buf_reverify trip on a buffer that doesn't
have XBF_DONE set because it comes from an ifree transaction (see
my current not fully thought out bandaid below).
The way we currently set and check XBF_DONE seems a bit undefined. The
one clear use case is that read uses it to see if a buffer was read in.
But places that use buf_get and manually fill in data only use it in a
few cases. Do we need to define clear semantics for it? Or maybe
replace with an XBF_READ_DONE flag for that main read use case and
then think what do do with the rest?
---
>From 80d148555ca261777ad728455a9a240e0007f54e Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@xxxxxx>
Date: Mon, 13 Nov 2023 12:02:15 -0500
Subject: xfs: remove the XBF_DONE asserts in xfs_buf_reverify and
xfs_trans_read_buf_map
When xfs_trans_read_buf_map is called from xfs_inode_item_precommit
through xfs_imap_to_bp, we can find an inode buffer that doesn't have
XBF_DONE because xfs_ifree_cluster doesn't read the buffer but instead
just gets it before marking all buffers stale.
[ 206.728129] ------------[ cut here ]------------
[ 206.728573] WARNING: CPU: 0 PID: 6320 at fs/xfs/xfs_trans_buf.c:256 xfs_trans_read_buf_map+0x20
[ 206.728971] Modules linked in:
[ 206.729099] CPU: 0 PID: 6320 Comm: kworker/0:124 Not tainted 6.6.0+ #1598
[ 206.729368] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 044
[ 206.729744] Workqueue: xfs-inodegc/nvme1n1 xfs_inodegc_worker
[ 206.729984] RIP: 0010:xfs_trans_read_buf_map+0x2ab/0x570
[ 206.730295] Code: 0f 84 ae fe ff ff b9 4e 01 00 00 48 c7 c6 c8 62 0d 83 48 c7 c2 a5 15 ff 82 3d
[ 206.731131] RSP: 0018:ffffc9000751fc38 EFLAGS: 00010246
[ 206.731414] RAX: 0000000002110040 RBX: ffff88824c47dbd8 RCX: 0000000000000000
[ 206.731748] RDX: ffff88824c47dc90 RSI: 0000000000000000 RDI: ffff8881705b46c0
[ 206.732139] RBP: ffffc9000751fca8 R08: ffff888106416e00 R09: ffffc9000751fca8
[ 206.732428] R10: ffff8881e09faa00 R11: ffff8881e09fb408 R12: 0000000000000001
[ 206.732786] R13: ffff88810ddb2000 R14: ffffffff82b36660 R15: ffffc9000751fcf0
[ 206.733092] FS: 0000000000000000(0000) GS:ffff888277c00000(0000) knlGS:0000000000000000
[ 206.733460] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 206.733797] CR2: 00007f9833f399c0 CR3: 00000002113de000 CR4: 0000000000750ef0
[ 206.734131] PKRU: 55555554
[ 206.734320] Call Trace:
[ 206.734438] <TASK>
[ 206.734537] ? xfs_trans_read_buf_map+0x2ab/0x570
[ 206.734824] ? __warn+0x80/0x170
[ 206.735051] ? xfs_trans_read_buf_map+0x2ab/0x570
[ 206.735342] ? report_bug+0x18d/0x1c0
[ 206.735533] ? handle_bug+0x41/0x70
[ 206.735679] ? exc_invalid_op+0x17/0x60
[ 206.735852] ? asm_exc_invalid_op+0x1a/0x20
[ 206.736054] ? xfs_trans_read_buf_map+0x2ab/0x570
[ 206.736262] ? xfs_trans_read_buf_map+0x6b/0x570
[ 206.736500] xfs_imap_to_bp+0x60/0xc0
[ 206.736668] xfs_inode_item_precommit+0x172/0x2a0
[ 206.736947] ? xfs_iunlink_item_precommit+0xae/0x220
[ 206.737191] xfs_trans_run_precommits+0x60/0xc0
[ 206.737460] __xfs_trans_commit+0x67/0x3a0
[ 206.737668] xfs_inactive_ifree+0xfb/0x1f0
[ 206.737889] xfs_inactive+0x22f/0x420
[ 206.738059] xfs_inodegc_worker+0xa3/0x200
[ 206.738227] ? process_one_work+0x171/0x4a0
[ 206.738450] process_one_work+0x1d8/0x4a0
[ 206.738651] worker_thread+0x1ce/0x3b0
[ 206.738864] ? wq_sysfs_prep_attrs+0x90/0x90
[ 206.739074] kthread+0xf2/0x120
[ 206.739216] ? kthread_complete_and_exit+0x20/0x20
[ 206.739482] ret_from_fork+0x2c/0x40
[ 206.739675] ? kthread_complete_and_exit+0x20/0x20
[ 206.740074] ret_from_fork_asm+0x11/0x20
[ 206.740402] </TASK>
Signed-off-by: Christoph Hellwig <hch@xxxxxx>
---
fs/xfs/xfs_buf.c | 1 -
fs/xfs/xfs_trans_buf.c | 1 -
2 files changed, 2 deletions(-)
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 9bf79172efc824..4ff9f1b1abb698 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -863,7 +863,6 @@ xfs_buf_reverify(
struct xfs_buf *bp,
const struct xfs_buf_ops *ops)
{
- ASSERT(bp->b_flags & XBF_DONE);
ASSERT(bp->b_error == 0);
if (!ops || bp->b_ops)
diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
index 8e886ecfd69a3b..575922c64d4d3a 100644
--- a/fs/xfs/xfs_trans_buf.c
+++ b/fs/xfs/xfs_trans_buf.c
@@ -253,7 +253,6 @@ xfs_trans_read_buf_map(
ASSERT(bp->b_transp == tp);
ASSERT(bp->b_log_item != NULL);
ASSERT(!bp->b_error);
- ASSERT(bp->b_flags & XBF_DONE);
/*
* We never locked this buf ourselves, so we shouldn't
--
2.39.2
