Date: Wed, 21 Sep 2005 11:38:59 GMT From: Robert Watson <rwatson@FreeBSD.org> To: Perforce Change Reviews <perforce@freebsd.org> Subject: PERFORCE change 84050 for review Message-ID: <200509211138.j8LBcxij043298@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=84050 Change 84050 by rwatson@rwatson_zoo on 2005/09/21 11:38:53 In audit_record_write(), discussion of Giant isn't required anymore. Remove Giant frobbing from audit_worker(), where it is also no longer required. This simplifies things some. Affected files ... .. //depot/projects/trustedbsd/audit3/sys/security/audit/kern_audit.c#35 edit Differences ... ==== //depot/projects/trustedbsd/audit3/sys/security/audit/kern_audit.c#35 (text+ko) ==== @@ -516,13 +516,10 @@ /* * XXX - * This function must be called with Giant held, - * which is not optimal. We should break the write functionality - * away from the BSM record generation and have the BSM generation - * done before this function is called. This function will then - * take the BSM record as a parameter. - * - * XXXRW: In the new world order, this is no longer true. + * We should break the write functionality away from the BSM record + * generation and have the BSM generation done before this function + * is called. This function will then take the BSM record as a + * parameter. */ ret = (vn_rdwr(UIO_WRITE, vp, (void *)bsm->data, bsm->len, (off_t)0, UIO_SYSSPACE, IO_APPEND|IO_UNIT, cred, NULL, NULL, td)); @@ -556,13 +553,11 @@ * to a thread-local work queue. In addition, the audit_work performs the * actual exchange of audit log vnode pointer, as audit_vp is a thread-local * variable. - * - * XXXAUDIT: Giant is now less required here. */ static void audit_worker(void *arg) { - int do_replacement_signal, error, release_giant; + int do_replacement_signal, error; TAILQ_HEAD(, kaudit_record) ar_worklist; struct kaudit_record *ar; struct vnode *audit_vp, *old_vp; @@ -584,10 +579,9 @@ while (1) { /* * First priority: replace the audit log target if requested. - * As we actually close the vnode in the worker thread, we - * need to grab Giant, which means releasing audit_mtx. - * In case another replacement was scheduled while the mutex - * we released, we loop. + * Accessing the vnode here requires dropping the audit_mtx; + * in case another replacement was scheduled while the mutex + * was released, we loop. * * XXX It could well be we should drain existing records * first to ensure that the timestamps and ordering @@ -605,20 +599,16 @@ audit_enabled = (audit_vp != NULL); - if (old_vp != NULL || audit_vp != NULL) { - mtx_unlock(&audit_mtx); - mtx_lock(&Giant); - release_giant = 1; - } else - release_giant = 0; /* * XXX: What to do about write failures here? */ if (old_vp != NULL) { AUDIT_PRINTF(("Closing old audit file\n")); + mtx_unlock(&audit_mtx); vn_close(old_vp, audit_close_flags, old_cred, audit_td); crfree(old_cred); + mtx_lock(&audit_mtx); old_cred = NULL; old_vp = NULL; AUDIT_PRINTF(("Audit file closed\n")); @@ -626,10 +616,6 @@ if (audit_vp != NULL) { AUDIT_PRINTF(("Opening new audit file\n")); } - if (release_giant) { - mtx_unlock(&Giant); - mtx_lock(&audit_mtx); - } do_replacement_signal = 1; } /* @@ -711,20 +697,9 @@ } mtx_unlock(&audit_mtx); - release_giant = 0; while ((ar = TAILQ_FIRST(&ar_worklist))) { TAILQ_REMOVE(&ar_worklist, ar, k_q); if (audit_vp != NULL) { - /* - * XXX: What should happen if there's a write - * error here? - */ - if (!release_giant) { - mtx_lock(&Giant); - release_giant = 1; - } - VOP_LEASE(audit_vp, audit_td, audit_cred, - LEASE_WRITE); error = audit_record_write(audit_vp, ar, audit_cred, audit_td); if (error && audit_panic_on_write_fail) @@ -736,8 +711,6 @@ } audit_free(ar); } - if (release_giant) - mtx_unlock(&Giant); mtx_lock(&audit_mtx); } }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200509211138.j8LBcxij043298>