Date: Tue, 19 Feb 2013 15:35:51 +0100 From: Fabian Keil <freebsd-listen@fabiankeil.de> To: "Steven Hartland" <smh@freebsd.org> Cc: freebsd-fs@FreeBSD.org, John-Mark Gurney <jmg@funkthat.com> Subject: Re: ZFS on 9.1 doesn't see errors on geli volumes... Message-ID: <20130219153551.175ad31f@fabiankeil.de> In-Reply-To: <EF8E2A888F8D44FBA7362610150DAEA1@multiplay.co.uk> References: <20130218191242.GI55866@funkthat.com> <20130218200121.GJ55866@funkthat.com> <EF8E2A888F8D44FBA7362610150DAEA1@multiplay.co.uk>
next in thread | previous in thread | raw e-mail | index | archive | help
--Sig_/=MYq4uS8VhumqmOEv5iB7oq
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: quoted-printable
"Steven Hartland" <smh@freebsd.org> wrote:
> From: "John-Mark Gurney" <jmg@funkthat.com>
> > so, we just end up overwriting the bio_completed error...
> >
> > pjd, should we just put the bio_completed =3D line under an else?
> >
> > something like:
> > if (pbp->bio_error !=3D 0) {
> > G_ELI_LOGREQ(0, pbp, "Crypto WRITE request failed (error=3D%d).",
> > pbp->bio_error);
> > pbp->bio_completed =3D 0;
> > } else
> > pbp->bio_completed =3D pbp->bio_length;
> >
> > /* Write is finished, send it up. */
> > g_io_deliver(pbp, pbp->bio_error);
> > sc =3D pbp->bio_to->geom->softc;
> > atomic_subtract_int(&sc->sc_inflight, 1);
> >
> > But doesn't explain why read's aren't being counted though...
>=20
> Looks like the read case will loose the error if its not the last
> bio in sector group.
>=20
> The attached should fix both cases.
Works for me on 10-CURRENT, thanks.
> A question for someone familiar with geom: why is bio_completed
> not set to bio_length in the read success case? Is this correct
> or is this another little bug?
No idea, but I reported "another little bug" a while ago:
http://www.freebsd.org/cgi/query-pr.cgi?pr=3Dkern/162036
and while testing how the patch affects it, I discovered
that the panic can be prevented with:
diff --git a/sys/geom/eli/g_eli.c b/sys/geom/eli/g_eli.c
index 4e35297..24969b0 100644
--- a/sys/geom/eli/g_eli.c
+++ b/sys/geom/eli/g_eli.c
@@ -183,7 +183,8 @@ g_eli_read_done(struct bio *bp)
pbp->bio_driver2 =3D NULL;
}
g_io_deliver(pbp, pbp->bio_error);
- atomic_subtract_int(&sc->sc_inflight, 1);
+ if (sc !=3D NULL)
+ atomic_subtract_int(&sc->sc_inflight, 1);
return;
}
mtx_lock(&sc->sc_queue_mtx);
atomic_*_int(&sc->sc_inflight, 1) seems to be used without
checking that sc isn't NULL pretty much everywhere in g_eli.c,
though, and it's not clear to me when it's safe and when it isn't.
> On a related note, if anyone's got some pointers to docs about
> the internals of geom, I'd be interested :)
Every time I looked for internal geom documentation in the
past I came up empty.
Fabian
--Sig_/=MYq4uS8VhumqmOEv5iB7oq
Content-Type: application/pgp-signature; name=signature.asc
Content-Disposition: attachment; filename=signature.asc
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (FreeBSD)
iEYEARECAAYFAlEjjckACgkQBYqIVf93VJ23AQCfUjev5ucpAJ+pZy4TlhrecbwO
7XQAoLzeQ0t0lRZqyYGHB7xGpEfHuDSA
=Gebs
-----END PGP SIGNATURE-----
--Sig_/=MYq4uS8VhumqmOEv5iB7oq--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130219153551.175ad31f>