Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660

On 12/10/08, Paul B. Mahol <onemda@xxxxxxxxx> wrote:
On 12/9/08, John Baldwin <jhb@xxxxxxxxxxx> wrote:
On Friday 05 December 2008 04:54:23 pm Paul B. Mahol wrote:
On 12/5/08, John Baldwin <jhb@xxxxxxxxxxx> wrote:
On Friday 05 December 2008 03:56:31 pm Paul B. Mahol wrote:
On 12/5/08, John Baldwin <jhb@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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
_______________________________________________
freebsd-current@xxxxxxxxxxx mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@xxxxxxxxxxx"

Relevant Pages

  • Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660
    ... With this patch I'm unable to kldunload libiconv.ko once it is ... John Baldwin ... lock order reversal: ... Do you get the same messages w/o the patch with the same ISO image / ...
    (freebsd-current)
  • Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660
    ... With this patch I'm unable to kldunload libiconv.ko once it is ... getting unloaded after 'foo' gets unloaded and using a destroyed lock ... John Baldwin ... Do you get the same messages w/o the patch with the same ISO image / ...
    (freebsd-current)
  • Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660
    ... With this patch I'm unable to kldunload libiconv.ko once it is ... John Baldwin ... but without success. ... lock order reversal: ...
    (freebsd-current)
  • Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660
    ... With this patch I'm unable to kldunload libiconv.ko once it is ... John Baldwin ... but without success. ... lock order reversal: ...
    (freebsd-current)
  • [ANNOUNCE] 2.6.31-rc4-rt1
    ... This is a major rework of the rt patch series. ... interrupt threading is now a pure extension of the mainline ... type in the lock definition. ...
    (Linux-Kernel)