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
