Date: Tue, 7 Aug 2007 20:02:59 +0300 From: Kostik Belousov <kostikbel@gmail.com> To: Bruce Evans <brde@optusnet.com.au> Cc: bugs@FreeBSD.org, fs@FreeBSD.org Subject: Re: msdosfs not MPSAFE Message-ID: <20070807170259.GJ2738@deviant.kiev.zoral.com.ua> In-Reply-To: <20070808004001.O926@besplex.bde.org> References: <20070710233455.O2101@besplex.bde.org> <20070712084115.GA2200@deviant.kiev.zoral.com.ua> <20070712225324.F9515@besplex.bde.org> <20070712142127.GD2200@deviant.kiev.zoral.com.ua> <20070716195556.P12807@besplex.bde.org> <20070721063434.GI2200@deviant.kiev.zoral.com.ua> <20070721233613.Q3366@besplex.bde.org> <20070804075730.GP2738@deviant.kiev.zoral.com.ua> <20070808004001.O926@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
On Wed, Aug 08, 2007 at 12:48:06AM +1000, Bruce Evans wrote:
> 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).
Why not allocate nambuf statically ?
It seems that using functions for mbnambuf_acquire()/mbnambuf_release()
causes unneeded overhead. Macros could be used as well.
Anyway, these notes are not objections against patch.
>
> %%%
> 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
[-- Attachment #2 --]
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (FreeBSD)
iD8DBQFGuKXDC3+MBN1Mb4gRAoTVAKCOgm0BH7z4eg8bFyNwNBsI3AXs4gCcDUIt
j0zTe8jwWllTzfvTyRH0z/k=
=/kNT
-----END PGP SIGNATURE-----
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070807170259.GJ2738>