msdosfs not MPSAFE
Bruce Evans
brde at optusnet.com.au
Fri Aug 10 03:54:56 UTC 2007
On Tue, 7 Aug 2007, Kostik Belousov wrote:
> 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 ?
No reason. I just move the allocation to the init function without noticing
that this made dynamic allocation completely bogus. (When the allocation
was only done on first use, it saved about 256 bytes of data space until
first use, at a costs of < 256 bytes of code, except with the memory leak
it sometimes gave a negative savings for data space too.)
> It seems that using functions for mbnambuf_acquire()/mbnambuf_release()
> causes unneeded overhead. Macros could be used as well.
Efficiency is not important here, and macros would require a lot of namespace
pollution.
> Anyway, these notes are not objections against patch.
I wrote yet another patch, with allocation on the stack so that no locking
is required. This is simpler and doesn't require any new functions.
Unfortunately, it is larger because it changes the interfaces for most
functions. The interface changes are routine, so this is probably better.
Note that 'struct dirent's are already allocated on the stack. This
patch adds allocation of 'struct mbnambuf's which are slightly smaller
(~256 bytes). I think this is just small enough for stack allocation.
% 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 9 Aug 2007 01:59:52 -0000
% @@ -134,10 +134,16 @@
%
% #ifdef _KERNEL
% +struct mbnambuf {
% + size_t nb_len;
% + int nb_last_id;
% + char nb_buf[WIN_MAXLEN + 1];
% +};
% +
% struct dirent;
% struct msdosfsmount;
%
% -char *mbnambuf_flush(struct dirent *dp);
% -void mbnambuf_init(void);
% -void mbnambuf_write(char *name, int id);
% +char *mbnambuf_flush(struct mbnambuf *nbp, struct dirent *dp);
% +void mbnambuf_init(struct mbnambuf *nbp);
% +void mbnambuf_write(struct mbnambuf *nbp, char *name, int id);
% int dos2unixfn(u_char dn[11], u_char *un, int lower,
% struct msdosfsmount *pmp);
% @@ -146,7 +152,8 @@
% int unix2winfn(const u_char *un, size_t unlen, struct winentry *wep, int cnt,
% int chksum, struct msdosfsmount *pmp);
% -int winChkName(const u_char *un, size_t unlen, int chksum,
% +int winChkName(struct mbnambuf *nbp, const u_char *un, size_t unlen,
% + int chksum, struct msdosfsmount *pmp);
% +int win2unixfn(struct mbnambuf *nbp, struct winentry *wep, int chksum,
% struct msdosfsmount *pmp);
% -int win2unixfn(struct winentry *wep, int chksum, struct msdosfsmount *pmp);
% u_int8_t winChksum(struct direntry *dep);
% int winSlotCnt(const u_char *un, size_t unlen, struct msdosfsmount *pmp);
% 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 9 Aug 2007 02:28:00 -0000
% @@ -53,5 +53,4 @@
% #include <sys/dirent.h>
% #include <sys/iconv.h>
% -#include <sys/malloc.h>
% #include <sys/mount.h>
%
% @@ -68,8 +67,4 @@
% static u_int16_t unix2winchr(const u_char **, size_t *, int, struct msdosfsmount *);
%
% -static char *nambuf_ptr;
% -static size_t nambuf_len;
% -static int nambuf_last_id;
% -
% /*
% * 0 - character disallowed in long file name.
% @@ -602,5 +597,6 @@
% */
% int
% -winChkName(un, unlen, chksum, pmp)
% +winChkName(nbp, un, unlen, chksum, pmp)
% + struct mbnambuf *nbp;
% const u_char *un;
% size_t unlen;
% @@ -614,7 +610,7 @@
%
% /*
% - * We alread have winentry in mbnambuf
% + * We already have winentry in *nbp.
% */
% - if (!mbnambuf_flush(&dirbuf) || !dirbuf.d_namlen)
% + if (!mbnambuf_flush(nbp, &dirbuf) || dirbuf.d_namlen == 0)
% return -1;
%
% @@ -651,5 +647,6 @@
% */
% int
% -win2unixfn(wep, chksum, pmp)
% +win2unixfn(nbp, wep, chksum, pmp)
% + struct mbnambuf *nbp;
% struct winentry *wep;
% int chksum;
% @@ -684,5 +681,5 @@
% case 0:
% *np = '\0';
% - mbnambuf_write(name, (wep->weCnt & WIN_CNT) - 1);
% + mbnambuf_write(nbp, name, (wep->weCnt & WIN_CNT) - 1);
% return chksum;
% case '/':
% @@ -703,5 +700,5 @@
% case 0:
% *np = '\0';
% - mbnambuf_write(name, (wep->weCnt & WIN_CNT) - 1);
% + mbnambuf_write(nbp, name, (wep->weCnt & WIN_CNT) - 1);
% return chksum;
% case '/':
% @@ -722,5 +719,5 @@
% case 0:
% *np = '\0';
% - mbnambuf_write(name, (wep->weCnt & WIN_CNT) - 1);
% + mbnambuf_write(nbp, name, (wep->weCnt & WIN_CNT) - 1);
% return chksum;
% case '/':
% @@ -737,5 +734,5 @@
% }
% *np = '\0';
% - mbnambuf_write(name, (wep->weCnt & WIN_CNT) - 1);
% + mbnambuf_write(nbp, name, (wep->weCnt & WIN_CNT) - 1);
% return chksum;
% }
% @@ -1038,17 +1035,13 @@
%
% /*
% - * Initialize the temporary concatenation buffer (once) and mark it as
% - * empty on subsequent calls.
% + * Initialize the temporary concatenation buffer.
% */
% void
% -mbnambuf_init(void)
% +mbnambuf_init(struct mbnambuf *nbp)
% {
%
% - if (nambuf_ptr == NULL) {
% - nambuf_ptr = malloc(MAXNAMLEN + 1, M_MSDOSFSMNT, M_WAITOK);
% - nambuf_ptr[MAXNAMLEN] = '\0';
% - }
% - nambuf_len = 0;
% - nambuf_last_id = -1;
% + nbp->nb_len = 0;
% + nbp->nb_last_id = -1;
% + nbp->nb_buf[sizeof(nbp->nb_buf) - 1] = '\0';
% }
%
% @@ -1063,28 +1056,29 @@
% */
% void
% -mbnambuf_write(char *name, int id)
% +mbnambuf_write(struct mbnambuf *nbp, char *name, int id)
% {
% - size_t count;
% char *slot;
% + size_t count, newlen;
%
% - KASSERT(nambuf_len == 0 || id == nambuf_last_id - 1,
% - ("non-decreasing id, id %d last id %d", id, nambuf_last_id));
% + KASSERT(nbp->nb_len == 0 || id == nbp->nb_last_id - 1,
% + ("non-decreasing id: id %d, last id %d", id, nbp->nb_last_id));
%
% - /* Store this substring in a WIN_CHAR-aligned slot. */
% - slot = nambuf_ptr + (id * WIN_CHARS);
% + /* Will store this substring in a WIN_CHARS-aligned slot. */
% + slot = &nbp->nb_buf[id * WIN_CHARS];
% count = strlen(name);
% - if (nambuf_len + count > MAXNAMLEN) {
% - printf("msdosfs: file name %zu too long\n", nambuf_len + count);
% + newlen = nbp->nb_len + count;
% + if (newlen > WIN_MAXLEN || newlen > MAXNAMLEN) {
% + printf("msdosfs: file name length %zu too large\n", newlen);
% return;
% }
%
% /* Shift suffix upwards by the amount length exceeds WIN_CHARS. */
% - if (count > WIN_CHARS && nambuf_len != 0)
% - bcopy(slot + WIN_CHARS, slot + count, nambuf_len);
% + if (count > WIN_CHARS && nbp->nb_len != 0)
% + bcopy(slot + WIN_CHARS, slot + count, nbp->nb_len);
%
% /* Copy in the substring to its slot and update length so far. */
% bcopy(name, slot, count);
% - nambuf_len += count;
% - nambuf_last_id = id;
% + nbp->nb_len = newlen;
% + nbp->nb_last_id = id;
% }
%
% @@ -1097,16 +1091,16 @@
% */
% char *
% -mbnambuf_flush(struct dirent *dp)
% +mbnambuf_flush(struct mbnambuf *nbp, struct dirent *dp)
% {
%
% - if (nambuf_len > sizeof(dp->d_name) - 1) {
% - mbnambuf_init();
% + if (nbp->nb_len > sizeof(dp->d_name) - 1) {
% + mbnambuf_init(nbp);
% return (NULL);
% }
% - bcopy(nambuf_ptr, dp->d_name, nambuf_len);
% - dp->d_name[nambuf_len] = '\0';
% - dp->d_namlen = nambuf_len;
% + bcopy(&nbp->nb_buf[0], dp->d_name, nbp->nb_len);
% + dp->d_name[nbp->nb_len] = '\0';
% + dp->d_namlen = nbp->nb_len;
%
% - mbnambuf_init();
% + mbnambuf_init(nbp);
% return (dp->d_name);
% }
% 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 9 Aug 2007 01:56:31 -0000
% @@ -85,4 +85,5 @@
% } */ *ap;
% {
% + struct mbnambuf nb;
% struct vnode *vdp = ap->a_dvp;
% struct vnode **vpp = ap->a_vpp;
% @@ -186,5 +187,5 @@
% */
% tdp = NULL;
% - mbnambuf_init();
% + mbnambuf_init(&nb);
% /*
% * The outer loop ranges over the clusters that make up the
% @@ -226,5 +227,5 @@
% */
% chksum = -1;
% - mbnambuf_init();
% + mbnambuf_init(&nb);
%
% if (slotcount < wincnt) {
% @@ -251,14 +252,13 @@
% continue;
%
% - chksum = win2unixfn((struct winentry *)dep,
% - chksum,
% - pmp);
% + chksum = win2unixfn(&nb,
% + (struct winentry *)dep, chksum,
% + pmp);
% continue;
% }
%
% - chksum = winChkName((const u_char *)cnp->cn_nameptr,
% - unlen,
% - chksum,
% - pmp);
% + chksum = winChkName(&nb,
% + (const u_char *)cnp->cn_nameptr, unlen,
% + chksum, pmp);
% if (chksum == -2) {
% chksum = -1;
% 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 9 Aug 2007 02:25:37 -0000
% @@ -1511,4 +1511,5 @@
% } */ *ap;
% {
% + struct mbnambuf nb;
% int error = 0;
% int diff;
% @@ -1630,5 +1631,5 @@
% }
%
% - mbnambuf_init();
% + mbnambuf_init(&nb);
% off = offset;
% while (uio->uio_resid > 0) {
% @@ -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;
% }
%
This hunk is no longer needed for deallocation, but might be needed to fix
an old bug.
% @@ -1677,5 +1679,5 @@
% if (dentp->deName[0] == SLOT_DELETED) {
% chksum = -1;
% - mbnambuf_init();
% + mbnambuf_init(&nb);
% continue;
% }
% @@ -1687,6 +1689,6 @@
% if (pmp->pm_flags & MSDOSFSMNT_SHORTNAME)
% continue;
% - chksum = win2unixfn((struct winentry *)dentp,
% - chksum, pmp);
% + chksum = win2unixfn(&nb,
% + (struct winentry *)dentp, chksum, pmp);
% continue;
% }
% @@ -1697,5 +1699,5 @@
% if (dentp->deAttributes & ATTR_VOLUME) {
% chksum = -1;
% - mbnambuf_init();
% + mbnambuf_init(&nb);
% continue;
% }
% @@ -1739,7 +1741,7 @@
% (LCASE_BASE | LCASE_EXT) : 0),
% pmp);
% - mbnambuf_init();
% + mbnambuf_init(&nb);
% } else
% - mbnambuf_flush(&dirbuf);
% + mbnambuf_flush(&nb, &dirbuf);
% chksum = -1;
% dirbuf.d_reclen = GENERIC_DIRSIZ(&dirbuf);
% @@ -1765,4 +1767,5 @@
% }
% out:
% +
% /* Subtract unused cookies */
% if (ap->a_ncookies)
Last hunk is certainly no longer needed (was cleanup for deallocation).
Bruce
More information about the freebsd-bugs
mailing list