On 25 Jan 2020, at 14:36, Trond Myklebust wrote: > On Fri, 2020-01-24 at 20:33 -0500, Benjamin Coddington wrote: >> On 24 Jan 2020, at 14:24, Trond Myklebust wrote: >> >>> On Fri, 2020-01-24 at 09:09 -0500, Benjamin Coddington wrote: >>> >>> This patch needs to fix nfs_force_lookup_revalidate() to avoid the >>> value NFS_DELEGATION_VERF. >> >> I don't understand why. Doesn't nfs_force_lookup_revalidate() just >> bump the >> directory's cache_change_attribute, which is value we don't care >> about at >> all here. This patch just sets a magic value on the dentry's d_time >> after a >> revalidation occurs for that dentry while holding a delegation. It >> doesn't >> care at all about the directory's change_attr. >> >> What am I missing? > > Those calls to nfs_set_verifier(dentry, nfs_save_change_attribute(dir)) > are storing the parent directory cache_change_attribute() in the d_time > field of the child dentry. That's the normal value for the child dentry > verifiers of that directory when there is no delegation. > > So to avoid collisions, you need to ensure that dir- > cache_change_attribute never takes the value NFS_DELEGATION_VERF. Ah - of course. A _complete_ fix. Thanks for helping me with the obvious. Ok, after not feeling good about adding a comparison and jump into that path - maybe its not as big a deal as I'm thinking, but ugh it seems gross for just this single case - I wondered about doing this other ways. As alternatives, I tried to get d_fsdata freed up for this case (there's already two users of that, it was ugly), and then playing with only some of the bits in d_time for flags (every approach I tried is slower than just checking and skipping NFS_DELEGATION_VERF). I thought maybe we could grow another d_flag for this sort of thing, but I doubt that will fly. Oh well, I'll just send a V2 that skips the magic value. Thanks again, Ben
