From 651ff543f83c6f5ef243e6aa25c72c84ed7e902f Mon Sep 17 00:00:00 2001 From: Rick Macklem Date: Wed, 27 Jan 2010 15:22:20 +0000 Subject: [PATCH] Fix a race that can occur when nfs nfsiod threads are being created. Without this patch it was possible for a different thread that calls nfs_asyncio() to snitch a newly created nfsiod thread that was intended for another caller of nfs_asyncio(), because the nfs_iod_mtx mutex was unlocked while the new nfsiod thread was created. This patch labels the newly created nfsiod, so that it is not taken by another caller of nfs_asyncio(). This is believed to fix the problem reported on the freebsd-stable email list under the subject: FreeBSD NFS client/Linux NFS server issue. Tested by: to DOT my DOT trociny AT gmail DOT com Reviewed by: jhb MFC after: 2 weeks --- sys/nfsclient/nfs.h | 2 +- sys/nfsclient/nfs_bio.c | 6 +++--- sys/nfsclient/nfs_nfsiod.c | 20 +++++++++++++------- sys/nfsclient/nfs_subs.c | 4 ++-- sys/nfsclient/nfs_vnops.c | 2 +- sys/nfsclient/nfsnode.h | 15 ++++++++++++++- 6 files changed, 34 insertions(+), 15 deletions(-) diff --git a/sys/nfsclient/nfs.h b/sys/nfsclient/nfs.h index 18eae3bae743..6f6e0d33ad50 100644 --- a/sys/nfsclient/nfs.h +++ b/sys/nfsclient/nfs.h @@ -252,7 +252,7 @@ int nfs_writerpc(struct vnode *, struct uio *, struct ucred *, int *, int nfs_commit(struct vnode *vp, u_quad_t offset, int cnt, struct ucred *cred, struct thread *td); int nfs_readdirrpc(struct vnode *, struct uio *, struct ucred *); -int nfs_nfsiodnew(void); +int nfs_nfsiodnew(int); int nfs_asyncio(struct nfsmount *, struct buf *, struct ucred *, struct thread *); int nfs_doio(struct vnode *, struct buf *, struct ucred *, struct thread *); void nfs_doio_directwrite (struct buf *); diff --git a/sys/nfsclient/nfs_bio.c b/sys/nfsclient/nfs_bio.c index 945bc51597de..cec0220b7f15 100644 --- a/sys/nfsclient/nfs_bio.c +++ b/sys/nfsclient/nfs_bio.c @@ -1377,7 +1377,7 @@ nfs_asyncio(struct nfsmount *nmp, struct buf *bp, struct ucred *cred, struct thr * Find a free iod to process this request. */ for (iod = 0; iod < nfs_numasync; iod++) - if (nfs_iodwant[iod]) { + if (nfs_iodwant[iod] == NFSIOD_AVAILABLE) { gotiod = TRUE; break; } @@ -1386,7 +1386,7 @@ nfs_asyncio(struct nfsmount *nmp, struct buf *bp, struct ucred *cred, struct thr * Try to create one if none are free. */ if (!gotiod) { - iod = nfs_nfsiodnew(); + iod = nfs_nfsiodnew(1); if (iod != -1) gotiod = TRUE; } @@ -1398,7 +1398,7 @@ nfs_asyncio(struct nfsmount *nmp, struct buf *bp, struct ucred *cred, struct thr */ NFS_DPF(ASYNCIO, ("nfs_asyncio: waking iod %d for mount %p\n", iod, nmp)); - nfs_iodwant[iod] = NULL; + nfs_iodwant[iod] = NFSIOD_NOT_AVAILABLE; nfs_iodmount[iod] = nmp; nmp->nm_bufqiods++; wakeup(&nfs_iodwant[iod]); diff --git a/sys/nfsclient/nfs_nfsiod.c b/sys/nfsclient/nfs_nfsiod.c index 3fafa1e2d94b..5302c5695a22 100644 --- a/sys/nfsclient/nfs_nfsiod.c +++ b/sys/nfsclient/nfs_nfsiod.c @@ -113,7 +113,7 @@ sysctl_iodmin(SYSCTL_HANDLER_ARGS) * than the new minimum, create some more. */ for (i = nfs_iodmin - nfs_numasync; i > 0; i--) - nfs_nfsiodnew(); + nfs_nfsiodnew(0); out: mtx_unlock(&nfs_iod_mtx); return (0); @@ -147,7 +147,7 @@ sysctl_iodmax(SYSCTL_HANDLER_ARGS) */ iod = nfs_numasync - 1; for (i = 0; i < nfs_numasync - nfs_iodmax; i++) { - if (nfs_iodwant[iod]) + if (nfs_iodwant[iod] == NFSIOD_AVAILABLE) wakeup(&nfs_iodwant[iod]); iod--; } @@ -160,7 +160,7 @@ SYSCTL_PROC(_vfs_nfs, OID_AUTO, iodmax, CTLTYPE_UINT | CTLFLAG_RW, 0, "Max number of nfsiod kthreads"); int -nfs_nfsiodnew(void) +nfs_nfsiodnew(int set_iodwant) { int error, i; int newiod; @@ -176,12 +176,17 @@ nfs_nfsiodnew(void) } if (newiod == -1) return (-1); + if (set_iodwant > 0) + nfs_iodwant[i] = NFSIOD_CREATED_FOR_NFS_ASYNCIO; mtx_unlock(&nfs_iod_mtx); error = kproc_create(nfssvc_iod, nfs_asyncdaemon + i, NULL, RFHIGHPID, 0, "nfsiod %d", newiod); mtx_lock(&nfs_iod_mtx); - if (error) + if (error) { + if (set_iodwant > 0) + nfs_iodwant[i] = NFSIOD_NOT_AVAILABLE; return (-1); + } nfs_numasync++; return (newiod); } @@ -199,7 +204,7 @@ nfsiod_setup(void *dummy) nfs_iodmin = NFS_MAXASYNCDAEMON; for (i = 0; i < nfs_iodmin; i++) { - error = nfs_nfsiodnew(); + error = nfs_nfsiodnew(0); if (error == -1) panic("nfsiod_setup: nfs_nfsiodnew failed"); } @@ -236,7 +241,8 @@ nfssvc_iod(void *instance) goto finish; if (nmp) nmp->nm_bufqiods--; - nfs_iodwant[myiod] = curthread->td_proc; + if (nfs_iodwant[myiod] == NFSIOD_NOT_AVAILABLE) + nfs_iodwant[myiod] = NFSIOD_AVAILABLE; nfs_iodmount[myiod] = NULL; /* * Always keep at least nfs_iodmin kthreads. @@ -303,7 +309,7 @@ nfssvc_iod(void *instance) nfs_asyncdaemon[myiod] = 0; if (nmp) nmp->nm_bufqiods--; - nfs_iodwant[myiod] = NULL; + nfs_iodwant[myiod] = NFSIOD_NOT_AVAILABLE; nfs_iodmount[myiod] = NULL; /* Someone may be waiting for the last nfsiod to terminate. */ if (--nfs_numasync == 0) diff --git a/sys/nfsclient/nfs_subs.c b/sys/nfsclient/nfs_subs.c index 329294bae30e..94cbe8448d45 100644 --- a/sys/nfsclient/nfs_subs.c +++ b/sys/nfsclient/nfs_subs.c @@ -347,7 +347,7 @@ nfs_init(struct vfsconf *vfsp) nfs_ticks = 1; /* Ensure async daemons disabled */ for (i = 0; i < NFS_MAXASYNCDAEMON; i++) { - nfs_iodwant[i] = NULL; + nfs_iodwant[i] = NFSIOD_NOT_AVAILABLE; nfs_iodmount[i] = NULL; } nfs_nhinit(); /* Init the nfsnode table */ @@ -375,7 +375,7 @@ nfs_uninit(struct vfsconf *vfsp) mtx_lock(&nfs_iod_mtx); nfs_iodmax = 0; for (i = 0; i < nfs_numasync; i++) - if (nfs_iodwant[i]) + if (nfs_iodwant[i] == NFSIOD_AVAILABLE) wakeup(&nfs_iodwant[i]); /* The last nfsiod to exit will wake us up when nfs_numasync hits 0 */ while (nfs_numasync) diff --git a/sys/nfsclient/nfs_vnops.c b/sys/nfsclient/nfs_vnops.c index 6b13bb4573c4..64953fe19cd3 100644 --- a/sys/nfsclient/nfs_vnops.c +++ b/sys/nfsclient/nfs_vnops.c @@ -212,7 +212,7 @@ static int nfs_renameit(struct vnode *sdvp, struct componentname *scnp, * Global variables */ struct mtx nfs_iod_mtx; -struct proc *nfs_iodwant[NFS_MAXASYNCDAEMON]; +enum nfsiod_state nfs_iodwant[NFS_MAXASYNCDAEMON]; struct nfsmount *nfs_iodmount[NFS_MAXASYNCDAEMON]; int nfs_numasync = 0; vop_advlock_t *nfs_advlock_p = nfs_dolock; diff --git a/sys/nfsclient/nfsnode.h b/sys/nfsclient/nfsnode.h index 402fc796023a..46d1bd6cf6c3 100644 --- a/sys/nfsclient/nfsnode.h +++ b/sys/nfsclient/nfsnode.h @@ -176,11 +176,24 @@ struct nfsnode { #define NFS_TIMESPEC_COMPARE(T1, T2) (((T1)->tv_sec != (T2)->tv_sec) || ((T1)->tv_nsec != (T2)->tv_nsec)) +/* + * NFS iod threads can be in one of these three states once spawned. + * NFSIOD_NOT_AVAILABLE - Cannot be assigned an I/O operation at this time. + * NFSIOD_AVAILABLE - Available to be assigned an I/O operation. + * NFSIOD_CREATED_FOR_NFS_ASYNCIO - Newly created for nfs_asyncio() and + * will be used by the thread that called nfs_asyncio(). + */ +enum nfsiod_state { + NFSIOD_NOT_AVAILABLE = 0, + NFSIOD_AVAILABLE = 1, + NFSIOD_CREATED_FOR_NFS_ASYNCIO = 2, +}; + /* * Queue head for nfsiod's */ extern TAILQ_HEAD(nfs_bufq, buf) nfs_bufq; -extern struct proc *nfs_iodwant[NFS_MAXASYNCDAEMON]; +extern enum nfsiod_state nfs_iodwant[NFS_MAXASYNCDAEMON]; extern struct nfsmount *nfs_iodmount[NFS_MAXASYNCDAEMON]; #if defined(_KERNEL)