On Fri, Dec 22, 2017 at 04:43:53PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> Create some helper functions that we'll use later to deal with problems
> we might encounter while cross referencing metadata with other metadata.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> ---
> fs/xfs/scrub/btree.c | 72 +++++++++++++++++++++-----
> fs/xfs/scrub/btree.h | 9 +++
> fs/xfs/scrub/common.c | 138 +++++++++++++++++++++++++++++++++++++++++++++----
> fs/xfs/scrub/common.h | 28 ++++++++++
> fs/xfs/scrub/scrub.c | 10 ++++
> fs/xfs/scrub/trace.h | 22 ++++++++
> 6 files changed, 257 insertions(+), 22 deletions(-)
....
> -bool
> -xfs_scrub_btree_process_error(
> +static bool
> +__xfs_scrub_btree_process_error(
> struct xfs_scrub_context *sc,
> struct xfs_btree_cur *cur,
> int level,
> - int *error)
> + int *error,
> + bool xref,
> + void *ret_ip)
> {
> if (*error == 0)
> return true;
> @@ -60,36 +62,81 @@ xfs_scrub_btree_process_error(
> case -EFSBADCRC:
> case -EFSCORRUPTED:
> /* Note the badness but don't abort. */
> - sc->sm->sm_flags |= XFS_SCRUB_OFLAG_CORRUPT;
> + sc->sm->sm_flags |= xfs_scrub_process_error_flag(xref);
Hmmmm.
WHy not just pass in the relevant error flag, rather than a boolean
used to choose the error flag?
> *error = 0;
> /* fall through */
> default:
> if (cur->bc_flags & XFS_BTREE_ROOT_IN_INODE)
> trace_xfs_scrub_ifork_btree_op_error(sc, cur, level,
> - *error, __return_address);
> + *error, ret_ip);
> else
> trace_xfs_scrub_btree_op_error(sc, cur, level,
> - *error, __return_address);
> + *error, ret_ip);
> break;
> }
> return false;
> }
>
> +bool
> +xfs_scrub_btree_process_error(
> + struct xfs_scrub_context *sc,
> + struct xfs_btree_cur *cur,
> + int level,
> + int *error)
> +{
> + return __xfs_scrub_btree_process_error(sc, cur, level, error, false,
> + __return_address);
These then get easier to read, because there isn't a boolean that
you don't know what it means without looking at the function being
called. i.e
return __xfs_scrub_btree_process_error(sc, cur, level, error,
XFS_SCRUB_OFLAG_CORRUPT,
__return_address);
> +}
> +
> +bool
> +xfs_scrub_btree_xref_process_error(
> + struct xfs_scrub_context *sc,
> + struct xfs_btree_cur *cur,
> + int level,
> + int *error)
> +{
> + return __xfs_scrub_btree_process_error(sc, cur, level, error, true,
> + __return_address);
return __xfs_scrub_btree_process_error(sc, cur, level, error,
XFS_SCRUB_OFLAG_XFAIL,
__return_address);
> +}
> +
> /* Record btree block corruption. */
> -void
> -xfs_scrub_btree_set_corrupt(
> +static void
> +__xfs_scrub_btree_set_corrupt(
> struct xfs_scrub_context *sc,
> struct xfs_btree_cur *cur,
> - int level)
> + int level,
> + bool xref,
> + void *ret_ip)
> {
> - sc->sm->sm_flags |= XFS_SCRUB_OFLAG_CORRUPT;
> + if (xref)
> + sc->sm->sm_flags |= XFS_SCRUB_OFLAG_XCORRUPT;
> + else
> + sc->sm->sm_flags |= XFS_SCRUB_OFLAG_CORRUPT;
>
> if (cur->bc_flags & XFS_BTREE_ROOT_IN_INODE)
> trace_xfs_scrub_ifork_btree_error(sc, cur, level,
> - __return_address);
> + ret_ip);
> else
> trace_xfs_scrub_btree_error(sc, cur, level,
> - __return_address);
> + ret_ip);
> +}
> +
> +void
> +xfs_scrub_btree_set_corrupt(
> + struct xfs_scrub_context *sc,
> + struct xfs_btree_cur *cur,
> + int level)
> +{
> + __xfs_scrub_btree_set_corrupt(sc, cur, level, false, __return_address);
> +}
> +
> +void
> +xfs_scrub_btree_xref_set_corrupt(
> + struct xfs_scrub_context *sc,
> + struct xfs_btree_cur *cur,
> + int level)
> +{
> + __xfs_scrub_btree_set_corrupt(sc, cur, level, true, __return_address);
> }
Same for these (and the other equivalent wrapper sets in the patch).
> +
> +/*
> + * Predicate that decides if we need to evaluate the cross-reference check.
> + * If there was an error accessing the cross-reference btree, just delete
> + * the cursor and skip the check.
> + */
> +bool
> +xfs_scrub_should_xref(
> + struct xfs_scrub_context *sc,
> + int *error,
> + struct xfs_btree_cur **curpp)
> +{
> + /* If not a btree cross-reference, just check the error code. */
> + if (curpp == NULL) {
> + if (*error == 0)
> + return true;
> + goto fail;
> + }
> +
> + ASSERT(*curpp != NULL);
> + /* If no error or we've already given up on xref, just bail out. */
> + if (*error == 0 || *curpp == NULL)
> + return true;
Why the assert if we handle the null case just fine?
> +
> + /* xref error, delete cursor and bail out. */
> + xfs_btree_del_cursor(*curpp, XFS_BTREE_ERROR);
> + *curpp = NULL;
> +fail:
I think the logic up to this point can be cleaned up to be:
if (*error == 0)
return true;
if (curpp) {
/* If we've already given up on xref, just bail out. */
if (!*curpp)
return true;
/* xref error, delete cursor and bail out. */
xfs_btree_del_cursor(*curpp, XFS_BTREE_ERROR);
*curpp = NULL;
}
> + sc->sm->sm_flags |= XFS_SCRUB_OFLAG_XFAIL;
> + trace_xfs_scrub_xref_error(sc, *error, __return_address);
> +
> + /*
> + * Errors encountered during cross-referencing with another
> + * data structure should not cause this scrubber to abort.
> + */
> + *error = 0;
> + return false;
> +}
.....
> @@ -139,4 +155,16 @@ int xfs_scrub_get_inode(struct xfs_scrub_context *sc, struct xfs_inode *ip_in);
> int xfs_scrub_setup_inode_contents(struct xfs_scrub_context *sc,
> struct xfs_inode *ip, unsigned int resblks);
>
> +/*
> + * A libxfs function returned an error while scrubbing an object.
> + * If the function failed while operating on the object (!xref) then
> + * mark the object itself corrupt. If the function failed while
> + * collecting cross-referencing data from other metadata (xref), then
> + * mark that the cross referencing failed.
> + */
> +static inline __u32 xfs_scrub_process_error_flag(bool xref)
> +{
> + return xref ? XFS_SCRUB_OFLAG_XFAIL : XFS_SCRUB_OFLAG_CORRUPT;
> +}
If this is really needed, I'd like a better name - "process" doesn't
read right. Maybe xfs_scrub_xref_fail_flag()?
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
