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@xxxxxxxxxxxxxxx
