On Wed, 2020-01-29 at 09:18 -0500, Benjamin Coddington wrote: > 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? I did look into this a few weeks ago, and it seemed to me that we're already low on free bits in d_flags. An alternative might just be to reserve a whole bit in d_time as being the 'delegated dentry revalidated' bit. e.g. reserve bit 'BITS_PER_LONG - 1' (or just bit '0') for that purpose. We would then want to change nfs_set_verifier() to set the verifier in the remaining bits, and have it set the delegated dentry revalidated bit depending on whether there is a delegation for the inode at the time of revalidation. We may also want to do the same setting of the delegation bit in nfs_verify_change_attribute() when there is a successful match of the remaining verifier. Finally, we should have nfs_mark_delegation_revoked() and nfs_start_delegation_return_locked() clear that delegated dentry revalidated bit, perhaps by iterating over the inode->i_dentry list? Note that this has the advantage that since the actual verifier part of dentry->d_time can be kept up to date while we hold the delegation, we don't have to worry about the dentry getting revalidated for no reason after the delegation is returned. -- Trond Myklebust Linux NFS client maintainer, Hammerspace trond.myklebust@xxxxxxxxxxxxxxx
