Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 4 Jul 2020 12:01:56 -0700
From:      Conrad Meyer <cem@freebsd.org>
To:        Xin LI <delphij@freebsd.org>
Cc:        src-committers@freebsd.org, svn-src-all@freebsd.org,  svn-src-head@freebsd.org
Subject:   Re: svn commit: r362936 - head/sbin/newfs_msdos
Message-ID:  <CAG6CVpWQ0JzcyxPG1wvrmN_AxNK_juZto=Fbof01m8BHOiNZDw@mail.gmail.com>
In-Reply-To: <202007041837.064Ib4PL082461@repo.freebsd.org>
References:  <202007041837.064Ib4PL082461@repo.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
Hi Xin Li,

Maybe we can use C11 static_assert instead of the CTASSERT array mechanism?

Best,
Conrad

On Sat, Jul 4, 2020 at 11:37 Xin LI <delphij@freebsd.org> wrote:

> Author: delphij
> Date: Sat Jul  4 18:37:04 2020
> New Revision: 362936
> URL: https://svnweb.freebsd.org/changeset/base/362936
>
> Log:
>   Gather writes to larger chunks (MAXPHYS) instead of issuing them in
>   sectors.
>
>   On my SanDisk Cruzer Blade 16GB USB stick this made formatting much
> faster:
>
>   x before
>   + after
>
> +--------------------------------------------------------------------------+
>   |+
>    |
>   |+
> x  |
>   |+
> x x|
>   |A
> MA||
>
> +--------------------------------------------------------------------------+
>       N           Min           Max        Median           Avg
> Stddev
>   x   3         15.89         16.38            16         16.09
>  0.2570992
>   +   3          0.32          0.37          0.35    0.34666667
>  0.025166115
>   Difference at 95.0% confidence
>         -15.7433 +/- 0.414029
>         -97.8455% +/- 0.25668%
>         (Student's t, pooled s = 0.182665)
>
>   Reviewed by:  emaste
>   MFC after:    2 weeks
>   Differential Revision:        https://reviews.freebsd.org/D24508
>
> Modified:
>   head/sbin/newfs_msdos/mkfs_msdos.c
>
> Modified: head/sbin/newfs_msdos/mkfs_msdos.c
>
> ==============================================================================
> --- head/sbin/newfs_msdos/mkfs_msdos.c  Sat Jul  4 18:01:29 2020
> (r362935)
> +++ head/sbin/newfs_msdos/mkfs_msdos.c  Sat Jul  4 18:37:04 2020
> (r362936)
> @@ -64,6 +64,7 @@ static const char rcsid[] =
>
>  #define        DOSMAGIC  0xaa55        /* DOS magic number */
>  #define        MINBPS    512           /* minimum bytes per sector */
> +#define        MAXBPS    4096          /* maximum bytes per sector */
>  #define        MAXSPC    128           /* maximum sectors per cluster */
>  #define        MAXNFT    16            /* maximum number of FATs */
>  #define        DEFBLK    4096          /* default block size */
> @@ -77,6 +78,25 @@ static const char rcsid[] =
>  #define        MAXCLS16  0xfff4U       /* maximum FAT16 clusters */
>  #define        MAXCLS32  0xffffff4U    /* maximum FAT32 clusters */
>
> +#ifndef        CTASSERT
> +#define        CTASSERT(x)             _CTASSERT(x, __LINE__)
> +#define        _CTASSERT(x, y)         __CTASSERT(x, y)
> +#define        __CTASSERT(x, y)        typedef char __assert_ ## y [(x) ?
> 1 : -1]
> +#endif
> +
> +/*
> + * For better performance, we want to write larger chunks instead of
> + * individual sectors (the size can only be 512, 1024, 2048 or 4096
> + * bytes). Assert that MAXPHYS can always hold an integer number of
> + * sectors by asserting that both are power of two numbers and the
> + * MAXPHYS is greater than MAXBPS.
> + */
> +CTASSERT(powerof2(MAXPHYS));
> +CTASSERT(powerof2(MAXBPS));
> +CTASSERT(MAXPHYS > MAXBPS);
> +
> +const static ssize_t chunksize = MAXPHYS;
> +
>  #define        mincls(fat)  ((fat) == 12 ? MINCLS12 :  \
>                       (fat) == 16 ? MINCLS16 :  \
>                                     MINCLS32)
> @@ -243,6 +263,7 @@ mkfs_msdos(const char *fname, const char *dtype, const
>      struct bsx *bsx;
>      struct de *de;
>      u_int8_t *img;
> +    u_int8_t *physbuf, *physbuf_end;
>      const char *bname;
>      ssize_t n;
>      time_t now;
> @@ -252,7 +273,7 @@ mkfs_msdos(const char *fname, const char *dtype, const
>      int fd, fd1, rv;
>      struct msdos_options o = *op;
>
> -    img = NULL;
> +    physbuf = NULL;
>      rv = -1;
>      fd = fd1 = -1;
>
> @@ -343,15 +364,13 @@ mkfs_msdos(const char *fname, const char *dtype,
> const
>                 bpb.bpbSecPerClust = 64;                /* otherwise 32k */
>         }
>      }
> -    if (!powerof2(bpb.bpbBytesPerSec)) {
> -       warnx("bytes/sector (%u) is not a power of 2", bpb.bpbBytesPerSec);
> +    if (bpb.bpbBytesPerSec < MINBPS ||
> +        bpb.bpbBytesPerSec > MAXBPS ||
> +       !powerof2(bpb.bpbBytesPerSec)) {
> +       warnx("Invalid bytes/sector (%u): must be 512, 1024, 2048 or 4096",
> +           bpb.bpbBytesPerSec);
>         goto done;
>      }
> -    if (bpb.bpbBytesPerSec < MINBPS) {
> -       warnx("bytes/sector (%u) is too small; minimum is %u",
> -            bpb.bpbBytesPerSec, MINBPS);
> -       goto done;
> -    }
>
>      if (o.volume_label && !oklabel(o.volume_label)) {
>         warnx("%s: bad volume label", o.volume_label);
> @@ -621,11 +640,14 @@ mkfs_msdos(const char *fname, const char *dtype,
> const
>             tm = localtime(&now);
>         }
>
> -
> -       if (!(img = malloc(bpb.bpbBytesPerSec))) {
> +       physbuf = malloc(chunksize);
> +       if (physbuf == NULL) {
>             warn(NULL);
>             goto done;
>         }
> +       physbuf_end = physbuf + chunksize;
> +       img = physbuf;
> +
>         dir = bpb.bpbResSectors + (bpb.bpbFATsecs ? bpb.bpbFATsecs :
>                                    bpb.bpbBigFATsecs) * bpb.bpbFATs;
>         memset(&si_sa, 0, sizeof(si_sa));
> @@ -750,19 +772,37 @@ mkfs_msdos(const char *fname, const char *dtype,
> const
>                     (u_int)tm->tm_mday;
>                 mk2(de->deMDate, x);
>             }
> -           if ((n = write(fd, img, bpb.bpbBytesPerSec)) == -1) {
> -               warn("%s", fname);
> -               goto done;
> +           /*
> +            * Issue a write of chunksize once we have collected
> +            * enough sectors.
> +            */
> +           img += bpb.bpbBytesPerSec;
> +           if (img >= physbuf_end) {
> +               n = write(fd, physbuf, chunksize);
> +               if (n != chunksize) {
> +                   warnx("%s: can't write sector %u", fname, lsn);
> +                   goto done;
> +               }
> +               img = physbuf;
>             }
> -           if ((unsigned)n != bpb.bpbBytesPerSec) {
> -               warnx("%s: can't write sector %u", fname, lsn);
> -               goto done;
> -           }
>         }
> +       /*
> +        * Write remaining sectors, if the last write didn't end
> +        * up filling a whole chunk.
> +        */
> +       if (img != physbuf) {
> +               ssize_t tailsize = img - physbuf;
> +
> +               n = write(fd, physbuf, tailsize);
> +               if (n != tailsize) {
> +                   warnx("%s: can't write sector %u", fname, lsn);
> +                   goto done;
> +               }
> +       }
>      }
>      rv = 0;
>  done:
> -    free(img);
> +    free(physbuf);
>      if (fd != -1)
>             close(fd);
>      if (fd1 != -1)
>



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAG6CVpWQ0JzcyxPG1wvrmN_AxNK_juZto=Fbof01m8BHOiNZDw>