msdosfs not MPSAFE
Bruce Evans
brde at optusnet.com.au
Tue Aug 7 07:48:12 PDT 2007
On Sat, 4 Aug 2007, Kostik Belousov wrote:
> 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:
>> 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:
>> ...
>> 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.
Thanks.
Here is a cleaned up version for -current for further review. I can't
see how to do it cleanly without all the little functions. It also
fixes a memory leak on module unload, and some style bugs in
msdosfs_vfsops.c. This has been tested lightly runtime. Module unload
has not been tested at all (I don't use modules).
%%%
Index: direntry.h
===================================================================
RCS file: /home/ncvs/src/sys/fs/msdosfs/direntry.h,v
retrieving revision 1.23
diff -u -2 -r1.23 direntry.h
--- direntry.h 24 Oct 2006 11:14:05 -0000 1.23
+++ direntry.h 7 Aug 2007 11:51:16 -0000
@@ -137,6 +137,10 @@
struct msdosfsmount;
+void mbnambuf_acquire(void);
+void mbnambuf_create(void);
+void mbnambuf_destroy(void);
char *mbnambuf_flush(struct dirent *dp);
void mbnambuf_init(void);
+void mbnambuf_release(void);
void mbnambuf_write(char *name, int id);
int dos2unixfn(u_char dn[11], u_char *un, int lower,
Index: msdosfs_conv.c
===================================================================
RCS file: /home/ncvs/src/sys/fs/msdosfs/msdosfs_conv.c,v
retrieving revision 1.52
diff -u -2 -r1.52 msdosfs_conv.c
--- msdosfs_conv.c 7 Aug 2007 02:11:16 -0000 1.52
+++ msdosfs_conv.c 7 Aug 2007 12:18:03 -0000
@@ -53,6 +53,9 @@
#include <sys/dirent.h>
#include <sys/iconv.h>
+#include <sys/lock.h>
#include <sys/malloc.h>
#include <sys/mount.h>
+#include <sys/pcpu.h>
+#include <sys/sx.h>
#include <fs/msdosfs/bpb.h>
@@ -68,4 +71,5 @@
static u_int16_t unix2winchr(const u_char **, size_t *, int, struct msdosfsmount *);
+static struct sx nambuf_lock;
static char *nambuf_ptr;
static size_t nambuf_len;
@@ -1038,6 +1042,36 @@
/*
- * Initialize the temporary concatenation buffer (once) and mark it as
- * empty on subsequent calls.
+ * nambuf is static, so it must be locked for exclusive access even in the
+ * non-SMP case, since although msdosfs is Giant-locked, calls like bread()
+ * which can block are made while nambuf is in use.
+ */
+void
+mbnambuf_acquire(void)
+{
+
+ sx_xlock(&nambuf_lock);
+}
+
+void
+mbnambuf_create(void)
+{
+
+ KASSERT(nambuf_ptr == NULL, ("mbnambuf_create: already created"));
+ nambuf_ptr = malloc(MAXNAMLEN + 1, M_MSDOSFSMNT, M_WAITOK);
+ nambuf_ptr[MAXNAMLEN] = '\0';
+ sx_init(&nambuf_lock, "mbnambuf");
+}
+
+void
+mbnambuf_destroy(void)
+{
+
+ free(nambuf_ptr, M_MSDOSFSMNT);
+ nambuf_ptr = NULL;
+ sx_destroy(&nambuf_lock);
+}
+
+/*
+ * Mark the temporary concatenation buffer as empty.
*/
void
@@ -1045,12 +1079,15 @@
{
- if (nambuf_ptr == NULL) {
- nambuf_ptr = malloc(MAXNAMLEN + 1, M_MSDOSFSMNT, M_WAITOK);
- nambuf_ptr[MAXNAMLEN] = '\0';
- }
nambuf_len = 0;
nambuf_last_id = -1;
}
+void
+mbnambuf_release(void)
+{
+
+ sx_xunlock(&nambuf_lock);
+}
+
/*
* Fill out our concatenation buffer with the given substring, at the offset
Index: msdosfs_lookup.c
===================================================================
RCS file: /home/ncvs/src/sys/fs/msdosfs/msdosfs_lookup.c,v
retrieving revision 1.50
diff -u -2 -r1.50 msdosfs_lookup.c
--- msdosfs_lookup.c 7 Aug 2007 02:25:56 -0000 1.50
+++ msdosfs_lookup.c 7 Aug 2007 11:51:16 -0000
@@ -186,4 +186,5 @@
*/
tdp = NULL;
+ mbnambuf_acquire();
mbnambuf_init();
/*
@@ -200,4 +201,5 @@
if (error == E2BIG)
break;
+ mbnambuf_release();
return (error);
}
@@ -205,4 +207,5 @@
if (error) {
brelse(bp);
+ mbnambuf_release();
return (error);
}
@@ -234,4 +237,5 @@
if (dep->deName[0] == SLOT_EMPTY) {
brelse(bp);
+ mbnambuf_release();
goto notfound;
}
@@ -310,4 +314,5 @@
}
+ mbnambuf_release();
goto found;
}
@@ -319,4 +324,5 @@
brelse(bp);
} /* for (frcn = 0; ; frcn++) */
+ mbnambuf_release();
notfound:
Index: msdosfs_vfsops.c
===================================================================
RCS file: /home/ncvs/src/sys/fs/msdosfs/msdosfs_vfsops.c,v
retrieving revision 1.173
diff -u -2 -r1.173 msdosfs_vfsops.c
--- msdosfs_vfsops.c 7 Aug 2007 03:38:36 -0000 1.173
+++ msdosfs_vfsops.c 7 Aug 2007 12:07:37 -0000
@@ -104,9 +104,5 @@
static int mountmsdosfs(struct vnode *devvp, struct mount *mp,
struct thread *td);
-static vfs_fhtovp_t msdosfs_fhtovp;
-static vfs_mount_t msdosfs_mount;
static vfs_root_t msdosfs_root;
-static vfs_statfs_t msdosfs_statfs;
-static vfs_sync_t msdosfs_sync;
static vfs_unmount_t msdosfs_unmount;
@@ -939,11 +935,29 @@
}
+static int
+msdosfs_init(struct vfsconf *vfsp)
+{
+
+ mbnambuf_create();
+ return (0);
+}
+
+static int
+msdosfs_uninit(struct vfsconf *vfsp)
+{
+
+ mbnambuf_destroy();
+ return (0);
+}
+
static struct vfsops msdosfs_vfsops = {
+ .vfs_cmount = msdosfs_cmount,
.vfs_fhtovp = msdosfs_fhtovp,
+ .vfs_init = msdosfs_init,
.vfs_mount = msdosfs_mount,
- .vfs_cmount = msdosfs_cmount,
.vfs_root = msdosfs_root,
.vfs_statfs = msdosfs_statfs,
.vfs_sync = msdosfs_sync,
+ .vfs_uninit = msdosfs_uninit,
.vfs_unmount = msdosfs_unmount,
};
Index: msdosfs_vnops.c
===================================================================
RCS file: /home/ncvs/src/sys/fs/msdosfs/msdosfs_vnops.c,v
retrieving revision 1.178
diff -u -2 -r1.178 msdosfs_vnops.c
--- msdosfs_vnops.c 7 Aug 2007 10:35:27 -0000 1.178
+++ msdosfs_vnops.c 7 Aug 2007 11:55:27 -0000
@@ -1630,4 +1630,5 @@
}
+ mbnambuf_acquire();
mbnambuf_init();
off = offset;
@@ -1646,10 +1647,11 @@
if (error) {
brelse(bp);
- return (error);
+ break;
}
n = min(n, blsize - bp->b_resid);
if (n == 0) {
brelse(bp);
- return (EIO);
+ error = EIO;
+ break;
}
@@ -1765,4 +1767,6 @@
}
out:
+ mbnambuf_release();
+
/* Subtract unused cookies */
if (ap->a_ncookies)
%%%
Bruce
More information about the freebsd-fs
mailing list