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

[John Baldwin]

On Thursday 20 May 2010 8:45:13 pm Rick Macklem wrote:
> 
> On Thu, 20 May 2010, John Baldwin wrote:
> 
> >
> > It doesn't change the RPC count because of changes that Mohan added to the
> > NFS client a while ago so that nfs_open() doesn't invalide the attribute
> > cache during nfs_open() if it was already updated via nfs_lookup() during
> > the same system call.  With Mohan's changes in place, all this change does
> > is move the GETATTR/ACCESS RPC earlier in the case of a namecache hit.
> >
> Well, it sounds like a good theory. Something like:
> - VOP_LOOKUP() locks the vnode, which is then passed to VOP_OPEN() and
>    since it is locked, other threads can't perform VOPs on the vp.

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.

> I ran a single pass of a kernel "make cleandepend; make depend; make"
> here (1 without the patch and one with the patch). Now, it could be
> random variation, but since the other RPC counts changed by < 1%, I
> suspect not. (I'll run another pass of each to see how much variation
> I see w.r.t. Getattr.)
> 
> Here's the counts for the 5 RPCs I think might be interesting:
> RPC		Count without patch	Count with patch
> Getattr		590936			625987		+5.9%
> Lookup		157194			157528
> Access		59040			59690		+1.1%
> Read		70585			70586
> Write		112531			112530
> 
> I let you know what another pass of each gives, but it looks like
> it has caused an increase in RPC cnts. I don't know if the increase
> is enough to deter adding the patch, but it might be worth exploring
> it more?
> 
> rick
> 
> 

-- 
John Baldwin