Re: [PATCH] xfs: fix broken logic when detecting mergeable bmap records

[Dave Chinner <david@fromorbit.com>]

On Mon, May 08, 2023 at 08:31:07AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Commit 6bc6c99a944c was a well-intentioned effort to initiate
> consolidation of adjacent bmbt mapping records by setting the PREEN
> flag.  Consolidation can only happen if the length of the combined
> record doesn't overflow the 21-bit blockcount field of the bmbt
> recordset.  Unfortunately, the length test is inverted, leading to it
> triggering on data forks like these:
> 
>  EXT: FILE-OFFSET           BLOCK-RANGE        AG AG-OFFSET               TOTAL
>    0: [0..16777207]:        76110848..92888055  0 (76110848..92888055) 16777208
>    1: [16777208..20639743]: 92888056..96750591  0 (92888056..96750591)  3862536
> 
> Note that record 0 has a length of 16777208 512b blocks.  This
> corresponds to 2097151 4k fsblocks, which is the maximum.  Hence the two
> records cannot be merged.
> 
> However, the logic is still wrong even if we change the in-loop
> comparison, because the scope of our examination isn't broad enough
> inside the loop to detect mappings like this:
> 
>    0: [0..9]:               76110838..76110847  0 (76110838..76110847)       10
>    1: [10..16777217]:       76110848..92888055  0 (76110848..92888055) 16777208
>    2: [16777218..20639753]: 92888056..96750591  0 (92888056..96750591)  3862536
> 
> These three records could be merged into two, but one cannot determine
> this purely from looking at records 0-1 or 1-2 in isolation.
> 
> Hoist the mergability detection outside the loop, and base its decision
> making on whether or not a merged mapping could be expressed in fewer
> bmbt records.  While we're at it, fix the incorrect return type of the
> iter function.
> 
> Fixes: 6bc6c99a944c ("xfs: alert the user about data/attr fork mappings that could be merged")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/scrub/bmap.c |   25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)

Looks OK, will throw into the test tree.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com