[PATCH] nfs: reset cookieverf even when no cached pages

[PATCH] nfs: reset cookieverf even when no cached pages

[Nagendra Tomar]

From: Nagendra S Tomar <natomar@microsoft.com>

If NFS_INO_INVALID_DATA cache_validity flag is set, a subsequent call
to nfs_invalidate_mapping() does the following two things:

 1. Clears mapping.
 2. Resets cookieverf to 0, if inode refers to a directory.

If there are no mapped pages, we don't need #1, but we still need #2.

Signed-off-by: Nagendra S Tomar <natomar@microsoft.com>
---
 fs/nfs/inode.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index aa6493905bbe..40f2bfaa4e46 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -209,8 +209,13 @@ static void nfs_set_cache_invalid(struct inode *inode, unsigned long flags)
 				| NFS_INO_INVALID_XATTR);
 	}
 
-	if (inode->i_mapping->nrpages == 0)
+	if (inode->i_mapping->nrpages == 0) {
+		if (S_ISDIR(inode->i_mode) &&
+		    (flags & (NFS_INO_INVALID_DATA | NFS_INO_DATA_INVAL_DEFER)))
+			memset(nfsi->cookieverf, 0, sizeof(nfsi->cookieverf));
 		flags &= ~(NFS_INO_INVALID_DATA|NFS_INO_DATA_INVAL_DEFER);
+	}
+
 	nfsi->cache_validity |= flags;
 	if (flags & NFS_INO_INVALID_DATA)
 		nfs_fscache_invalidate(inode);

Re: [PATCH] nfs: reset cookieverf even when no cached pages

[Trond Myklebust]

On Tue, 2020-09-15 at 15:33 +0000, Nagendra Tomar wrote:
> From: Nagendra S Tomar <natomar@microsoft.com>
> 
> If NFS_INO_INVALID_DATA cache_validity flag is set, a subsequent call
> to nfs_invalidate_mapping() does the following two things:
> 
>  1. Clears mapping.
>  2. Resets cookieverf to 0, if inode refers to a directory.
> 
> If there are no mapped pages, we don't need #1, but we still need #2.

I don't think this patch is correct.

Firstly, we don't support servers that use non-persistent readdir
cookies, so if the cookieverf changes on the server, then we're not
going to behave correctly w.r.t. expected POSIX readdir() behaviour.

Secondly, even if we did support non-persistent cookies, then the NFS
spec says that we need to set the verifier to zero when we are caching
no readdir cookies that need validation, and we are reading the entire
directory back from scratch (i.e. we're also supplying a zero
dir_cookie). So the right place to do this set/reset of the verifier is
in the readdir code, which knows not only the state of our cache, but
also know what we're sending as RPC call arguments.

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

RE: [PATCH] nfs: reset cookieverf even when no cached pages

[Nagendra Tomar]

Thanks for your comments, Trond.

This is what happens w/o this patch. Lets take the case of new file being 
created.  

nfs3_do_create()->
nfs_post_op_update_inode()->
nfs_post_op_update_inode_locked(), sets NFS_INO_INVALID_DATA to
mark the cached directory data as invalid, so that later when we do a readdir, 

nfs_readdir()->
nfs_revalidate_mapping()->
nfs_invalidate_mapping(), unmaps the mapping *and* resets the cookieverf.

This causes us to correctly ask for fresh dir contents using a cookieverf=0
and cookie=0 (exactly what the RFC expects us to do). All good till here.
Now consider the case where 
nfs_post_op_update_inode_locked()->nfs_set_cache_invalid() found that dir
pages are already unmapped. Today we just clear the NFS_INO_INVALID_DATA
flag since we note that no cached dir pages and hence no invalidation needed. 
Without the NFS_INO_INVALID_DATA, nfs_readdir() now makes a READDIR RPC
with a non-zero cookieverf and cookie=0. This is non-standard combination of 
cookiverf and cookie for asking fresh dir contents.
Note that servers that don't care about the cookieverf do not care about this
non-standard combination and they work fine w/o the patch, but servers that
want cookieverf to be 0 to signal fresh dir read may not treat it well.

The patch tries to make sure that we reset the cookieverf even for the case
where dir pages somehow got purged when we reached nfs_set_cache_invalid()
above.
I felt nfs_set_cache_invalid() is the right place to reset the cookiverf, as 
whenever  we set dir cache as invalid, on next readdir we would like to read 
the entire dir  from the server and for that we need to send cookieverf=0.

