From owner-freebsd-fs@FreeBSD.ORG Thu Jul 12 03:12:34 2007 Return-Path: X-Original-To: fs@freebsd.org Delivered-To: freebsd-fs@FreeBSD.ORG Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 3513516A400 for ; Thu, 12 Jul 2007 03:12:34 +0000 (UTC) (envelope-from soc@hbar.us) Received: from ug-out-1314.google.com (ug-out-1314.google.com [66.249.92.174]) by mx1.freebsd.org (Postfix) with ESMTP id B12C213C468 for ; Thu, 12 Jul 2007 03:12:33 +0000 (UTC) (envelope-from soc@hbar.us) Received: by ug-out-1314.google.com with SMTP id o4so277972uge for ; Wed, 11 Jul 2007 20:12:32 -0700 (PDT) Received: by 10.78.170.6 with SMTP id s6mr30703hue.1184208298208; Wed, 11 Jul 2007 19:44:58 -0700 (PDT) Received: by 10.78.138.5 with HTTP; Wed, 11 Jul 2007 19:44:58 -0700 (PDT) Message-ID: <47a4f3080707111944u2b9ad091t2f4b8be482ab3a69@mail.gmail.com> Date: Wed, 11 Jul 2007 22:44:58 -0400 From: "Brian Chu" To: "Bruce Evans" In-Reply-To: <20070710233455.O2101@besplex.bde.org> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <20070710233455.O2101@besplex.bde.org> Cc: bugs@freebsd.org, fs@freebsd.org Subject: Re: msdosfs not MPSAFE X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 12 Jul 2007 03:12:34 -0000 Bruce, I've already planned to look at this. I had not been aware that calls to the Giant Lock were being ignored in the current kernel. Brian On 7/10/07, Bruce Evans wrote: > msdsofs has been broken since Giant locking for file systems (or syscalls) > was removed. It allows multiple threads to race accessing the shared > static buffer `nambuf' and related variables. This causes remarkably > few problems. One case that breaks reliably is concurrent tars or finds, > especially with a small cluster size, even on a UP system. Then > getdirentries() returns garbage entries and/or lookup() of correct entries > can't find things. On my UP system, I think the race occurs mainly when > getdirentries() blocks on i/o and a directory entry spans the block > boundary. Then another getdirentries() or lookup() can run, and if it > does then it will almost certainly corrupt the state for the blocked > getdirentries(). On SMP systems, I think the race occurs much more often > and suspect that it is more harmful. > > Quick fix for an old version of FreeBSD. The part in msdosfs_lookup.c > has not been tested at runtime. The part in msdosfs_vnops.c may also > fix bugs involving cookie handling (but not a memory leak like I first > thought?). msdosfs_lookup.c is harder to fix because it has no common > cleanup code. > > %%% > 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 10 Jul 2007 13:33:37 -0000 > @@ -78,11 +78,11 @@ > * memory denode's will be in synch. > */ > -int > -msdosfs_lookup(ap) > +static int > +msdosfs_lookup_locked( > struct vop_cachedlookup_args /* { > struct vnode *a_dvp; > struct vnode **a_vpp; > struct componentname *a_cnp; > - } */ *ap; > + } */ *ap) > { > struct vnode *vdp = ap->a_dvp; > @@ -560,4 +561,20 @@ > > /* > + * XXX msdosfs_lookup() is split up because unlocking Giant before all the > + * returns in the original function would be too churning. > + */ > +int > +msdosfs_lookup(ap) > + struct vop_cachedlookup_args *ap; > +{ > + int error; > + > + mtx_lock(&Giant); /* XXX for exclusive access to nambuf. */ > + error = msdosfs_lookup_locked(ap); > + mtx_unlock(&Giant); > + return (error); > +} > + > +/* > * dep - directory entry to copy into the directory > * ddep - directory to add to > Index: msdosfs_vnops.c > =================================================================== > RCS file: /home/ncvs/src/sys/fs/msdosfs/msdosfs_vnops.c,v > retrieving revision 1.147 > diff -u -2 -r1.147 msdosfs_vnops.c > --- msdosfs_vnops.c 4 Feb 2004 21:52:53 -0000 1.147 > +++ msdosfs_vnops.c 10 Jul 2007 05:08:17 -0000 > @@ -1559,4 +1592,5 @@ > } > > + mtx_lock(&Giant); /* XXX for exclusive access to nambuf. */ > mbnambuf_init(); > off = offset; > @@ -1575,10 +1609,13 @@ > if (error) { > brelse(bp); > - return (error); > + Debugger("used to leak cookies 1"); > + break; > } > n = min(n, blsize - bp->b_resid); > if (n == 0) { > brelse(bp); > - return (EIO); > + error = EIO; > + Debugger("used to leak cookies 2"); > + break; > } > > @@ -1687,4 +1725,6 @@ > } > out: > + mtx_unlock(&Giant); > + > /* Subtract unused cookies */ > if (ap->a_ncookies) > %%% > > Please fix this better. mbnambuf_init() could return a non-static buffer > that doesn't require locking. Deallocation would be painful. Note that > even the details for Giant locking can't be hidden in msdosfs_conv.c like > the current interfaces intend, since mbnambuf_init() is used a lot to > reinitialize an in-use buffer, and there is no interface to drop usage. > > Bruce > _______________________________________________ > freebsd-fs@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-fs > To unsubscribe, send any mail to "freebsd-fs-unsubscribe@freebsd.org" >