Re: [PATCH] NFS: Revalidate once when holding a delegation

["Benjamin Coddington" <bcodding@redhat.com>]

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