> So the right place to do this set/reset of the verifier is in the readdir code

Yes, but we don't do that if the NFS_INO_INVALID_DATA is not set.

Thanks,
Tomar

-----Original Message-----
From: Trond Myklebust <trondmy@hammerspace.com> 
Sent: Wednesday, September 16, 2020 8:23 PM
To: linux-nfs@vger.kernel.org; Nagendra Tomar <Nagendra.Tomar@microsoft.com>
Cc: anna.schumaker@netapp.com
Subject: [EXTERNAL] Re: [PATCH] nfs: reset cookieverf even when no cached pages

On Tue, 2020-09-15 at 15:33 +0000, Nagendra Tomar wrote:
> From: Nagendra S Tomar <natomar@microsoft.com>
> 
> If NFS_INO_INVALID_DATA cache_validity flag is set, a subsequent call
> to nfs_invalidate_mapping() does the following two things:
> 
>  1. Clears mapping.
>  2. Resets cookieverf to 0, if inode refers to a directory.
> 
> If there are no mapped pages, we don't need #1, but we still need #2.

I don't think this patch is correct.

Firstly, we don't support servers that use non-persistent readdir
cookies, so if the cookieverf changes on the server, then we're not
going to behave correctly w.r.t. expected POSIX readdir() behaviour.

Secondly, even if we did support non-persistent cookies, then the NFS
spec says that we need to set the verifier to zero when we are caching
no readdir cookies that need validation, and we are reading the entire
directory back from scratch (i.e. we're also supplying a zero
dir_cookie). So the right place to do this set/reset of the verifier is
in the readdir code, which knows not only the state of our cache, but
also know what we're sending as RPC call arguments.

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

Re: [PATCH] nfs: reset cookieverf even when no cached pages

[Trond Myklebust]

On Wed, 2020-09-16 at 15:58 +0000, Nagendra Tomar wrote:
> Thanks for your comments, Trond.
> 
> This is what happens w/o this patch. Lets take the case of new file
> being 
> created.  
> 
> nfs3_do_create()->
> nfs_post_op_update_inode()->
> nfs_post_op_update_inode_locked(), sets NFS_INO_INVALID_DATA to
> mark the cached directory data as invalid, so that later when we do a
> readdir, 
> 
> nfs_readdir()->
> nfs_revalidate_mapping()->
> nfs_invalidate_mapping(), unmaps the mapping *and* resets the
> cookieverf.
> 
> This causes us to correctly ask for fresh dir contents using a
> cookieverf=0
> and cookie=0 (exactly what the RFC expects us to do). All good till
> here.
> Now consider the case where 
> nfs_post_op_update_inode_locked()->nfs_set_cache_invalid() found that
> dir
> pages are already unmapped. Today we just clear the
> NFS_INO_INVALID_DATA
> flag since we note that no cached dir pages and hence no invalidation
> needed. 
> Without the NFS_INO_INVALID_DATA, nfs_readdir() now makes a READDIR
> RPC
> with a non-zero cookieverf and cookie=0. This is non-standard
> combination of 
> cookiverf and cookie for asking fresh dir contents.

No it is not 'non-standard'. The language used in RFC5661 is as
follows:

   The request's cookieverf field should be set to 0 zero) when the
   r
equest's cookie field is zero (first read of the directory).  On
   subs
equent requests, the cookieverf field must match the cookieverf
   retur
ned by the READDIR in which the cookie was acquired.  If the
   server
determines that the cookieverf is no longer valid for the
   directory,
the error NFS4ERR_NOT_SAME must be returned.

Note that there there is no RFC 2119 normative MUST or even a SHOULD.
We therefore expect to be able to reuse the cookie verifier with a zero
cookie in order to validate that the cookies that our client may still
be caching (e.g. in another open file context) are good.

> Note that servers that don't care about the cookieverf do not care
> about this
> non-standard combination and they work fine w/o the patch, but
> servers that
> want cookieverf to be 0 to signal fresh dir read may not treat it
> well.

See above. The standard does not require this behaviour of the client.

> The patch tries to make sure that we reset the cookieverf even for
> the case
> where dir pages somehow got purged when we reached
> nfs_set_cache_invalid()
> above.
> I felt nfs_set_cache_invalid() is the right place to reset the
> cookiverf, as 
> whenever  we set dir cache as invalid, on next readdir we would like
> to read 
> the entire dir  from the server and for that we need to send
> cookieverf=0.
> 
> > So the right place to do this set/reset of the verifier is in the
> > readdir code
> 
> Yes, but we don't do that if the NFS_INO_INVALID_DATA is not set.
> 
> Thanks,
> Tomar
> 

