Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <18827b350fbf6719733fda814255ec20d6dcf00f.camel@linux.ibm.com>
Subject: Re: [PATCH] iversion: update comments with info about atime updates
From:   Mimi Zohar <zohar@linux.ibm.com>
To:     Jeff Layton <jlayton@kernel.org>, linux-fsdevel@vger.kernel.org
Cc:     linux-kernel@vger.kernel.org, linux-integrity@vger.kernel.org,
        linux-nfs@vger.kernel.org, linux-xfs@vger.kernel.org,
        linux-ext4@vger.kernel.org, NeilBrown <neilb@suse.de>,
        Trond Myklebust <trondmy@hammerspace.com>,
        Dave Chinner <david@fromorbit.com>
Date:   Mon, 22 Aug 2022 13:39:42 -0400
In-Reply-To: <f17b9d627703bee2a7b531a051461671648a9dbd.camel@kernel.org>
References: <20220822133309.86005-1-jlayton@kernel.org>
         <ceb8f09a4cb2de67f40604d03ee0c475feb3130a.camel@linux.ibm.com>
         <f17b9d627703bee2a7b531a051461671648a9dbd.camel@kernel.org>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
Precedence: bulk

On Mon, 2022-08-22 at 12:22 -0400, Jeff Layton wrote:
> On Mon, 2022-08-22 at 11:40 -0400, Mimi Zohar wrote:
> > On Mon, 2022-08-22 at 09:33 -0400, Jeff Layton wrote:
> > > Add an explicit paragraph codifying that atime updates due to reads
> > > should not be counted against the i_version counter. None of the
> > > existing subsystems that use the i_version want those counted, and
> > > there is an easy workaround for those that do.
> > > 
> > > 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 | 10 ++++++++--
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/include/linux/iversion.h b/include/linux/iversion.h
> > > index 3bfebde5a1a6..da6cc1cc520a 100644
> > > --- a/include/linux/iversion.h
> > > +++ b/include/linux/iversion.h
> > > @@ -9,8 +9,8 @@
> > >   * ---------------------------
> > >   * 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.
> > >   *
> > >   * Observers see the i_version as a 64-bit number that never decreases. If it
> > >   * remains the same since it was last checked, then nothing has changed in the
> > > @@ -18,6 +18,12 @@
> > >   * anything about the nature or magnitude of the changes from the value, only
> > >   * that the inode has changed in some fashion.
> > >   *
> > > + * Note that atime updates due to reads or similar activity do _not_ represent
> > > + * an explicit change to the inode. If the only change is to the atime and it
> > 
> > Thanks, Jeff.  The ext4 patch increments i_version on file metadata
> > changes.  Could the wording here be more explicit to reflect changes
> > based on either inode data or metadata changes?b
> > 
> > 
> 
> Thanks Mimi,
> 
> Care to suggest some wording?
> 
> The main issue we have is that ext4 and xfs both increment i_version on
> atime updates due to reads. I have patches in flight to fix those, but
> going forward, we want to ensure that i_version gets incremented on all
> changes _except_ for atime updates.
> 
> The best wording we have at the moment is what Trond suggested, which is
> to classify the changes to the inode as "explicit" (someone or something
> made a deliberate change to the inode) and "implicit" (the change to the
> inode was due to activity such as reads that don't actually change
> anything).
> 
> Is there a better way to describe this?

"explicit change to the inode" probably implies both the inode file
data and metadata, but let's call it out by saying "an explicit change
to either the inode data or metadata".

> 
> > > + * wasn't set via utimes() or a similar mechanism, then i_version should not be
> > > + * incremented. If an observer cares about atime updates, it should plan to
> > > + * fetch and store them in conjunction with the i_version.
> > > + *
> > >   * Not all filesystems properly implement the i_version counter. Subsystems that
> > >   * want to use i_version field on an inode should first check whether the
> > >   * filesystem sets the SB_I_VERSION flag (usually via the IS_I_VERSION macro).
> > 
> > 
>