Re: [PATCH 04/11] libxfs: clear buffer state flags in libxfs_getbuf and variants

[Dave Chinner <david@fromorbit.com>]

On Tue, Aug 25, 2015 at 05:32:46PM -0700, Darrick J. Wong wrote:
> When we're running xfs_repair with prefetch enabled, it's possible
> that repair will decide to clear an inode without examining all
> metadata blocks owned by that inode.  This leaves the unreferenced
> prefetched buffers marked UNCHECKED, which will cause a subsequent CRC
> error if the block is reallocated to a different structure and read
> more than once.  Typically this happens when a large directory is
> corrupted and lost+found has to grow to accomodate all the
> disconnected inodes.
> 
> In libxfs_getbuf*(), we're supposed to return an unused buffer which
> has a clean state.  Unfortunately, things like UNCHECKED can hang
> around to cause incorrect verifier errors later, so change those
> functions to launder the state bits clean.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  libxfs/rdwr.c |   47 +++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 41 insertions(+), 6 deletions(-)
> 
> 
> diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
> index 4f8212f..d28cea8 100644
> --- a/libxfs/rdwr.c
> +++ b/libxfs/rdwr.c
> @@ -631,15 +631,39 @@ libxfs_getbuf_flags(struct xfs_buftarg *btp, xfs_daddr_t blkno, int len,
>  	return __cache_lookup(&key, flags);
>  }
>  
> +/*
> + * Clean the buffer flags for libxfs_getbuf*(), which wants to return
> + * an unused buffer with clean state.  This prevents CRC errors on a
> + * re-read of a corrupt block that was prefetched and freed.  This
> + * can happen with a massively corrupt directory that is discarded,
> + * but whose blocks are then recycled into expanding lost+found.
> + *
> + * Note however that if the buffer's dirty (prefetch calls getbuf)
> + * we'll leave the state alone because we don't want to discard blocks
> + * that have been fixed.
> + */
> +static void
> +try_clean_buf(

Only thing I don't like about this patch is the name of this
function. It's really a "reset buffer state" function, so I think
that calling it something like reset_buf_state() would be more
appropriate.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs