From: Brian Foster <bfoster@redhat.com>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 08/12] xfs: remove xfs_ifork_ops
Date: Fri, 1 May 2020 11:56:49 -0400 [thread overview]
Message-ID: <20200501155649.GO40250@bfoster> (raw)
In-Reply-To: <20200501081424.2598914-9-hch@lst.de>
On Fri, May 01, 2020 at 10:14:20AM +0200, Christoph Hellwig wrote:
> xfs_ifork_ops add up to two indirect calls per inode read and flush,
> despite just having a single instance in the kernel. In xfsprogs
> phase6 in xfs_repair overrides the verify_dir method to deal with inodes
> that do not have a valid parent. Instead of the costly indirection just
> life the repair code into xfs_dir2_sf.c under a condition that ensures
> it is compiled as part of a kernel build, but instantly eliminated as
> it is unreachable.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/xfs/libxfs/xfs_dir2_sf.c | 64 ++++++++++++++++++++++++++++++++--
> fs/xfs/libxfs/xfs_inode_fork.c | 19 +++-------
> fs/xfs/libxfs/xfs_inode_fork.h | 15 ++------
> fs/xfs/xfs_inode.c | 4 +--
> 4 files changed, 71 insertions(+), 31 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
> index 7b7f6fb2ea3b2..1f6c30b68917c 100644
> --- a/fs/xfs/libxfs/xfs_dir2_sf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_sf.c
...
> @@ -804,6 +804,66 @@ xfs_dir2_sf_verify(
> return NULL;
> }
>
> +/*
> + * When we're checking directory inodes, we're allowed to set a directory's
> + * dotdot entry to zero to signal that the parent needs to be reconnected
> + * during xfs_repair phase 6. If we're handling a shortform directory the ifork
> + * verifiers will fail, so temporarily patch out this canary so that we can
> + * verify the rest of the fork and move on to fixing the dir.
> + */
> +static xfs_failaddr_t
> +xfs_dir2_sf_verify_dir_check(
> + struct xfs_inode *ip)
> +{
> + struct xfs_mount *mp = ip->i_mount;
> + struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> + struct xfs_dir2_sf_hdr *sfp =
> + (struct xfs_dir2_sf_hdr *)ifp->if_u1.if_data;
> + int size = ifp->if_bytes;
> + bool parent_bypass = false;
> + xfs_ino_t old_parent;
> + xfs_failaddr_t fa;
> +
> + /*
> + * If this is a shortform directory, phase4 in xfs_repair may have set
> + * the parent inode to zero to indicate that it must be fixed.
> + * Temporarily set a valid parent so that the directory verifier will
> + * pass.
> + */
> + if (size > offsetof(struct xfs_dir2_sf_hdr, parent) &&
> + size >= xfs_dir2_sf_hdr_size(sfp->i8count)) {
> + old_parent = xfs_dir2_sf_get_parent_ino(sfp);
> + if (!old_parent) {
> + xfs_dir2_sf_put_parent_ino(sfp, mp->m_sb.sb_rootino);
> + parent_bypass = true;
> + }
> + }
> +
> + fa = __xfs_dir2_sf_verify(ip);
> +
> + /* Put it back. */
> + if (parent_bypass)
> + xfs_dir2_sf_put_parent_ino(sfp, old_parent);
> + return fa;
> +}
I'm not sure the cleanup is worth the kludge of including repair code in
the kernel like this. It might be better to reduce or replace ifork_ops
to a single directory function pointer until there's a reason for this
to become common. I dunno, maybe others have thoughts...
Brian
> +
> +/*
> + * Allow xfs_repair to enable the parent bypass mode. For now this is entirely
> + * unused in the kernel, but might come in useful for online repair eventually.
> + */
> +#ifndef xfs_inode_parent_bypass
> +#define xfs_inode_parent_bypass(ip) 0
> +#endif
> +
> +xfs_failaddr_t
> +xfs_dir2_sf_verify(
> + struct xfs_inode *ip)
> +{
> + if (xfs_inode_parent_bypass(ip))
> + return xfs_dir2_sf_verify_dir_check(ip);
> + return __xfs_dir2_sf_verify(ip);
> +}
> +
> /*
> * Create a new (shortform) directory.
> */
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index f30d43364aa92..f6dcee919f59e 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -673,18 +673,10 @@ xfs_ifork_init_cow(
> ip->i_cnextents = 0;
> }
>
> -/* Default fork content verifiers. */
> -struct xfs_ifork_ops xfs_default_ifork_ops = {
> - .verify_attr = xfs_attr_shortform_verify,
> - .verify_dir = xfs_dir2_sf_verify,
> - .verify_symlink = xfs_symlink_shortform_verify,
> -};
> -
> /* Verify the inline contents of the data fork of an inode. */
> xfs_failaddr_t
> xfs_ifork_verify_data(
> - struct xfs_inode *ip,
> - struct xfs_ifork_ops *ops)
> + struct xfs_inode *ip)
> {
> /* Non-local data fork, we're done. */
> if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL)
> @@ -693,9 +685,9 @@ xfs_ifork_verify_data(
> /* Check the inline data fork if there is one. */
> switch (VFS_I(ip)->i_mode & S_IFMT) {
> case S_IFDIR:
> - return ops->verify_dir(ip);
> + return xfs_dir2_sf_verify(ip);
> case S_IFLNK:
> - return ops->verify_symlink(ip);
> + return xfs_symlink_shortform_verify(ip);
> default:
> return NULL;
> }
> @@ -704,13 +696,12 @@ xfs_ifork_verify_data(
> /* Verify the inline contents of the attr fork of an inode. */
> xfs_failaddr_t
> xfs_ifork_verify_attr(
> - struct xfs_inode *ip,
> - struct xfs_ifork_ops *ops)
> + struct xfs_inode *ip)
> {
> /* There has to be an attr fork allocated if aformat is local. */
> if (ip->i_d.di_aformat != XFS_DINODE_FMT_LOCAL)
> return NULL;
> if (!XFS_IFORK_PTR(ip, XFS_ATTR_FORK))
> return __this_address;
> - return ops->verify_attr(ip);
> + return xfs_attr_shortform_verify(ip);
> }
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index 8487b0c88a75e..3f84d33abd3b7 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -176,18 +176,7 @@ extern struct kmem_zone *xfs_ifork_zone;
>
> extern void xfs_ifork_init_cow(struct xfs_inode *ip);
>
> -typedef xfs_failaddr_t (*xfs_ifork_verifier_t)(struct xfs_inode *);
> -
> -struct xfs_ifork_ops {
> - xfs_ifork_verifier_t verify_symlink;
> - xfs_ifork_verifier_t verify_dir;
> - xfs_ifork_verifier_t verify_attr;
> -};
> -extern struct xfs_ifork_ops xfs_default_ifork_ops;
> -
> -xfs_failaddr_t xfs_ifork_verify_data(struct xfs_inode *ip,
> - struct xfs_ifork_ops *ops);
> -xfs_failaddr_t xfs_ifork_verify_attr(struct xfs_inode *ip,
> - struct xfs_ifork_ops *ops);
> +xfs_failaddr_t xfs_ifork_verify_data(struct xfs_inode *ip);
> +xfs_failaddr_t xfs_ifork_verify_attr(struct xfs_inode *ip);
>
> #endif /* __XFS_INODE_FORK_H__ */
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index d1772786af29d..93967278355de 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -3769,7 +3769,7 @@ xfs_inode_verify_forks(
> struct xfs_ifork *ifp;
> xfs_failaddr_t fa;
>
> - fa = xfs_ifork_verify_data(ip, &xfs_default_ifork_ops);
> + fa = xfs_ifork_verify_data(ip);
> if (fa) {
> ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> xfs_inode_verifier_error(ip, -EFSCORRUPTED, "data fork",
> @@ -3777,7 +3777,7 @@ xfs_inode_verify_forks(
> return false;
> }
>
> - fa = xfs_ifork_verify_attr(ip, &xfs_default_ifork_ops);
> + fa = xfs_ifork_verify_attr(ip);
> if (fa) {
> ifp = XFS_IFORK_PTR(ip, XFS_ATTR_FORK);
> xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
> --
> 2.26.2
>
next prev parent reply other threads:[~2020-05-01 15:56 UTC|newest]
Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-01 8:14 dinode reading cleanups Christoph Hellwig
2020-05-01 8:14 ` [PATCH 01/12] xfs: xfs_bmapi_read doesn't take a fork id as the last argument Christoph Hellwig
2020-05-01 13:33 ` Brian Foster
2020-05-01 8:14 ` [PATCH 02/12] xfs: call xfs_iformat_fork from xfs_inode_from_disk Christoph Hellwig
2020-05-01 13:33 ` Brian Foster
2020-05-01 8:14 ` [PATCH 03/12] xfs: split xfs_iformat_fork Christoph Hellwig
2020-05-01 13:34 ` Brian Foster
2020-05-07 12:27 ` Christoph Hellwig
2020-05-07 13:40 ` Brian Foster
2020-05-07 13:42 ` Christoph Hellwig
2020-05-01 8:14 ` [PATCH 04/12] xfs: handle unallocated inodes in xfs_inode_from_disk Christoph Hellwig
2020-05-01 13:34 ` Brian Foster
2020-05-01 8:14 ` [PATCH 05/12] xfs: call xfs_dinode_verify from xfs_inode_from_disk Christoph Hellwig
2020-05-01 13:34 ` Brian Foster
2020-05-01 8:14 ` [PATCH 06/12] xfs: don't reset i_delayed_blks in xfs_iread Christoph Hellwig
2020-05-01 13:34 ` Brian Foster
2020-05-01 8:14 ` [PATCH 07/12] xfs: remove xfs_iread Christoph Hellwig
2020-05-01 15:56 ` Brian Foster
2020-05-01 8:14 ` [PATCH 08/12] xfs: remove xfs_ifork_ops Christoph Hellwig
2020-05-01 15:56 ` Brian Foster [this message]
2020-05-01 16:08 ` Darrick J. Wong
2020-05-01 16:38 ` Christoph Hellwig
2020-05-01 16:50 ` Christoph Hellwig
2020-05-01 18:23 ` Brian Foster
2020-05-07 12:34 ` Christoph Hellwig
2020-05-07 13:43 ` Brian Foster
2020-05-07 16:28 ` Brian Foster
2020-05-07 17:18 ` Christoph Hellwig
2020-05-12 23:50 ` Darrick J. Wong
2020-05-01 8:14 ` [PATCH 09/12] xfs: refactor xfs_inode_verify_forks Christoph Hellwig
2020-05-01 15:57 ` Brian Foster
2020-05-01 16:40 ` Christoph Hellwig
2020-05-01 17:02 ` Brian Foster
2020-05-01 17:08 ` Christoph Hellwig
2020-05-01 8:14 ` [PATCH 10/12] xfs: improve local fork verification Christoph Hellwig
2020-05-01 8:14 ` [PATCH 11/12] xfs: remove the special COW fork handling in xfs_bmapi_read Christoph Hellwig
2020-05-01 15:57 ` Brian Foster
2020-05-01 8:14 ` [PATCH 12/12] xfs: remove the NULL " Christoph Hellwig
2020-05-01 15:58 ` Brian Foster
-- strict thread matches above, loose matches on Subject: below --
2020-05-08 6:34 dinode reading cleanups v2 Christoph Hellwig
2020-05-08 6:34 ` [PATCH 08/12] xfs: remove xfs_ifork_ops Christoph Hellwig
2020-05-08 15:05 ` Brian Foster
2020-05-09 8:17 ` Christoph Hellwig
2020-05-09 11:13 ` Brian Foster
2020-05-16 17:48 ` Darrick J. Wong
2020-05-18 13:35 ` Brian Foster
2020-05-18 16:07 ` Darrick J. Wong
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=20200501155649.GO40250@bfoster \
--to=bfoster@redhat.com \
--cc=hch@lst.de \
--cc=linux-xfs@vger.kernel.org \
/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