From owner-freebsd-net@FreeBSD.ORG Fri Jul 22 13:12:06 2011 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 39C241065672; Fri, 22 Jul 2011 13:12:06 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from mail.zoral.com.ua (mx0.zoral.com.ua [91.193.166.200]) by mx1.freebsd.org (Postfix) with ESMTP id CAFE58FC08; Fri, 22 Jul 2011 13:12:05 +0000 (UTC) Received: from deviant.kiev.zoral.com.ua (root@deviant.kiev.zoral.com.ua [10.1.1.148]) by mail.zoral.com.ua (8.14.2/8.14.2) with ESMTP id p6MDBxh8081365 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 22 Jul 2011 16:11:59 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: from deviant.kiev.zoral.com.ua (kostik@localhost [127.0.0.1]) by deviant.kiev.zoral.com.ua (8.14.4/8.14.4) with ESMTP id p6MDBxYO025652; Fri, 22 Jul 2011 16:11:59 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: (from kostik@localhost) by deviant.kiev.zoral.com.ua (8.14.4/8.14.4/Submit) id p6MDBxi1025651; Fri, 22 Jul 2011 16:11:59 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: deviant.kiev.zoral.com.ua: kostik set sender to kostikbel@gmail.com using -f Date: Fri, 22 Jul 2011 16:11:59 +0300 From: Kostik Belousov To: John Baldwin Message-ID: <20110722131159.GR17489@deviant.kiev.zoral.com.ua> References: <44626428-CF14-4B20-AB57-6D4E8F4678AE@averesystems.com> <201107220855.10774.jhb@freebsd.org> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="Fcn+O7u6afXSKWdN" Content-Disposition: inline In-Reply-To: <201107220855.10774.jhb@freebsd.org> User-Agent: Mutt/1.4.2.3i X-Virus-Scanned: clamav-milter 0.95.2 at skuns.kiev.zoral.com.ua X-Virus-Status: Clean X-Spam-Status: No, score=-3.3 required=5.0 tests=ALL_TRUSTED,AWL,BAYES_00, DNS_FROM_OPENWHOIS autolearn=no version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on skuns.kiev.zoral.com.ua Cc: freebsd-net@freebsd.org, rmacklem@freebsd.org, Jeremiah Lott Subject: Re: LOR with nfsclient "sillyrename" X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 22 Jul 2011 13:12:06 -0000 --Fcn+O7u6afXSKWdN Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Jul 22, 2011 at 08:55:10AM -0400, John Baldwin wrote: > On Thursday, July 21, 2011 4:19:59 pm Jeremiah Lott wrote: > > We're seeing nfsclient deadlocks with what looks like lock order revers= al after removing a "silly rename". It is fairly rare, but we've seen it= =20 > happen a few times. I included relevant back traces from an occurrence. = From what I can see, nfs_inactive() is called with the vnode locked. If= =20 > there is a silly-rename, it will call vrele() on its parent directory, wh= ich can potentially try to lock the parent directory. Since this is the=20 > opposite order of the lock acquisition in lookup, it can deadlock. This = happened in a FreeBSD7 build, but I looked through freebsd head and=20 > didn't see any change that addressed this. Anyone seen this before? >=20 > I haven't seen this before, but your analysis looks correct to me. >=20 > Perhaps the best fix would be to defer the actual freeing of the sillyren= ame > to an asynchronous task? Maybe something like this (untested, uncompiled= ): >=20 > Index: nfsclient/nfsnode.h > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- nfsclient/nfsnode.h (revision 224254) > +++ nfsclient/nfsnode.h (working copy) > @@ -36,6 +36,7 @@ > #ifndef _NFSCLIENT_NFSNODE_H_ > #define _NFSCLIENT_NFSNODE_H_ > =20 > +#include > #if !defined(_NFSCLIENT_NFS_H_) && !defined(_KERNEL) > #include > #endif > @@ -45,8 +46,10 @@ > * can be removed by nfs_inactive() > */ > struct sillyrename { > + struct task s_task; > struct ucred *s_cred; > struct vnode *s_dvp; > + struct vnode *s_vp; > int (*s_removeit)(struct sillyrename *sp); > long s_namlen; > char s_name[32]; > Index: nfsclient/nfs_vnops.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- nfsclient/nfs_vnops.c (revision 224254) > +++ nfsclient/nfs_vnops.c (working copy) > @@ -1757,7 +1757,6 @@ > { > /* > * Make sure that the directory vnode is still valid. > - * XXX we should lock sp->s_dvp here. > */ > if (sp->s_dvp->v_type =3D=3D VBAD) > return (0); > @@ -2754,8 +2753,10 @@ > M_NFSREQ, M_WAITOK); > sp->s_cred =3D crhold(cnp->cn_cred); > sp->s_dvp =3D dvp; > + sp->s_vp =3D vp; > sp->s_removeit =3D nfs_removeit; > VREF(dvp); > + vhold(vp); > =20 > /*=20 > * Fudge together a funny name. > Index: nfsclient/nfs_node.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- nfsclient/nfs_node.c (revision 224254) > +++ nfsclient/nfs_node.c (working copy) > @@ -47,6 +47,7 @@ > #include > #include > #include > +#include > #include > =20 > #include > @@ -185,6 +186,26 @@ > return (0); > } > =20 > +static void > +nfs_freesillyrename(void *arg, int pending) > +{ > + struct sillyrename *sp; > + > + sp =3D arg; > + vn_lock(sp->s_dvp, LK_SHARED | LK_RETRY); I think taking an exclusive lock is somewhat more clean. > + vn_lock(sp->s_vp, LK_EXCLUSIVE | LK_RETRY); I believe that you have to verify that at least dvp is not doomed. Due to this, I propose to only move the vrele() call to taskqueue. > + (void)nfs_vinvalbuf(ap->a_vp, 0, td, 1); > + /* > + * Remove the silly file that was rename'd earlier > + */ > + (sp->s_removeit)(sp); > + crfree(sp->s_cred); > + vput(sp->s_dvp); > + VOP_UNLOCK(sp->s_vp, 0); > + vdrop(sp->s_vp); > + free((caddr_t)sp, M_NFSREQ); > +} > + > int > nfs_inactive(struct vop_inactive_args *ap) > { > @@ -200,15 +221,9 @@ > } else > sp =3D NULL; > if (sp) { > + TASK_INIT(&sp->task, 0, nfs_freesillyrename, sp); > + taskqueue_enqueue(taskqueue_thread, &sp->task); > mtx_unlock(&np->n_mtx); > - (void)nfs_vinvalbuf(ap->a_vp, 0, td, 1); > - /* > - * Remove the silly file that was rename'd earlier > - */ > - (sp->s_removeit)(sp); > - crfree(sp->s_cred); > - vrele(sp->s_dvp); > - free((caddr_t)sp, M_NFSREQ); > mtx_lock(&np->n_mtx); > } > np->n_flag &=3D NMODIFIED; >=20 > --=20 > John Baldwin > _______________________________________________ > freebsd-net@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-net > To unsubscribe, send any mail to "freebsd-net-unsubscribe@freebsd.org" --Fcn+O7u6afXSKWdN Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (FreeBSD) iEYEARECAAYFAk4pdx8ACgkQC3+MBN1Mb4gYwwCePg47CM8VYF1BG3zKgf3VPQ68 BnAAoOW3ff1Se7g+T3VgjdC+6zDGKRkY =pZXt -----END PGP SIGNATURE----- --Fcn+O7u6afXSKWdN--