[PATCH] xfs_repair: Bump link count if longform_dir2_rebuild yields shortform dir

[PATCH] xfs_repair: Bump link count if longform_dir2_rebuild yields shortform dir

[user.mail]

From: Eric Sandeen <sandeen@redhat.com>

If longform_dir2_rebuild() has so few entries in *hashtab that it results
in a short form directory, bump the link count manually as shortform
directories have no explicit "." entry.

Without this, repair will end with i.e.:

resetting inode 131 nlinks from 2 to 1

in this case, because it thinks this directory inode only has 1 link
discovered, and then a 2nd repair will fix it:

resetting inode 131 nlinks from 1 to 2

because shortform_dir2_entry_check() explicitly adds the extra ref when
the (newly-created)shortform directory is checked:

        /*
         * no '.' entry in shortform dirs, just bump up ref count by 1
         * '..' was already (or will be) accounted for and checked when
         * the directory is reached or will be taken care of when the
         * directory is moved to orphanage.
         */
        add_inode_ref(current_irec, current_ino_offset);

Avoid this by adding the extra ref if we convert from longform to
shortform.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Signed-off-by: user.mail <sandeen@redhat.com>
---
 repair/phase6.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/repair/phase6.c b/repair/phase6.c
index dbc090a5..8804278a 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -1392,6 +1392,13 @@ _("name create failed in ino %" PRIu64 " (%d)\n"), ino, error);
 _("name create failed (%d) during rebuild\n"), error);
 	}
 
+	/*
+	 * If we added too few entries to retain longform, add the extra
+	 * ref for . as this is now a shortform directory.
+	 */
+	if (ip->i_df.if_format == XFS_DINODE_FMT_LOCAL)
+		add_inode_ref(irec, ino_offset);
+
 	return;
 
 out_bmap_cancel:
-- 
2.49.0

Re: [PATCH] xfs_repair: Bump link count if longform_dir2_rebuild yields shortform dir

[Eric Sandeen]

On 4/15/25 1:09 PM, user.mail wrote:
> From: Eric Sandeen <sandeen@redhat.com>
> 
> If longform_dir2_rebuild() has so few entries in *hashtab that it results
> in a short form directory, bump the link count manually as shortform
> directories have no explicit "." entry.
> 
> Without this, repair will end with i.e.:
> 
> resetting inode 131 nlinks from 2 to 1
> 
> in this case, because it thinks this directory inode only has 1 link
> discovered, and then a 2nd repair will fix it:
> 
> resetting inode 131 nlinks from 1 to 2
> 
> because shortform_dir2_entry_check() explicitly adds the extra ref when
> the (newly-created)shortform directory is checked:
> 
>         /*
>          * no '.' entry in shortform dirs, just bump up ref count by 1
>          * '..' was already (or will be) accounted for and checked when
>          * the directory is reached or will be taken care of when the
>          * directory is moved to orphanage.
>          */
>         add_inode_ref(current_irec, current_ino_offset);
> 
> Avoid this by adding the extra ref if we convert from longform to
> shortform.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: user.mail <sandeen@redhat.com>

Ugh, sorry about the user.mail business, not sure how that happened.

New VM, who dis? :(

FWIW this fix is related to the fix Bill has been working on, but it
is independent. Bill's hoping to address the problem where we fail
to build up any directory entries for a long-form rebuild, and when
that is fixed, in most cases the entries will be there and the dir
will remain long form.

Still, it is possible that the entry scan turns up so few entries that
the directory becomes short form when rebuilt, so I think this patch
still makes sense.

Thanks,
-Eric

> ---
>  repair/phase6.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/repair/phase6.c b/repair/phase6.c
> index dbc090a5..8804278a 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -1392,6 +1392,13 @@ _("name create failed in ino %" PRIu64 " (%d)\n"), ino, error);
>  _("name create failed (%d) during rebuild\n"), error);
>  	}
>  
> +	/*
> +	 * If we added too few entries to retain longform, add the extra
> +	 * ref for . as this is now a shortform directory.
> +	 */
> +	if (ip->i_df.if_format == XFS_DINODE_FMT_LOCAL)
> +		add_inode_ref(irec, ino_offset);
> +
>  	return;
>  
>  out_bmap_cancel:

