Re: [PATCH] fs: i_version mntopt gets visible through /proc/mounts

From: Dave Chinner <david@fromorbit.com>
To: Masayoshi Mizuma <msys.mizuma@gmail.com>
Cc: "J. Bruce Fields" <bfields@fieldses.org>,
	Eric Sandeen <sandeen@sandeen.net>,
	"Darrick J. Wong" <darrick.wong@oracle.com>,
	Christoph Hellwig <hch@infradead.org>,
	Theodore Ts'o <tytso@mit.edu>,
	Andreas Dilger <adilger.kernel@dilger.ca>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>,
	linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-xfs <linux-xfs@vger.kernel.org>
Subject: Re: [PATCH] fs: i_version mntopt gets visible through /proc/mounts
Date: Thu, 18 Jun 2020 13:05:39 +1000	[thread overview]
Message-ID: <20200618030539.GH2005@dread.disaster.area> (raw)
In-Reply-To: <20200618013026.ewnhvf64nb62k2yx@gabell>

On Wed, Jun 17, 2020 at 09:30:26PM -0400, Masayoshi Mizuma wrote:
> On Wed, Jun 17, 2020 at 02:45:07PM -0400, J. Bruce Fields wrote:
> > On Wed, Jun 17, 2020 at 01:28:11PM -0500, Eric Sandeen wrote:
> > > but mount(8) has already exposed this interface:
> > > 
> > >        iversion
> > >               Every time the inode is modified, the i_version field will be incremented.
> > > 
> > >        noiversion
> > >               Do not increment the i_version inode field.
> > > 
> > > so now what?
> > 
> > It's not like anyone's actually depending on i_version *not* being
> > incremented.  (Can you even observe it from userspace other than over
> > NFS?)
> > 
> > So, just silently turn on the "iversion" behavior and ignore noiversion,
> > and I doubt you're going to break any real application.
> 
> I suppose it's probably good to remain the options for user compatibility,
> however, it seems that iversion and noiversiont are useful for
> only ext4.
> How about moving iversion and noiversion description on mount(8)
> to ext4 specific option?
> 
> And fixing the remount issue for XFS (maybe btrfs has the same
> issue as well)?
> For XFS like as:
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 379cbff438bc..2ddd634cfb0b 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1748,6 +1748,9 @@ xfs_fc_reconfigure(
>                         return error;
>         }
> 
> +       if (XFS_SB_VERSION_NUM(&mp->m_sb) == XFS_SB_VERSION_5)
> +               mp->m_super->s_flags |= SB_I_VERSION;
> +
>         return 0;
>  }

no this doesn't work, because the sueprblock flags are modified
after ->reconfigure is called.

i.e. reconfigure_super() does this:

	if (fc->ops->reconfigure) {
		retval = fc->ops->reconfigure(fc);
		if (retval) {
			if (!force)
				goto cancel_readonly;
			/* If forced remount, go ahead despite any errors */
			WARN(1, "forced remount of a %s fs returned %i\n",
			     sb->s_type->name, retval);
		}
	}

	WRITE_ONCE(sb->s_flags, ((sb->s_flags & ~fc->sb_flags_mask) |
				 (fc->sb_flags & fc->sb_flags_mask)));

And it's the WRITE_ONCE() line that clears SB_I_VERSION out of
sb->s_flags. Hence adding it in ->reconfigure doesn't help.

What we actually want to do here in xfs_fc_reconfigure() is this:

	if (XFS_SB_VERSION_NUM(&mp->m_sb) == XFS_SB_VERSION_5)
		fc->sb_flags_mask |= SB_I_VERSION;

So that the SB_I_VERSION is not cleared from sb->s_flags.

I'll also note that btrfs will need the same fix, because it also
sets SB_I_VERSION unconditionally, as will any other filesystem that
does this, too.

Really, this is just indicative of the mess that the mount
flags vs superblock feature flags are. Filesystems can choose to
unconditionally support various superblock features, and no mount
option futzing from userspace should -ever- be able to change that
feature. Filesystems really do need to be able to override mount
options that were parsed in userspace and turned into a binary
flag...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com