From owner-freebsd-bugs@FreeBSD.ORG Sat Jul 21 13:52:09 2007 Return-Path: Delivered-To: bugs@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id DC4E016A418; Sat, 21 Jul 2007 13:52:09 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail16.syd.optusnet.com.au (mail16.syd.optusnet.com.au [211.29.132.197]) by mx1.freebsd.org (Postfix) with ESMTP id 77EB913C458; Sat, 21 Jul 2007 13:52:09 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from besplex.bde.org (c220-239-235-248.carlnfd3.nsw.optusnet.com.au [220.239.235.248]) by mail16.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id l6LDq4Tg021276 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 21 Jul 2007 23:52:06 +1000 Date: Sat, 21 Jul 2007 23:52:04 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Kostik Belousov In-Reply-To: <20070721063434.GI2200@deviant.kiev.zoral.com.ua> Message-ID: <20070721233613.Q3366@besplex.bde.org> References: <20070710233455.O2101@besplex.bde.org> <20070712084115.GA2200@deviant.kiev.zoral.com.ua> <20070712225324.F9515@besplex.bde.org> <20070712142127.GD2200@deviant.kiev.zoral.com.ua> <20070716195556.P12807@besplex.bde.org> <20070721063434.GI2200@deviant.kiev.zoral.com.ua> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: bugs@FreeBSD.org, fs@FreeBSD.org, Bruce Evans Subject: Re: msdosfs not MPSAFE X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 21 Jul 2007 13:52:10 -0000 On Sat, 21 Jul 2007, Kostik Belousov wrote: > On Mon, Jul 16, 2007 at 08:18:14PM +1000, Bruce Evans wrote: >> sx xlocking works, but is not quite right: >> % /* >> % + * XXX msdosfs_lookup() is split up because unlocking before all the >> returns >> % + * in the original function would be too churning. >> % + */ >> % +int >> % +msdosfs_lookup(ap) >> % + struct vop_cachedlookup_args *ap; >> % +{ >> % + int error; >> % + >> % + sx_xlock(&mbnambuf_lock); >> % + error = msdosfs_lookup_locked(ap); >> % + sx_xunlock(&mbnambuf_lock); >> % + return (error); >> % +} >> % + >> % +/* > > Assume that a directory A is participating in lookup() from two threads: > thread 1 lookup the A itself; > thread 2 lookup some entry in the A. > Then, thread 1 would have mbnambuf_lock locked, and may wait for A' > vnode lock; > thread 2 shall own vnode lock for A, then locking mbnambuf_lock. > > I do not see what may prevent this LOR scenario from realizing, or what > make it harmless. Nothing I can see either. The wrapper is too global. Next try: move locking into the inner loop in msdosfs_lookup(). Unlocking is not as ugly as I feared. The following has only been tested at compile time: % Index: msdosfs_lookup.c % =================================================================== % RCS file: /home/ncvs/src/sys/fs/msdosfs/msdosfs_lookup.c,v % retrieving revision 1.40 % diff -u -2 -r1.40 msdosfs_lookup.c % --- msdosfs_lookup.c 26 Dec 2003 17:24:37 -0000 1.40 % +++ msdosfs_lookup.c 21 Jul 2007 13:27:37 -0000 % @@ -54,4 +54,5 @@ % #include % #include % +#include % #include % #include % @@ -63,4 +64,6 @@ % #include % % +extern struct sx mbnambuf_lock; % + % /* % * When we search a directory the blocks containing directory entries are % @@ -192,4 +195,5 @@ % */ % tdp = NULL; % + sx_xlock(&mbnambuf_lock); % mbnambuf_init(); % /* % @@ -206,4 +210,5 @@ % if (error == E2BIG) % break; % + sx_xunlock(&mbnambuf_lock); % return (error); % } % @@ -211,4 +216,5 @@ % if (error) { % brelse(bp); % + sx_xunlock(&mbnambuf_lock); % return (error); % } % @@ -240,4 +246,5 @@ % if (dep->deName[0] == SLOT_EMPTY) { % brelse(bp); % + sx_xunlock(&mbnambuf_lock); % goto notfound; % } % @@ -301,4 +308,5 @@ % dp->de_fndcnt = wincnt - 1; % % + sx_xunlock(&mbnambuf_lock); % goto found; % } % @@ -310,4 +318,5 @@ % brelse(bp); % } /* for (frcn = 0; ; frcn++) */ % + sx_xunlock(&mbnambuf_lock); % % notfound: After moving the locking into msdosfs_conv.c and adding assertions there, this should be a good enough fix until the mbnambuf interface is changed. This bug is in all versions since 5.2-RELEASE. Bruce