Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 29 Aug 2022 17:56:51 +1000
From:   Dave Chinner <david@fromorbit.com>
To:     Jeff Layton <jlayton@kernel.org>
Cc:     tytso@mit.edu, adilger.kernel@dilger.ca, djwong@kernel.org,
        trondmy@hammerspace.com, neilb@suse.de, viro@zeniv.linux.org.uk,
        zohar@linux.ibm.com, xiubli@redhat.com, chuck.lever@oracle.com,
        lczerner@redhat.com, jack@suse.cz, brauner@kernel.org,
        linux-api@vger.kernel.org, linux-btrfs@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-ceph@vger.kernel.org, linux-ext4@vger.kernel.org,
        linux-nfs@vger.kernel.org, linux-xfs@vger.kernel.org,
        Colin Walters <walters@verbum.org>
Subject: Re: [PATCH v3 1/7] iversion: update comments with info about atime
 updates
Message-ID: <20220829075651.GS3600936@dread.disaster.area>
References: <20220826214703.134870-1-jlayton@kernel.org>
 <20220826214703.134870-2-jlayton@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220826214703.134870-2-jlayton@kernel.org>
Precedence: bulk

On Fri, Aug 26, 2022 at 05:46:57PM -0400, Jeff Layton wrote:
> The i_version field in the kernel has had different semantics over
> the decades, but we're now proposing to expose it to userland via
> statx. This means that we need a clear, consistent definition of
> what it means and when it should change.
> 
> Update the comments in iversion.h to describe how a conformant
> i_version implementation is expected to behave. This definition
> suits the current users of i_version (NFSv4 and IMA), but is
> loose enough to allow for a wide range of possible implementations.
> 
> Cc: Colin Walters <walters@verbum.org>
> Cc: NeilBrown <neilb@suse.de>
> Cc: Trond Myklebust <trondmy@hammerspace.com>
> Cc: Dave Chinner <david@fromorbit.com>
> Link: https://lore.kernel.org/linux-xfs/166086932784.5425.17134712694961326033@noble.neil.brown.name/#t
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  include/linux/iversion.h | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/iversion.h b/include/linux/iversion.h
> index 3bfebde5a1a6..45e93e1b4edc 100644
> --- a/include/linux/iversion.h
> +++ b/include/linux/iversion.h
> @@ -9,8 +9,19 @@
>   * ---------------------------
>   * The change attribute (i_version) is mandated by NFSv4 and is mostly for
>   * knfsd, but is also used for other purposes (e.g. IMA). The i_version must
> - * appear different to observers if there was a change to the inode's data or
> - * metadata since it was last queried.
> + * appear different to observers if there was an explicit change to the inode's
> + * data or metadata since it was last queried.
> + *
> + * An explicit change is one that would ordinarily result in a change to the
> + * inode status change time (aka ctime). The version must appear to change, even
> + * if the ctime does not (since the whole point is to avoid missing updates due
> + * to timestamp granularity). If POSIX mandates that the ctime must change due
> + * to an operation, then the i_version counter must be incremented as well.
> + *
> + * A conformant implementation is allowed to increment the counter in other
> + * cases, but this is not optimal. NFSv4 and IMA both use this value to determine
> + * whether caches are up to date. Spurious increments can cause false cache
> + * invalidations.

"not optimal", but never-the-less allowed - that's "unspecified
behaviour" if I've ever seen it. How is userspace supposed to
know/deal with this?

Indeed, this loophole clause doesn't exist in the man pages that
define what statx.stx_ino_version means. The man pages explicitly
define that stx_ino_version only ever changes when stx_ctime
changes.

IOWs, the behaviour userspace developers are going to expect *does
not include* stx_ino_version changing it more often than ctime is
changed. Hence a kernel iversion implementation that bumps the
counter more often than ctime changes *is not conformant with the
statx version counter specification*. IOWs, we can't export such
behaviour to userspace *ever* - it is a non-conformant
implementation.

Hence I think anything that bumps iversion outside the bounds of the
statx definition should be declared as such:

"Non-conformant iversion implementations:
	- MUST NOT be exported by statx() to userspace
	- MUST be -tolerated- by kernel internal applications that
	  use iversion for their own purposes."

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com