Date: Fri, 21 May 2010 10:39:04 -0400 From: John Baldwin <jhb@freebsd.org> To: Rick Macklem <rmacklem@uoguelph.ca> Cc: Rick Macklem <rmacklem@freebsd.org>, Robert Watson <rwatson@freebsd.org>, fs@freebsd.org Subject: Re: [PATCH] Better handling of stale filehandles in open() in the NFS client Message-ID: <201005211039.04108.jhb@freebsd.org> In-Reply-To: <Pine.GSO.4.63.1005202036570.15248@muncher.cs.uoguelph.ca> References: <201005191144.00382.jhb@freebsd.org> <201005200922.17245.jhb@freebsd.org> <Pine.GSO.4.63.1005202036570.15248@muncher.cs.uoguelph.ca>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201005211039.04108.jhb>