From: "Darrick J. Wong" <djwong@kernel.org>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org, jlayton@kernel.org,
linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH] xfs: stop advertising SB_I_VERSION
Date: Wed, 28 Feb 2024 08:08:48 -0800 [thread overview]
Message-ID: <20240228160848.GF1927156@frogsfrogsfrogs> (raw)
In-Reply-To: <20240228042859.841623-1-david@fromorbit.com>
On Wed, Feb 28, 2024 at 03:28:59PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> The redefinition of how NFS wants inode->i_version to be updated is
> incomaptible with the XFS i_version mechanism. The VFS now wants
> inode->i_version to only change when ctime changes (i.e. it has
> become a ctime change counter, not an inode change counter). XFS has
> fine grained timestamps, so it can just use ctime for the NFS change
> cookie like it still does for V4 XFS filesystems.
>
> We still want XFS to update the inode change counter as it currently
> does, so convert all the code that checks SB_I_VERSION to check for
> v5 format support. Then we can remove the SB_I_VERSION flag from the
> VFS superblock to indicate that inode->i_version is not a valid
> change counter and should not be used as such.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
Seeing as NFS and XFS' definition of i_version have diverged, I suppose
divorce is the only option. But please, let's get rid of all the
*iversion() calls in the codebase.
With my paranoia hat on: let's add an i_changecounter to xfs_inode and
completely stop using the inode.i_version to prevent the vfs from
messing with us.
At some point we can rev the ondisk format to add a new field so that
"di_version" can be whatever u64 cookie the vfs passes us through
i_version.
--D
> ---
> fs/xfs/libxfs/xfs_trans_inode.c | 15 +++++----------
> fs/xfs/xfs_iops.c | 16 +++-------------
> fs/xfs/xfs_super.c | 8 --------
> 3 files changed, 8 insertions(+), 31 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> index 70e97ea6eee7..8071aefad728 100644
> --- a/fs/xfs/libxfs/xfs_trans_inode.c
> +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> @@ -97,17 +97,12 @@ xfs_trans_log_inode(
>
> /*
> * First time we log the inode in a transaction, bump the inode change
> - * counter if it is configured for this to occur. While we have the
> - * inode locked exclusively for metadata modification, we can usually
> - * avoid setting XFS_ILOG_CORE if no one has queried the value since
> - * the last time it was incremented. If we have XFS_ILOG_CORE already
> - * set however, then go ahead and bump the i_version counter
> - * unconditionally.
> + * counter if it is configured for this to occur.
> */
> - if (!test_and_set_bit(XFS_LI_DIRTY, &iip->ili_item.li_flags)) {
> - if (IS_I_VERSION(inode) &&
> - inode_maybe_inc_iversion(inode, flags & XFS_ILOG_CORE))
> - flags |= XFS_ILOG_IVERSION;
> + if (!test_and_set_bit(XFS_LI_DIRTY, &iip->ili_item.li_flags) &&
> + xfs_has_crc(ip->i_mount)) {
> + inode->i_version++;
> + flags |= XFS_ILOG_IVERSION;
> }
>
> iip->ili_dirty_flags |= flags;
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index be102fd49560..97e792d9d79a 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -584,11 +584,6 @@ xfs_vn_getattr(
> }
> }
>
> - if ((request_mask & STATX_CHANGE_COOKIE) && IS_I_VERSION(inode)) {
> - stat->change_cookie = inode_query_iversion(inode);
> - stat->result_mask |= STATX_CHANGE_COOKIE;
> - }
> -
> /*
> * Note: If you add another clause to set an attribute flag, please
> * update attributes_mask below.
> @@ -1044,16 +1039,11 @@ xfs_vn_update_time(
> struct timespec64 now;
>
> trace_xfs_update_time(ip);
> + ASSERT(!(flags & S_VERSION));
>
> if (inode->i_sb->s_flags & SB_LAZYTIME) {
> - if (!((flags & S_VERSION) &&
> - inode_maybe_inc_iversion(inode, false))) {
> - generic_update_time(inode, flags);
> - return 0;
> - }
> -
> - /* Capture the iversion update that just occurred */
> - log_flags |= XFS_ILOG_CORE;
> + generic_update_time(inode, flags);
> + return 0;
> }
>
> error = xfs_trans_alloc(mp, &M_RES(mp)->tr_fsyncts, 0, 0, 0, &tp);
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 6ce1e6deb7ec..657ce0423f1d 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1692,10 +1692,6 @@ xfs_fs_fill_super(
>
> set_posix_acl_flag(sb);
>
> - /* version 5 superblocks support inode version counters. */
> - if (xfs_has_crc(mp))
> - sb->s_flags |= SB_I_VERSION;
> -
> if (xfs_has_dax_always(mp)) {
> error = xfs_setup_dax_always(mp);
> if (error)
> @@ -1917,10 +1913,6 @@ xfs_fs_reconfigure(
> int flags = fc->sb_flags;
> int error;
>
> - /* version 5 superblocks always support version counters. */
> - if (xfs_has_crc(mp))
> - fc->sb_flags |= SB_I_VERSION;
> -
> error = xfs_fs_validate_params(new_mp);
> if (error)
> return error;
> --
> 2.43.0
>
>
next prev parent reply other threads:[~2024-02-28 16:08 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-28 4:28 [PATCH] xfs: stop advertising SB_I_VERSION Dave Chinner
2024-02-28 16:08 ` Darrick J. Wong [this message]
2024-02-28 23:55 ` Dave Chinner
2024-02-29 0:09 ` Darrick J. Wong
2024-03-01 13:42 ` Jeff Layton
2024-03-03 23:35 ` Dave Chinner
2024-03-04 0:45 ` Jeff Layton
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=20240228160848.GF1927156@frogsfrogsfrogs \
--to=djwong@kernel.org \
--cc=david@fromorbit.com \
--cc=jlayton@kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--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;
as well as URLs for NNTP newsgroup(s).