Re: [PATCH v2] xfs_repair: fix link counts update following repair of a bad block

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

On Tue, Apr 15, 2025 at 10:01:04AM -0500, bodonnel@redhat.com wrote:
> From: Bill O'Donnell <bodonnel@redhat.com>
> 
> Updating nlinks, following repair of a bad block needs a bit of work.
> In unique cases, 2 runs of xfs_repair is needed to adjust the count to
> the proper value. This patch modifies location of longform_dir2_entry_check,
> moving longform_dir2_entry_check_data to run after the check_dir3_header
> error check. This results in the hashtab to be correctly filled and those
> entries don't end up in lost+found, and nlinks is properly adjusted on the
> first xfs_repair pass.
> 
> Suggested-by: Eric Sandeen <sandeen@sandeen.net>
> 
> Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
> ---
> v2: add logic to cover shortform directory.
> 
> 
>  repair/phase6.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/repair/phase6.c b/repair/phase6.c
> index dbc090a54139..8fc1c3896d2b 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -2426,6 +2426,23 @@ longform_dir2_entry_check(
>  
>  		/* check v5 metadata */
>  		if (xfs_has_crc(mp)) {
> +			longform_dir2_entry_check_data(mp, ip, num_illegal,
> +				need_dot,
> +				irec, ino_offset, bp, hashtab,
> +				&freetab, da_bno, fmt == XFS_DIR2_FMT_BLOCK);
> +			error = check_dir3_header(mp, bp, ino);
> +			if (error) {
> +				fixit++;

I think what you're trying to do here is to get
longform_dir2_entry_check_data to try to find directory entries in the
directory block (no matter how damaged it is).  Then if the dir3 header
fields are wrong, we bump fixit so that the directory gets rebuilt from
the salvaged directory entries.  Right?

So I think you could structure this more like:

		/* salvage any dirents that look ok */
		longform_dir2_entry_check_data(...);

		/* check v5 metadata */
		if (xfs_has_crc(mp)) {
			error = check_dir3_header(mp, bp, ino);
			if (error) {
				fixit++;
				if (fmt == XFS_DIR2_FMT_BLOCK)
					goto out_fix;

				libxfs_buf_relse(bp);
				bp = NULL;
				continue;
			}
		}

		if (fmt == XFS_DIR2_FMT_BLOCK)
			break;

		libxfs_buf_relse(bp);
		bp = NULL;
	}

> +				if (fmt == XFS_DIR2_FMT_BLOCK)
> +					goto out_fix;
> +
> +				libxfs_buf_relse(bp);
> +				bp = NULL;
> +				continue;
> +			}
> +		}
> +		else {
> +			/* No crc. Directory appears to be shortform. */
>  			error = check_dir3_header(mp, bp, ino);

dir3 headers (as opposed to dir2 headers) are a crc-only feature, so
this isn't correct either.

>  			if (error) {
>  				fixit++;
> @@ -2438,9 +2455,6 @@ longform_dir2_entry_check(
>  			}
>  		}
>  
> -		longform_dir2_entry_check_data(mp, ip, num_illegal, need_dot,
> -				irec, ino_offset, bp, hashtab,
> -				&freetab, da_bno, fmt == XFS_DIR2_FMT_BLOCK);

and removing this call means that we never scan a V4 directory at all.

--D

>  		if (fmt == XFS_DIR2_FMT_BLOCK)
>  			break;
>  
> -- 
> 2.49.0
> 
>