svn commit: r227549 - stable/7/sys/nfsclient
Rick Macklem
rmacklem at uoguelph.ca
Thu Nov 17 05:45:38 UTC 2011
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 at 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 at 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
More information about the svn-src-stable-7
mailing list