Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 16 Nov 2011 20:01:09 -0800
From:      Alfred Perlstein <alfred@freebsd.org>
To:        Rick Macklem <rmacklem@FreeBSD.org>
Cc:        svn-src-stable@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, svn-src-stable-7@freebsd.org
Subject:   Re: svn commit: r227549 - stable/7/sys/nfsclient
Message-ID:  <20111117040109.GU1455@elvis.mu.org>
In-Reply-To: <201111160505.pAG55D5R032343@svn.freebsd.org>
References:  <201111160505.pAG55D5R032343@svn.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
Rick, I have a question, what will happen if the nfs_freesillyrename()
call happens when the mount is down?  Will it block the taskqueue_thread?

If so, it might make more sense to make a taskqueue per mount point.

If not, excuse me. :-)

-Alfred

* Rick Macklem <rmacklem@FreeBSD.org> [111115 21:05] wrote:
> Author: rmacklem
> Date: Wed Nov 16 05:05:13 2011
> New Revision: 227549
> URL: http://svn.freebsd.org/changeset/base/227549
> 
> Log:
>   MFC: r224604
>   Fix a LOR in the NFS client which could cause a deadlock.
>   This was reported to the mailing list freebsd-net@freebsd.org
>   on July 21, 2011 under the subject "LOR with nfsclient sillyrename".
>   The LOR occurred when nfs_inactive() called vrele(sp->s_dvp)
>   while holding the vnode lock on the file in s_dvp. This patch
>   modifies the client so that it performs the vrele(sp->s_dvp)
>   as a separate task to avoid the LOR. This fix was discussed
>   with jhb@ and kib@, who both proposed variations of it.
> 
> Modified:
>   stable/7/sys/nfsclient/nfs_node.c
>   stable/7/sys/nfsclient/nfsnode.h
> Directory Properties:
>   stable/7/sys/   (props changed)
>   stable/7/sys/cddl/contrib/opensolaris/   (props changed)
>   stable/7/sys/contrib/dev/acpica/   (props changed)
>   stable/7/sys/contrib/pf/   (props changed)
> 
> Modified: stable/7/sys/nfsclient/nfs_node.c
> ==============================================================================
> --- stable/7/sys/nfsclient/nfs_node.c	Wed Nov 16 02:52:24 2011	(r227548)
> +++ stable/7/sys/nfsclient/nfs_node.c	Wed Nov 16 05:05:13 2011	(r227549)
> @@ -45,6 +45,7 @@ __FBSDID("$FreeBSD$");
>  #include <sys/proc.h>
>  #include <sys/socket.h>
>  #include <sys/sysctl.h>
> +#include <sys/taskqueue.h>
>  #include <sys/vnode.h>
>  
>  #include <vm/uma.h>
> @@ -59,6 +60,8 @@ __FBSDID("$FreeBSD$");
>  
>  static uma_zone_t nfsnode_zone;
>  
> +static void	nfs_freesillyrename(void *arg, __unused int pending);
> +
>  #define TRUE	1
>  #define	FALSE	0
>  
> @@ -191,6 +194,20 @@ nfs_nget(struct mount *mntp, nfsfh_t *fh
>  	return (0);
>  }
>  
> +/*
> + * Do the vrele(sp->s_dvp) as a separate task in order to avoid a
> + * deadlock because of a LOR when vrele() locks the directory vnode.
> + */
> +static void
> +nfs_freesillyrename(void *arg, __unused int pending)
> +{
> +	struct sillyrename *sp;
> +
> +	sp = arg;
> +	vrele(sp->s_dvp);
> +	free(sp, M_NFSREQ);
> +}
> +
>  int
>  nfs_inactive(struct vop_inactive_args *ap)
>  {
> @@ -213,8 +230,8 @@ nfs_inactive(struct vop_inactive_args *a
>  		 */
>  		(sp->s_removeit)(sp);
>  		crfree(sp->s_cred);
> -		vrele(sp->s_dvp);
> -		FREE((caddr_t)sp, M_NFSREQ);
> +		TASK_INIT(&sp->s_task, 0, nfs_freesillyrename, sp);
> +		taskqueue_enqueue(taskqueue_thread, &sp->s_task);
>  	}
>  	np->n_flag &= NMODIFIED;
>  	return (0);
> 
> Modified: stable/7/sys/nfsclient/nfsnode.h
> ==============================================================================
> --- stable/7/sys/nfsclient/nfsnode.h	Wed Nov 16 02:52:24 2011	(r227548)
> +++ stable/7/sys/nfsclient/nfsnode.h	Wed Nov 16 05:05:13 2011	(r227549)
> @@ -36,6 +36,7 @@
>  #ifndef _NFSCLIENT_NFSNODE_H_
>  #define _NFSCLIENT_NFSNODE_H_
>  
> +#include <sys/_task.h>
>  #if !defined(_NFSCLIENT_NFS_H_) && !defined(_KERNEL)
>  #include <nfs/nfs.h>
>  #endif
> @@ -45,6 +46,7 @@
>   * can be removed by nfs_inactive()
>   */
>  struct sillyrename {
> +	struct	task s_task;
>  	struct	ucred *s_cred;
>  	struct	vnode *s_dvp;
>  	int	(*s_removeit)(struct sillyrename *sp);

-- 
- Alfred Perlstein
.- VMOA #5191, 03 vmax, 92 gs500, 85 ch250, 07 zx10
.- FreeBSD committer



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20111117040109.GU1455>