On Mon, May 08, 2023 at 08:31:07AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
--
Dave Chinner
david@xxxxxxxxxxxxx
