Switch vfs.nfsd.issue_delegations to TUNABLE ?

Emmanuel Vadot

2017-11-28 15:38:33 UTC

On Tue, 28 Nov 2017 15:40:09 +0200

On Tue, 28 Nov 2017 13:04:28 +0200

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.

That won't work with current code.

Why ?

Currently if you have delegation enabled and connect one client to a
mountpoint, then disable delegation, the current client will still have
delegation while new ones will not. I have not tested restarting nfsd in
this situation but I suspect that client will behave badly. This is a
another +1 for making it a tunable I think.

It is up to the filesystem to handle remount, in particular, fs can disable
changing mount options on mount upgrade if the operation is not supported.
In other words, you would do
mount -o nodeleg ... /mnt
and
mount -u -o nonodeleg ... /mnt
needs to return EINVAL.

You are talking about client here while I'm talking about server.

Post by Konstantin Belousov

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

Date: Fri, 24 Nov 2017 11:17:18 +0100
Subject: [PATCH 2/4] Do not call nfsrv_checkgetttr if delegation isn't
enable.
---
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
--
_______________________________________________
https://lists.freebsd.org/mailman/listinfo/freebsd-fs

_______________________________________________
https://lists.freebsd.org/mailman/listinfo/freebsd-current

--
Emmanuel Vadot <***@bidouilliste.com> <***@freebsd.org>