msdosfs not MPSAFE

[Bruce Evans]

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.

Bruce