Date: Thu, 17 Nov 2011 00:16:34 -0500 (EST) From: Rick Macklem <rmacklem@uoguelph.ca> To: Alfred Perlstein <alfred@freebsd.org> Cc: svn-src-stable@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, svn-src-stable-7@freebsd.org, Rick Macklem <rmacklem@FreeBSD.org> Subject: Re: svn commit: r227549 - stable/7/sys/nfsclient Message-ID: <462306074.1954190.1321506994004.JavaMail.root@erie.cs.uoguelph.ca> In-Reply-To: <20111117040109.GU1455@elvis.mu.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <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?462306074.1954190.1321506994004.JavaMail.root>