From owner-freebsd-current@FreeBSD.ORG Fri Jan 27 17:15:51 2012 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 3CCC0106564A for ; Fri, 27 Jan 2012 17:15:51 +0000 (UTC) (envelope-from ambrisko@ambrisko.com) Received: from mail.ambrisko.com (mail.ambrisko.com [70.91.206.90]) by mx1.freebsd.org (Postfix) with ESMTP id 18CD18FC14 for ; Fri, 27 Jan 2012 17:15:50 +0000 (UTC) X-Ambrisko-Me: Yes Received: from server2.ambrisko.com (HELO internal.ambrisko.com) ([192.168.1.2]) by ironport.ambrisko.com with ESMTP; 27 Jan 2012 09:15:53 -0800 Received: from ambrisko.com (localhost [127.0.0.1]) by internal.ambrisko.com (8.14.4/8.14.4) with ESMTP id q0RHFq4O086860; Fri, 27 Jan 2012 09:15:52 -0800 (PST) (envelope-from ambrisko@ambrisko.com) Received: (from ambrisko@localhost) by ambrisko.com (8.14.4/8.14.4/Submit) id q0RHFqZc086859; Fri, 27 Jan 2012 09:15:52 -0800 (PST) (envelope-from ambrisko) From: Doug Ambrisko Message-Id: <201201271715.q0RHFqZc086859@ambrisko.com> In-Reply-To: <201201271013.55474.jhb@freebsd.org> To: John Baldwin Date: Fri, 27 Jan 2012 09:15:52 -0800 (PST) X-Mailer: ELM [version 2.4ME+ PL124d (25)] MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="US-ASCII" Cc: Kostik Belousov , freebsd-current@freebsd.org Subject: Re: knlist_empty locking fix X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 27 Jan 2012 17:15:51 -0000 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.