Re: [PATCH 4/4] xfs: constrain dirty buffers while formatting a staged btree

Darrick J. Wong
2023-11-28T20:11:33Z

On Mon, Nov 27, 2023 at 02:56:31PM -0800, Darrick J. Wong wrote:
> On Fri, Nov 24, 2023 at 09:53:38PM -0800, Christoph Hellwig wrote:
> > > @@ -480,7 +500,7 @@ xfs_btree_bload_node(
> > >
> > > ASSERT(!xfs_btree_ptr_is_null(cur, child_ptr));
> > >
> > > - ret = xfs_btree_get_buf_block(cur, child_ptr, &child_block,
> > > + ret = xfs_btree_read_buf_block(cur, child_ptr, 0, &child_block,
> > > &child_bp);
> >
> > How is this (and making xfs_btree_read_buf_block outside of xfs_buf.c)
> > related to the dirty limit?
>
> Oh! Looking through my notes, I wanted the /new/ btree block buffers
> have the same lru reference count the old ones did. I probably should
> have exported xfs_btree_set_refs instead of reading whatever is on disk
> into a buffer, only to blow away the contents anyway.

And now that I've dug further through my notes, I've realized that
there's a better reason for this unexplained _get_buf -> _read_buf
transition and the setting of XBF_DONE in _delwri_queue_here.

This patch introduces the behavior that we flush the delwri list to disk
every 256k. Flushing the buffers releases them, which means that
reclaim could free the buffer before xfs_btree_bload_node needs it again
to build the next level up. If that's the case, then _get_buf will get
us a !DONE buffer with zeroes instead of reading the (freshly written)
buffer back in from disk. We'll then end up formatting garbage keys
into the node block, which is bad.

/*
* Read the lower-level block in case the buffer for it has
* been reclaimed. LRU refs will be set on the block, which is
* desirable if the new btree commits.
*/
ret = xfs_btree_read_buf_block(cur, child_ptr, 0, &child_block,
&child_bp);

The behavior of setting XBF_DONE in xfs_buf_delwri_queue_here is an
optimization if _delwri_submit releases the buffer and it is /not/
reclaimed. In that case, xfs_btree_read_buf_block will find the buffer
without the DONE flag set and reread the contents from disk, which is
unnecessary.

I'll split those changes into separate patches with fuller explanations
of what's going on.

--D

> --D
>