On Tue, Apr 26, 2022 at 06:46:42AM -0700, Christoph Hellwig wrote:
> On Thu, Apr 14, 2022 at 03:54:31PM -0700, Darrick J. Wong wrote:
> > diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
> > index 327ba25e9e17..a07ebaecba73 100644
> > --- a/fs/xfs/libxfs/xfs_refcount.c
> > +++ b/fs/xfs/libxfs/xfs_refcount.c
> > @@ -960,6 +960,7 @@ xfs_refcount_adjust_extents(
> > * Either cover the hole (increment) or
> > * delete the range (decrement).
> > */
> > + cur->bc_ag.refc.nr_ops++;
> > if (tmp.rc_refcount) {
> > error = xfs_refcount_insert(cur, &tmp,
> > &found_tmp);
> > @@ -970,7 +971,6 @@ xfs_refcount_adjust_extents(
> > error = -EFSCORRUPTED;
> > goto out_error;
> > }
> > - cur->bc_ag.refc.nr_ops++;
>
> How do these changes fit into the rest of the patch?
Long ago before we used deferred ops to update the refcount btree, we
would restrict the number of blocks that could be bunmapped in order to
avoid overflowing the transaction reservation while doing refcount
updates for an unmapped extent.
Later on, I added deferred refcount updates so at least the refcountbt
updates went in their own transaction, which meant that if we started
running out of space because the unmapped extent had many many different
refcount records, we could at least return EAGAIN to get a clean
transaction to continue. I needed a way to guess when we were getting
close to the reservation limit, so I added this nr_ops counter to track
the number of record updates. Granted, 32 bytes per refcountbt record
update is quite an overestimate, since the records are contiguous in a
btree leaf block.
In the past, the deferred ops code had a defect where it didn't maintain
correct ordering of deferred ops when one dfops' ->finish_item function
queued even more dfops to the transaction, which is what happens when a
refcount update queues EFIs for blocks that are no longer referenced.
This resulted in enormous chains of defer ops, so I left the bunmap
piece in to throttle the chain lengths.
Now we've fixed the deferred ops code to maintain correct dfops order
between existing ops and newly queued ones, so the chaining problem went
away, and we can get rid of the bunmap throttle. However, as part of
doing that, it's necessary to track the number of EFIs logged to the
transaction as well, which is what the above code change fixes.
Granted, Dave has also commented that EFIs use a bit more than 32 bytes,
so I think it would be warranted to capture the above comment and the
xfs_refcount.c changes in a separate patch now.
--D
