[PATCH] Better handling of stale filehandles in open() in the NFS client

[Rick Macklem]

On Fri, 21 May 2010, John Baldwin wrote:


> No, it's not the lock, it's this thing Mohan added here in nfs_open():
>
> 		struct thread *td = curthread;
>
> 		if (np->n_ac_ts_syscalls != td->td_syscalls ||
> 		    np->n_ac_ts_tid != td->td_tid ||
> 		    td->td_proc == NULL ||
> 		    np->n_ac_ts_pid != td->td_proc->p_pid) {
> 			np->n_attrstamp = 0;
> 			KDTRACE_NFS_ATTRCACHE_FLUSH_DONE(vp);
> 		}
>
> Which used to be an unconditional clearing of n_attrstamp so that the
> VOP_GETATTR() in nfs_open() would always go over the wire.  Now it doesn't
> clear the attributes if the attribute cache was last updated during the same
> system call that is invoking nfs_open() by the same thread.
>
> Hmm, however, concurrent lookups of the same pathname may cause this test to
> fail and still clear n_attrstamp since we now use shared vnode locks for
> pathname lookups.  That was true before my change as well.  In fact, using
> shared vnode locks for read-only opens (the MNTK_EXTENDED_SHARED flag)
> probably does cause Mohan's patch to not work in many cases which probably
> accounts for the small increase in RPC counts.
>
> Try setting the vfs.lookup_shared sysctl to 0 to see if that removes all the
> extra RPCs.  Another change would be to readd the change to flush the
> attribute cache on close and see if that also brings back extra
> attribute/access RPCs without my patch (but with lookup_shared left enabled)
> to verify that shared lookups are the root cause of extra RPCs.
>
Ok, I've played with this a bit and here's what I've seen:
- Disabling vfs.lookup_shared had no effect. Since I'm running a single
   thread (no "-j" on the make) ad nothing else on the machine during the
   tests, that seems to make sense.

- When I added some printfs, here's what seems to be happening.
   snippet of nfs_lookup():
 		if ((cnp->cn_flags & (ISLASTCN | ISOPEN)) ==
 		    (ISLASTCN | ISOPEN)) {
 			newnp = VTONFS(newvp);
 			mtx_lock(&newnp->n_mtx);
 			newnp->n_attrstamp = 0;
 			mtx_unlock(&newnp->n_mtx);
 		}
 		if (!VOP_GETATTR(newvp, &vattr, cnp->cn_cred)
 		    && vattr.va_ctime.tv_sec == newnp->n_ctime) {
 			nfsstats.lookupcache_hits++;
 			if (cnp->cn_nameiop != LOOKUP &&
 			    (flags & ISLASTCN))
 				cnp->cn_flags |= SAVENAME;
 			return (0);
 		}
--> gets here about 9,000 times
     --> As far as I can see, these getattrs are part of the price
         you pay for your patch. (Cache misses that aren't caused by
         VOP_GETATTR() failing.)

snippet of nfs_open():
 	if (np->n_flag & NMODIFIED) {
 		...
--> gets here about 6,000 times. The extra Getattrs for this case can
         be avoied by adding if ((newnp->n_flag & NMODIFIED) == 0)
 	for setting newnp->n_attrstamp = 0; above.
 	} else {
 		struct thread *td = curthread;

 		if (np->n_ac_ts_syscalls != td->td_syscalls ||
 		    np->n_ac_ts_tid != td->td_tid ||
 		    td->td_proc == NULL ||
 		    np->n_ac_ts_pid != td->td_proc->p_pid) {
 			np->n_attrstamp = 0;
 			KDTRACE_NFS_ATTRCACHE_FLUSH_DONE(vp);
--> never gets here, which makes sense since your patch forces the Getattr
     to be done in nfs_lookup(). I wouldn't say that this can "never"
     happen with the patch applied, because of concurrency with shared
     vnode locks, but it isn't happening for this test.
 		}
 		mtx_unlock(&np->n_mtx);
 		error = VOP_GETATTR(vp, &vattr, ap->a_cred);

So, I can see where about 15,000 of the extra Getattrs occur, but haven't
nailed down the other 20,000. I do see a lot of cases where the namei()
call in vn_open_cred() at line#189 of vfs_vnops.c returns an error. I
haven't figured out if those account for the other 20,000. (ie. are there
cases where namei()/nfs_lookup() fails after having forced the Getattr)
Since namei() fails, VOP_OPEN() doesn't get called.

In summary, I haven't nailed down where 20,000 of the extra Getattr RPCs
happen, but if it still achieves what you want, adding the
 	if ((newnp->n_flag & NMODIFIED) == 0)
before 		newnp->n_attrstamp = 0;
gets rid of some of them.

I'm going to run some tests with Readdirplus enabled, to see what effect
the patch has for that case, rick.