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

Dave Chinner
2015-08-26T01:03:27Z

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