negative cache hits for nfs (was: cvs commit: ...)

[Chuck Lever]

Hi Bruce-

On 10/18/06, Bruce Evans <bde at zeta.org.au> wrote:
> Here is a merge of some bits from NetBSD for review.  It is mostly the
> 1997 version, with updates to use timespecs instead of time_t's, but
> not updates to use changes that don't seem to be related to correctness,
> or ones less than 18 months old (if any).
>
> % Index: nfs_vnops.c
> % ===================================================================
> % RCS file: /home/ncvs/src/sys/nfsclient/nfs_vnops.c,v
> % retrieving revision 1.270
> % diff -u -2 -r1.270 nfs_vnops.c
> % --- nfs_vnops.c       14 Oct 2006 07:25:11 -0000      1.270
> % +++ nfs_vnops.c       18 Oct 2006 01:41:14 -0000
> % @@ -852,7 +869,17 @@
> %               return (error);
> %       }
> % -     if ((error = cache_lookup(dvp, vpp, cnp)) && error != ENOENT) {
> % +     if ((error = cache_lookup(dvp, vpp, cnp)) != 0) {
> %               struct vattr vattr;
> %
> % +             if (error == ENOENT) {
> % +                     /* Negative cache hit.  Use it unless stale. */
> % +                     if (VOP_GETATTR(dvp, &vattr, cnp->cn_cred, td) == 0 &&
> % +                         timespeccmp(&vattr.va_mtime, &np->n_nctime, ==))
> % +                             return (ENOENT);
> % +
> % +                     cache_purge(dvp);
> % +                     timespecclear(&np->n_nctime);
> % +                     goto dorpc;
> % +             }
> %               newvp = *vpp;
> %               if (!VOP_GETATTR(newvp, &vattr, cnp->cn_cred, td)
> % @@ -871,4 +898,5 @@
> %               *vpp = NULLVP;
> %       }
> % +dorpc:
> %       error = 0;
> %       newvp = NULLVP;
> % @@ -951,4 +979,11 @@
> %  nfsmout:
> %       if (error) {
> % +             if (error == ENOENT && (cnp->cn_flags & MAKEENTRY) &&
> % +                 cnp->cn_nameiop != CREATE) {
> % +                     /* Negative cache entry. */
> % +                     if (!timespecisset(&np->n_nctime))
> % +                             np->n_nctime = np->n_vattr.va_mtime;
> % +                     cache_enter(dvp, NULL, cnp);
> % +             }
> %               if (newvp != NULLVP) {
> %                       vput(newvp);
> % @@ -1931,6 +1966,9 @@
> %               if (newvp)
> %                       vput(newvp);
> % -     } else
> % +     } else {
> % +             if (cnp->cn_flags & MAKEENTRY)
> % +                     cache_enter(dvp, newvp, cnp);
> %               *ap->a_vpp = newvp;
> % +     }
> %       return (error);
> %  }
> % Index: nfsnode.h
> % ===================================================================
> % RCS file: /home/ncvs/src/sys/nfsclient/nfsnode.h,v
> % retrieving revision 1.59
> % diff -u -2 -r1.59 nfsnode.h
> % --- nfsnode.h 13 Sep 2006 18:39:09 -0000      1.59
> % +++ nfsnode.h 18 Oct 2006 00:48:44 -0000
> % @@ -100,4 +100,5 @@
> %       struct timespec         n_mtime;        /* Prev modify time. */
> %       time_t                  n_ctime;        /* Prev create time. */
> % +     struct timespec         n_nctime;       /* Last neg cache entry (dir) */
> %       time_t                  n_expiry;       /* Lease expiry time */
> %       nfsfh_t                 *n_fhp;         /* NFS File Handle */

This looks reasonable, but there are always tricky corner cases for
this kind of change.  The reason many of these changes haven't been
made sooner is because they usually break something else.

Now that you've verified the positive performance impact of this
change, you should start by testing this with the Connectathon test
suite (both NFSv2 and NFSv3).  Another useful test in this case is to
observe how the client behaves in the face of replaced file objects.
Use another client to remove and recreate a file that your test client
already has cached, or in this case create first after your client has
already cached the negative lookup.  Another interesting test case is
to enable READDIRPLUS on NFSv3 mounts to see if there is any strange
interaction with your patch.

Additionally, I would hold off on putting this in 6.2.  Try it in
CURRENT for a while, then MFC it after 6.3 branches.

Have you reviewed the NetBSD change logs for any fixes in this area since 1997?

> Now another pessimization is more obvious -- why should building kernels
> be doing all those Fsstat's?  I haven't located them for sure, but
> that opendir always calls statfs(2) for the silly purpose of determining
> whether it needs to do extra work to support unionfs.

I've also noticed that behavior.  Certainly the FreeBSD client can do
a better job of caching the results of Fsstat.

-- 
"We who cut mere stones must always be envisioning cathedrals"
   -- Quarry worker's creed