Date: Fri, 27 Jan 2012 09:15:52 -0800 (PST) From: Doug Ambrisko <ambrisko@ambrisko.com> To: John Baldwin <jhb@freebsd.org> Cc: Kostik Belousov <kostikbel@gmail.com>, freebsd-current@freebsd.org Subject: Re: knlist_empty locking fix Message-ID: <201201271715.q0RHFqZc086859@ambrisko.com> In-Reply-To: <201201271013.55474.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
@@ -2509,9 +2509,12 @@ 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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201201271715.q0RHFqZc086859>