NFS: revalidate on open if dcache is negative.

PATCH In review NeilBrown 2 participants 3 messages May 12, 2014 – May 13, 2014

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

NeilBrown

author

2014-05-12 03:50 UTC #0 lore raw

PATCH 1/1 NFS: revalidate on open if dcache is negative.

NFS CTO semantics require that (absent a delegation) the server must be contacted at each open.

nfs_lookup_verify_inode() implements this when the dcache contains a positive cached entry. However it is not called when the dcache contains a negative cached entry. That path uses nfs_neg_need_reval() which doesn't impose CTO semantics.

So a sequence like:

rm -f testfile
ls -l testfile
ssh $server touch testfile
cat testfile

will fail:

cat: testfile: No such file or directory

an 'strace' will confirm that this resulted from an 'open' system call.

So add code to nfs_neg_need_reval implement CTO semantics much like that in nfs_lookup_verify_inode().

Signed-off-by NeilBrown <[email protected]>

fs/nfs/dir.c +2 -0

index d9f3d067cd15..f8022da72460 100644
@@ -1062,6 +1062,8 @@ int nfs_neg_need_reval(struct inode *dir, struct dentry *dentry,
 	/* Don't revalidate a negative dentry if we're creating a new file */
 	if (flags & LOOKUP_CREATE)
 		return 0;
+	if ((flags & LOOKUP_OPEN) && !(NFS_SERVER(dir)->flags & NFS_MOUNT_NOCTO))
+		return 1;
 	if (NFS_SERVER(dir)->flags & NFS_MOUNT_LOOKUP_CACHE_NONEG)
 		return 1;
 	return !nfs_check_verifier(dir, dentry);

Trond Myklebust

2014-05-12 20:59 UTC #1 lore raw

Quoted 19 lines · NeilBrown

NFS CTO semantics require that (absent a delegation) the server must be contacted at each open.
nfs_lookup_verify_inode() implements this when the dcache contains a positive cached entry. However it is not called when the dcache contains a negative cached entry. That path uses nfs_neg_need_reval() which doesn't impose CTO semantics.
So a sequence like:
rm -f testfile
ls -l testfile
ssh $server touch testfile
cat testfile
will fail:

Quoted 7 lines

cat: testfile: No such file or directory
an 'strace' will confirm that this resulted from an 'open' system call.
So add code to nfs_neg_need_reval implement CTO semantics much like that in nfs_lookup_verify_inode().

Hi Neil,

To me, close-to-open is about ensuring that the data and metadata caches for the file (or directory) in question are revalidated correctly on open (or opendir). Close-to-open semantics are enabled/disabled by the cto/nocto mount options (with cto being the default).

The lookup semantics are about ensuring that the namespace cache is consistent with what is on the server. They are controlled by the ‘lookupcache’ mount option, which defaults to the behaviour that you describe above (lookupcache=all). I believe that we have discussed changing the default to be lookupcache=positive, which would give the semantics that your patch enforces, but I believe the consensus was to keep the current behaviour.

Cheers
Trond

_________________________________
Trond Myklebust
Linux NFS client maintainer, PrimaryData
[email protected]

NeilBrown

author

2014-05-13 00:42 UTC #2 lore raw

Quoted 27 lines · On Mon, 12 May 2014 13:58:59 -0700 Trond Myklebust…

Quoted 24 lines · NeilBrown
NFS CTO semantics require that (absent a delegation) the server must be contacted at each open.
nfs_lookup_verify_inode() implements this when the dcache contains a positive cached entry. However it is not called when the dcache contains a negative cached entry. That path uses nfs_neg_need_reval() which doesn't impose CTO semantics.
So a sequence like:
rm -f testfile
ls -l testfile
ssh $server touch testfile
cat testfile
will fail:
cat: testfile: No such file or directory
an 'strace' will confirm that this resulted from an 'open' system call.

Quoted 9 lines

Quoted 2 lines
So add code to nfs_neg_need_reval implement CTO semantics much like that in nfs_lookup_verify_inode().
Hi Neil,
To me, close-to-open is about ensuring that the data and metadata caches for the file (or directory) in question are revalidated correctly on open (or opendir). Close-to-open semantics are enabled/disabled by the cto/nocto mount options (with cto being the default).
The lookup semantics are about ensuring that the namespace cache is consistent with what is on the server. They are controlled by the ‘lookupcache’ mount option, which defaults to the behaviour that you describe above (lookupcache=all). I believe that we have discussed changing the default to be lookupcache=positive, which would give the semantics that your patch enforces, but I believe the consensus was to keep the current behaviour.

Yes - of course. Thanks for clarifying that for me.
I had in mind that lookup-for-open is special. It is, but not that special.

Thanks,
NeilBrown