From: Brian Foster <bfoster@redhat.com>
To: Eric Sandeen <sandeen@redhat.com>
Cc: linux-xfs <linux-xfs@vger.kernel.org>
Subject: Re: [PATCH 09/10] xfs: add errno to verifier context and populate it
Date: Fri, 7 Dec 2018 08:41:14 -0500 [thread overview]
Message-ID: <20181207134114.GD55482@bfoster> (raw)
In-Reply-To: <cc218ca1-a572-0dd0-d01b-89b33eeace4b@redhat.com>
On Wed, Dec 05, 2018 at 03:11:00PM -0600, Eric Sandeen wrote:
> Add errno to verifier context and set it on verifier failures;
> now rather than passing errno and vc->fa to xfs_verifier_error,
> we pass vc directly and let xfs_verifier_error suss out the
> errno and failaddr.
>
> Also make 3 new macros, XFS_CORRUPTED, XFS_BADCRC, and
> XFS_VERIFIED which set errno and failaddr into the verifier context
> without returning.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> fs/xfs/libxfs/xfs_alloc.c | 12 ++++----
> fs/xfs/libxfs/xfs_alloc_btree.c | 6 ++--
> fs/xfs/libxfs/xfs_attr_leaf.c | 6 ++--
> fs/xfs/libxfs/xfs_attr_remote.c | 49 ++++++++++++------------------
> fs/xfs/libxfs/xfs_bmap_btree.c | 6 ++--
> fs/xfs/libxfs/xfs_da_btree.c | 9 +++---
> fs/xfs/libxfs/xfs_dir2_block.c | 6 ++--
> fs/xfs/libxfs/xfs_dir2_data.c | 9 +++---
> fs/xfs/libxfs/xfs_dir2_leaf.c | 6 ++--
> fs/xfs/libxfs/xfs_dir2_node.c | 8 ++---
> fs/xfs/libxfs/xfs_ialloc.c | 6 ++--
> fs/xfs/libxfs/xfs_ialloc_btree.c | 6 ++--
> fs/xfs/libxfs/xfs_refcount_btree.c | 6 ++--
> fs/xfs/libxfs/xfs_rmap_btree.c | 6 ++--
> fs/xfs/libxfs/xfs_sb.c | 14 ++++++---
> fs/xfs/libxfs/xfs_symlink_remote.c | 6 ++--
> fs/xfs/libxfs/xfs_types.h | 16 ++++++++--
> fs/xfs/xfs_error.c | 7 ++---
> fs/xfs/xfs_error.h | 3 +-
> 19 files changed, 96 insertions(+), 91 deletions(-)
>
...
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index 07e866103dc2..50726c54c2ca 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -719,9 +719,13 @@ xfs_sb_read_verify(
> error = xfs_validate_sb_read(mp, &sb);
>
> out_error:
> - if (error == -EFSCORRUPTED || error == -EFSBADCRC)
> - xfs_verifier_error(bp, error, __this_address);
> - else if (error)
> + if (error == -EFSCORRUPTED) {
> + XFS_CORRUPTED(vc);
Can't this clobber a previous corruption state in the vc, depending on
how we get here?
> + xfs_verifier_error(bp, vc);
> + } else if (error == -EFSBADCRC) {
> + XFS_BADCRC(vc);
> + xfs_verifier_error(bp, vc);
> + } else if (error)
> xfs_buf_ioerror(bp, error);
> }
>
> @@ -779,7 +783,9 @@ xfs_sb_write_verify(
> return;
>
> out_error:
> - xfs_verifier_error(bp, error, __this_address);
> + vc->fa = __this_address;
> + vc->errno = error;
> + xfs_verifier_error(bp, vc);
> }
>
> const struct xfs_buf_ops xfs_sb_buf_ops = {
...
> diff --git a/fs/xfs/libxfs/xfs_types.h b/fs/xfs/libxfs/xfs_types.h
> index ab045e8dfcb9..4f0b8c73b599 100644
> --- a/fs/xfs/libxfs/xfs_types.h
> +++ b/fs/xfs/libxfs/xfs_types.h
...
> @@ -51,10 +57,14 @@ struct xfs_vc {
> * return, which throws off the reported address.
> */
> #define __this_address ({ __label__ __here; __here: barrier(); &&__here; })
> +
> +#define XFS_CORRUPTED(vc) ({(vc)->fa = __this_address; (vc)->errno = -EFSCORRUPTED;})
> +#define XFS_BADCRC(vc) ({(vc)->fa = __this_address; (vc)->errno = -EFSBADCRC;})
> +#define XFS_VERIFIED(vc) ({(vc)->fa = NULL; (vc)->errno = 0;})
>
> -#define XFS_CORRUPTED_RETURN(vc) ({(vc)->fa = __this_address; false;})
> -#define XFS_BADCRC_RETURN(vc) ({(vc)->fa = __this_address; false;})
> -#define XFS_VERIFIED_RETURN(vc) ({(vc)->fa = NULL; true;})
> +#define XFS_CORRUPTED_RETURN(vc) ({(vc)->fa = __this_address; (vc)->errno = -EFSCORRUPTED; false;})
> +#define XFS_BADCRC_RETURN(vc) ({(vc)->fa = __this_address; (vc)->errno = -EFSBADCRC; false;})
> +#define XFS_VERIFIED_RETURN(vc) ({(vc)->fa = NULL; (vc)->errno = 0; true;})
>
Case in point wrt the naming thoughts on the previous patch: what's the
need for separate XFS_CORRUPTED() macros if the _RETURN() ones don't
actually change execution flow? They just evaluate to a logical
true/false, which should be perfectly fine outside of a return
statement.
Hmm, maybe it would be better to stick with that "return value" model
after all, but just drop the _RETURN() bit of the name and use the same
macro in both contexts.
Brian
> /*
> * Null values for the types.
> diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
> index 9866f542e77b..4d305287823c 100644
> --- a/fs/xfs/xfs_error.c
> +++ b/fs/xfs/xfs_error.c
> @@ -381,11 +381,10 @@ xfs_buf_verifier_error(
> void
> xfs_verifier_error(
> struct xfs_buf *bp,
> - int error,
> - xfs_failaddr_t failaddr)
> + struct xfs_vc *vc)
> {
> - return xfs_buf_verifier_error(bp, error, "", xfs_buf_offset(bp, 0),
> - XFS_CORRUPTION_DUMP_LEN, failaddr);
> + return xfs_buf_verifier_error(bp, vc->errno, "", xfs_buf_offset(bp, 0),
> + XFS_CORRUPTION_DUMP_LEN, vc->fa);
> }
>
> /*
> diff --git a/fs/xfs/xfs_error.h b/fs/xfs/xfs_error.h
> index 246d3e989c6c..9b0ac387007d 100644
> --- a/fs/xfs/xfs_error.h
> +++ b/fs/xfs/xfs_error.h
> @@ -18,8 +18,7 @@ extern void xfs_corruption_error(const char *tag, int level,
> extern void xfs_buf_verifier_error(struct xfs_buf *bp, int error,
> const char *name, void *buf, size_t bufsz,
> xfs_failaddr_t failaddr);
> -extern void xfs_verifier_error(struct xfs_buf *bp, int error,
> - xfs_failaddr_t failaddr);
> +extern void xfs_verifier_error(struct xfs_buf *bp, struct xfs_vc *vc);
> extern void xfs_inode_verifier_error(struct xfs_inode *ip, int error,
> const char *name, void *buf, size_t bufsz,
> xfs_failaddr_t failaddr);
> --
> 2.17.0
>
>
next prev parent reply other threads:[~2018-12-07 13:41 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-05 21:01 [PATCH RFC 0/10] xfs: add verifier context structure Eric Sandeen
2018-12-05 21:02 ` [PATCH 01/10] xfs: change xfs_attr3_rmt_hdr_ok to return bool Eric Sandeen
2018-12-07 13:36 ` Brian Foster
2018-12-17 18:23 ` Darrick J. Wong
2018-12-05 21:03 ` [PATCH 02/10] xfs: make checksum verifiers consistently return bools Eric Sandeen
2018-12-07 13:36 ` Brian Foster
2018-12-17 18:24 ` Darrick J. Wong
2018-12-05 21:03 ` [PATCH 03/10] xfs: pass a verifier context down verifier callchains Eric Sandeen
2018-12-17 18:29 ` Darrick J. Wong
2018-12-05 21:04 ` [PATCH 04/10] xfs: pass a verifier context to crc validation functions Eric Sandeen
2018-12-05 21:05 ` [PATCH 05/10] xfs: define new macros to set verifier context on return Eric Sandeen
2018-12-05 21:06 ` [PATCH 06/10] xfs: teach xfs_btree_[sl]block_verify_crc to populate verifier context Eric Sandeen
2018-12-05 21:08 ` [PATCH 07/10] xfs: change all verifiers to return booleans Eric Sandeen
2018-12-05 21:09 ` [PATCH 08/10] xfs: set failaddr into vc for checksum failures Eric Sandeen
2018-12-07 13:37 ` Brian Foster
2018-12-10 16:00 ` Eric Sandeen
2018-12-17 18:39 ` Darrick J. Wong
2018-12-05 21:11 ` [PATCH 09/10] xfs: add errno to verifier context and populate it Eric Sandeen
2018-12-07 13:41 ` Brian Foster [this message]
2018-12-05 21:11 ` [PATCH 10/10] xfs: condense crc and verifier checks where possible Eric Sandeen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181207134114.GD55482@bfoster \
--to=bfoster@redhat.com \
--cc=linux-xfs@vger.kernel.org \
--cc=sandeen@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.