Martin Pieuchot wrote:
> On 03/01/16(Sun) 23:10, Martin Pieuchot wrote:
> > Reading files on msdos-formated USB sticks under OpenBSD is really slow.
> > *One* of the reasons is that only one block is currently read-ahead if
> > possible.
> >
> > Diff below converts msdosfs_read() to use bread_cluster() which at least
> > double the transfer rate when reading sequential blocks here.
> >
> > When combined with the previous bread_cluster() diff, I get almost the
> > same read performances as with the raw device.
> >
> > With a cheap USB device here, I have:
> >
> > Kernel: Max measure read speed:
> >
> > -current 3.2M
> > -current+this diff 6.8M
> > -current+this diff+bread_cluster 11.8M
> > physio(9) 12.4M
>
> I forgot to mention that these tests were performed with a 64k blocks.
>
> Here's an updated diff that gets rid of the now unused 'de_lastr'
> argument. It makes use of 'mp->mnt_stat.f_iosize' to reduce differences
> with UFS and the old cluster code. Finally it renames some variables
> from 'lbn' to 'cn' to better reflect what we're dealing with.
Hi,
I've been testing the two diff lately (this one + bread_cluster), i've
seen no regression and I can confirm that it speeds up the reading here
too (by a smaller factor though, but it's with shitty hardware).
>
> Index: msdosfs/denode.h
> ===================================================================
> RCS file: /cvs/src/sys/msdosfs/denode.h,v
> retrieving revision 1.27
> diff -u -p -r1.27 denode.h
> --- msdosfs/denode.h 23 Oct 2015 10:45:31 -0000 1.27
> +++ msdosfs/denode.h 7 Jan 2016 16:36:08 -0000
> @@ -142,7 +142,6 @@ struct denode {
> struct vnode *de_devvp; /* vnode of blk dev we live on */
> uint32_t de_flag; /* flag bits */
> dev_t de_dev; /* device where direntry lives */
> - daddr_t de_lastr;
> uint32_t de_dirclust; /* cluster of the directory file containing
> this entry */
> uint32_t de_diroffset; /* offset of this entry in the directory
> cluster */
> uint32_t de_fndoffset; /* offset of found dir entry */
> Index: msdosfs/msdosfs_vnops.c
> ===================================================================
> RCS file: /cvs/src/sys/msdosfs/msdosfs_vnops.c,v
> retrieving revision 1.104
> diff -u -p -r1.104 msdosfs_vnops.c
> --- msdosfs/msdosfs_vnops.c 23 Oct 2015 18:04:37 -0000 1.104
> +++ msdosfs/msdosfs_vnops.c 7 Jan 2016 16:36:55 -0000
> @@ -77,13 +77,13 @@
>
> static uint32_t fileidhash(uint64_t);
>
> +int msdosfs_bmaparray(struct vnode *, daddr_t, daddr_t *, int *);
> int msdosfs_kqfilter(void *);
> int filt_msdosfsread(struct knote *, long);
> int filt_msdosfswrite(struct knote *, long);
> int filt_msdosfsvnode(struct knote *, long);
> void filt_msdosfsdetach(struct knote *);
>
> -
> /*
> * Some general notes:
> *
> @@ -511,18 +511,15 @@ int
> msdosfs_read(void *v)
> {
> struct vop_read_args *ap = v;
> - int error = 0;
> - uint32_t diff;
> - int blsize;
> - int isadir;
> - uint32_t n;
> - long on;
> - daddr_t lbn, rablock, rablkno;
> - struct buf *bp;
> struct vnode *vp = ap->a_vp;
> struct denode *dep = VTODE(vp);
> struct msdosfsmount *pmp = dep->de_pmp;
> struct uio *uio = ap->a_uio;
> + int isadir, error = 0;
> + uint32_t n, diff, size;
> + struct buf *bp;
> + daddr_t cn;
> + long on;
>
> /*
> * If they didn't ask for any data, then we are done.
> @@ -537,7 +534,8 @@ msdosfs_read(void *v)
> if (uio->uio_offset >= dep->de_FileSize)
> return (0);
>
> - lbn = de_cluster(pmp, uio->uio_offset);
> + cn = de_cluster(pmp, uio->uio_offset);
> + size = pmp->pm_bpcluster;
> on = uio->uio_offset & pmp->pm_crbomask;
> n = min((uint32_t) (pmp->pm_bpcluster - on), uio->uio_resid);
>
> @@ -550,31 +548,21 @@ msdosfs_read(void *v)
> if (diff < n)
> n = diff;
>
> - /* convert cluster # to block # if a directory */
> - if (isadir) {
> - error = pcbmap(dep, lbn, &lbn, 0, &blsize);
> - if (error)
> - return (error);
> - }
> /*
> * If we are operating on a directory file then be sure to
> * do i/o with the vnode for the filesystem instead of the
> * vnode for the directory.
> */
> if (isadir) {
> - error = bread(pmp->pm_devvp, lbn, blsize, &bp);
> + error = pcbmap(dep, cn, &cn, 0, &size);
> + if (error)
> + return (error);
> + error = bread(pmp->pm_devvp, cn, size, &bp);
> } else {
> - rablock = lbn + 1;
> - rablkno = de_cn2bn(pmp, rablock);
> - if (dep->de_lastr + 1 == lbn &&
> - de_cn2off(pmp, rablock) < dep->de_FileSize)
> - error = breadn(vp, de_cn2bn(pmp, lbn),
> - pmp->pm_bpcluster, &rablkno,
> - &pmp->pm_bpcluster, 1, &bp);
> + if (de_cn2off(pmp, cn + 1) >= dep->de_FileSize)
> + error = bread(vp, cn, size, &bp);
> else
> - error = bread(vp, de_cn2bn(pmp, lbn),
> - pmp->pm_bpcluster, &bp);
> - dep->de_lastr = lbn;
> + error = bread_cluster(vp, cn, size, &bp);
> }
> n = min(n, pmp->pm_bpcluster - bp->b_resid);
> if (error) {
> @@ -604,7 +592,7 @@ msdosfs_write(void *v)
> uint32_t osize;
> int error = 0;
> uint32_t count, lastcn;
> - daddr_t bn;
> + daddr_t cn;
> struct buf *bp;
> int ioflag = ap->a_ioflag;
> struct uio *uio = ap->a_uio;
> @@ -680,32 +668,34 @@ msdosfs_write(void *v)
> lastcn = de_clcount(pmp, osize) - 1;
>
> do {
> - if (de_cluster(pmp, uio->uio_offset) > lastcn) {
> + croffset = uio->uio_offset & pmp->pm_crbomask;
> + cn = de_cluster(pmp, uio->uio_offset);
> +
> + if (cn > lastcn) {
> error = ENOSPC;
> break;
> }
>
> - bn = de_blk(pmp, uio->uio_offset);
> - if ((uio->uio_offset & pmp->pm_crbomask) == 0
> - && (de_blk(pmp, uio->uio_offset + uio->uio_resid) >
> de_blk(pmp, uio->uio_offset)
> - || uio->uio_offset + uio->uio_resid >=
> dep->de_FileSize)) {
> + if (croffset == 0 &&
> + (de_cluster(pmp, uio->uio_offset + uio->uio_resid) > cn ||
> + (uio->uio_offset + uio->uio_resid) >= dep->de_FileSize)) {
> /*
> * If either the whole cluster gets written,
> * or we write the cluster from its start beyond EOF,
> * then no need to read data from disk.
> */
> - bp = getblk(thisvp, bn, pmp->pm_bpcluster, 0, 0);
> + bp = getblk(thisvp, cn, pmp->pm_bpcluster, 0, 0);
> clrbuf(bp);
> /*
> * Do the bmap now, since pcbmap needs buffers
> * for the fat table. (see msdosfs_strategy)
> */
> if (bp->b_blkno == bp->b_lblkno) {
> - error = pcbmap(dep,
> - de_bn2cn(pmp, bp->b_lblkno),
> - &bp->b_blkno, 0, 0);
> + error = pcbmap(dep, bp->b_lblkno, &cn, 0, 0);
> if (error)
> bp->b_blkno = -1;
> + else
> + bp->b_blkno = cn;
> }
> if (bp->b_blkno == -1) {
> brelse(bp);
> @@ -715,16 +705,16 @@ msdosfs_write(void *v)
> }
> } else {
> /*
> - * The block we need to write into exists, so read it
> in.
> + * The block we need to write into exists, so
> + * read it in.
> */
> - error = bread(thisvp, bn, pmp->pm_bpcluster, &bp);
> + error = bread(thisvp, cn, pmp->pm_bpcluster, &bp);
> if (error) {
> brelse(bp);
> break;
> }
> }
>
> - croffset = uio->uio_offset & pmp->pm_crbomask;
> n = min(uio->uio_resid, pmp->pm_bpcluster - croffset);
> if (uio->uio_offset + n > dep->de_FileSize) {
> dep->de_FileSize = uio->uio_offset + n;
> @@ -1756,7 +1746,7 @@ msdosfs_islocked(void *v)
>
> /*
> * vp - address of vnode file the file
> - * bn - which cluster we are interested in mapping to a filesystem block
> number.
> + * bn - which cluster we are interested in mapping to a filesystem block
> number
> * vpp - returns the vnode for the block special file holding the filesystem
> * containing the file of interest
> * bnp - address of where to return the filesystem relative block number
> @@ -1766,19 +1756,51 @@ msdosfs_bmap(void *v)
> {
> struct vop_bmap_args *ap = v;
> struct denode *dep = VTODE(ap->a_vp);
> - struct msdosfsmount *pmp = dep->de_pmp;
>
> if (ap->a_vpp != NULL)
> *ap->a_vpp = dep->de_devvp;
> if (ap->a_bnp == NULL)
> return (0);
> - if (ap->a_runp) {
> +
> + return (msdosfs_bmaparray(ap->a_vp, ap->a_bn, ap->a_bnp, ap->a_runp));
> +}
> +
> +int
> +msdosfs_bmaparray(struct vnode *vp, daddr_t cn, daddr_t *bnp, int *runp)
> +{
> + struct denode *dep = VTODE(vp);
> + struct msdosfsmount *pmp = dep->de_pmp;
> + struct mount *mp;
> + int error, maxrun = 0, run;
> + daddr_t daddr;
> +
> + mp = vp->v_mount;
> +
> + if (runp) {
> /*
> - * Sequential clusters should be counted here.
> + * XXX
> + * If MAXBSIZE is the largest transfer the disks can handle,
> + * we probably want maxrun to be 1 block less so that we
> + * don't create a block larger than the device can handle.
> */
> - *ap->a_runp = 0;
> + *runp = 0;
> + maxrun = min(MAXBSIZE / mp->mnt_stat.f_iosize - 1,
> + pmp->pm_maxcluster - cn);
> }
> - return (pcbmap(dep, de_bn2cn(pmp, ap->a_bn), ap->a_bnp, 0, 0));
> +
> + if ((error = pcbmap(dep, cn, bnp, 0, 0)) != 0)
> + return (error);
> +
> + for (run = 1; run <= maxrun; run++) {
> + error = pcbmap(dep, cn + run, &daddr, 0, 0);
> + if (error != 0 || (daddr != *bnp + de_cn2bn(pmp, run)))
> + break;
> + }
> +
> + if (runp)
> + *runp = run - 1;
> +
> + return (0);
> }
>
> int
> @@ -1800,8 +1822,7 @@ msdosfs_strategy(void *v)
> * don't allow files with holes, so we shouldn't ever see this.
> */
> if (bp->b_blkno == bp->b_lblkno) {
> - error = pcbmap(dep, de_bn2cn(dep->de_pmp, bp->b_lblkno),
> - &bp->b_blkno, 0, 0);
> + error = pcbmap(dep, bp->b_lblkno, &bp->b_blkno, 0, 0);
> if (error)
> bp->b_blkno = -1;
> if (bp->b_blkno == -1)
>
