Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660

On Friday 05 December 2008 03:56:31 pm Paul B. Mahol wrote:
> On 12/5/08, John Baldwin <jhb_at_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_at_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) _at_ /usr/src/sys/kern/vfs_lookup.c:442
>  2nd 0xd7d8d740 bufwait (bufwait) _at_ /usr/src/sys/kern/vfs_bio.c:2443
>  3rd 0xc4322bdc isofs (isofs) _at_
> /usr/src/sys/modules/cd9660/../../fs/cd9660/cd9660_vfsops.c:694

This LOR should be addressed in the latest cd9660 locking patches.

-- 
John Baldwin