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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox