On 16 Nov 2021, at 9:49, Trond Myklebust wrote: > On Tue, 2021-11-16 at 09:34 -0500, Benjamin Coddington wrote: >> On 16 Nov 2021, at 9:17, Trond Myklebust wrote: >> >>> On Tue, 2021-11-16 at 09:12 -0500, Benjamin Coddington wrote: >>>> On 16 Nov 2021, at 9:03, Trond Myklebust wrote: >>>>> No, we really shouldn't need to care what the server thinks or >>>>> does. >>>>> The client is authoritative for the change attribute while it >>>>> holds >>>>> a >>>>> delegation, not the server. >>>> >>>> My understanding of the intention of the code (which I had to >>>> sort of >>>> put >>>> together from historical patches in this area) is that we want to >>>> see >>>> ctime, >>>> mtime, and block size updates after copy/clone even if we hold a >>>> delegation, >>>> but without NFS_INO_INVALID_CHANGE, the client won't update those >>>> attributes. >>>> >>>> If that's not necessary, we can drop this patch. >>>> >>> >>> We will still see the ctime/mtime/block size updates even if >>> NFS_INO_INVALID_CHANGE is not set. Those attributes' cache status >>> are >>> tracked separately through their own NFS_INO_INVALID_* bits. >>> >>> That said, there really is no reason why we shouldn't treat the >>> copy >>> and clone code exactly the same way we would treat a regular write. >>> Perhaps we can fix up the arguments of nfs_writeback_update_inode() >>> so >>> that it can be called here? >> >> I wonder if that just kicks the problem down the road to when we >> return the >> delegation, and we'll see cases where ctime/mtime move backward. And >> I >> don't think we can ever be authoritative for space_used, but maybe it >> doesn't >> matter if we're within the attribute timeouts. >> > > I don't understand your point. Mine is that we _should_ be setting the > NFS_INO_INVALID_MTIME, NFS_INO_INVALID_CTIME, NFS_INO_INVALID_BLOCKS... > flags here, and as far as I can tell, we are indeed doing so in > nfs42_copy_dest_done(). Yes, -- I misunderstood your suggestion to use nfs_writeback_update_inode() as a way to fill in our cached attributes with values we'd expect from the result of the copy. > At least for clone(), we're then calling nfs_post_op_update_inode(), > which updates the attributes with whatever was retrieved in the CLONE > call. That will usually contain new values for mtime, ctime and blocks, > and so the cache is refreshed. > > If the client failed to retrieve attributes as part of the CLONE or > COPY call, then we are indeed kicking the can of revalidating the > attributes down the road, but only as far as until the next call to > nfs_getattr(), or the delegation return, whichever comes first. The > fact that we do set the NFS_INO_INVALID_MTIME, ... means that we will > update those cached values before replying to a stat() or statx() call. Ok, thanks for the looks at this, you've convinced me that this patch is unnecessary. I still need the first one, let me know if you want me to resend it as a stand-alone fix. Ben
