Re: [PATCH 1/7] xfs: add a function to deal with corrupt buffers post-verifiers

[Dave Chinner <david@fromorbit.com>]

On Tue, Mar 10, 2020 at 05:47:19PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Add a helper function to get rid of buffers that we have decided are
> corrupt after the verifiers have run.  This function is intended to
> handle metadata checks that can't happen in the verifiers, such as
> inter-block relationship checking.  Note that we now mark the buffer
> stale so that it will not end up on any LRU and will be purged on
> release.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
....
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 11a97bc35f70..9d26e37f78f5 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -16,6 +16,8 @@
>  #include "xfs_log.h"
>  #include "xfs_errortag.h"
>  #include "xfs_error.h"
> +#include "xfs_trans.h"
> +#include "xfs_buf_item.h"
>  
>  static kmem_zone_t *xfs_buf_zone;
>  
> @@ -1572,6 +1574,29 @@ xfs_buf_zero(
>  	}
>  }
>  
> +/*
> + * Log a message about and stale a buffer that a caller has decided is corrupt.
> + *
> + * This function should be called for the kinds of metadata corruption that
> + * cannot be detect from a verifier, such as incorrect inter-block relationship
> + * data.  Do /not/ call this function from a verifier function.

So if it's called from a read verifier, the buffer will not have the
XBF_DONE flag set on it. Maybe we should assert that this flag is
set on the buffer? Yes, I know XBF_DONE will be set at write time,
but most verifiers are called from both the read and write path so
it should catch invalid use at read time...

> + * The buffer must not be dirty prior to the call.  Afterwards, the buffer will

Why can't it be dirty?

> + * be marked stale, but b_error will not be set.  The caller is responsible for
> + * releasing the buffer or fixing it.
> + */
> +void
> +__xfs_buf_mark_corrupt(
> +	struct xfs_buf		*bp,
> +	xfs_failaddr_t		fa)
> +{
> +	ASSERT(bp->b_log_item == NULL ||
> +	       !(bp->b_log_item->bli_flags & XFS_BLI_DIRTY));

XFS_BLI_DIRTY isn't a complete definition of a dirty buffer. What it
means is "modifications to this buffer are not yet
committed to the journal". It may have been linked into a
transaction but not modified, but it can still be XFS_BLI_DIRTY
because it is in the CIL. IOWs, transactions can cancel safely
aborted even when the items in it are dirty as long as the
transaction itself is clean and made no modifications to the object.

Hence I'm not sure what you are trying to protect against here?

The rest of the code looks fine.

Cheers,

Dave,
-- 
Dave Chinner
david@fromorbit.com