Home » Mailing list archives » freebsd-current » 2008 » December » 5

Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660

Previous thread: [PATCH] ppbus/ppc locking by John Baldwin on Wednesday, November 19, 2008 - 1:03 pm. (11 messages)

Next thread: atrtc0: Warnings about mappings of I/O and interrupt by Roman Divacky on Thursday, November 20, 2008 - 10:13 am. (7 messages)
[view original message]
From: John Baldwin
Subject: [PATCH] MPSAFE/LOOKUP_SHARED cd9660
Date: Wednesday, November 19, 2008 - 1:10 pm

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

-- 
John Baldwin
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"

[ message continues ]
[view original message]
From: Paul B. Mahol
Subject: Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660
Date: Thursday, November 20, 2008 - 2:30 pm

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.

BTW:

Machine crashed during clean shutdown (with old kernel without this patch)
after atapicd where kldloaded and after that used some time and tham
kldunloaded.
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"

[ message continues ]
[view original message]
From: John Baldwin
Subject: Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660
Date: Thursday, November 20, 2008 - 3:31 pm

-- 
John Baldwin
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"

[ message continues ]
[view original message]
From: John Baldwin
Subject: Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660
Date: Thursday, November 20, 2008 - 3:47 pm

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).

-- 
John Baldwin
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"

[ message continues ]
[view original message]
From: Paul B. Mahol
Subject: Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660
Date: Thursday, November 20, 2008 - 4:26 pm

Yes, I tested it on kernel without ddb and friends.
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"

[ message continues ]
[view original message]
From: John Baldwin
Subject: Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660
Date: Friday, December 5, 2008 - 10:06 am

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
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"

[ message continues ]
[view original message]
From: Paul B. Mahol
Subject: Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660
Date: Friday, December 5, 2008 - 1:56 pm

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
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) @ /usr/src/sys/kern/vfs_mount.c:1207
 2nd 0xc440fad0 devfs (devfs) @ /usr/src/sys/kern/vfs_subr.c:2179
KDB: stack ...
[ message continues ]
[view original message]
From: John Baldwin
Subject: Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660
Date: Friday, December 5, 2008 - 2:08 pm

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

-- 
John Baldwin
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"

[ message continues ]
[view original message]
From: Paul B. Mahol
Subject: Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660
Date: Friday, December 5, 2008 - 2:54 pm

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 ....

-- 
Paul
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"

[ message continues ]
[view original message]
From: John Baldwin
Subject: Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660
Date: Tuesday, December 9, 2008 - 2:02 pm

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
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"

[ message continues ]
[view original message]
From: Paul B. Mahol
Subject: Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660
Date: Tuesday, December 9, 2008 - 4:15 pm

No I do not, but I may try other CDs I have many of them, including FreeBSD one.
If it is irrelevant than it should not be displayed.

-- 
Paul
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"

[ message continues ]
[view original message]
From: Paul B. Mahol
Subject: Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660
Date: Tuesday, December 9, 2008 - 4:16 pm

-- 
Paul
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"

[ message continues ]
[view original message]
From: Paul B. Mahol
Subject: Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660
Date: Tuesday, December 9, 2008 - 4:56 pm

FreeBSD snapshot CD have same "issue".
It doesnt appear always (I mean I do not count vfs cache) maybe disabling SMP

-- 
Paul
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"

[ message continues ]
[view original message]
From: Paul B. Mahol
Subject: Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660
Date: Tuesday, December 9, 2008 - 5:28 pm

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
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"

[ message continues ]
[view original message]
From: John Baldwin
Subject: Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660
Date: Wednesday, December 10, 2008 - 12:50 pm

Ok, I will try to reproduce this locally.

-- 
John Baldwin
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"

[ message continues ]
[view original message]
From: Paul B. Mahol
Subject: Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660
Date: Wednesday, December 10, 2008 - 10:22 am

I searched little for this message and found  kern/63446 PR interesting comment:

	Caused by cd9660_vnops.c rev. 1.77. VOP_READDIR returns bogus
	d_fileno, VFS_VGET on this value returns bogus vnode with zeroed attributes.

I think that whatever locking is done is done wrong.

-- 
Paul
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"

[ message continues ]
[view original message]
From: John Baldwin
Subject: Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660
Date: Wednesday, December 10, 2008 - 12:50 pm

That issue isn't a locking issue, it's an issue with VOP_READDIR() using a 
different meaning for i-node numbers than VFS_VGET(), and would happen 
regardless of any Giant or vnode locking.

-- 
John Baldwin
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"

[ message continues ]
[view original message]
From: Paul B. Mahol
Subject: Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660
Date: Wednesday, December 10, 2008 - 4:54 pm

Yes, I know that that issue/PR doesnt have anything to do with locking.
But displaying bogus messages and still having
functional cd9660 for general use means that something
is not correctly locked.

-- 
Paul
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"

[ message continues ]
[view original message]
From: Jaakko Heinonen
Subject: Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660
Date: Friday, November 21, 2008 - 12:40 am

There are several bugs related to unloading the atapicd module.

* g_wither_geom() race against module unload
* return value from g_modevent() is ignored
* detaching/unloading is allowed even if the device is in use

You could try these patches:

http://www.saunalahti.fi/~jh3/patches/atapi-cd-locking+tray-control.diff
http://www.saunalahti.fi/~jh3/patches/geom-unload-class-race.diff

-- 
Jaakko
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"

[ message continues ]
[view original message]
From: Paul B. Mahol
Subject: Re: [PATCH] MPSAFE/LOOKUP_SHARED cd9660
Date: Friday, November 21, 2008 - 4:57 pm

Works fine for me. With that patches panic doesnt not happen during shutdown,

-- 
Paul
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"

[ message continues ]
Previous thread: [PATCH] ppbus/ppc locking by John Baldwin on Wednesday, November 19, 2008 - 1:03 pm. (11 messages)

Next thread: atrtc0: Warnings about mappings of I/O and interrupt by Roman Divacky on Thursday, November 20, 2008 - 10:13 am. (7 messages)