Date: Tue, 28 Nov 2017 13:04:28 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Emmanuel Vadot <manu@bidouilliste.com> Cc: FreeBSD Current <freebsd-current@freebsd.org>, freebsd-fs@freebsd.org, Rick Macklem <rmacklem@freebsd.org> Subject: Re: Switch vfs.nfsd.issue_delegations to TUNABLE ? Message-ID: <20171128110428.GN2272@kib.kiev.ua> In-Reply-To: <20171128114136.10e44d5bcfd563e9b4ab5453@bidouilliste.com> References: <20171128114136.10e44d5bcfd563e9b4ab5453@bidouilliste.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Nov 28, 2017 at 11:41:36AM +0100, Emmanuel Vadot wrote: > > Hello, > > I would like to switch the vfs.nfsd.issue_delegations sysctl to a > tunable. > The reason behind it is recent problem at work on some on our filer > related to NFS. > We use NFSv4 without delegation as we never been able to have good > performance with FreeBSD server and Linux client (we need to do test > again but that's for later). We recently see all of our filers with NFS > enabled perform pourly, resulting in really bad performance on the > service. > After doing some analyze with pmcstat we've seen that we spend ~50% > of our time in lock delay, called by nfsrv_checkgetattr (See [1]). > This function is only usefull when using delegation, as it search for > some write delegations issued to client, but it's called anyway when > there so no delegation and cause a lot of problem when having a lot of > activities. > We've patched the kernel with the included diff and now everything is > fine (tm) (See [2]). > The problem for upstreaming this patch is that since issue_delegations > is a sysctl we cannot know if the checkgetattr called is needed, hence > the question to switch it to a TUNABLE. Also maybe some other code path > could benefit from it, I haven't read all the source of nfsserver yet. > > Note that I won't MFC the changes as it would break POLA. Perhaps make nodeleg per-mount flag ? The you can check it safely by dereferencing vp->v_mount->mnt_data without acquiring any locks, while the vnode lock is owned and the vnode is not reclaimed. > > Cheers, > > [1] https://people.freebsd.org/~manu/m8.svg > [2] https://people.freebsd.org/~manu/m8-new.svg > > >From 0cba277f406d3ccf3c9e943a3d4e17b529e31c89 Mon Sep 17 00:00:00 2001 > From: Emmanuel Vadot <manu@freebsd.org> > Date: Fri, 24 Nov 2017 11:17:18 +0100 > Subject: [PATCH 2/4] Do not call nfsrv_checkgetttr if delegation isn't > enable. > > Signed-off-by: Emmanuel Vadot <manu@freebsd.org> > --- > sys/fs/nfsserver/nfs_nfsdserv.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/sys/fs/nfsserver/nfs_nfsdserv.c > b/sys/fs/nfsserver/nfs_nfsdserv.c index 8102c5810a9..8daf0142360 100644 > --- a/sys/fs/nfsserver/nfs_nfsdserv.c > +++ b/sys/fs/nfsserver/nfs_nfsdserv.c > @@ -54,6 +54,7 @@ extern struct timeval nfsboottime; > extern int nfs_rootfhset; > extern int nfsrv_enable_crossmntpt; > extern int nfsrv_statehashsize; > +extern int nfsrv_issuedelegs; > #endif /* !APPLEKEXT */ > > static int nfs_async = 0; > @@ -240,7 +241,7 @@ nfsrvd_getattr(struct nfsrv_descript *nd, int > isdgram, if (nd->nd_flag & ND_NFSV4) { > if (NFSISSET_ATTRBIT(&attrbits, > NFSATTRBIT_FILEHANDLE)) nd->nd_repstat = nfsvno_getfh(vp, &fh, p); > - if (!nd->nd_repstat) > + if (nd->nd_repstat == 0 && nfsrv_issuedelegs > == 1) nd->nd_repstat = nfsrv_checkgetattr(nd, vp, > &nva, &attrbits, nd->nd_cred, p); > if (nd->nd_repstat == 0) { > -- > 2.14.2 > > > -- > Emmanuel Vadot <manu@bidouilliste.com> <manu@freebsd.org> > _______________________________________________ > freebsd-fs@freebsd.org mailing list > https://lists.freebsd.org/mailman/listinfo/freebsd-fs > To unsubscribe, send any mail to "freebsd-fs-unsubscribe@freebsd.org"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20171128110428.GN2272>