On Tue, Nov 28, 2023 at 08:58:31AM -0800, Darrick J. Wong wrote: > > 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? > > I thought XBF_DONE meant "contents have been read in from disk and > have passed/will pass verifiers" That's what I though too. But there's clearly code that treats it differently.. > Dave and I wondered if xfs_inode_item_precommit should be grabbing the > buffer at all when ISTALE is set, since xfs_ifree_cluster should have > staled (and invalidated) the buffer after setting ISTALE. That does sound reasonable. > > +++ 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); > > I don't think this is the right thing to do here -- if the buffer is > attached to a transaction, it ought to be XBF_DONE. I think every > transaction that calls _get_buf and rewrites the buffer contents will > set XBF_DONE via xfs_trans_dirty_buf, right? > > Hmm. Maybe I'm wrong -- a transaction could bjoin a buffer and then > call xfs_trans_read_buf_map before dirtying it. That strikes me as a > suspicious thing to do, though. I suspect it's happening here somehow. I can try to find some more time pinning it down.
