From: Amir Goldstein <amir73il@gmail.com>
To: Jeff Layton <jlayton@kernel.org>
Cc: Theodore Tso <tytso@mit.edu>,
Andreas Dilger <adilger.kernel@dilger.ca>,
"Darrick J. Wong" <djwong@kernel.org>,
Dave Chinner <david@fromorbit.com>,
Trond Myklebust <trondmy@hammerspace.com>,
Neil Brown <neilb@suse.de>, Al Viro <viro@zeniv.linux.org.uk>,
Mimi Zohar <zohar@linux.ibm.com>,
xiubli@redhat.com, Chuck Lever <chuck.lever@oracle.com>,
Lukas Czerner <lczerner@redhat.com>, Jan Kara <jack@suse.cz>,
Christian Brauner <brauner@kernel.org>,
Linux API <linux-api@vger.kernel.org>,
Linux Btrfs <linux-btrfs@vger.kernel.org>,
linux-fsdevel <linux-fsdevel@vger.kernel.org>,
linux-kernel <linux-kernel@vger.kernel.org>,
Ext4 <linux-ext4@vger.kernel.org>,
Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
linux-xfs <linux-xfs@vger.kernel.org>,
David Wysochanski <dwysocha@redhat.com>,
ceph-devel <ceph-devel@vger.kernel.org>
Subject: Re: [PATCH v3 4/7] xfs: don't bump the i_version on an atime update in xfs_vn_update_time
Date: Sat, 27 Aug 2022 11:01:22 +0300 [thread overview]
Message-ID: <CAOQ4uxge86g=+HPnds-wRXkFHg67G=m9rGK7V_T8yS+2=w9tmg@mail.gmail.com> (raw)
In-Reply-To: <CAOQ4uxjzE_B_EQktLr8z8gXOhFDNm-_YpUTycfZCdaZNp-i0hQ@mail.gmail.com>
On Sat, Aug 27, 2022 at 10:26 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Sat, Aug 27, 2022 at 12:49 AM Jeff Layton <jlayton@kernel.org> wrote:
> >
> > xfs will update the i_version when updating only the atime value, which
> > is not desirable for any of the current consumers of i_version. Doing so
> > leads to unnecessary cache invalidations on NFS and extra measurement
> > activity in IMA.
> >
> > Add a new XFS_ILOG_NOIVER flag, and use that to indicate that the
> > transaction should not update the i_version. Set that value in
> > xfs_vn_update_time if we're only updating the atime.
> >
> > Cc: Dave Chinner <david@fromorbit.com>
> > Cc: NeilBrown <neilb@suse.de>
> > Cc: Trond Myklebust <trondmy@hammerspace.com>
> > Cc: David Wysochanski <dwysocha@redhat.com>
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > fs/xfs/libxfs/xfs_log_format.h | 2 +-
> > fs/xfs/libxfs/xfs_trans_inode.c | 2 +-
> > fs/xfs/xfs_iops.c | 11 +++++++++--
> > 3 files changed, 11 insertions(+), 4 deletions(-)
> >
> > Dave has NACK'ed this patch, but I'm sending it as a way to illustrate
> > the problem. I still think this approach should at least fix the worst
> > problems with atime updates being counted. We can look to carve out
> > other "spurious" i_version updates as we identify them.
> >
>
> AFAIK, "spurious" is only inode blocks map changes due to writeback
> of dirty pages. Anybody know about other cases?
>
> Regarding inode blocks map changes, first of all, I don't think that there is
> any practical loss from invalidating NFS client cache on dirty data writeback,
> because NFS server should be serving cold data most of the time.
> If there are a few unneeded cache invalidations they would only be temporary.
>
Unless there is an issue with a writer NFS client that invalidates its
own attribute
caches on server data writeback?
> One may even consider if NFSv4 server should not flush dirty data of an inode
> before granting a read lease to client.
> After all, if read lease was granted, client cached data and then server crashed
> before persisting the dirty data, then client will have cached a
> "future" version
> of the data and if i_version on the server did not roll back in that situation,
> we are looking at possible data corruptions.
>
> Same goes for IMA. IIUC, IMA data checksum would be stored in xattr?
> Storing in xattr a data checksum for data that is not persistent on disk
> would be an odd choice.
>
> So in my view, I only see benefits to current i_version users in the xfs
> i_version implementations and I don't think that it contradicts the
> i_version definition in the man page patch.
>
> > If however there are offline analysis tools that require atime updates
> > to be counted, then we won't be able to do this. If that's the case, how
> > can we fix this such that serving xfs via NFSv4 doesn't suck?
> >
>
> If I read the arguments correctly, implicit atime updates could be relaxed
> as long as this behavior is clearly documented and coherent on all
> implementations.
>
> Forensics and other applications that care about atime updates can and
> should check atime and don't need i_version to know that it was changed.
> The reliability of atime as an audit tool has dropped considerably since
> the default in relatime.
> If we want to be paranoid, maybe we can leave i_version increment on
> atime updates in case the user opted-in to strict '-o atime' updates, but
> IMO, there is no need for that.
>
> Thanks,
> Amir.
next prev parent reply other threads:[~2022-08-27 8:01 UTC|newest]
Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-26 21:46 [PATCH v3 0/7] vfs: clean up i_version behavior and expose it via statx Jeff Layton
2022-08-26 21:46 ` [PATCH v3 1/7] iversion: update comments with info about atime updates Jeff Layton
2022-08-29 7:56 ` Dave Chinner
2022-08-29 10:39 ` Jeff Layton
2022-08-29 22:58 ` NeilBrown
2022-08-30 11:40 ` Jeff Layton
2022-08-30 13:24 ` J. Bruce Fields
2022-08-30 13:50 ` Jeff Layton
2022-08-30 14:44 ` J. Bruce Fields
2022-08-30 14:58 ` Trond Myklebust
2022-08-30 15:17 ` J. Bruce Fields
2022-08-30 15:43 ` Trond Myklebust
2022-08-30 17:02 ` Jeff Layton
2022-08-30 17:47 ` Trond Myklebust
2022-08-30 17:53 ` Jeff Layton
2022-08-30 18:25 ` Trond Myklebust
2022-08-30 19:11 ` Jeff Layton
2022-08-30 18:32 ` J. Bruce Fields
2022-08-30 19:30 ` Jeff Layton
2022-08-30 19:46 ` J. Bruce Fields
2022-08-30 19:57 ` Jeff Layton
2022-08-30 20:08 ` J. Bruce Fields
2022-08-30 1:04 ` Dave Chinner
2022-08-30 12:38 ` Jeff Layton
2022-08-26 21:46 ` [PATCH v3 2/7] ext4: fix i_version handling in ext4 Jeff Layton
2022-08-26 21:46 ` [PATCH v3 3/7] ext4: unconditionally enable the i_version counter Jeff Layton
2022-08-29 14:51 ` Jan Kara
2022-08-26 21:47 ` [PATCH v3 4/7] xfs: don't bump the i_version on an atime update in xfs_vn_update_time Jeff Layton
2022-08-27 7:26 ` Amir Goldstein
2022-08-27 8:01 ` Amir Goldstein [this message]
2022-08-27 13:14 ` Jeff Layton
2022-08-27 15:46 ` Darrick J. Wong
2022-08-27 16:03 ` Trond Myklebust
2022-08-27 16:10 ` Jeff Layton
2022-08-27 17:06 ` Trond Myklebust
2022-08-28 13:25 ` Amir Goldstein
2022-08-28 14:37 ` Jeff Layton
2022-08-28 16:53 ` Amir Goldstein
2022-08-29 5:48 ` Dave Chinner
2022-08-29 10:33 ` Jeff Layton
2022-08-30 0:08 ` Dave Chinner
2022-08-30 11:20 ` Jeff Layton
2022-08-28 17:30 ` Amir Goldstein
2022-08-26 21:47 ` [PATCH v3 5/7] vfs: report an inode version in statx for IS_I_VERSION inodes Jeff Layton
2022-08-26 21:47 ` [PATCH v3 6/7] nfs: report the inode version in statx if requested Jeff Layton
2022-08-26 21:47 ` [PATCH v3 7/7] ceph: fill in the change attribute in statx requests 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='CAOQ4uxge86g=+HPnds-wRXkFHg67G=m9rGK7V_T8yS+2=w9tmg@mail.gmail.com' \
--to=amir73il@gmail.com \
--cc=adilger.kernel@dilger.ca \
--cc=brauner@kernel.org \
--cc=ceph-devel@vger.kernel.org \
--cc=chuck.lever@oracle.com \
--cc=david@fromorbit.com \
--cc=djwong@kernel.org \
--cc=dwysocha@redhat.com \
--cc=jack@suse.cz \
--cc=jlayton@kernel.org \
--cc=lczerner@redhat.com \
--cc=linux-api@vger.kernel.org \
--cc=linux-btrfs@vger.kernel.org \
--cc=linux-ext4@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=linux-xfs@vger.kernel.org \
--cc=neilb@suse.de \
--cc=trondmy@hammerspace.com \
--cc=tytso@mit.edu \
--cc=viro@zeniv.linux.org.uk \
--cc=xiubli@redhat.com \
--cc=zohar@linux.ibm.com \
/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).