On Sat, 2022-08-27 at 11:01 +0300, Amir Goldstein wrote: > On Sat, Aug 27, 2022 at 10:26 AM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > > 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. > > > > Unless there is an issue with a writer NFS client that invalidates its > own attribute > caches on server data writeback? > The client just looks at the file attributes (of which i_version is but one), and if certain attributes have changed (mtime, ctime, i_version, etc...) then it invalidates its cache. In the case of blocks map changes, could that mean a difference in the observable sparse regions of the file? If so, then a READ_PLUS before the change and a READ_PLUS after could give different results. Since that difference is observable by the client, I'd think we'd want to bump i_version for that anyway. > > 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. > > We don't hand out read leases if there are file descriptions open for write. NFS clients usually issue a COMMIT before closing a stateid in order to satisfy close-to-open cache coherency. So in most cases, this is probably not an issue. It might still be worthwhile to make sure of it by doing a filemap_write_and_wait before we hand out a delegation, but that's likely to be a no-op in most cases anyway. Note too that the client will still revalidate its caches when it receives attributes even when it holds a read delegation. In fact, this behavior mostly papered over a rather nasty knfsd bug we found recently where it was allowing conflicting activity to proceed even when there was a read delegation outstanding. > > 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, -- Jeff Layton <jlayton@xxxxxxxxxx>
