Re: Switch vfs.nfsd.issue_delegations to TUNABLE ?

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_at_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_at_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
> _at__at_ -54,6 +54,7 _at__at_ 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;
> _at__at_ -240,7 +241,7 _at__at_ 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_at_bidouilliste.com> <manu_at_freebsd.org>
> _______________________________________________
> freebsd-fs_at_freebsd.org mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-fs
> To unsubscribe, send any mail to "freebsd-fs-unsubscribe_at_freebsd.org"