Re: client skips revalidation if holding a delegation

[Trond Myklebust <trondmy@hammerspace.com>]

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.

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


-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com