Hi,
On Mon, 13 Mar 2017, Jiri B wrote:
>
> it seems virtio-scsi is not working correctly in OpenBSD, I gave it
> a try today and OpenBSD VM was killed with:
>
> 2017-03-13T15:29:00.814657Z qemu-kvm: wrong size for virtio-scsi headers
>
> on EL7 with qemu-kvm-rhev-2.6.0-28.el7_3.6.x86_64.
>
> I found a bug stating it is OpenBSD's fault
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=768517
>
> I'd like to provide more info but could you give me some hints
> please? I tried to attach debugger to qemu-kvm process but I get
> only this :/
last time I looked at this I found a bug how vioscsi uses
virtio_dequeue_commit(). After that was fixed, it did not cause qemu to
complain anymore but there were occasional data errors that would result
in filesystem corruption. I don't have a really good idea how to debug
that. Maybe write a test program that writes known patterns to the raw
disk and then reads them again and shows the diffs. Or does anyone know a
program that does this? In any case, I ran out of time before I got any
further.
The attached diff fixes the bug and adds lots of debug output. If you
comment out the printfs, you could try if you have more luck. But this is
probably more a topic for tech@
Cheers,
Stefan
diff --git sys/dev/pv/vioscsi.c sys/dev/pv/vioscsi.c
index 6a8bb55..1783bd5 100644
--- sys/dev/pv/vioscsi.c
+++ sys/dev/pv/vioscsi.c
@@ -32,12 +32,23 @@
enum { vioscsi_debug = 0 };
#define DPRINTF(f...) do { if (vioscsi_debug) printf(f); } while (0)
+#define STATE_ASSERT(vr, want) do { \
+ if (vr->vr_state != want) { \
+ panic("%s:%d: vr_state is %d should be %d\n", \
+ __func__, __LINE__, vr->vr_state, want); \
+ } \
+ } while (0)
+
+
+enum vioscsi_req_state { FREE, ALLOC, INQUEUE, DONE };
+
struct vioscsi_req {
struct virtio_scsi_req_hdr vr_req;
struct virtio_scsi_res_hdr vr_res;
struct scsi_xfer *vr_xs;
bus_dmamap_t vr_control;
bus_dmamap_t vr_data;
+ enum vioscsi_req_state vr_state;
};
struct vioscsi_softc {
@@ -166,16 +177,19 @@ vioscsi_scsi_cmd(struct scsi_xfer *xs)
struct virtio_scsi_req_hdr *req = &vr->vr_req;
struct virtqueue *vq = &sc->sc_vqs[2];
int slot = vr - sc->sc_reqs;
+ int ec = 0;
DPRINTF("vioscsi_scsi_cmd: enter\n");
+ STATE_ASSERT(vr, ALLOC);
// TODO(matthew): Support bidirectional SCSI commands?
if ((xs->flags & (SCSI_DATA_IN | SCSI_DATA_OUT))
== (SCSI_DATA_IN | SCSI_DATA_OUT)) {
+ ec = __LINE__;
stuffup:
xs->error = XS_DRIVER_STUFFUP;
xs->resid = xs->datalen;
- DPRINTF("vioscsi_scsi_cmd: stuffup\n");
+ printf("vioscsi_scsi_cmd: stuffup l.%d\n", ec);
scsi_done(xs);
return;
}
@@ -187,16 +201,20 @@ vioscsi_scsi_cmd(struct scsi_xfer *xs)
* 1, second byte set to target, third and fourth byte representing a
* single level LUN structure, followed by four zero bytes."
*/
- if (xs->sc_link->target >= 256 || xs->sc_link->lun >= 16384)
+ if (xs->sc_link->target >= 256 || xs->sc_link->lun >= 16384) {
+ ec = __LINE__;
goto stuffup;
+ }
req->lun[0] = 1;
req->lun[1] = xs->sc_link->target;
req->lun[2] = 0x40 | (xs->sc_link->lun >> 8);
req->lun[3] = xs->sc_link->lun;
memset(req->lun + 4, 0, 4);
- if ((size_t)xs->cmdlen > sizeof(req->cdb))
+ if ((size_t)xs->cmdlen > sizeof(req->cdb)) {
+ ec = __LINE__;
goto stuffup;
+ }
memset(req->cdb, 0, sizeof(req->cdb));
memcpy(req->cdb, xs->cmd, xs->cmdlen);
@@ -207,16 +225,21 @@ vioscsi_scsi_cmd(struct scsi_xfer *xs)
if (bus_dmamap_load(vsc->sc_dmat, vr->vr_data,
xs->data, xs->datalen, NULL,
((isread ? BUS_DMA_READ : BUS_DMA_WRITE) |
- BUS_DMA_NOWAIT)))
+ BUS_DMA_NOWAIT))) {
+ ec = __LINE__;
goto stuffup;
+ }
nsegs += vr->vr_data->dm_nsegs;
}
int s = splbio();
int r = virtio_enqueue_reserve(vq, slot, nsegs);
splx(s);
- if (r)
+ if (r) {
+ ec = __LINE__;
+ printf("nsegs: %d seg_max: %d datalen %d isread %d\n", nsegs,
sc->sc_seg_max, xs->datalen, isread);
goto stuffup;
+ }
bus_dmamap_sync(vsc->sc_dmat, vr->vr_control,
offsetof(struct vioscsi_req, vr_req),
@@ -245,6 +268,7 @@ vioscsi_scsi_cmd(struct scsi_xfer *xs)
virtio_enqueue(vq, slot, vr->vr_data, 0);
virtio_enqueue_commit(vsc, vq, slot, 1);
+ vr->vr_state = INQUEUE;
if (ISSET(xs->flags, SCSI_POLL)) {
DPRINTF("vioscsi_scsi_cmd: polling...\n");
@@ -259,7 +283,7 @@ vioscsi_scsi_cmd(struct scsi_xfer *xs)
// TODO(matthew): Abort the request.
xs->error = XS_TIMEOUT;
xs->resid = xs->datalen;
- DPRINTF("vioscsi_scsi_cmd: polling timeout\n");
+ panic("vioscsi_scsi_cmd: polling timeout\n");
scsi_done(xs);
}
DPRINTF("vioscsi_scsi_cmd: done (timeout=%d)\n", timeout);
@@ -272,8 +296,8 @@ vioscsi_req_done(struct vioscsi_softc *sc, struct
virtio_softc *vsc,
struct vioscsi_req *vr)
{
struct scsi_xfer *xs = vr->vr_xs;
-
- DPRINTF("vioscsi_req_done: enter\n");
+ STATE_ASSERT(vr, INQUEUE);
+ DPRINTF("vioscsi_req_done: enter vr: %p xs: %p\n", vr, xs);
int isread = !!(xs->flags & SCSI_DATA_IN);
bus_dmamap_sync(vsc->sc_dmat, vr->vr_control,
@@ -291,7 +315,7 @@ vioscsi_req_done(struct vioscsi_softc *sc, struct
virtio_softc *vsc,
if (vr->vr_res.response != VIRTIO_SCSI_S_OK) {
xs->error = XS_DRIVER_STUFFUP;
xs->resid = xs->datalen;
- DPRINTF("vioscsi_req_done: stuffup: %d\n", vr->vr_res.response);
+ printf("vioscsi_req_done: stuffup: %d\n", vr->vr_res.response);
goto done;
}
@@ -307,6 +331,7 @@ vioscsi_req_done(struct vioscsi_softc *sc, struct
virtio_softc *vsc,
done:
vr->vr_xs = NULL;
+ vr->vr_state = DONE;
scsi_done(xs);
}
@@ -374,6 +399,8 @@ vioscsi_req_get(void *cookie)
goto err4;
DPRINTF("vioscsi_req_get: %p, %d\n", vr, slot);
+ STATE_ASSERT(vr, FREE);
+ vr->vr_state = ALLOC;
return (vr);
@@ -383,6 +410,7 @@ err3:
bus_dmamap_destroy(vsc->sc_dmat, vr->vr_control);
err2:
s = splbio();
+ printf("%s: virtio_enqueue_abort\n", __func__);
virtio_enqueue_abort(vq, slot);
splx(s);
err1:
@@ -404,7 +432,14 @@ vioscsi_req_put(void *cookie, void *io)
bus_dmamap_destroy(vsc->sc_dmat, vr->vr_data);
int s = splbio();
- virtio_dequeue_commit(vq, slot);
+ if (vr->vr_state == DONE) {
+ virtio_dequeue_commit(vq, slot);
+ } else if (vr->vr_state == ALLOC) {
+ // virtio_enqueue_abort(vq, slot);
+ } else {
+ panic("invalid vr_state[%d]: %d", slot, vr->vr_state);
+ }
+ vr->vr_state = FREE;
splx(s);
}
