Date: Wed, 10 Dec 2008 00:16:43 +0100 From: "Paul B. Mahol" <onemda@gmail.com> To: "John Baldwin" <jhb@freebsd.org> Cc: freebsd-current@freebsd.org Subject: Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660 Message-ID: <3a142e750812091516n6bb0880ewb298314e9ba296c@mail.gmail.com> In-Reply-To: <3a142e750812091515u3f2e5807j3652f2cc3551a3b2@mail.gmail.com> References: <200811191510.53793.jhb@FreeBSD.org> <200812051608.50120.jhb@freebsd.org> <3a142e750812051354n747bcbcayb31d8d5f4cc15098@mail.gmail.com> <200812091602.10850.jhb@freebsd.org> <3a142e750812091515u3f2e5807j3652f2cc3551a3b2@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 12/10/08, Paul B. Mahol <onemda@gmail.com> wrote: > On 12/9/08, John Baldwin <jhb@freebsd.org> wrote: >> On Friday 05 December 2008 04:54:23 pm Paul B. Mahol wrote: >>> On 12/5/08, John Baldwin <jhb@freebsd.org> wrote: >>> > On Friday 05 December 2008 03:56:31 pm Paul B. Mahol wrote: >>> >> On 12/5/08, John Baldwin <jhb@freebsd.org> 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 <jhb@freebsd.org> 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 s/Yes I do. Its to late here ... > one. > If it is irrelevant than it should not be displayed. -- Paul
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3a142e750812091516n6bb0880ewb298314e9ba296c>