Date: Wed, 3 Aug 2016 23:44:04 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Konstantin Belousov <kib@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r303710 - head/sys/fs/nfsclient Message-ID: <20160803231538.U935@besplex.bde.org> In-Reply-To: <201608031149.u73BnHOt049332@repo.freebsd.org> References: <201608031149.u73BnHOt049332@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 3 Aug 2016, Konstantin Belousov wrote:
> Log:
> Remove unneeded (recursing) Giant acquisition around vprintf(9).
>
> Reviewed by: rmacklem
> Sponsored by: The FreeBSD Foundation
> MFC after: 1 week
>
> Modified:
> head/sys/fs/nfsclient/nfs_clsubs.c
>
> Modified: head/sys/fs/nfsclient/nfs_clsubs.c
> ==============================================================================
> --- head/sys/fs/nfsclient/nfs_clsubs.c Wed Aug 3 10:23:42 2016 (r303709)
> +++ head/sys/fs/nfsclient/nfs_clsubs.c Wed Aug 3 11:49:17 2016 (r303710)
> @@ -166,11 +166,9 @@ ncl_printf(const char *fmt, ...)
> {
> va_list ap;
>
> - mtx_lock(&Giant);
> va_start(ap, fmt);
> vprintf(fmt, ap);
> va_end(ap);
> - mtx_unlock(&Giant);
> }
>
> #ifdef NFS_ACDEBUG
This leaves the less than needed function nfs_printf(). Old versions
of nfs used the correct function printf(). All nfs_printf() ever did
was:
- when it was first implemented, almost never work. It had
printf(fmt, ap) instead of vprintf(fmt, ap). This only worked when
fmt had no % directives in it. Otherwise, it gave undefined behaviour.
FreeBSD-7 still has this version.
- to hold the Giant locking. This was not completely unneeded. It had
the side effect of serializing printfs within nfs.
Serialization in -current is normally done using PRINTF_BUFR_SIZE, but
this is not the default and it gives deadlocks or drops output so I
don't use it.
I refined my old fix for serializing printf() and it now works very well.
> @@ -197,9 +195,6 @@ ncl_getattrcache(struct vnode *vp, struc
> vap = &np->n_vattr.na_vattr;
> nmp = VFSTONFS(vp->v_mount);
> mustflush = nfscl_mustflush(vp); /* must be before mtx_lock() */
> -#ifdef NFS_ACDEBUG
> - mtx_lock(&Giant); /* ncl_printf() */
> -#endif
> mtx_lock(&np->n_mtx);
> /* XXX n_mtime doesn't seem to be updated on a miss-and-reload */
> timeo = (time_second - np->n_mtime.tv_sec) / 10;
> @@ -236,9 +231,6 @@ ncl_getattrcache(struct vnode *vp, struc
> (mustflush != 0 || np->n_attrstamp == 0)) {
> newnfsstats.attrcache_misses++;
> mtx_unlock(&np->n_mtx);
> -#ifdef NFS_ACDEBUG
> - mtx_unlock(&Giant); /* ncl_printf() */
> -#endif
> KDTRACE_NFS_ATTRCACHE_GET_MISS(vp);
> return( ENOENT);
> }
> @@ -266,9 +258,6 @@ ncl_getattrcache(struct vnode *vp, struc
> vaper->va_mtime = np->n_mtim;
> }
> mtx_unlock(&np->n_mtx);
> -#ifdef NFS_ACDEBUG
> - mtx_unlock(&Giant); /* ncl_printf() */
> -#endif
> KDTRACE_NFS_ATTRCACHE_GET_HIT(vp, vap);
> return (0);
> }
Unfortunately, these are probably still "needed". printf() serialization
using PRINTF_BUFR_SIZE or my method only works for individual printf()s
or possibly single lines. Just about any lock around a group of printf()s
serializes them as a group (only across subsystems using the same lock of
course).
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160803231538.U935>