Skip site navigation (1)Skip section navigation (2)
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>