Il 24/09/2014 10:27, Fam Zheng ha scritto:
> For VIRTIO_SCSI_T_TMF_ABORT_TASK and VIRTIO_SCSI_T_TMF_ABORT_TASK_SET,
> use scsi_req_cancel_async to start the cancellation.
>
> Because each tmf command may cancel multiple requests, we need to use a
> counter to track the number of remaining requests we still need to wait
> for.
>
> Signed-off-by: Fam Zheng <f...@redhat.com>
> ---
> hw/scsi/virtio-scsi.c | 84
> ++++++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 77 insertions(+), 7 deletions(-)
>
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index fa36e23..9bd7d8a 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -208,12 +208,40 @@ static void *virtio_scsi_load_request(QEMUFile *f,
> SCSIRequest *sreq)
> return req;
> }
>
> -static void virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
> +typedef struct {
> + VirtIOSCSIReq *tmf_req;
> + int remaining;
> +} VirtIOSCSICancelTracker;
What about putting "remaining" directly in VirtIOSCSIReq?
> +typedef struct {
> + Notifier notifier;
> + VirtIOSCSICancelTracker *tracker;
> +} VirtIOSCSICancelNotifier;
> +
> +static void virtio_scsi_cancel_notify(Notifier *notifier, void *data)
> +{
> + VirtIOSCSICancelNotifier *n = container_of(notifier,
> + VirtIOSCSICancelNotifier,
> + notifier);
> +
> + if (--n->tracker->remaining == 0) {
> + virtio_scsi_complete_req(n->tracker->tmf_req);
> + g_free(n->tracker);
> + }
> + g_free(n);
> +}
> +
> +/* Return true if the request is ready to be completed and return to guest;
> + * false if the request will be completed (by some other events) later, for
> + * example in the case of async cancellation. */
> +static bool virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
Perhaps return 0/-EINPROGRESS so that it's easier to remember the
calling convention?
> {
> SCSIDevice *d = virtio_scsi_device_find(s, req->req.tmf.lun);
> SCSIRequest *r, *next;
> BusChild *kid;
> int target;
> + bool ret = true;
> + int cancel_count;
>
> if (s->dataplane_started && bdrv_get_aio_context(d->conf.bs) != s->ctx) {
> aio_context_acquire(s->ctx);
> @@ -251,7 +279,18 @@ static void virtio_scsi_do_tmf(VirtIOSCSI *s,
> VirtIOSCSIReq *req)
> */
> req->resp.tmf.response = VIRTIO_SCSI_S_FUNCTION_SUCCEEDED;
> } else {
> - scsi_req_cancel(r);
> + VirtIOSCSICancelNotifier *notifier;
> + VirtIOSCSICancelTracker *tracker;
> +
> + notifier = g_new(VirtIOSCSICancelNotifier, 1);
Slice allocator?
> + notifier->notifier.notify
> + = virtio_scsi_cancel_notify;
> + tracker = g_new(VirtIOSCSICancelTracker, 1);
Same here if you keep VirtIOSCSICancelTracker.
> + tracker->tmf_req = req;
> + tracker->remaining = 1;
> + notifier->tracker = tracker;
> + scsi_req_cancel_async(r, ¬ifier->notifier);
> + ret = false;
> }
> }
> break;
> @@ -277,6 +316,7 @@ static void virtio_scsi_do_tmf(VirtIOSCSI *s,
> VirtIOSCSIReq *req)
> if (d->lun != virtio_scsi_get_lun(req->req.tmf.lun)) {
> goto incorrect_lun;
> }
> + cancel_count = 0;
> QTAILQ_FOREACH_SAFE(r, &d->requests, next, next) {
> if (r->hba_private) {
> if (req->req.tmf.subtype ==
> VIRTIO_SCSI_T_TMF_QUERY_TASK_SET) {
> @@ -286,10 +326,36 @@ static void virtio_scsi_do_tmf(VirtIOSCSI *s,
> VirtIOSCSIReq *req)
> req->resp.tmf.response =
> VIRTIO_SCSI_S_FUNCTION_SUCCEEDED;
> break;
> } else {
> - scsi_req_cancel(r);
> + /* Before we actually cancel any requests in the next for
> + * loop, let's count them. This way, if the bus starts
> + * calling back to the notifier even before we finish the
> + * loop, the counter, which value is already seen in
> + * virtio_scsi_cancel_notify, will prevent us from
> + * completing the tmf too quickly. */
> + cancel_count++;
> }
> }
> }
> + if (cancel_count) {
> + VirtIOSCSICancelNotifier *notifier;
> + VirtIOSCSICancelTracker *tracker;
> +
> + tracker = g_new(VirtIOSCSICancelTracker, 1);
Same as above.
> + tracker->tmf_req = req;
> + tracker->remaining = cancel_count;
> +
> + QTAILQ_FOREACH_SAFE(r, &d->requests, next, next) {
> + if (r->hba_private) {
> + notifier = g_new(VirtIOSCSICancelNotifier, 1);
Same as above.
> + notifier->notifier.notify
> + = virtio_scsi_cancel_notify;
> + notifier->tracker = tracker;
> + scsi_req_cancel_async(r, ¬ifier->notifier);
> + }
> + }
> + ret = false;
> + }
> +
> break;
>
> case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET:
> @@ -310,20 +376,22 @@ static void virtio_scsi_do_tmf(VirtIOSCSI *s,
> VirtIOSCSIReq *req)
> break;
> }
>
> - return;
> + return ret;
>
> incorrect_lun:
> req->resp.tmf.response = VIRTIO_SCSI_S_INCORRECT_LUN;
> - return;
> + return ret;
>
> fail:
> req->resp.tmf.response = VIRTIO_SCSI_S_BAD_TARGET;
> + return ret;
> }
>
> void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s, VirtIOSCSIReq *req)
> {
> VirtIODevice *vdev = (VirtIODevice *)s;
> int type;
> + bool should_complete = true;
>
> if (iov_to_buf(req->elem.out_sg, req->elem.out_num, 0,
> &type, sizeof(type)) < sizeof(type)) {
> @@ -337,7 +405,7 @@ void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s,
> VirtIOSCSIReq *req)
> sizeof(VirtIOSCSICtrlTMFResp)) < 0) {
> virtio_scsi_bad_req();
> } else {
> - virtio_scsi_do_tmf(s, req);
> + should_complete = virtio_scsi_do_tmf(s, req);
> }
>
> } else if (req->req.tmf.type == VIRTIO_SCSI_T_AN_QUERY ||
> @@ -350,7 +418,9 @@ void virtio_scsi_handle_ctrl_req(VirtIOSCSI *s,
> VirtIOSCSIReq *req)
> req->resp.an.response = VIRTIO_SCSI_S_OK;
> }
> }
> - virtio_scsi_complete_req(req);
> + if (should_complete) {
> + virtio_scsi_complete_req(req);
> + }
> }
>
> static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
>
Very nice apart from these comments.
Paolo
