Re: client skips revalidation if holding a delegation

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

On 4 Jun 2019, at 8:56, Trond Myklebust wrote:

> On Tue, 2019-06-04 at 08:41 -0400, Benjamin Coddington wrote:
>> Hey linux-nfs, and especially maintainers,
>>
>> I'm still interested in working on a problem raised a couple weeks
>> ago, but
>> confusion muddled that discussion and it died:
>>
>> If the client holds a read delegation, it will skip revalidation of a
>> dentry
>> in lookup.  If the file was moved on the server, the client can end
>> up with
>> two positive dentries in cache for the same inode, and the dentry
>> that
>> doesn't exist on the server will never time out of the cache.
>>
>> The client can detect this happening because the directory of the
>> dentry
>> that should be revalidated updates it's change attribute.  Skipping
>> revalidation is an optimization in the case we hold a delegation, but
>> this
>> optimization should only be used when the delegation was obtained via
>> a
>> lookup of the dentry we are currently revalidating.
>>
>> Keeping the optimization might be done by tying the delegation to the
>> dentry.  Lacking some (easy?) way to do that currently, it seems
>> simpler to
>> remove the optimization altogether, and I will send a patch to remove
>> it.
>
> A delegation normally applies to the entire inode. It covers _all_
> dentries that point to that inode too because create, rename and unlink
> are always atomically accompanied by an inode change attribute.

It should cover all dentries that point to that inode at the time the
delegation was handed out.  Shouldn't dentries cached _before_ the
delegation be invalidated?  The client doesn't currently care about the
order of dentries cached with respect to delegations.

> IOW: The proposed restriction is both unnecessary and incorrect.

But then I think: need to store that change attribute on the dentry instead
of what we currently use - a client-only monotonic counter.  Then, we could
compare the delegation's change attr to the dentry's.

But that assumes they are both globally related -- that a directory's
change_attr on lookup relates to an inode's change attribute.  I don't see
that anywhere (I'm looking in 7530)..

>> Any thoughts on this?  Any response, even asserting that this is not
>> something
>> we will fix are welcome.

I think, what I am lacking (and I admit to have a tendency to become
fixated) is proper guidance on whether or not work on this front is
acceptable.

I am happy to work on this, but not if my time is wasted.  Help!

Ben