From owner-freebsd-fs@FreeBSD.ORG Tue Aug 7 14:48:12 2007 Return-Path: Delivered-To: fs@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 5CC2216A417; Tue, 7 Aug 2007 14:48:12 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail11.syd.optusnet.com.au (mail11.syd.optusnet.com.au [211.29.132.192]) by mx1.freebsd.org (Postfix) with ESMTP id D67E913C459; Tue, 7 Aug 2007 14:48:11 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from besplex.bde.org (c220-239-235-248.carlnfd3.nsw.optusnet.com.au [220.239.235.248]) by mail11.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id l77Em7OI024496 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 8 Aug 2007 00:48:09 +1000 Date: Wed, 8 Aug 2007 00:48:06 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Kostik Belousov In-Reply-To: <20070804075730.GP2738@deviant.kiev.zoral.com.ua> Message-ID: <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> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: bugs@FreeBSD.org, fs@FreeBSD.org Subject: Re: msdosfs not MPSAFE X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 07 Aug 2007 14:48:12 -0000 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 #include +#include #include #include +#include +#include #include @@ -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