From owner-p4-projects@FreeBSD.ORG Sat Mar 25 22:23:05 2006 Return-Path: X-Original-To: p4-projects@freebsd.org Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id 0C3AB16A423; Sat, 25 Mar 2006 22:23:05 +0000 (UTC) X-Original-To: perforce@freebsd.org Delivered-To: perforce@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id A9BA616A401 for ; Sat, 25 Mar 2006 22:23:04 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from repoman.freebsd.org (repoman.freebsd.org [216.136.204.115]) by mx1.FreeBSD.org (Postfix) with ESMTP id 2C53643D46 for ; Sat, 25 Mar 2006 22:23:04 +0000 (GMT) (envelope-from jhb@freebsd.org) Received: from repoman.freebsd.org (localhost [127.0.0.1]) by repoman.freebsd.org (8.13.1/8.13.1) with ESMTP id k2PMN4YI035350 for ; Sat, 25 Mar 2006 22:23:04 GMT (envelope-from jhb@freebsd.org) Received: (from perforce@localhost) by repoman.freebsd.org (8.13.1/8.13.1/Submit) id k2PMN3Wv035347 for perforce@freebsd.org; Sat, 25 Mar 2006 22:23:03 GMT (envelope-from jhb@freebsd.org) Date: Sat, 25 Mar 2006 22:23:03 GMT Message-Id: <200603252223.k2PMN3Wv035347@repoman.freebsd.org> X-Authentication-Warning: repoman.freebsd.org: perforce set sender to jhb@freebsd.org using -f From: John Baldwin To: Perforce Change Reviews Cc: Subject: PERFORCE change 94001 for review X-BeenThere: p4-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: p4 projects tree changes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 25 Mar 2006 22:23:05 -0000 http://perforce.freebsd.org/chv.cgi?CH=94001 Change 94001 by jhb@jhb_zion on 2006/03/25 22:22:54 Various VFS Giant locking for ndis, acct, alq, and ktrace. Affected files ... .. //depot/projects/smpng/sys/compat/ndis/subr_ndis.c#36 edit .. //depot/projects/smpng/sys/kern/kern_acct.c#42 edit .. //depot/projects/smpng/sys/kern/kern_alq.c#10 edit .. //depot/projects/smpng/sys/kern/kern_ktrace.c#50 edit Differences ... ==== //depot/projects/smpng/sys/compat/ndis/subr_ndis.c#36 (text+ko) ==== @@ -2899,7 +2899,7 @@ char *afilename = NULL; struct thread *td = curthread; struct nameidata nd; - int flags, error; + int flags, error, vfslocked; struct vattr vat; struct vattr *vap = &vat; ndis_fh *fh; @@ -2986,21 +2986,19 @@ snprintf(path, MAXPATHLEN, "%s/%s", ndis_filepath, afilename); - mtx_lock(&Giant); - /* Some threads don't have a current working directory. */ + /* XXX: locking? Which threads don't have these, kthreads maybe? */ if (td->td_proc->p_fd->fd_rdir == NULL) td->td_proc->p_fd->fd_rdir = rootvnode; if (td->td_proc->p_fd->fd_cdir == NULL) td->td_proc->p_fd->fd_cdir = rootvnode; - NDINIT(&nd, LOOKUP, FOLLOW, UIO_SYSSPACE, path, td); + NDINIT(&nd, LOOKUP, FOLLOW | MPSAFE, UIO_SYSSPACE, path, td); flags = FREAD; error = vn_open(&nd, &flags, 0, -1); if (error) { - mtx_unlock(&Giant); *status = NDIS_STATUS_FILE_NOT_FOUND; ExFreePool(fh); printf("NDIS: open file %s failed: %d\n", path, error); @@ -3008,6 +3006,7 @@ free(afilename, M_DEVBUF); return; } + vfslocked = NDHASGIANT(&nd); ExFreePool(path); @@ -3016,7 +3015,7 @@ /* Get the file size. */ VOP_GETATTR(nd.ni_vp, vap, td->td_ucred, td); VOP_UNLOCK(nd.ni_vp, 0, td); - mtx_unlock(&Giant); + VFS_UNLOCK_GIANT(vfslocked); fh->nf_vp = nd.ni_vp; fh->nf_map = NULL; @@ -3038,7 +3037,7 @@ struct thread *td = curthread; linker_file_t lf; caddr_t kldstart; - int error, resid; + int error, resid, vfslocked; if (filehandle == NULL) { *status = NDIS_STATUS_FAILURE; @@ -3076,10 +3075,10 @@ return; } - mtx_lock(&Giant); + vfslocked = VFS_LOCK_GIANT(vp->v_mount); error = vn_rdwr(UIO_READ, fh->nf_vp, fh->nf_map, fh->nf_maplen, 0, UIO_SYSSPACE, 0, td->td_ucred, NOCRED, &resid, td); - mtx_unlock(&Giant); + VFS_UNLOCK_GIANT(vfslocked); if (error) *status = NDIS_STATUS_FAILURE; @@ -3114,6 +3113,7 @@ { struct thread *td = curthread; ndis_fh *fh; + int vfslocked; if (filehandle == NULL) return; @@ -3129,9 +3129,9 @@ return; if (fh->nf_type == NDIS_FH_TYPE_VFS) { - mtx_lock(&Giant); + vfslocked = VFS_LOCK_GIANT(vp->v_mount); vn_close(fh->nf_vp, FREAD, td->td_ucred, td); - mtx_unlock(&Giant); + VFS_UNLOCK_GIANT(vfslocked); } fh->nf_vp = NULL; ==== //depot/projects/smpng/sys/kern/kern_acct.c#42 (text+ko) ==== @@ -159,7 +159,7 @@ acct(struct thread *td, struct acct_args *uap) { struct nameidata nd; - int error, flags; + int error, flags, vfslocked; /* Make sure that the caller is root. */ error = suser(td); @@ -168,38 +168,38 @@ /* * If accounting is to be started to a file, open that file for - * appending and make sure it's a 'normal'. While we could - * conditionally acquire Giant here, we're actually interacting with - * vnodes from possibly two file systems, making the logic a bit - * complicated. For now, use Giant unconditionally. + * appending and make sure it's a 'normal'. */ - mtx_lock(&Giant); if (uap->path != NULL) { - NDINIT(&nd, LOOKUP, NOFOLLOW, UIO_USERSPACE, uap->path, td); + NDINIT(&nd, LOOKUP, NOFOLLOW | MPSAFE, UIO_USERSPACE, + uap->path, td); flags = FWRITE | O_APPEND; error = vn_open(&nd, &flags, 0, -1); if (error) - goto done; + return (error); + vfslocked = NDHASGIANT(&nd); NDFREE(&nd, NDF_ONLY_PNBUF); #ifdef MAC error = mac_check_system_acct(td->td_ucred, nd.ni_vp); if (error) { VOP_UNLOCK(nd.ni_vp, 0, td); vn_close(nd.ni_vp, flags, td->td_ucred, td); - goto done; + VFS_UNLOCK_GIANT(vfslocked); + return (error); } #endif VOP_UNLOCK(nd.ni_vp, 0, td); if (nd.ni_vp->v_type != VREG) { vn_close(nd.ni_vp, flags, td->td_ucred, td); - error = EACCES; - goto done; + VFS_UNLOCK_GIANT(vfslocked); + return (EACCES); } + VFS_UNLOCK_GIANT(vfslocked); #ifdef MAC } else { error = mac_check_system_acct(td->td_ucred, NULL); if (error) - goto done; + return (error); #endif } @@ -216,15 +216,18 @@ * enabled. */ acct_suspended = 0; - if (acct_vp != NULL) + if (acct_vp != NULL) { + vfslocked = VFS_LOCK_GIANT(acct_vp->v_mount); error = acct_disable(td); + VFS_UNLOCK_GIANT(vfslocked); + } if (uap->path == NULL) { if (acct_state & ACCT_RUNNING) { acct_state |= ACCT_EXITREQ; wakeup(&acct_state); } sx_xunlock(&acct_sx); - goto done; + return (error); } /* @@ -245,20 +248,20 @@ error = kthread_create(acct_thread, NULL, NULL, 0, 0, "accounting"); if (error) { + vfslocked = VFS_LOCK_GIANT(acct_vp->v_mount); (void) vn_close(acct_vp, acct_flags, acct_cred, td); + VFS_UNLOCK_GIANT(vfslocked); crfree(acct_cred); acct_vp = NULL; acct_cred = NULL; acct_flags = 0; sx_xunlock(&acct_sx); log(LOG_NOTICE, "Unable to start accounting thread\n"); - goto done; + return (error); } } sx_xunlock(&acct_sx); log(LOG_NOTICE, "Accounting enabled\n"); -done: - mtx_unlock(&Giant); return (error); } ==== //depot/projects/smpng/sys/kern/kern_alq.c#10 (text+ko) ==== @@ -172,8 +172,6 @@ int needwakeup; struct alq *alq; - mtx_lock(&Giant); - ald_thread = FIRST_THREAD_IN_PROC(ald_proc); EVENTHANDLER_REGISTER(shutdown_pre_sync, ald_shutdown, NULL, @@ -250,6 +248,7 @@ struct ale *alstart; int totlen; int iov; + int vfslocked; vp = alq->aq_vp; td = curthread; @@ -291,6 +290,7 @@ /* * Do all of the junk required to write now. */ + vfslocked = VFS_LOCK_GIANT(vp->v_mount); vn_start_write(vp, &mp, V_WAIT); vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, td); VOP_LEASE(vp, td, alq->aq_cred, LEASE_WRITE); @@ -303,6 +303,7 @@ VOP_WRITE(vp, &auio, IO_UNIT | IO_APPEND, alq->aq_cred); VOP_UNLOCK(vp, 0, td); vn_finished_write(mp); + VFS_UNLOCK_GIANT(vfslocked); ALQ_LOCK(alq); alq->aq_flags &= ~AQ_FLUSHING; @@ -345,21 +346,23 @@ char *bufp; int flags; int error; - int i; + int i, vfslocked; *alqp = NULL; td = curthread; - NDINIT(&nd, LOOKUP, NOFOLLOW, UIO_SYSSPACE, file, td); + NDINIT(&nd, LOOKUP, NOFOLLOW | MPSAFE, UIO_SYSSPACE, file, td); flags = FWRITE | O_NOFOLLOW | O_CREAT; error = vn_open_cred(&nd, &flags, cmode, cred, -1); if (error) return (error); + vfslocked = NDHASGIANT(&nd); NDFREE(&nd, NDF_ONLY_PNBUF); /* We just unlock so we hold a reference */ VOP_UNLOCK(nd.ni_vp, 0, td); + VFS_UNLOCK_GIANT(vfslocked); alq = malloc(sizeof(*alq), M_ALD, M_WAITOK|M_ZERO); alq->aq_entbuf = malloc(count * size, M_ALD, M_WAITOK|M_ZERO); ==== //depot/projects/smpng/sys/kern/kern_ktrace.c#50 (text+ko) ==== @@ -598,7 +598,7 @@ int ops = KTROP(uap->ops); int descend = uap->ops & KTRFLAG_DESCEND; int nfound, ret = 0; - int flags, error = 0; + int flags, error = 0, vfslocked; struct nameidata nd; struct ucred *cred; @@ -613,25 +613,25 @@ /* * an operation which requires a file argument. */ - NDINIT(&nd, LOOKUP, NOFOLLOW, UIO_USERSPACE, uap->fname, td); + NDINIT(&nd, LOOKUP, NOFOLLOW | MPSAFE, UIO_USERSPACE, + uap->fname, td); flags = FREAD | FWRITE | O_NOFOLLOW; - mtx_lock(&Giant); error = vn_open(&nd, &flags, 0, -1); if (error) { - mtx_unlock(&Giant); ktrace_exit(td); return (error); } + vfslocked = NDHASGIANT(&nd); NDFREE(&nd, NDF_ONLY_PNBUF); vp = nd.ni_vp; VOP_UNLOCK(vp, 0, td); if (vp->v_type != VREG) { (void) vn_close(vp, FREAD|FWRITE, td->td_ucred, td); - mtx_unlock(&Giant); + VFS_UNLOCK_GIANT(vfslocked); ktrace_exit(td); return (EACCES); } - mtx_unlock(&Giant); + VFS_UNLOCK_GIANT(vfslocked); } /* * Clear all uses of the tracefile. @@ -649,10 +649,10 @@ p->p_traceflag = 0; mtx_unlock(&ktrace_mtx); PROC_UNLOCK(p); - mtx_lock(&Giant); + vfslocked = VFS_LOCK_GIANT(vp->v_mount); (void) vn_close(vp, FREAD|FWRITE, cred, td); - mtx_unlock(&Giant); + VFS_UNLOCK_GIANT(vfslocked); crfree(cred); } else { PROC_UNLOCK(p); @@ -732,9 +732,9 @@ error = EPERM; done: if (vp != NULL) { - mtx_lock(&Giant); + vfslocked = VFS_LOCK_GIANT(vp->v_mount); (void) vn_close(vp, FWRITE, td->td_ucred, td); - mtx_unlock(&Giant); + VFS_UNLOCK_GIANT(vfslocked); } ktrace_exit(td); return (error); @@ -888,7 +888,7 @@ struct iovec aiov[3]; struct mount *mp; int datalen, buflen, vrele_count; - int error; + int error, vfslocked; /* * We hold the vnode and credential for use in I/O in case ktrace is @@ -944,7 +944,7 @@ auio.uio_iovcnt++; } - mtx_lock(&Giant); + vfslocked = VFS_LOCK_GIANT(vp->v_mount); vn_start_write(vp, &mp, V_WAIT); vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, td); (void)VOP_LEASE(vp, td, cred, LEASE_WRITE); @@ -956,7 +956,7 @@ VOP_UNLOCK(vp, 0, td); vn_finished_write(mp); vrele(vp); - mtx_unlock(&Giant); + VFS_UNLOCK_GIANT(vfslocked); if (!error) return; /* @@ -1000,10 +1000,10 @@ * them but not yet committed them, as those are per-thread. The * thread will have to clear it itself on system call return. */ - mtx_lock(&Giant); + vfslocked = VFS_LOCK_GIANT(vp->v_mount); while (vrele_count-- > 0) vrele(vp); - mtx_unlock(&Giant); + VFS_UNLOCK_GIANT(vfslocked); } /*