From owner-svn-src-all@FreeBSD.ORG Thu Nov 17 05:45:38 2011 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 4708E1065672; Thu, 17 Nov 2011 05:45:38 +0000 (UTC) (envelope-from rmacklem@uoguelph.ca) Received: from esa-jnhn.mail.uoguelph.ca (esa-jnhn.mail.uoguelph.ca [131.104.91.44]) by mx1.freebsd.org (Postfix) with ESMTP id ADE6E8FC0C; Thu, 17 Nov 2011 05:45:37 +0000 (UTC) X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: Ap4EAAiYxE6DaFvO/2dsb2JhbABChQGmCoFyAQEFIwRHCxsUBAICDRkCWQYTiAqgeZF6gTCCKYUogRYEiBSMIJIa X-IronPort-AV: E=Sophos;i="4.69,524,1315195200"; d="scan'208";a="145808292" Received: from erie.cs.uoguelph.ca (HELO zcs3.mail.uoguelph.ca) ([131.104.91.206]) by esa-jnhn-pri.mail.uoguelph.ca with ESMTP; 17 Nov 2011 00:16:34 -0500 Received: from zcs3.mail.uoguelph.ca (localhost.localdomain [127.0.0.1]) by zcs3.mail.uoguelph.ca (Postfix) with ESMTP id 0ADE5B3F25; Thu, 17 Nov 2011 00:16:34 -0500 (EST) Date: Thu, 17 Nov 2011 00:16:34 -0500 (EST) From: Rick Macklem To: Alfred Perlstein Message-ID: <462306074.1954190.1321506994004.JavaMail.root@erie.cs.uoguelph.ca> In-Reply-To: <20111117040109.GU1455@elvis.mu.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Originating-IP: [172.17.91.201] X-Mailer: Zimbra 6.0.10_GA_2692 (ZimbraWebClient - FF3.0 (Win)/6.0.10_GA_2692) Cc: svn-src-stable@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, svn-src-stable-7@freebsd.org, Rick Macklem Subject: Re: svn commit: r227549 - stable/7/sys/nfsclient X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 17 Nov 2011 05:45:38 -0000 Alfred Perlstein wrote: > 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. :-) > Well, all nfs_freesillyrename() does is a vrele() on the parent directory when the file node's use count has gone to 0. I can't think why that would do any RPC, so I don't see a problem? If you do see a problem with vrele() on the directory, please let me know. The problem this fixes is a LOR that would occur when the vrele() on the directory was done by the thread doing VOP_INACTIVE(), since it already has the file vnode lock and the vrele() was locking the parent directory. This could cause a fairly rare deadlock. rick > -Alfred > > * Rick Macklem [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 > > #include > > #include > > +#include > > #include > > > > #include > > @@ -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 > > #if !defined(_NFSCLIENT_NFS_H_) && !defined(_KERNEL) > > #include > > #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