Re: [PATCH] xfs_repair: Bump link count if longform_dir2_rebuild yields shortform dir

[Bill O'Donnell]

On Tue, Apr 15, 2025 at 01:09:23PM -0500, user.mail wrote:
> From: Eric Sandeen <sandeen@redhat.com>
> 
> If longform_dir2_rebuild() has so few entries in *hashtab that it results
> in a short form directory, bump the link count manually as shortform
> directories have no explicit "." entry.
> 
> Without this, repair will end with i.e.:
> 
> resetting inode 131 nlinks from 2 to 1
> 
> in this case, because it thinks this directory inode only has 1 link
> discovered, and then a 2nd repair will fix it:
> 
> resetting inode 131 nlinks from 1 to 2
> 
> because shortform_dir2_entry_check() explicitly adds the extra ref when
> the (newly-created)shortform directory is checked:
> 
>         /*
>          * no '.' entry in shortform dirs, just bump up ref count by 1
>          * '..' was already (or will be) accounted for and checked when
>          * the directory is reached or will be taken care of when the
>          * directory is moved to orphanage.
>          */
>         add_inode_ref(current_irec, current_ino_offset);
> 
> Avoid this by adding the extra ref if we convert from longform to
> shortform.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: user.mail <sandeen@redhat.com>
> ---

I was about to send a v3 of my patch to handle this (fix link counts
update...) based on djwong's review. This looks cleaner. Thanks!

Reviewed-by: Bill O'Donnell <bodonnel@redhat.com>


>  repair/phase6.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/repair/phase6.c b/repair/phase6.c
> index dbc090a5..8804278a 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -1392,6 +1392,13 @@ _("name create failed in ino %" PRIu64 " (%d)\n"), ino, error);
>  _("name create failed (%d) during rebuild\n"), error);
>  	}
>  
> +	/*
> +	 * If we added too few entries to retain longform, add the extra
> +	 * ref for . as this is now a shortform directory.
> +	 */
> +	if (ip->i_df.if_format == XFS_DINODE_FMT_LOCAL)
> +		add_inode_ref(irec, ino_offset);
> +
>  	return;
>  
>  out_bmap_cancel:
> -- 
> 2.49.0
> 

Re: [PATCH] xfs_repair: Bump link count if longform_dir2_rebuild yields shortform dir

[Eric Sandeen]

On 4/15/25 1:23 PM, Bill O'Donnell wrote:
> On Tue, Apr 15, 2025 at 01:09:23PM -0500, user.mail wrote:
>> From: Eric Sandeen <sandeen@redhat.com>
>>
>> If longform_dir2_rebuild() has so few entries in *hashtab that it results
>> in a short form directory, bump the link count manually as shortform
>> directories have no explicit "." entry.
>>
>> Without this, repair will end with i.e.:
>>
>> resetting inode 131 nlinks from 2 to 1
>>
>> in this case, because it thinks this directory inode only has 1 link
>> discovered, and then a 2nd repair will fix it:
>>
>> resetting inode 131 nlinks from 1 to 2
>>
>> because shortform_dir2_entry_check() explicitly adds the extra ref when
>> the (newly-created)shortform directory is checked:
>>
>>         /*
>>          * no '.' entry in shortform dirs, just bump up ref count by 1
>>          * '..' was already (or will be) accounted for and checked when
>>          * the directory is reached or will be taken care of when the
>>          * directory is moved to orphanage.
>>          */
>>         add_inode_ref(current_irec, current_ino_offset);
>>
>> Avoid this by adding the extra ref if we convert from longform to
>> shortform.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> Signed-off-by: user.mail <sandeen@redhat.com>
>> ---
> 
> I was about to send a v3 of my patch to handle this (fix link counts
> update...) based on djwong's review. This looks cleaner. Thanks!

This is related to, but independent of, your patch (see my self-reply).
Please continue to fix your case, so that all entries do not end up in
lost+found when the header is bad.

Thanks,
-Eric

> Reviewed-by: Bill O'Donnell <bodonnel@redhat.com>
> 
> 
>>  repair/phase6.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/repair/phase6.c b/repair/phase6.c
>> index dbc090a5..8804278a 100644
>> --- a/repair/phase6.c
>> +++ b/repair/phase6.c
>> @@ -1392,6 +1392,13 @@ _("name create failed in ino %" PRIu64 " (%d)\n"), ino, error);
>>  _("name create failed (%d) during rebuild\n"), error);
>>  	}
>>  
>> +	/*
>> +	 * If we added too few entries to retain longform, add the extra
>> +	 * ref for . as this is now a shortform directory.
>> +	 */
>> +	if (ip->i_df.if_format == XFS_DINODE_FMT_LOCAL)
>> +		add_inode_ref(irec, ino_offset);
>> +
>>  	return;
>>  
>>  out_bmap_cancel:
>> -- 
>> 2.49.0
>>
> 
> 

Re: [PATCH] xfs_repair: Bump link count if longform_dir2_rebuild yields shortform dir

[Bill O'Donnell]

On Tue, Apr 15, 2025 at 01:44:25PM -0500, Eric Sandeen wrote:
> On 4/15/25 1:23 PM, Bill O'Donnell wrote:
> > On Tue, Apr 15, 2025 at 01:09:23PM -0500, user.mail wrote:
> >> From: Eric Sandeen <sandeen@redhat.com>
> >>
> >> If longform_dir2_rebuild() has so few entries in *hashtab that it results
> >> in a short form directory, bump the link count manually as shortform
> >> directories have no explicit "." entry.
> >>
> >> Without this, repair will end with i.e.:
> >>
> >> resetting inode 131 nlinks from 2 to 1
> >>
> >> in this case, because it thinks this directory inode only has 1 link
> >> discovered, and then a 2nd repair will fix it:
> >>
> >> resetting inode 131 nlinks from 1 to 2
> >>
> >> because shortform_dir2_entry_check() explicitly adds the extra ref when
> >> the (newly-created)shortform directory is checked:
> >>
> >>         /*
> >>          * no '.' entry in shortform dirs, just bump up ref count by 1
> >>          * '..' was already (or will be) accounted for and checked when
> >>          * the directory is reached or will be taken care of when the
> >>          * directory is moved to orphanage.
> >>          */
> >>         add_inode_ref(current_irec, current_ino_offset);
> >>
> >> Avoid this by adding the extra ref if we convert from longform to
> >> shortform.
> >>
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> Signed-off-by: user.mail <sandeen@redhat.com>
> >> ---
> > 
> > I was about to send a v3 of my patch to handle this (fix link counts
> > update...) based on djwong's review. This looks cleaner. Thanks!
> 
> This is related to, but independent of, your patch (see my self-reply).
> Please continue to fix your case, so that all entries do not end up in
> lost+found when the header is bad.

Ah, right. Looks good still ;)

Reviewed-by: Bill O'Donnell <bodonnel@redhat.com>

> 
> Thanks,
> -Eric
> 
> > Reviewed-by: Bill O'Donnell <bodonnel@redhat.com>
> > 
> > 
> >>  repair/phase6.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/repair/phase6.c b/repair/phase6.c
> >> index dbc090a5..8804278a 100644
> >> --- a/repair/phase6.c
> >> +++ b/repair/phase6.c
> >> @@ -1392,6 +1392,13 @@ _("name create failed in ino %" PRIu64 " (%d)\n"), ino, error);
> >>  _("name create failed (%d) during rebuild\n"), error);
> >>  	}
> >>  
> >> +	/*
> >> +	 * If we added too few entries to retain longform, add the extra
> >> +	 * ref for . as this is now a shortform directory.
> >> +	 */
> >> +	if (ip->i_df.if_format == XFS_DINODE_FMT_LOCAL)
> >> +		add_inode_ref(irec, ino_offset);
> >> +
> >>  	return;
> >>  
> >>  out_bmap_cancel:
> >> -- 
> >> 2.49.0
> >>
> > 
> > 
> 

