On 7/26/25 3:03 PM, Jeff Layton wrote: > On Sat, 2025-07-26 at 14:48 -0400, Chuck Lever wrote: >> Hi Jeff - >> >> Thanks again for your focus on getting this straightened out! >> >> >> On 7/26/25 10:31 AM, Jeff Layton wrote: >>> Ensure that notify_change() doesn't clobber a delegated ctime update >>> with current_time() by setting ATTR_CTIME_SET for those updates. >>> >>> Also, set the tv_nsec field the nfsd4_decode_fattr4 to the correct >>> value. >> >> I don't yet see the connection of the above tv_nsec fix to the other >> changes in this patch. Wouldn't this be an independent fix? >> > > I felt like they were related. Yes, the ia_ctime field is currently > being set wrong, but it's also being clobbered by notify_change(), so > it doesn't matter much. I can break this into a separate patch (with a > Fixes: tag) if you prefer though.
Ah, got it, this patch exposes a latent bug. The usual thing to do is to fix the latent bug in a preceding/pre-requisite patch, so that's my preference. >>> Don't bother setting the timestamps in cb_getattr_update_times() in the >>> non-delegated case. notify_change() will do that itself. >>> >>> Signed-off-by: Jeff Layton <[email protected]> >> >> General comments: >> >> I don't feel that any of the patches in this series need to be tagged >> for stable, since there is already a Kconfig setting that defaults to >> leaving timestamp delegation disabled. But I would like to see Fixes: >> tags, where that makes sense? >> > > I don't think any of these need to go to stable since this is still > under a non-default Kconfig option, and the main effect of the bug is > wonky timestamps. I should be able to add some Fixes: tags though. > >> Is this set on top of the set you posted a day or two ago with the new >> trace point? Or does this set replace that one? >> > > This set should replace those. I was confused because the trace point patch is missing, and dropping it wasn't mentioned in the cover letter's Change log. NBD, thanks for clarifying. Since the bulk of these are NFSD changes, I volunteer to take v3 once we have Acks from the VFS maintainers, as needed. >>> --- >>> fs/nfsd/nfs4state.c | 6 +++--- >>> fs/nfsd/nfs4xdr.c | 5 +++-- >>> 2 files changed, 6 insertions(+), 5 deletions(-) >>> >>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c >>> index >>> 88c347957da5b8f352be63f84f207d2225f81cb9..77eea2ad93cc07939f045fc4b983b1ac00d068b8 >>> 100644 >>> --- a/fs/nfsd/nfs4state.c >>> +++ b/fs/nfsd/nfs4state.c >>> @@ -9167,7 +9167,6 @@ static bool set_cb_time(struct timespec64 *cb, const >>> struct timespec64 *orig, >>> static int cb_getattr_update_times(struct dentry *dentry, struct >>> nfs4_delegation *dp) >>> { >>> struct inode *inode = d_inode(dentry); >>> - struct timespec64 now = current_time(inode); >>> struct nfs4_cb_fattr *ncf = &dp->dl_cb_fattr; >>> struct iattr attrs = { }; >>> int ret; >>> @@ -9175,6 +9174,7 @@ static int cb_getattr_update_times(struct dentry >>> *dentry, struct nfs4_delegation >>> if (deleg_attrs_deleg(dp->dl_type)) { >>> struct timespec64 atime = inode_get_atime(inode); >>> struct timespec64 mtime = inode_get_mtime(inode); >>> + struct timespec64 now = current_time(inode); >>> >>> attrs.ia_atime = ncf->ncf_cb_atime; >>> attrs.ia_mtime = ncf->ncf_cb_mtime; >>> @@ -9183,12 +9183,12 @@ static int cb_getattr_update_times(struct dentry >>> *dentry, struct nfs4_delegation >>> attrs.ia_valid |= ATTR_ATIME | ATTR_ATIME_SET; >>> >>> if (set_cb_time(&attrs.ia_mtime, &mtime, &now)) { >>> - attrs.ia_valid |= ATTR_CTIME | ATTR_MTIME | >>> ATTR_MTIME_SET; >>> + attrs.ia_valid |= ATTR_CTIME | ATTR_CTIME_SET | >>> + ATTR_MTIME | ATTR_MTIME_SET; >>> attrs.ia_ctime = attrs.ia_mtime; >>> } >>> } else { >>> attrs.ia_valid |= ATTR_MTIME | ATTR_CTIME; >>> - attrs.ia_mtime = attrs.ia_ctime = now; >>> } >>> >>> if (!attrs.ia_valid) >>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c >>> index >>> 8b68f74a8cf08c6aa1305a2a3093467656085e4a..c0a3c6a7c8bb70d62940115c3101e9f897401456 >>> 100644 >>> --- a/fs/nfsd/nfs4xdr.c >>> +++ b/fs/nfsd/nfs4xdr.c >>> @@ -538,8 +538,9 @@ nfsd4_decode_fattr4(struct nfsd4_compoundargs *argp, >>> u32 *bmval, u32 bmlen, >>> iattr->ia_mtime.tv_sec = modify.seconds; >>> iattr->ia_mtime.tv_nsec = modify.nseconds; >>> iattr->ia_ctime.tv_sec = modify.seconds; >>> - iattr->ia_ctime.tv_nsec = modify.seconds; >>> - iattr->ia_valid |= ATTR_CTIME | ATTR_MTIME | ATTR_MTIME_SET | >>> ATTR_DELEG; >>> + iattr->ia_ctime.tv_nsec = modify.nseconds; >>> + iattr->ia_valid |= ATTR_CTIME | ATTR_CTIME_SET | >>> + ATTR_MTIME | ATTR_MTIME_SET | ATTR_DELEG; >>> } >>> >>> /* request sanity: did attrlist4 contain the expected number of words? >>> */ >>> >> >> > -- Chuck Lever
