Date: Wed, 11 Jul 2007 22:44:58 -0400 From: "Brian Chu" <soc@hbar.us> To: "Bruce Evans" <brde@optusnet.com.au> Cc: bugs@freebsd.org, fs@freebsd.org Subject: Re: msdosfs not MPSAFE Message-ID: <47a4f3080707111944u2b9ad091t2f4b8be482ab3a69@mail.gmail.com> In-Reply-To: <20070710233455.O2101@besplex.bde.org> References: <20070710233455.O2101@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <brde@optusnet.com.au> 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" >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?47a4f3080707111944u2b9ad091t2f4b8be482ab3a69>