xfs
[Top] [All Lists]

Re: [PATCH 03/10] xfs_repair: ignore "repaired" flag after we decide to

To: "Darrick J. Wong" <djwong@xxxxxxxxxxxxxxxx>, david@xxxxxxxxxxxxx, darrick.wong@xxxxxxxxxx
Subject: Re: [PATCH 03/10] xfs_repair: ignore "repaired" flag after we decide to clear xattr block
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Mon, 17 Aug 2015 14:20:32 -0500
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20150815014357.1839.68720.stgit@xxxxxxxxxxxxxxxx>
References: <20150815014338.1839.37405.stgit@xxxxxxxxxxxxxxxx> <20150815014357.1839.68720.stgit@xxxxxxxxxxxxxxxx>
On 8/14/15 8:43 PM, Darrick J. Wong wrote:
> If in the course of examining extended attribute block contents we
> first decide to repair an entry (*repair = 1) but secondly decide to
> clear the whole block, set *repair = 0 because the clearing action
> only happens if *repair == 0.  Put another way, if we're nuking a
> block, don't pretend like we've fixed it too.

Hm, or what happens otherwise?

TBH this is making my brain hurt, chasing it all the way back up
the callchain.  It's *repair not *repaired; it's not saying we
fixed it, but that we should fix it, right?  It'll take me a while
to work out what all the callers do with the "clearit" return from
process_attr_leaf_block and with *repair.

Did you have a specific example of something going wrong?

thanks,
-Eric

> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> ---
>  repair/attr_repair.c |   14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/repair/attr_repair.c b/repair/attr_repair.c
> index 62f80e7..2bd9334 100644
> --- a/repair/attr_repair.c
> +++ b/repair/attr_repair.c
> @@ -1311,6 +1311,13 @@ process_leaf_attr_block(
>               * we can add it then.
>               */
>       }
> +     /*
> +      * If we're just going to zap the block, don't pretend like we
> +      * repaired it, because repairing the block stops the clear
> +      * operation.
> +      */
> +     if (clearit)
> +             *repair = 0;
>       if (*repair)
>               xfs_attr3_leaf_hdr_to_disk(mp->m_attr_geo, leaf, &leafhdr);
>  
> @@ -1524,6 +1531,7 @@ process_longform_attr(
>       xfs_dahash_t    next_hashval;
>       int             repairlinks = 0;
>       struct xfs_attr3_icleaf_hdr leafhdr;
> +     int             error;
>  
>       *repair = 0;
>  
> @@ -1604,12 +1612,16 @@ process_longform_attr(
>                       libxfs_writebuf(bp, 0);
>               } else
>                       libxfs_putbuf(bp);
> -             return (process_node_attr(mp, ino, dip, blkmap)); /* + repair */
> +             error = process_node_attr(mp, ino, dip, blkmap); /* + repair */
> +             if (error)
> +                     *repair = 0;
> +             return error;
>       default:
>               do_warn(
>       _("bad attribute leaf magic # %#x for dir ino %" PRIu64 "\n"),
>                       be16_to_cpu(leaf->hdr.info.magic), ino);
>               libxfs_putbuf(bp);
> +             *repair = 0;
>               return(1);
>       }
>  
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
> 

<Prev in Thread] Current Thread [Next in Thread>