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

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

On Tue, Apr 15, 2025 at 02:12:31PM -0500, Eric Sandeen wrote:
> On 4/15/25 1:48 PM, 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,
> > to handle both longform and shortform directory cases. 
> 
> This is not accurate; short form directories are not generally in play here.
> They only arise due to the directory rebuild orphaning all entries without
> the prior scan, yielding an empty directory.
> 
> > 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.
> 
> Changelog suggestion:
> 
> xfs_repair: phase6: scan longform entries before header check
> 
> In longform_dir2_entry_check, if check_dir3_header() fails for v5
> metadata, we immediately go to out_fix: and try to rebuild the directory
> via longform_dir2_rebuild. But because we haven't yet called
> longform_dir2_entry_check_data, the *hashtab used to rebuild the
> directory is empty, which results in all existing entries getting
> moved to lost+found, and an empty rebuilt directory. On top of that,
> the empty directory is now short form, so its nlinks come out wrong
> and this requires another repair run to fix.
> 
> Scan the entries before checking the header, so that we have a decent
> chance of properly rebuilding the dir if the header is corrupt, rather
> than orphaning all the entries and moving them to lost+found.
> 
> > Suggested-by: Eric Sandeen <sandeen@sandeen.net>
> > 
> > Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
> 
> Other than the commit log, 
> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>

With the changelog amended,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> 
> > ---
> > 
> > v3: fix logic to cover the shortform directory case, and fix the description
> > v2: attempt to cover the case where header indicates shortform directory
> > v1:
> > 
> > 
> > 
> >  repair/phase6.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/repair/phase6.c b/repair/phase6.c
> > index dbc090a54139..4a3fafab3522 100644
> > --- a/repair/phase6.c
> > +++ b/repair/phase6.c
> > @@ -2424,6 +2424,11 @@ longform_dir2_entry_check(
> >  			continue;
> >  		}
> >  
> > +		/* salvage any dirents that look ok */
> > +		longform_dir2_entry_check_data(mp, ip, num_illegal, need_dot,
> > +				irec, ino_offset, bp, hashtab,
> > +				&freetab, da_bno, fmt == XFS_DIR2_FMT_BLOCK);
> > +
> >  		/* check v5 metadata */
> >  		if (xfs_has_crc(mp)) {
> >  			error = check_dir3_header(mp, bp, ino);
> > @@ -2438,9 +2443,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);
> >  		if (fmt == XFS_DIR2_FMT_BLOCK)
> >  			break;
> >  
> 
>