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