Re: [PATCH 1/4] repair: set the in-core inode parent in phase 3

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Wed, Jul 15, 2020 at 10:08:33AM -0400, Brian Foster wrote:
> The inode processing code checks and resets invalid parent values on
> physical inodes in phase 3 but waits to update the parent value in
> the in-core tracking until phase 4. There doesn't appear to be any
> specific reason for the latter beyond caution. In reality, the only
> reason this doesn't cause problems is that phase 3 replaces an
> invalid on-disk parent with another invalid value, so the in-core
> parent returned by phase 4 translates to NULLFSINO.
> 
> This is subtle and fragile. To eliminate this duplicate processing
> behavior and break the subtle dependency of requiring an invalid
> dummy value in physical directory inodes, update the in-core parent
> tracking structure at the same point in phase 3 that physical inodes
> are updated. Invalid on-disk parent values will still translate to
> NULLFSINO for the in-core tracking to be identified by later phases.
> This ensures that if a valid dummy value is placed in a physical
> inode (such as rootino) with an invalid parent in phase 3, phase 4
> won't mistakenly return the valid dummy value to be incorrectly set
> in the in-core tracking over the NULLFSINO value that represents the
> broken on-disk state.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks fine to me,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  repair/dino_chunks.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/repair/dino_chunks.c b/repair/dino_chunks.c
> index 6685a4d2..96ed6a5b 100644
> --- a/repair/dino_chunks.c
> +++ b/repair/dino_chunks.c
> @@ -859,14 +859,7 @@ next_readbuf:
>  		 */
>  		if (isa_dir)  {
>  			set_inode_isadir(ino_rec, irec_offset);
> -			/*
> -			 * we always set the parent but
> -			 * we may as well wait until
> -			 * phase 4 (no inode discovery)
> -			 * because the parent info will
> -			 * be solid then.
> -			 */
> -			if (!ino_discovery)  {
> +			if (ino_discovery)  {
>  				ASSERT(parent != 0);
>  				set_inode_parent(ino_rec, irec_offset, parent);
>  				ASSERT(parent ==
> -- 
> 2.21.3
>