Date: Fri, 27 Jan 2012 10:13:55 -0500 From: John Baldwin <jhb@freebsd.org> To: freebsd-current@freebsd.org Cc: Kostik Belousov <kostikbel@gmail.com> Subject: Re: knlist_empty locking fix Message-ID: <201201271013.55474.jhb@freebsd.org> In-Reply-To: <20120127085656.GY2726@deviant.kiev.zoral.com.ua> References: <201201262103.q0QL3QWr083496@ambrisko.com> <20120127085656.GY2726@deviant.kiev.zoral.com.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201201271013.55474.jhb>