Either way, as I said previously, the patch is incorrect since it is
setting the verifier to zero in a context where it cannot guarantee
that the next cookie sent in a READDIR call will be a zero. I agree
that the code in nfs_revalidate_mapping() is equally broken, but that's
not a reason to propagate the bug.

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

RE: [PATCH] nfs: reset cookieverf even when no cached pages

[Nagendra Tomar]

Thanks for your comments, Trond.

> Note that there there is no RFC 2119 normative MUST or even a SHOULD.
> We therefore expect to be able to reuse the cookie verifier with a zero
> cookie in order to validate that the cookies that our client may still
> be caching (e.g. in another open file context) are good.
> Either way, as I said previously, the patch is incorrect since it is
> setting the verifier to zero in a context where it cannot guarantee
> that the next cookie sent in a READDIR call will be a zero. 

Pls correct me if my understanding is not correct, but since we store the 
cookieverf in the inode (shared by all open contexts), while every open
context has its own dir_cookie, all changes to cookieverf  _are_ being done
regardless of the dir_cookie value held by other open contexts. If after the
cookieverf change, an open context's cookiverf and cookie combination
becomes invalid at the server, that open context deals with the EBADCOOKIE 
as appropriate.
f.e. nfs_zap_caches_locked() also clears cookieverf when the dir cache is
zapped, irrespective of other open contexts.

Also, since we did intend to set the cookieverf to 0 in nfs_invalidate_mapping(),
just that if the dir cache happens to be already purged (say by an unrelated
vm  reclaim), we cancel the invalidation. This causes us to use the old
non-zero cookiverf, a different behavior than if the dir cache was not
purged.
Is this ok ? Isn't it better to consistently use zero cookieverf in both cases ?

Thanks,
Tomar

Re: [PATCH] nfs: reset cookieverf even when no cached pages

[Trond Myklebust]

On Wed, 2020-09-16 at 22:48 +0000, Nagendra Tomar wrote:
> Thanks for your comments, Trond.
> 
> > Note that there there is no RFC 2119 normative MUST or even a
> > SHOULD.
> > We therefore expect to be able to reuse the cookie verifier with a
> > zero
> > cookie in order to validate that the cookies that our client may
> > still
> > be caching (e.g. in another open file context) are good.
> > Either way, as I said previously, the patch is incorrect since it
> > is
> > setting the verifier to zero in a context where it cannot guarantee
> > that the next cookie sent in a READDIR call will be a zero. 
> 
> Pls correct me if my understanding is not correct, but since we store
> the 
> cookieverf in the inode (shared by all open contexts), while every
> open
> context has its own dir_cookie, all changes to cookieverf  _are_
> being done
> regardless of the dir_cookie value held by other open contexts.

...and that would be a bug. The dir_cookie are tied to a specific value
of the cookieverf, so you can't just acknowledge a change to the latter
without also changing the former.

We're not just caching the dir_cookies in the page cache. We're also
caching them in all the nfs_open_dir_context that are associated with
the various open file descriptors for that file, and we're not caching
a cookieverf to ensure they are invalidated when the inode cookieverf
changes.

>  If after the
> cookieverf change, an open context's cookiverf and cookie combination
> becomes invalid at the server, that open context deals with the
> EBADCOOKIE 
> as appropriate.
> f.e. nfs_zap_caches_locked() also clears cookieverf when the dir
> cache is
> zapped, irrespective of other open contexts.

No. See above. 

> Also, since we did intend to set the cookieverf to 0 in
> nfs_invalidate_mapping(),
> just that if the dir cache happens to be already purged (say by an
> unrelated
> vm  reclaim), we cancel the invalidation. This causes us to use the
> old
> non-zero cookiverf, a different behavior than if the dir cache was
> not
> purged.
> Is this ok ? Isn't it better to consistently use zero cookieverf in
> both cases ?
> 

Compounding the existing bug for the sake of consistency is not a
useful exercise.

If there are servers out there that use the cookieverf, then we'd be
better served by fixing the code. However note that servers that change
cookieverf will break POSIX readdir (because we're no longer able to
use cookies as a cursor), so I'm not sure that I really care to support
such servers.

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