[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/3] scsi/virtio-scsi: Properly flush I/Os infli
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH 1/3] scsi/virtio-scsi: Properly flush I/Os inflight during an unplug |
Date: |
Tue, 14 Jan 2014 15:45:42 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130923 Thunderbird/17.0.9 |
Il 14/01/2014 15:24, Eric Farman ha scritto:
> When an unplug is triggered via QMP, the routine scsi_req_cancel
> is called to cancel any outstanding requests. However, the I/Os
> themselves were instantiated via an asynchronous call that will
> drive scsi_*_complete routines after the unplug call stack finishes.
> As all references to the request have been released by the cancel
> thread, the scsi_*_complete routines experience a range of failures
> when it attempts to manipulate the released storage.
This should never happen. See scsi_req_cancel:
void scsi_req_cancel(SCSIRequest *req)
{
trace_scsi_req_cancel(req->dev->id, req->lun, req->tag);
if (!req->enqueued) {
return;
}
scsi_req_ref(req);
scsi_req_dequeue(req);
req->io_canceled = true;
if (req->ops->cancel_io) {
req->ops->cancel_io(req);
}
if (req->bus->info->cancel) {
req->bus->info->cancel(req);
}
scsi_req_unref(req);
}
After req->ops->cancel_io returns, the following invariant must hold:
Any AIO callbacks will have been called before req->ops->cancel_io
returns, or they never will.
The invariant is also present in bdrv_aio_cancel, and should respected
at all levels: dma_aio_cancel in dma-helpers.c, thread_pool_cancel in
thread-pool.c, laio_cancel in block/linux-aio.c, and so on.
scsi_cancel_io (in hw/scsi/scsi-disk.c) is very careful in its handling
of reference counts and aiocb, with the exact purpose of triggering an
assertion failure if the invariant is not respected.
Now that I look more at the code, at least this patch is needed:
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index bce617c..ee1f5eb 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2306,6 +2306,7 @@ static const SCSIReqOps scsi_disk_emulate_reqops
.send_command = scsi_disk_emulate_command,
.read_data = scsi_disk_emulate_read_data,
.write_data = scsi_disk_emulate_write_data,
+ .cancel_io = scsi_cancel_io,
.get_buf = scsi_get_buf,
};
but it should only have an effect in very special cases, with commands
such as UNMAP, WRITE SAME or MODE SELECT.
Paolo