svn commit: r303710 - head/sys/fs/nfsclient

[Bruce Evans]

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