From owner-freebsd-current@FreeBSD.ORG Tue Dec 9 21:26:10 2008 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 0C23E106574A for ; Tue, 9 Dec 2008 21:26:09 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from server.baldwin.cx (bigknife-pt.tunnel.tserv9.chi1.ipv6.he.net [IPv6:2001:470:1f10:75::2]) by mx1.freebsd.org (Postfix) with ESMTP id 15E548FC0C for ; Tue, 9 Dec 2008 21:26:08 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from localhost.corp.yahoo.com (john@localhost [IPv6:::1]) (authenticated bits=0) by server.baldwin.cx (8.14.3/8.14.3) with ESMTP id mB9LPpFu014957; Tue, 9 Dec 2008 16:26:02 -0500 (EST) (envelope-from jhb@freebsd.org) From: John Baldwin To: "Paul B. Mahol" Date: Tue, 9 Dec 2008 16:02:10 -0500 User-Agent: KMail/1.9.7 References: <200811191510.53793.jhb@FreeBSD.org> <200812051608.50120.jhb@freebsd.org> <3a142e750812051354n747bcbcayb31d8d5f4cc15098@mail.gmail.com> In-Reply-To: <3a142e750812051354n747bcbcayb31d8d5f4cc15098@mail.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200812091602.10850.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH authentication, not delayed by milter-greylist-2.0.2 (server.baldwin.cx [IPv6:::1]); Tue, 09 Dec 2008 16:26:03 -0500 (EST) X-Virus-Scanned: ClamAV 0.94.2/8738/Tue Dec 9 14:31:40 2008 on server.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-2.6 required=4.2 tests=AWL,BAYES_00,NO_RELAYS autolearn=ham version=3.1.3 X-Spam-Checker-Version: SpamAssassin 3.1.3 (2006-06-01) on server.baldwin.cx Cc: freebsd-current@freebsd.org Subject: Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660 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: Tue, 09 Dec 2008 21:26:10 -0000 On Friday 05 December 2008 04:54:23 pm Paul B. Mahol wrote: > On 12/5/08, John Baldwin wrote: > > On Friday 05 December 2008 03:56:31 pm Paul B. Mahol wrote: > >> On 12/5/08, John Baldwin wrote: > >> > On Thursday 20 November 2008 05:47:28 pm John Baldwin wrote: > >> >> On Thursday 20 November 2008 04:30:57 pm Paul B. Mahol wrote: > >> >> > On 11/19/08, John Baldwin wrote: > >> >> > > This is a relatively simple patch to mark cd9660 MPSAFE and enable > >> > shared > >> >> > > lookups. The changes to cd9660_lookup() mirror similar changes to > >> >> > > ufs_lookup() to use static variables for local data rather than > >> >> > > abusing > >> >> > > i-node members of the parent directory. I've done some light > >> >> > > testing > >> >> > > of > >> >> > > this, but not super-strenuous. This patch also includes simple > >> >> > > locking > >> >> for > >> >> > > the iconv support in the kernel. That locking uses an sx lock to > >> >> serialize > >> >> > > open and close of translator tables and the associated refcount. > >> >> > > Actual > >> >> > > conversions do not need any locks, however as the mount holds a > >> > reference > >> >> on > >> >> > > the table. > >> >> > > > >> >> > > http://www.FreeBSD.org/~jhb/patches/cd9660_mpsafe.patch > >> >> > > > >> >> > > >> >> > With this patch I'm unable to kldunload libiconv.ko once it is > >> >> > loaded. > >> >> > And trying to kldunload libiconv.ko will make any next > >> >> kldload/kldstat/kldunload > >> >> > to fail waiting forever(livelock). > >> >> > > >> >> > Regression were not encountered while only cd9660.ko were kldloaded. > >> >> > >> >> So this is actually due to a bug in the module code. If you have two > >> > modules > >> >> like this: > >> >> > >> >> DECLARE_MODULE(foo, SI_SUB_DRIVERS, SI_ORDER_FIRST); > >> >> DECLARE_MODULE(bar, SI_SUB_DRIVERS, SI_ORDER_SECOND); > >> >> > >> >> The SI_* constants ensure that foo's module handler is called before > > bar's > >> >> > >> >> module handler for MOD_LOAD. However, we don't enforce a reverse order > >> >> (bar > >> >> then foo) for MOD_UNLOAD. In fact, the order of MOD_UNLOAD events is > >> >> random > >> >> and has no relation to the SI_* constants. :( > >> >> > >> >> What is happening here is that one of the 'bar' modules in libiconv.ko > >> >> is > >> >> getting unloaded after 'foo' gets unloaded and using a destroyed lock > > (you > >> >> > >> >> get a panic if you run with INVARIANTS). > >> > > >> > So this should now be fixed with this commit. If you could verify that > >> > iconv > >> > works ok with the latest kern_module.c I would appreciate it. > >> > > >> > Author: jhb > >> > Date: Fri Dec 5 16:47:30 2008 > >> > New Revision: 185642 > >> > URL: http://svn.freebsd.org/changeset/base/185642 > >> > > >> > Log: > >> > When the SYSINIT() to load a module invokes the MOD_LOAD event > >> > successfully, > >> > move that module to the head of the associated linker file's list of > >> > modules. > >> > The end result is that once all the modules are loaded, they are > >> > sorted > > in > >> > the reverse of their load order. This causes the kernel linker to > > invoke > >> > the MOD_QUIESCE and MOD_UNLOAD events in the reverse of the order that > >> > MOD_LOAD was invoked. This means that the ordering of MOD_LOAD events > >> > that > >> > is set by the SI_* paramters to DECLARE_MODULE() are now honored in > >> > the > >> > same > >> > order they would be for SYSUNINIT() for the MOD_QUIESCE and MOD_UNLOAD > >> > events. > >> > > >> > MFC after: 1 month > >> > > >> > -- > >> > John Baldwin > >> > > >> > >> Yes it works, I tried hard multiple times kldload/kldunload > >> {libiconv,cd9660,cd9660_iconv in various order} to livelock/panic it, > >> but without success. > >> > >> FYI following LORs happened: > >> > >> lock order reversal: > >> 1st 0xc4322ce8 isofs (isofs) @ /usr/src/sys/kern/vfs_lookup.c:442 > >> 2nd 0xd7d8d740 bufwait (bufwait) @ /usr/src/sys/kern/vfs_bio.c:2443 > >> 3rd 0xc4322bdc isofs (isofs) @ > >> /usr/src/sys/modules/cd9660/../../fs/cd9660/cd9660_vfsops.c:694 > > > > This LOR should be addressed in the latest cd9660 locking patches. > > > > -- > > John Baldwin > > > > Oh, why I did not checked new version? > > Yes that LOR have gone, but when doing "ll -R" first time on /mnt > I got following messages from kernel: > > RRIP without PX field? x ~ 50 times. > > I see you changed LK_EXCLUSIVE to flags, and with MPSAFE .... The RRIP stuff is all done in cd9660_vget_internal() under an exclusive lock. It could be a property of the ISO image. "PX" holds permissions (owner, etc.). Do you get the same messages w/o the patch with the same ISO image / CD? -- John Baldwin