Date: Tue, 22 Jul 2014 10:46:24 -0700 From: Adrian Chadd <adrian@freebsd.org> To: Marcel Moolenaar <marcel@freebsd.org> Cc: "svn-src-head@freebsd.org" <svn-src-head@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "src-committers@freebsd.org" <src-committers@freebsd.org> Subject: Re: svn commit: r268986 - head/sys/geom/uzip Message-ID: <CAJ-Vmok0bBcTeKRgZED5DVvBP7Z6_GyCEGmTfCn5q2XOz95wwg@mail.gmail.com> In-Reply-To: <201407221730.s6MHU6Ej099771@svn.freebsd.org> References: <201407221730.s6MHU6Ej099771@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Wait, which consumers failed that test? -a On 22 July 2014 10:30, Marcel Moolenaar <marcel@freebsd.org> wrote: > Author: marcel > Date: Tue Jul 22 17:30:05 2014 > New Revision: 268986 > URL: http://svnweb.freebsd.org/changeset/base/268986 > > Log: > In r264504, we prevented doing I/O for more than MAXPHYS by making > the assumption that consumers would respect bio_completed and/or > bio_resid to detect short reads. This assumption proved false and > file corruption was the result. > Create as many bios as we need to satisfy the original request. > Check the cached chunk every time we need to do I/O to increase the > hit rate. > > Obtained from: junipre Networks, Inc. > MFC after: 1 week > > Modified: > head/sys/geom/uzip/g_uzip.c > > Modified: head/sys/geom/uzip/g_uzip.c > ============================================================================== > --- head/sys/geom/uzip/g_uzip.c Tue Jul 22 16:39:11 2014 (r268985) > +++ head/sys/geom/uzip/g_uzip.c Tue Jul 22 17:30:05 2014 (r268986) > @@ -1,5 +1,6 @@ > /*- > * Copyright (c) 2004 Max Khon > + * Copyright (c) 2014 Juniper Networks, Inc. > * All rights reserved. > * > * Redistribution and use in source and binary forms, with or without > @@ -86,6 +87,8 @@ struct g_uzip_softc { > int req_cached; /* cached requests */ > }; > > +static void g_uzip_done(struct bio *bp); > + > static void > g_uzip_softc_free(struct g_uzip_softc *sc, struct g_geom *gp) > { > @@ -120,213 +123,229 @@ z_free(void *nil, void *ptr) > free(ptr, M_GEOM_UZIP); > } > > +static int > +g_uzip_cached(struct g_geom *gp, struct bio *bp) > +{ > + struct g_uzip_softc *sc; > + off_t ofs; > + size_t blk, blkofs, usz; > + > + sc = gp->softc; > + ofs = bp->bio_offset + bp->bio_completed; > + blk = ofs / sc->blksz; > + mtx_lock(&sc->last_mtx); > + if (blk == sc->last_blk) { > + blkofs = ofs % sc->blksz; > + usz = sc->blksz - blkofs; > + if (bp->bio_resid < usz) > + usz = bp->bio_resid; > + memcpy(bp->bio_data + bp->bio_completed, sc->last_buf + blkofs, > + usz); > + sc->req_cached++; > + mtx_unlock(&sc->last_mtx); > + > + DPRINTF(("%s/%s: %p: offset=%jd: got %jd bytes from cache\n", > + __func__, gp->name, bp, (intmax_t)ofs, (intmax_t)usz)); > + > + bp->bio_completed += usz; > + bp->bio_resid -= usz; > + > + if (bp->bio_resid == 0) { > + g_io_deliver(bp, 0); > + return (1); > + } > + } else > + mtx_unlock(&sc->last_mtx); > + > + return (0); > +} > + > +static int > +g_uzip_request(struct g_geom *gp, struct bio *bp) > +{ > + struct g_uzip_softc *sc; > + struct bio *bp2; > + struct g_consumer *cp; > + struct g_provider *pp; > + off_t ofs; > + size_t start_blk, end_blk; > + > + if (g_uzip_cached(gp, bp) != 0) > + return (1); > + > + sc = gp->softc; > + > + bp2 = g_clone_bio(bp); > + if (bp2 == NULL) { > + g_io_deliver(bp, ENOMEM); > + return (1); > + } > + bp2->bio_done = g_uzip_done; > + > + cp = LIST_FIRST(&gp->consumer); > + pp = cp->provider; > + > + ofs = bp->bio_offset + bp->bio_completed; > + start_blk = ofs / sc->blksz; > + KASSERT(start_blk < sc->nblocks, ("start_blk out of range")); > + end_blk = (ofs + bp->bio_resid + sc->blksz - 1) / sc->blksz; > + KASSERT(end_blk <= sc->nblocks, ("end_blk out of range")); > + > + DPRINTF(("%s/%s: %p: start=%u (%jd), end=%u (%jd)\n", > + __func__, gp->name, bp, > + (u_int)start_blk, (intmax_t)sc->offsets[start_blk], > + (u_int)end_blk, (intmax_t)sc->offsets[end_blk])); > + > + bp2->bio_offset = sc->offsets[start_blk] - > + sc->offsets[start_blk] % pp->sectorsize; > + while (1) { > + bp2->bio_length = sc->offsets[end_blk] - bp2->bio_offset; > + bp2->bio_length = (bp2->bio_length + pp->sectorsize - 1) / > + pp->sectorsize * pp->sectorsize; > + if (bp2->bio_length <= MAXPHYS) > + break; > + > + end_blk--; > + } > + > + bp2->bio_data = malloc(bp2->bio_length, M_GEOM_UZIP, M_NOWAIT); > + if (bp2->bio_data == NULL) { > + g_destroy_bio(bp2); > + g_io_deliver(bp, ENOMEM); > + return (1); > + } > + > + DPRINTF(("%s/%s: %p: reading %jd bytes from offset %jd\n", > + __func__, gp->name, bp, > + (intmax_t)bp2->bio_length, (intmax_t)bp2->bio_offset)); > + > + g_io_request(bp2, cp); > + return (0); > +} > + > static void > g_uzip_done(struct bio *bp) > { > - int err; > - struct bio *bp2; > z_stream zs; > - struct g_provider *pp, *pp2; > + struct bio *bp2; > + struct g_provider *pp; > struct g_consumer *cp; > struct g_geom *gp; > struct g_uzip_softc *sc; > - off_t iolen, pos, upos; > - uint32_t start_blk, i; > - size_t bsize; > + char *data, *data2; > + off_t ofs; > + size_t blk, blkofs, len, ulen; > > bp2 = bp->bio_parent; > - pp = bp2->bio_to; > - gp = pp->geom; > - cp = LIST_FIRST(&gp->consumer); > - pp2 = cp->provider; > + gp = bp2->bio_to->geom; > sc = gp->softc; > - DPRINTF(("%s: done\n", gp->name)); > + > + cp = LIST_FIRST(&gp->consumer); > + pp = cp->provider; > > bp2->bio_error = bp->bio_error; > if (bp2->bio_error != 0) > goto done; > > - /* > - * Uncompress data. > - */ > + /* Make sure there's forward progress. */ > + if (bp->bio_completed == 0) { > + bp2->bio_error = ECANCELED; > + goto done; > + } > + > zs.zalloc = z_alloc; > zs.zfree = z_free; > - err = inflateInit(&zs); > - if (err != Z_OK) { > - bp2->bio_error = EIO; > + if (inflateInit(&zs) != Z_OK) { > + bp2->bio_error = EILSEQ; > goto done; > } > - start_blk = bp2->bio_offset / sc->blksz; > - bsize = pp2->sectorsize; > - iolen = bp->bio_completed; > - pos = sc->offsets[start_blk] % bsize; > - upos = 0; > - DPRINTF(("%s: done: start_blk %d, pos %jd, upos %jd, iolen %jd " > - "(%jd, %d, %zd)\n", > - gp->name, start_blk, (intmax_t)pos, (intmax_t)upos, > - (intmax_t)iolen, (intmax_t)bp2->bio_offset, sc->blksz, bsize)); > - for (i = start_blk; upos < bp2->bio_length; i++) { > - off_t len, ulen, uoff; > - > - uoff = i == start_blk ? bp2->bio_offset % sc->blksz : 0; > - ulen = MIN(sc->blksz - uoff, bp2->bio_length - upos); > - len = sc->offsets[i + 1] - sc->offsets[i]; > > + ofs = bp2->bio_offset + bp2->bio_completed; > + blk = ofs / sc->blksz; > + blkofs = ofs % sc->blksz; > + data = bp->bio_data + sc->offsets[blk] % pp->sectorsize; > + data2 = bp2->bio_data + bp2->bio_completed; > + while (bp->bio_completed && bp2->bio_resid) { > + ulen = MIN(sc->blksz - blkofs, bp2->bio_resid); > + len = sc->offsets[blk + 1] - sc->offsets[blk]; > + DPRINTF(("%s/%s: %p/%ju: data2=%p, ulen=%u, data=%p, len=%u\n", > + __func__, gp->name, gp, bp->bio_completed, > + data2, (u_int)ulen, data, (u_int)len)); > if (len == 0) { > /* All zero block: no cache update */ > - bzero(bp2->bio_data + upos, ulen); > - upos += ulen; > - bp2->bio_completed += ulen; > - continue; > - } > - if (len > iolen) { > - DPRINTF(("%s: done: early termination: len (%jd) > " > - "iolen (%jd)\n", > - gp->name, (intmax_t)len, (intmax_t)iolen)); > - break; > - } > - zs.next_in = bp->bio_data + pos; > - zs.avail_in = len; > - zs.next_out = sc->last_buf; > - zs.avail_out = sc->blksz; > - mtx_lock(&sc->last_mtx); > - err = inflate(&zs, Z_FINISH); > - if (err != Z_STREAM_END) { > - sc->last_blk = -1; > + bzero(data2, ulen); > + } else if (len <= bp->bio_completed) { > + zs.next_in = data; > + zs.avail_in = len; > + zs.next_out = sc->last_buf; > + zs.avail_out = sc->blksz; > + mtx_lock(&sc->last_mtx); > + if (inflate(&zs, Z_FINISH) != Z_STREAM_END) { > + sc->last_blk = -1; > + mtx_unlock(&sc->last_mtx); > + inflateEnd(&zs); > + bp2->bio_error = EILSEQ; > + goto done; > + } > + sc->last_blk = blk; > + memcpy(data2, sc->last_buf + blkofs, ulen); > mtx_unlock(&sc->last_mtx); > - DPRINTF(("%s: done: inflate failed (%jd + %jd -> %jd + %jd + %jd)\n", > - gp->name, (intmax_t)pos, (intmax_t)len, > - (intmax_t)uoff, (intmax_t)upos, (intmax_t)ulen)); > - inflateEnd(&zs); > - bp2->bio_error = EIO; > - goto done; > - } > - sc->last_blk = i; > - DPRINTF(("%s: done: inflated %jd + %jd -> %jd + %jd + %jd\n", > - gp->name, (intmax_t)pos, (intmax_t)len, (intmax_t)uoff, > - (intmax_t)upos, (intmax_t)ulen)); > - memcpy(bp2->bio_data + upos, sc->last_buf + uoff, ulen); > - mtx_unlock(&sc->last_mtx); > + if (inflateReset(&zs) != Z_OK) { > + inflateEnd(&zs); > + bp2->bio_error = EILSEQ; > + goto done; > + } > + data += len; > + } else > + break; > > - pos += len; > - iolen -= len; > - upos += ulen; > + data2 += ulen; > bp2->bio_completed += ulen; > - err = inflateReset(&zs); > - if (err != Z_OK) { > - inflateEnd(&zs); > - bp2->bio_error = EIO; > - goto done; > - } > - } > - err = inflateEnd(&zs); > - if (err != Z_OK) { > - bp2->bio_error = EIO; > - goto done; > + bp2->bio_resid -= ulen; > + bp->bio_completed -= len; > + blkofs = 0; > + blk++; > } > > + if (inflateEnd(&zs) != Z_OK) > + bp2->bio_error = EILSEQ; > + > done: > - /* > - * Finish processing the request. > - */ > - DPRINTF(("%s: done: (%d, %jd, %ld)\n", > - gp->name, bp2->bio_error, (intmax_t)bp2->bio_completed, > - bp2->bio_resid)); > + /* Finish processing the request. */ > free(bp->bio_data, M_GEOM_UZIP); > g_destroy_bio(bp); > - g_io_deliver(bp2, bp2->bio_error); > + if (bp2->bio_error != 0 || bp2->bio_resid == 0) > + g_io_deliver(bp2, bp2->bio_error); > + else > + g_uzip_request(gp, bp2); > } > > static void > g_uzip_start(struct bio *bp) > { > - struct bio *bp2; > - struct g_provider *pp, *pp2; > + struct g_provider *pp; > struct g_geom *gp; > - struct g_consumer *cp; > struct g_uzip_softc *sc; > - uint32_t start_blk, end_blk; > - size_t bsize; > > pp = bp->bio_to; > gp = pp->geom; > - DPRINTF(("%s: start (%d)\n", gp->name, bp->bio_cmd)); > > - if (bp->bio_cmd != BIO_READ) { > - g_io_deliver(bp, EOPNOTSUPP); > - return; > - } > + DPRINTF(("%s/%s: %p: cmd=%d, offset=%jd, length=%jd, buffer=%p\n", > + __func__, gp->name, bp, bp->bio_cmd, (intmax_t)bp->bio_offset, > + (intmax_t)bp->bio_length, bp->bio_data)); > > - cp = LIST_FIRST(&gp->consumer); > - pp2 = cp->provider; > sc = gp->softc; > - > - start_blk = bp->bio_offset / sc->blksz; > - end_blk = (bp->bio_offset + bp->bio_length + sc->blksz - 1) / sc->blksz; > - KASSERT(start_blk < sc->nblocks, ("start_blk out of range")); > - KASSERT(end_blk <= sc->nblocks, ("end_blk out of range")); > - > sc->req_total++; > - if (start_blk + 1 == end_blk) { > - mtx_lock(&sc->last_mtx); > - if (start_blk == sc->last_blk) { > - off_t uoff; > - > - uoff = bp->bio_offset % sc->blksz; > - KASSERT(bp->bio_length <= sc->blksz - uoff, > - ("cached data error")); > - memcpy(bp->bio_data, sc->last_buf + uoff, > - bp->bio_length); > - sc->req_cached++; > - mtx_unlock(&sc->last_mtx); > > - DPRINTF(("%s: start: cached 0 + %jd, %jd + 0 + %jd\n", > - gp->name, (intmax_t)bp->bio_length, (intmax_t)uoff, > - (intmax_t)bp->bio_length)); > - bp->bio_completed = bp->bio_length; > - g_io_deliver(bp, 0); > - return; > - } > - mtx_unlock(&sc->last_mtx); > - } > - > - bp2 = g_clone_bio(bp); > - if (bp2 == NULL) { > - g_io_deliver(bp, ENOMEM); > + if (bp->bio_cmd != BIO_READ) { > + g_io_deliver(bp, EOPNOTSUPP); > return; > } > - bp2->bio_done = g_uzip_done; > - DPRINTF(("%s: start (%d..%d), %s: %d + %jd, %s: %d + %jd\n", > - gp->name, start_blk, end_blk, > - pp->name, pp->sectorsize, (intmax_t)pp->mediasize, > - pp2->name, pp2->sectorsize, (intmax_t)pp2->mediasize)); > - bsize = pp2->sectorsize; > - bp2->bio_offset = sc->offsets[start_blk] - sc->offsets[start_blk] % bsize; > - while (1) { > - bp2->bio_length = sc->offsets[end_blk] - bp2->bio_offset; > - bp2->bio_length = (bp2->bio_length + bsize - 1) / bsize * bsize; > - if (bp2->bio_length < MAXPHYS) > - break; > > - end_blk--; > - DPRINTF(("%s: bio_length (%jd) > MAXPHYS: lowering end_blk " > - "to %u\n", gp->name, (intmax_t)bp2->bio_length, end_blk)); > - } > - DPRINTF(("%s: start %jd + %jd -> %ju + %ju -> %jd + %jd\n", > - gp->name, > - (intmax_t)bp->bio_offset, (intmax_t)bp->bio_length, > - (uintmax_t)sc->offsets[start_blk], > - (uintmax_t)sc->offsets[end_blk] - sc->offsets[start_blk], > - (intmax_t)bp2->bio_offset, (intmax_t)bp2->bio_length)); > - bp2->bio_data = malloc(bp2->bio_length, M_GEOM_UZIP, M_NOWAIT); > - if (bp2->bio_data == NULL) { > - g_destroy_bio(bp2); > - g_io_deliver(bp, ENOMEM); > - return; > - } > + bp->bio_resid = bp->bio_length; > + bp->bio_completed = 0; > > - g_io_request(bp2, cp); > - DPRINTF(("%s: start ok\n", gp->name)); > + g_uzip_request(gp, bp); > } > > static void >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-Vmok0bBcTeKRgZED5DVvBP7Z6_GyCEGmTfCn5q2XOz95wwg>