Re: [PATCH 2/2] xfs: estimate post-merge refcounts correctly

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Nov 30, 2022 at 05:32:38PM +0800, Yang, Xiao/杨 晓 wrote:
> On 2022/11/30 6:01, Darrick J. Wong wrote:
> > From: Darrick J. Wong<djwong@kernel.org>
> > 
> > Upon enabling fsdax + reflink for XFS, xfs/179 began to report refcount
> > metadata corruptions after being run.  Specifically, xfs_repair noticed
> > single-block refcount records that could be combined but had not been.
> Hi Darrick,
> 
> I am investigating the issue as well. Thanks a lot for your quick fix.
> I have confirmed that xfs/179 with the fix patch can works well in DAX mode.
> Reviewed-by: Xiao Yang <yangx.jy@fujitsu.com>
> 
> > 
> > The root cause of this is improper MAXREFCOUNT edge case handling in
> > xfs_refcount_merge_extents.  When we're trying to find candidates for a
> > refcount btree record merge, we compute the refcount attribute of the
> > merged record, but we fail to account for the fact that once a record
> > hits rc_refcount == MAXREFCOUNT, it is pinned that way forever.  Hence
> 
> One question:
> Is it reansonable/expected to pin rc_refcount forever when a record hits
> rc_refcount == MAXREFCOUNT?

In the ideal world we'd have a way for refcount_adjust to return early
if it hit a MAXREFCOUNT record, and stop the reflink operation right
there and then.

*Unfortunately* due to the way defer ops work, by the time we're walking
through the refcount btree, we've already committed to adding the entire
mapping.  There's no good way to back out once we start, since
transactions do not log undo items, only redo items.  Augmenting the log
to support undo items is a very big ask.

We could try to work around that by walking the refcount records
*before* committing log items, but that second walk will increase the
overhead for a fairly difficult to hit corner case.  We'd also need to
hold the AGF buffer lock from the start of the chain all the way until
we start the deferred refcount processing.  Or we'd need to add a
separate AG lock.

Even if we did all that, FICLONE* doesn't have any means to communicate
that a short reflink occurred.  copy_file_range does, but then it has
other weird warts, and cannot reflink more than 2^32 bytes in one go.

The simplest solution is to pin the extent if its refcount hits
MAXREFCOUNT, so that's what I went with. :/

--D

> > the computed refcount is wrong, and we fail to merge the extents.
> > 
> > Fix this by adjusting the merge predicates to compute the adjusted
> > refcount correctly.
> 
> Best Regards,
> Xiao Yang