On Tue, Apr 15, 2025 at 10:07:57AM -0700, Darrick J. Wong wrote:
> On Tue, Apr 15, 2025 at 10:01:04AM -0500, bodonnel@xxxxxxxxxx wrote:
> > From: Bill O'Donnell <bodonnel@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> >
> > Signed-off-by: Bill O'Donnell <bodonnel@xxxxxxxxxx>
> > ---
> > 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?
Yes.
>
> 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;
> }
Ah, this makes better sense.
>
> > + 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.
Agree.
>
> > 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.
Doh! I'll fix it up and send a v3. Thanks for your review.
-Bill
>
> --D
>
> > if (fmt == XFS_DIR2_FMT_BLOCK)
> > break;
> >
> > --
> > 2.49.0
> >
> >
>
