Re: XBF_DONE semantics

[Christoph Hellwig <hch@lst.de>]

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.