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@xxxxxxxxxxxxxxx> Sent: Wednesday, September 16, 2020 8:23 PM To: linux-nfs@xxxxxxxxxxxxxxx; Nagendra Tomar <Nagendra.Tomar@xxxxxxxxxxxxx> Cc: anna.schumaker@xxxxxxxxxx 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@xxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxxx
