On 28 Jan 2020, at 16:46, Trond Myklebust wrote: > On Tue, 2020-01-28 at 21:30 +0000, Trond Myklebust wrote: >> On Tue, 2020-01-28 at 16:20 -0500, Trond Myklebust wrote: >>> On Tue, 2020-01-28 at 15:21 -0500, Trond Myklebust wrote: >>>> On Tue, 2020-01-28 at 10:56 -0500, Trond Myklebust wrote: >>>>> On Tue, 2020-01-28 at 10:26 -0500, Benjamin Coddington wrote: ... >>>>>> + if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ)) >>>>>> + verifier = NFS_DELEGATION_VERF; >>>>>> + else >>>>>> + verifier = nfs_save_change_attribute(dir); >>>>>> + >>>>>> nfs_setsecurity(inode, fattr, label); >>>>>> nfs_set_verifier(dentry, >>>>>> nfs_save_change_attribute(dir)); >>>> >>>> Oops! When reviewing, I missed this. Shouldn't the above be >>>> changed >>>> to >>>> nfs_set_verifier(dentry, verifier) ? Ugh, yep. >>> ...and on a similar vein: nfs_lookup_revalidate_delegated() needs >>> to >>> change so as to not reset the verifier... >>> >>> Sorry for not catching that one either. >> >> Not my day... >> >> nfs_prime_dcache() will clobber the verifier too in the >> nfs_same_file() >> case. That one also needs to set NFS_DELEGATION_VERF if there is a >> delegation. >> >> Perhaps add a helper function for that + >> nfs_lookup_revalidate_dentry()? > > ....and finally, we should remove the call to nfs_set_verifier() from > nfs4_file_open(). Aside from being incorrect in the case where we used > an open-by-filehandle, that case is taken care of in the preceding > dentry revalidation. Ok, I'll get these done. This doesn't make the revalidation code any simpler. I am impressed that you can spot these problems just doing review. I do wish we could use d_fsdata, seems like exactly the kind of thing we need it for, but is it worth it to do another allocation every time we need a dentry. I wonder if we're going to end up having more cases like this, or want to have more private information per-dentry. Right now d_fsdata holds the devicename for IS_ROOT, and caches the appropriate info for a delayed removal of silly-renamed files. Problem is that we don't drop the delegation until after caching the silly-rename data, and then we're still doing the same sorts of things trying to figure out what data is in which dentry. Maybe Al would be willing to reserve some of the top of d_flags for filesystems to use privately? Al, can we have such? NFS already has DCACHE_NFSFS_RENAMED, that could move up above the core d_flags? Ben
