Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660

On 12/10/08, Paul B. Mahol <onemda_at_gmail.com> wrote:
> On 12/10/08, Paul B. Mahol <onemda_at_gmail.com> wrote:
>> On 12/9/08, John Baldwin <jhb_at_freebsd.org> wrote:
>>> On Friday 05 December 2008 04:54:23 pm Paul B. Mahol wrote:
>>>> On 12/5/08, John Baldwin <jhb_at_freebsd.org> wrote:
>>>> > 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
>>>> >
>>>>
>>>> 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 ...
Ignore that reply.

cd9660.ko without patch doesnt have this issue.
cd9660.ko with cd9660_mpsafe.patch have this issue, on every CD I tried.

-- 
Paul