Re: [PATCH] Make udf(4) MPSAFE and use shared lookups

On 11/21/08, John Baldwin <jhb_at_freebsd.org> wrote:
> On Thursday 20 November 2008 06:32:25 pm Paul B. Mahol wrote:
>> On 11/20/08, John Baldwin <jhb_at_freebsd.org> wrote:
>> > So this patch is fairly minimal since udf(4) is currently read-only.
>> > The
>> > changes include:
>> >
>> > * Set VV_ROOT in udf_vget() if we ever return a vnode instead of doing
>> > it
>> > only
>> >   in udf_root().  This matches the behavior of other operating systems
>> > and
>> >   correctly tags the root vnode with VV_ROOT in the unlikely case that
>> > we
>> >   create the vnode during a call to ufs_vget() that does not come from
>> >   ufs_root().
>> > * If the hash lookup in ufs_vget() fails, ensure an exclusive vnode lock
>> >
> is
>> >   used while creating the new vnode (same as UFS).
>> > * Allow lock recursion (XXX: not really sure this is needed actually).
>> > * Allow shared vnode locks on non-fifos.
>> > * Honor the requested locking flags (shared vs exclusive) instead of
> always
>> >   using exclusive vnode locks during a lookup operation.
>> > * Handle "." lookups the same way other filesystems do by just bumping
>> > the
>> >   reference on 'dvp' rather than calling udf_vget().
>> >
>> > http://www.FreeBSD.org/~jhb/patches/udf_mpsafe.patch
>> >
>> > I have only verified that this compiles and would appreciate it if some
>> > folks
>> > could test this, preferable with INVARIANTS and DEBUG_VFS_LOCKS enabled.
>>
>> I lightly tested it with md(4) on 47M size iso created with k3b
>> (it contains two files)
>>
>> I only got this lor related to udf:
>>
>> lock order reversal:
>>  1st 0xc4449270 udf (udf) _at_ /usr/src/sys/kern/vfs_lookup.c:442
>>  2nd 0xd7d10850 bufwait (bufwait) _at_ /usr/src/sys/kern/vfs_bio.c:2443
>>  3rd 0xc4399488 udf (udf) _at_
>> /usr/src/sys/modules/udf/../../fs/udf/udf_vfsops.c:625
>
> I think you would get this same LOR w/o the patch.  I think cd9660 might
> have
> the same bug.  The issue is holding a buffer from bread() (i.e. not calling
> brelse() yet) while calling udf_vget() in udf_lookup().

cd9660 have this LOR:

lock order reversal:
 1st 0xc43a0594 isofs (isofs) _at_ /usr/src/sys/kern/vfs_lookup.c:442
 2nd 0xd7d105b0 bufwait (bufwait) _at_ /usr/src/sys/kern/vfs_bio.c:2443
 3rd 0xc43a0488 isofs (isofs) _at_
/usr/src/sys/modules/cd9660/../../fs/cd9660/cd9660_vfsops.c:694
KDB: stack backtrace:
db_trace_self_wrapper(c069e216,c3b646d0,c04e793f,4,c069995b,...) at
db_trace_self_wrapper+0x26
kdb_backtrace(4,c069995b,c3cb7538,c3cb9ca0,c3b6472c,...) at kdb_backtrace+0x29
_witness_debugger(c06a0ee3,c43a0488,c443da9a,c3cb9ca0,c443d9de,...) at
_witness_debugger+0x1e
witness_checkorder(c43a0488,9,c443d9de,2b6,0,...) at witness_checkorder+0x811
__lockmgr_args(c43a0488,80000,0,0,0,...) at __lockmgr_args+0x762
cd9660_vget_internal(c443f500,1e0ee,200000,c3b648b0,0,...) at
cd9660_vget_internal+0x13e
cd9660_lookup(c3b649fc,c43a053c,c3b64bb4,c43a053c,c3b64a1c,...) at
cd9660_lookup+0x621
VOP_CACHEDLOOKUP_APV(c443e360,c3b649fc,c3b64bb4,c3b64ba0,c06fa1c0,...)
at VOP_CACHEDLOOKUP_APV+0xa0
vfs_cache_lookup(c3b64a7c,c3b64a7c,0,200000,c43a053c,...) at
vfs_cache_lookup+0xc3
VOP_LOOKUP_APV(c443e360,c3b64a7c,c06a6c14,1ba,c3b64ba0,...) at
VOP_LOOKUP_APV+0xaa
lookup(c3b64b88,c06a6c14,dc,bc,c4186a2c,...) at lookup+0x507
namei(c3b64b88,c06a693e,143,c108ba34,c3c80000,...) at namei+0x45b
kern_statat(c4442000,200,ffffff9c,282162f8,0,...) at kern_statat+0x66
kern_lstat(c4442000,282162f8,0,c3b64c1c,0,...) at kern_lstat+0x36
lstat(c4442000,c3b64cf8,8,c06a1b96,c06cfb50,...) at lstat+0x2b
syscall(c3b64d38) at syscall+0x261
Xint0x80_syscall() at Xint0x80_syscall+0x20
--- syscall (5, FreeBSD ELF32, open), eip = 0x281f13ab, esp =
0xbfbfe8ac, ebp = 0xbfbfe8c8 ---


-- 
Paul