Re: [PATCH] xfs_repair: Bump link count if longform_dir2_rebuild yields shortform dir

[Darrick J. Wong]

On Tue, Apr 15, 2025 at 01:09:23PM -0500, user.mail wrote:
> From: Eric Sandeen <sandeen@redhat.com>
> 
> If longform_dir2_rebuild() has so few entries in *hashtab that it results
> in a short form directory, bump the link count manually as shortform
> directories have no explicit "." entry.
> 
> Without this, repair will end with i.e.:
> 
> resetting inode 131 nlinks from 2 to 1
> 
> in this case, because it thinks this directory inode only has 1 link
> discovered, and then a 2nd repair will fix it:
> 
> resetting inode 131 nlinks from 1 to 2
> 
> because shortform_dir2_entry_check() explicitly adds the extra ref when
> the (newly-created)shortform directory is checked:
> 
>         /*
>          * no '.' entry in shortform dirs, just bump up ref count by 1
>          * '..' was already (or will be) accounted for and checked when
>          * the directory is reached or will be taken care of when the
>          * directory is moved to orphanage.
>          */
>         add_inode_ref(current_irec, current_ino_offset);
> 
> Avoid this by adding the extra ref if we convert from longform to
> shortform.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: user.mail <sandeen@redhat.com>

Whoever this is  ^^^^^^^^^, the change makes sense to me. ;)
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  repair/phase6.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/repair/phase6.c b/repair/phase6.c
> index dbc090a5..8804278a 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -1392,6 +1392,13 @@ _("name create failed in ino %" PRIu64 " (%d)\n"), ino, error);
>  _("name create failed (%d) during rebuild\n"), error);
>  	}
>  
> +	/*
> +	 * If we added too few entries to retain longform, add the extra
> +	 * ref for . as this is now a shortform directory.
> +	 */
> +	if (ip->i_df.if_format == XFS_DINODE_FMT_LOCAL)
> +		add_inode_ref(irec, ino_offset);
> +
>  	return;
>  
>  out_bmap_cancel:
> -- 
> 2.49.0
> 
> 

Re: [PATCH] xfs_repair: Bump link count if longform_dir2_rebuild yields shortform dir

[Andrey Albershteyn]

On 2025-04-15 13:09:23, user.mail wrote:
> From: Eric Sandeen <sandeen@redhat.com>
> 
> If longform_dir2_rebuild() has so few entries in *hashtab that it results
> in a short form directory, bump the link count manually as shortform
> directories have no explicit "." entry.
> 
> Without this, repair will end with i.e.:
> 
> resetting inode 131 nlinks from 2 to 1
> 
> in this case, because it thinks this directory inode only has 1 link
> discovered, and then a 2nd repair will fix it:
> 
> resetting inode 131 nlinks from 1 to 2
> 
> because shortform_dir2_entry_check() explicitly adds the extra ref when
> the (newly-created)shortform directory is checked:
> 
>         /*
>          * no '.' entry in shortform dirs, just bump up ref count by 1
>          * '..' was already (or will be) accounted for and checked when
>          * the directory is reached or will be taken care of when the
>          * directory is moved to orphanage.
>          */
>         add_inode_ref(current_irec, current_ino_offset);
> 
> Avoid this by adding the extra ref if we convert from longform to
> shortform.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: user.mail <sandeen@redhat.com>

I will drop this SoB when applying

> ---
>  repair/phase6.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/repair/phase6.c b/repair/phase6.c
> index dbc090a5..8804278a 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -1392,6 +1392,13 @@ _("name create failed in ino %" PRIu64 " (%d)\n"), ino, error);
>  _("name create failed (%d) during rebuild\n"), error);
>  	}
>  
> +	/*
> +	 * If we added too few entries to retain longform, add the extra
> +	 * ref for . as this is now a shortform directory.
> +	 */
> +	if (ip->i_df.if_format == XFS_DINODE_FMT_LOCAL)
> +		add_inode_ref(irec, ino_offset);
> +
>  	return;
>  
>  out_bmap_cancel:
> -- 
> 2.49.0
> 
> 

-- 
- Andrey