msdosfs not MPSAFE

[Kostik Belousov]

On Sat, Jul 21, 2007 at 11:52:04PM +1000, Bruce Evans wrote:
> 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 <sys/bio.h>
> %  #include <sys/buf.h>
> % +#include <sys/sx.h>
> %  #include <sys/vnode.h>
> %  #include <sys/mount.h>
> % @@ -63,4 +64,6 @@
> %  #include <fs/msdosfs/fat.h>
> % 
> % +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.

Once again, sorry for late answer.
The change seems to be good.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-fs/attachments/20070804/d4c06aca/attachment.pgp