Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660

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
KDB: stack backtrace:
db_trace_self_wrapper(c061bff9,c3b566c0,c04e75e5,4,c0617724,...) at
db_trace_self_wrapper+0x26
kdb_backtrace(4,c0617724,c3c91468,c3c93828,c3b5671c,...) at kdb_backtrace+0x29
_witness_debugger(c061ecc6,c4322bdc,c44e75c3,c3c93828,c44e7507,...) at
_witness_debugger+0x25
witness_checkorder(c4322bdc,9,c44e7507,2b6,0,...) at witness_checkorder+0x839
__lockmgr_args(c4322bdc,80000,0,0,0,...) at __lockmgr_args+0x797
cd9660_vget_internal(c3feea00,d000,200000,c3b568a8,0,...) at
cd9660_vget_internal+0x147
cd9660_lookup(c3b569f8,c4322c90,c3b56bb0,c4322c90,c3b56a18,...) at
cd9660_lookup+0x4c1
VOP_CACHEDLOOKUP_APV(c44e8a20,c3b569f8,c3b56bb0,c3b56b9c,c3fd1b00,...)
at VOP_CACHEDLOOKUP_APV+0xa5
vfs_cache_lookup(c3b56a78,c3b56a78,5000104,200000,c4322c90,...) at
vfs_cache_lookup+0xcc
VOP_LOOKUP_APV(c44e8a20,c3b56a78,c06249d7,1ba,c3b56b9c,...) at
VOP_LOOKUP_APV+0xa5
lookup(c3b56b84,c06249d7,dc,bc,c410482c,...) at lookup+0x51e
namei(c3b56b84,c108ba38,c108ba34,c3c5e000,c3b56b2c,...) at namei+0x48b
kern_statat(c442d480,200,ffffff9c,282162f8,0,...) at kern_statat+0x6b
kern_lstat(c442d480,282162f8,0,c3b56c18,0,...) at kern_lstat+0x36
lstat(c442d480,c3b56cf8,8,c061f959,c0647af0,...) at lstat+0x2f
syscall(c3b56d38) at syscall+0x283
Xint0x80_syscall() at Xint0x80_syscall+0x20
--- syscall (190, FreeBSD ELF32, lstat), eip = 0x281ac413, esp =
0xbfbfe5ac, ebp = 0xbfbfe638 ---
lock order reversal:
 1st 0xc440fbdc ufs (ufs) _at_ /usr/src/sys/kern/vfs_mount.c:1207
 2nd 0xc440fad0 devfs (devfs) _at_ /usr/src/sys/kern/vfs_subr.c:2179
KDB: stack backtrace:
db_trace_self_wrapper(c061bff9,c3b50a38,c04e75e5,4,c0617724,...) at
db_trace_self_wrapper+0x26
kdb_backtrace(4,c0617724,c3c934e8,c3c93418,c3b50a94,...) at kdb_backtrace+0x29
_witness_debugger(c061ecad,c440fad0,c0610530,c3c93418,c06252f3,...) at
_witness_debugger+0x25
witness_checkorder(c440fad0,9,c06252f3,883,0,...) at witness_checkorder+0x839
__lockmgr_args(c440fad0,80100,c440faec,0,0,...) at __lockmgr_args+0x797
vop_stdlock(c3b50b9c,4,c0617724,80100,c440fa78,...) at vop_stdlock+0x62
VOP_LOCK1_APV(c0644760,c3b50b9c,c067fa18,c066aac0,c440fa78,...) at
VOP_LOCK1_APV+0xa5
_vn_lock(c440fa78,80100,c06252f3,883,c44e7507,...) at _vn_lock+0x5e
vrele(c440fa78,0,c44e7507,208,0,...) at vrele+0x142
cd9660_unmount(c3feea00,8000000,c416d240,4fc,0,...) at cd9660_unmount+0xed
dounmount(c3feea00,8000000,c416d240,482,3,...) at dounmount+0x482
unmount(c416d240,c3b50cf8,8,c416d240,c0646b30,...) at unmount+0x2bf
syscall(c3b50d38) at syscall+0x283
Xint0x80_syscall() at Xint0x80_syscall+0x20
--- syscall (22, FreeBSD ELF32, unmount), eip = 0x280d1e7f, esp =
0xbfbfe5bc, ebp = 0xbfbfe688 ---

-- 
Paul