Re: [PATCH] repair: fix process_rt_rec_dups

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

On Wed, Nov 08, 2023 at 06:53:20PM +0100, Christoph Hellwig wrote:
> search_rt_dup_extent takes a xfs_rtblock_t, not an RT extent number.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> 
> What scares me about this is that no test seems to hit this and report
> false duplicates.  I'll need to see if I can come up with an
> artifical reproducers of some kind.

I think you've misread the code -- phase 4 builds the rt_dup tree by
walks all the rtextents, and adding the duplicates:

	for (rtx = 0; rtx < mp->m_sb.sb_rextents; rtx++)  {
		bstate = get_rtbmap(rtx);
		switch (bstate)  {
			...
		case XR_E_FS_MAP:
			if (rt_start == 0)
				continue;
			else  {
				/*
				 * add extent and reset extent state
				 */
				add_rt_dup_extent(rt_start, rt_len);
				rt_start = 0;
				rt_len = 0;
			}
			break;
		case XR_E_MULT:
			if (rt_start == 0)  {
				rt_start = rtx;
				rt_len = 1;
			} else if (rt_len == XFS_MAX_BMBT_EXTLEN)  {
				/*
				 * large extent case
				 */
				add_rt_dup_extent(rt_start, rt_len);
				rt_start = rtx;
				rt_len = 1;
			} else
				rt_len++;
			break;

So I think the reason why you've never seen false duplicates is that the
rt_dup tree intervals measure rt extents, not rt blocks.  The units
conversion in process_rt_rec_dups is correct.

However, none of that is at all obvious because of the dual uses of
xfs_rtblock_t for rt blocks and rt extents.

I guess I ought to post the xfsprogs version of those rt units cleanups.
:)

--D

>  repair/dinode.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/repair/dinode.c b/repair/dinode.c
> index c10dd1fa3..9aa367138 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -194,13 +194,11 @@ process_rt_rec_dups(
>  	struct xfs_bmbt_irec	*irec)
>  {
>  	xfs_fsblock_t		b;
> -	xfs_rtblock_t		ext;
>  
>  	for (b = rounddown(irec->br_startblock, mp->m_sb.sb_rextsize);
>  	     b < irec->br_startblock + irec->br_blockcount;
>  	     b += mp->m_sb.sb_rextsize) {
> -		ext = (xfs_rtblock_t) b / mp->m_sb.sb_rextsize;
> -		if (search_rt_dup_extent(mp, ext))  {
> +		if (search_rt_dup_extent(mp, b))  {
>  			do_warn(
>  _("data fork in rt ino %" PRIu64 " claims dup rt extent,"
>  "off - %" PRIu64 ", start - %" PRIu64 ", count %" PRIu64 "\n"),
> -- 
> 2.39.2
>