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.
