Re: knlist_empty locking fix

John Baldwin writes:
| On Friday, January 27, 2012 3:56:56 am Kostik Belousov wrote:
| > On Thu, Jan 26, 2012 at 01:03:26PM -0800, Doug Ambrisko wrote:
| > > Ran into problems with running kqueue/aio with WITNESS etc.  Sometimes
| > > things are locked sometimes not.  knlist_remove is called telling it
| > > whether it is locked or not ie:
| > > 	 extern void    knlist_remove(struct knlist *knl, struct knote *kn, 
| int islocked);
| > > so I changed:
| > > 	extern int     knlist_empty(struct knlist *knl);
| > > to:
| > > 	extern int     knlist_empty(struct knlist *knl, int islocked);
| > > 
| > > and then updated things to reflect that following what that state of the
| > > lock for knlist_remove.  If it is not locked, it gets a lock and 
| > > frees it after.
| > > 
| > > This now fixes a panic when a process using kqueue/aio is killed on
| > > shutdown with WITNESS.
| > > 
| > > It changes an API/ABI so it probably can't merged back.  If there are
| > > no objections then I'll commit it.
| > > 
| > Change to knlist_init() does not make sense at all, the knlist shall
| > not be exposed to other consumers during initialization, so no need
| > to exclude the parallel access.
| > 
| > Regarding the knlist_empty(), I propose to keep it as is. Locking
| > the knlist inside knlist_empty() does not make sense, because lock
| > is immediately dropped afterward, and relocked for remove. This way,
| > the entry could be removed from the list meantime (can it, really ?).
| > 
| > I think that you should take a lock around the whole if() {} statement,
| > and call knlist_remove with locked == 1.
| 
| Agreed, I think the missing locking should just be added to the aio code.

Okay so then just:

Index: vfs_aio.c
===================================================================
RCS file: /usr/local/cvsroot/freebsd/src/sys/kern/vfs_aio.c,v
retrieving revision 1.243.2.3.4.1
diff -u -p -r1.243.2.3.4.1 vfs_aio.c
--- vfs_aio.c	21 Dec 2010 17:09:25 -0000	1.243.2.3.4.1
+++ vfs_aio.c	27 Jan 2012 17:07:11 -0000
_at__at_ -2509,9 +2509,12 _at__at_ static void
 filt_aiodetach(struct knote *kn)
 {
 	struct aiocblist *aiocbe = kn->kn_ptr.p_aio;
+	struct knlist *knl = &aiocbe->klist;
 
-	if (!knlist_empty(&aiocbe->klist))
-		knlist_remove(&aiocbe->klist, kn, 0);
+	knl->kl_lock(knl->kl_lockarg);
+	if (!knlist_empty(knl))
+		knlist_remove(knl, kn, 1);
+	knl->kl_unlock(knl->kl_lockarg);
 }
 
 /* kqueue filter function */

I was trying to be consistant with knlist_remove but this is a much
smaller change that can be merge to older branches.

Thanks,

Doug A.