Re: [PATCH v3 4/7] xfs: don't bump the i_version on an atime update in xfs_vn_update_time

[Amir Goldstein]

On Sat, Aug 27, 2022 at 12:49 AM Jeff Layton <jlayton@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
> Cc: NeilBrown <neilb@xxxxxxx>
> Cc: Trond Myklebust <trondmy@xxxxxxxxxxxxxxx>
> Cc: David Wysochanski <dwysocha@xxxxxxxxxx>
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> ---
>  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.

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.