Re: client skips revalidation if holding a delegation

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

On 4 Jun 2019, at 10:53, Trond Myklebust wrote:

> On Tue, 2019-06-04 at 10:10 -0400, Benjamin Coddington wrote:
>> 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)..
>>
>
> OK. Now I think I see what you are saying. This would the case that is
> of interest:
>
> * A directory contains a file "foo", which has a hardlink "bar". Our
> client has both link names cached due to a previous set of lookups.
> * Some other client changes the name of "bar" to "barbar" on the
> server.
> * Our client then opens "foo" and gets a delegation.
> * Our client is then asked to open "bar", and succeeds, failing to
> recognise that it has been renamed to "barbar".
>
> Is that what you mean? That looks like it might happen with the current
> code, and would indeed be a bug.

Yes, that's the problem.  The practical case that was reported to be hitting
it is when `mv` stats source and destination and finds they are the same
file.

> Actually, in the NFSv4.1 open-by-filehandle case, we might even see a
> bug when "foo" is renamed on the server too.

Ok, some relief that you agree this is a bug.

Some ideas for fixing it:

- change d_time to hold the directory's change_attr
from the server, stash that in the (unused?) struct delegation.change_attr

- git rid of the optimization.

- investigate (maybe heuristically discover) whether a directory's
  change_attr is a global counter related to the inode's change_attr.

</hand waving>

At least now I can spend some time on it and not feel aimless, thanks for
the closer look.

Ben