From owner-freebsd-current@FreeBSD.ORG Tue Dec 9 23:15:09 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 B9D261065678 for ; Tue, 9 Dec 2008 23:15:09 +0000 (UTC) (envelope-from onemda@gmail.com) Received: from yw-out-2324.google.com (yw-out-2324.google.com [74.125.46.30]) by mx1.freebsd.org (Postfix) with ESMTP id 686058FC13 for ; Tue, 9 Dec 2008 23:15:09 +0000 (UTC) (envelope-from onemda@gmail.com) Received: by yw-out-2324.google.com with SMTP id 9so111014ywe.13 for ; Tue, 09 Dec 2008 15:15:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:message-id:date:from:to :subject:cc:in-reply-to:mime-version:content-type :content-transfer-encoding:content-disposition:references; bh=7FOOBsdDUcJsM5XhqlfDPbD2XRoPrTdR/0h0PATiYgM=; b=S7rbsmhxPaVxuONrr3eDmXeN5ARYJtEwgQd57YblC03049zspyK2g/O9gZEl5y8+tF 0IPJReM3eX6nlExUGYf4ixoyx9Iz1JcRAGmkxOXF4wmD7bXfJZJTCSUJMXqEj/JUhaUh fLjH+Xzg+2SmqvAMb8q04QbbMKXIgpxJbMReE= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=message-id:date:from:to:subject:cc:in-reply-to:mime-version :content-type:content-transfer-encoding:content-disposition :references; b=uUzRyeGulpxH5qytRGCdftwMqm2RsxBH6DfeZJCis28BOBJzK83a1ul94R+gXC1H7U Jp6iNCImhzZBzjEEZ2ZzfnvntBHayszEnokeZ03CHmN/0bgXgBka3j6HvhhN2qJWK59k RIQEldquaezD1Mb9YJBoWZ8FaXl8t/WPggQWo= Received: by 10.231.30.74 with SMTP id t10mr13415ibc.33.1228864507705; Tue, 09 Dec 2008 15:15:07 -0800 (PST) Received: by 10.231.15.194 with HTTP; Tue, 9 Dec 2008 15:15:07 -0800 (PST) Message-ID: <3a142e750812091515u3f2e5807j3652f2cc3551a3b2@mail.gmail.com> Date: Wed, 10 Dec 2008 00:15:07 +0100 From: "Paul B. Mahol" To: "John Baldwin" In-Reply-To: <200812091602.10850.jhb@freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <200811191510.53793.jhb@FreeBSD.org> <200812051608.50120.jhb@freebsd.org> <3a142e750812051354n747bcbcayb31d8d5f4cc15098@mail.gmail.com> <200812091602.10850.jhb@freebsd.org> 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 23:15:09 -0000 On 12/9/08, John Baldwin wrote: > 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 > No I do not, but I may try other CDs I have many of them, including FreeBSD one. If it is irrelevant than it should not be displayed. -- Paul