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

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

[bodonnel]

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 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>
---

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;
 
-- 
2.49.0

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

[Eric Sandeen]

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>

> ---
> 
> 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;
>  

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

[Darrick J. Wong]

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;
> >  
> 
>