Re: [PATCH] xfs: fix spurious len double-counting in rmap three-way merge check

[Jianyun Gao]

Hi Carlos,

Thanks for the feedback. I've now run the full auto group with the patch
applied, and as you suggested, this gives a much more thorough picture
than the two targeted tests.

Command and configuration:
  kvm-xfstests -c xfs/default -g auto -X generic/778
  KERNEL: 7.3.0-rc6-xfstests-00194-gf4a2f5951fee
  CPUS: 8, MEM: ~8G
  xfstests: v2025.11.04-10-gdf376ed02

Results:
  xfs/4k: 1214 tests, 7 failures, 231 skipped, 10890 seconds
  Totals: 1242 tests, 231 skipped, 31 failures, 0 errors, 10122s

Failure analysis:
- generic/633, generic/696, generic/697, generic/753, xfs/018: these are known
  pre-existing failures on this kernel version (also reported in
recent community test runs);
  they are unrelated to the rmap merge logic.
- xfs/013: the test's own output shows ENOSPC errors leaking from a known
  shell redirection bug that broke the filter_enospc logic; a fix was
  posted upstream in June 2026 but is not present in the v2025.11.04
  snapshot used here.
- xfs/155 (marked flaky at 20%): the full log shows xfs_repair returning
  an unexpected exit code (1) after the test intentionally injected write
  failures via LIBXFS_DEBUG_WRITE_CRASH. This is a userspace tool/test
  interaction issue, not a kernel rmap regression.
- generic/778 was excluded as it is a known flaky/performance-bug test
  (40% flaky in an earlier run), and Darrick's later fix for it is not in
  this xfstests snapshot.

There are no new failures attributable to the rmap three-way merge
change. The xfs/236 run from the previous series also still passes.

Please let me know if you'd like any additional configurations or
specific tests re-run.

Thanks,
Jianyun

On Wed, Nov 4, 2026 at 11:01 PM Carlos Maiolino <cem@xxxxxxxxxx> wrote:
>
> On Wed, Nov 04, 2026 at 11:27:54AM +0800, Jianyun Gao wrote:
> > Hi Carlos,
> >
> > Thanks for the review. To answer your questions:
> >
> > How I found it: I found this while reviewing the merge logic in
> > xfs_rmap_map. After tracing the left-edge merge path, I noticed that
> > ltrec.rm_blockcount is already updated at line 1119
> > (ltrec.rm_blockcount += len), so adding len again in the overflow check
> > is redundant. When the real merged length is still within
> > XFS_RMAP_LEN_MAX but the inflated value exceeds it, the three-way merge
> > is incorrectly rejected, leaving the right record unmerged.
> >
> > Testing: I ran the rmap-specific regression tests with this patch
> > applied, using kvm-xfstests on a 4k-block XFS configuration with
> > rmapbt=1 and reflink=1:
> >
> >   kvm-xfstests -c xfs/4k xfs/236 xfs/578
> >
> > Results:
> >   xfs/236      Pass     6s
> >   xfs/578      Pass     34s
> > Totals: 2 tests, 0 skipped, 0 failures, 0 errors
> >
> > xfs/236 directly exercises the rmap btree grow/shrink paths affected by
> > this change. I did not observe any regressions or warnings.
> > If there are specific test cases you'd like me to run, please let me know.
>
> Ok, I'd expect the tests to be a bit more thorough than two test cases,
> ie. I'd expect this to be ran against at least the auto group too. But
> nevertheless I'm glad you at did at least some of the homework.
>
> The patch also seems fine to me, so
>
> Reviewed-by: Carlos Maiolino <cmaiolino@xxxxxxxxxx>
>
> >
> > Thanks,
> > Jianyun
> >
> > On Tue, Nov 3, 2026 at 11:33 PM Carlos Maiolino <cem@xxxxxxxxxx> wrote:
> > >
> > > On Tue, Nov 03, 2026 at 10:41:44PM +0800, jianyungao89@xxxxxxxxx wrote:
> > > > From: Jianyun Gao <jianyungao89@xxxxxxxxx>
> > > >
> > > > In xfs_rmap_map, the left-edge merge path already executes
> > > > ltrec.rm_blockcount += len at line 1119 before checking whether the
> > > > right record is also contiguous and can be merged.  The subsequent
> > > > overflow guard therefore must compare the already-incremented
> > > > ltrec.rm_blockcount plus gtrec.rm_blockcount against XFS_RMAP_LEN_MAX,
> > > > but it incorrectly adds len a second time:
> > > >
> > > > (unsigned long)ltrec.rm_blockcount + len +
> > > >    gtrec.rm_blockcount <= XFS_RMAP_LEN_MAX
> > > >
> > > > The extra len makes the check too strict: when the real merged length
> > > > does not overflow but the inflated value exceeds XFS_RMAP_LEN_MAX, the
> > > > three-way merge is rejected and the code falls back to a two-way merge,
> > > > leaving the right record unmerged.  This produces rmap btree
> > > > fragmentation but does not corrupt on-disk metadata.
> > > >
> > > > Remove the spurious len from the overflow check.
> > > >
> > > > Fixes: 0a1b0b3855cf7 ("xfs: add an extent to the rmap btree")
> > > > Cc: <stable@xxxxxxxxxxxxxxx>
> > > > Signed-off-by: Jianyun Gao <jianyungao89@xxxxxxxxx>
> > >
> > > Hi, even though this looks reasonably correct, how did you find this
> > > issue?
> > > Did you test this at all against any regressions (i.e. running it
> > > against xfstests)? This is touching critical logic on the rmap trees
> > > which can potentially end really bad.
> > >
> > > Cheers.
> > >
> > > > ---
> > > >  fs/xfs/libxfs/xfs_rmap.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/fs/xfs/libxfs/xfs_rmap.c b/fs/xfs/libxfs/xfs_rmap.c
> > > > index 14aef87837a9..c2b2f8e7502d 100644
> > > > --- a/fs/xfs/libxfs/xfs_rmap.c
> > > > +++ b/fs/xfs/libxfs/xfs_rmap.c
> > > > @@ -1120,7 +1120,7 @@ xfs_rmap_map(
> > > >               if (have_gt &&
> > > >                   bno + len == gtrec.rm_startblock &&
> > > >                   (ignore_off || offset + len == gtrec.rm_offset) &&
> > > > -                 (unsigned long)ltrec.rm_blockcount + len +
> > > > +                 (unsigned long)ltrec.rm_blockcount +
> > > >                               gtrec.rm_blockcount <= XFS_RMAP_LEN_MAX) {
> > > >                       /*
> > > >                        * right edge also contiguous, delete right record
> > > > --
> > > > 2.34.1
> > > >
> > > >
> >