From patchwork Mon Sep 29 10:56:25 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paolo Bonzini X-Patchwork-Id: 394348 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 9E1C2140097 for ; Mon, 29 Sep 2014 20:57:15 +1000 (EST) Received: from localhost ([::1]:35729 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XYYdk-0002ac-C5 for incoming@patchwork.ozlabs.org; Mon, 29 Sep 2014 06:57:12 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33163) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XYYdK-0002IV-HJ for qemu-devel@nongnu.org; Mon, 29 Sep 2014 06:56:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XYYdA-0003nP-9r for qemu-devel@nongnu.org; Mon, 29 Sep 2014 06:56:46 -0400 Received: from mail-wi0-x236.google.com ([2a00:1450:400c:c05::236]:44974) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XYYdA-0003lh-0f for qemu-devel@nongnu.org; Mon, 29 Sep 2014 06:56:36 -0400 Received: by mail-wi0-f182.google.com with SMTP id ex7so1251242wid.9 for ; Mon, 29 Sep 2014 03:56:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:message-id:date:from:user-agent:mime-version:to:subject :references:in-reply-to:content-type:content-transfer-encoding; bh=cSdMdaFEDxh5qqh6vhG5KzDMtS63MW7xvnl8R6FkzVk=; b=tq1p64ZEb6Y+jmKf9PhiEdrM+RipKcAEkCX4Z4ZkqQVE4q0okJPUGNp0fHF2jJEaOF 05lJRMZRwXKty66Lsuzfd9HsMjuCMjUqR5AaFiLuq5VATVL3J0DJNkE8k8S6dDWru992 jN9f1X9M+7vPACjfsYjYB5P8BFuneZRPeO8GmGLay9LtVEt2fF76DJw349V58YU5ucpy 9fOHLFpbQgOOKXJhjvKaf5Pd9MRXJtZYJjQOlsf1WNdukkSTA2kevJK0hjlhd2oQ3MOl Xf6sHAa/3hvBf3ltQSupOJXSPoHVzRrJT/wik0SIn2Euw+nEyn5Kzs7iWUgi55gw9FJH Lqxg== X-Received: by 10.180.98.227 with SMTP id el3mr46728656wib.13.1411988190106; Mon, 29 Sep 2014 03:56:30 -0700 (PDT) Received: from yakj.usersys.redhat.com (net-37-116-201-55.cust.vodafonedsl.it. [37.116.201.55]) by mx.google.com with ESMTPSA id cy10sm15312959wjb.21.2014.09.29.03.56.26 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 29 Sep 2014 03:56:28 -0700 (PDT) Message-ID: <54293AD9.8020701@redhat.com> Date: Mon, 29 Sep 2014 12:56:25 +0200 From: Paolo Bonzini User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.1 MIME-Version: 1.0 To: Fam Zheng , qemu-devel@nongnu.org References: <1411868881-19110-1-git-send-email-famz@redhat.com> <1411868881-19110-8-git-send-email-famz@redhat.com> <5429303C.6080100@redhat.com> In-Reply-To: <5429303C.6080100@redhat.com> X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2a00:1450:400c:c05::236 Subject: Re: [Qemu-devel] [PATCH v4 7/7] virtio-scsi: Handle TMF request cancellation asynchronously X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Il 29/09/2014 12:11, Paolo Bonzini ha scritto: > Il 28/09/2014 03:48, Fam Zheng ha scritto: >> + virtio_scsi_complete_req(req); >> + } else { >> + assert(r = -EINPROGRESS); >> + } >> } > > = instead of == here. > > Apart from this, the patch looks good. Thanks! Hmm, there's actually another problem. I think you cannot be sure that the two visits of d->requests see the same elements. In particular, one request could have non-NULL r->hba_private in the first loop, but it could become NULL by the time you reach the second loop. So we either use one loop only, or we have to save the list of requests in a separate list (for example a GSList). We can use a single loop by adding 1 to the "remaining" count while virtio_scsi_do_tmf is running, and dropping it at the end. However, you then add unnecessary complication to check for QUERY_TASK_SET around the allocation of the VirtIOSCSICancelTracker, and to free the VirtIOSCSICancelTracker if the TMF actually had nothing to do. If we move the "remaining" field inside VirtIOSCSIReq, things become much simpler. Can you review this incremental patch on top of yours? diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index 7a6b71a..9e56528 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -209,13 +209,8 @@ static void *virtio_scsi_load_request(QEMUFile *f, SCSIRequest *sreq) } typedef struct { + Notifier notifier; VirtIOSCSIReq *tmf_req; - int remaining; -} VirtIOSCSICancelTracker; - -typedef struct { - Notifier notifier; - VirtIOSCSICancelTracker *tracker; } VirtIOSCSICancelNotifier; static void virtio_scsi_cancel_notify(Notifier *notifier, void *data) @@ -224,9 +219,8 @@ static void virtio_scsi_cancel_notify(Notifier *notifier, void *data) VirtIOSCSICancelNotifier, notifier); - if (--n->tracker->remaining == 0) { - virtio_scsi_complete_req(n->tracker->tmf_req); - g_slice_free(VirtIOSCSICancelTracker, n->tracker); + if (--n->tmf_req->remaining == 0) { + virtio_scsi_complete_req(n->tmf_req); } g_slice_free(VirtIOSCSICancelNotifier, n); } @@ -241,7 +235,6 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req) BusChild *kid; int target; int ret = 0; - int cancel_count; if (s->dataplane_started && bdrv_get_aio_context(d->conf.bs) != s->ctx) { aio_context_acquire(s->ctx); @@ -280,15 +273,10 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req) req->resp.tmf.response = VIRTIO_SCSI_S_FUNCTION_SUCCEEDED; } else { VirtIOSCSICancelNotifier *notifier; - VirtIOSCSICancelTracker *tracker; - - notifier = g_slice_new(VirtIOSCSICancelNotifier); - notifier->notifier.notify - = virtio_scsi_cancel_notify; - tracker = g_slice_new(VirtIOSCSICancelTracker); - tracker->tmf_req = req; - tracker->remaining = 1; - notifier->tracker = tracker; + + req->remaining = 1; + notifier = g_slice_new(VirtIOSCSICancelNotifier); + notifier->notifier.notify = virtio_scsi_cancel_notify; scsi_req_cancel_async(r, ¬ifier->notifier); ret = -EINPROGRESS; } @@ -316,7 +304,13 @@ static int 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; + + /* Add 1 to "remaining" until virtio_scsi_do_tmf returns. + * This way, if the bus starts calling back to the notifiers + * even before we finish the loop, virtio_scsi_cancel_notify + * will not complete the TMF too early. + */ + req->remaining = 1; QTAILQ_FOREACH_SAFE(r, &d->requests, next, next) { if (r->hba_private) { if (req->req.tmf.subtype == VIRTIO_SCSI_T_TMF_QUERY_TASK_SET) { @@ -324,35 +318,19 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req) req->resp.tmf.response = VIRTIO_SCSI_S_FUNCTION_SUCCEEDED; break; } else { - /* 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_slice_new(VirtIOSCSICancelTracker); - tracker->tmf_req = req; - tracker->remaining = cancel_count; + VirtIOSCSICancelNotifier *notifier; - QTAILQ_FOREACH_SAFE(r, &d->requests, next, next) { - if (r->hba_private) { + req->remaining++; notifier = g_slice_new(VirtIOSCSICancelNotifier); notifier->notifier.notify = virtio_scsi_cancel_notify; - notifier->tracker = tracker; + notifier->tmf_req = req; scsi_req_cancel_async(r, ¬ifier->notifier); } } + } + if (--req->remaining > 0) { ret = -EINPROGRESS; } - break; case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET: diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h index 60dbfc9..d6e5e79 100644 --- a/include/hw/virtio/virtio-scsi.h +++ b/include/hw/virtio/virtio-scsi.h @@ -214,8 +214,13 @@ typedef struct VirtIOSCSIReq { /* Set by dataplane code. */ VirtIOSCSIVring *vring; - /* Used for two-stage request submission */ - QTAILQ_ENTRY(VirtIOSCSIReq) next; + union { + /* Used for two-stage request submission */ + QTAILQ_ENTRY(VirtIOSCSIReq) next; + + /* Used for cancellation of request during TMFs */ + int remaining; + }; SCSIRequest *sreq; size_t resp_size;