Re: [PATCH v3 5/6] virtio-scsi: fix various bad behavior on aborted requests
On 6/4/14, Paolo Bonzini wrote: > Il 04/06/2014 19:29, Venkatesh Srinivas ha scritto: >> Do you really want to poll the request VQs for completions if the TMF >> was rejected? > > I wasn't sure, but bugs in this path are hard enough that I preferred > the safer patch. Ok. As long as it was deliberate. I'd slightly prefer only doing so in the success case, but simplicity is a compelling argument :) Reviewed-by: Venkatesh Srinivas -- vs; -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 5/6] virtio-scsi: fix various bad behavior on aborted requests
Il 04/06/2014 19:29, Venkatesh Srinivas ha scritto: Do you really want to poll the request VQs for completions if the TMF was rejected? I wasn't sure, but bugs in this path are hard enough that I preferred the safer patch. TMF ABORT may return FUNCTION REJECTED if the command to abort completed before the device saw the TMF ABORT message, for example. In such cases, this would unnecessarily lengthen the EH path. The cost of virtscsi_poll_requests should be nothing compared to the delay between the timeout and the invocation of the delayed_work, no? Paolo -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 5/6] virtio-scsi: fix various bad behavior on aborted requests
On 6/4/14, Paolo Bonzini wrote:
> Even though the virtio-scsi spec guarantees that all requests related
> to the TMF will have been completed by the time the TMF itself completes,
> the request queue's callback might not have run yet. This causes requests
> to be completed more than once, and as a result triggers a variety of
> BUGs or oopses.
>
> Cc: sta...@vger.kernel.org
> Signed-off-by: Paolo Bonzini
> ---
> drivers/scsi/virtio_scsi.c | 22 ++
> 1 file changed, 22 insertions(+)
>
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index d66c4ee2c774..fda9fb35 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -235,6 +235,16 @@ static void virtscsi_req_done(struct virtqueue *vq)
> virtscsi_vq_done(vscsi, req_vq, virtscsi_complete_cmd);
> };
>
> +static void virtscsi_poll_requests(struct virtio_scsi *vscsi)
> +{
> + int i, num_vqs;
> +
> + num_vqs = vscsi->num_queues;
> + for (i = 0; i < num_vqs; i++)
> + virtscsi_vq_done(vscsi, &vscsi->req_vqs[i],
> + virtscsi_complete_cmd);
> +}
> +
> static void virtscsi_complete_free(struct virtio_scsi *vscsi, void *buf)
> {
> struct virtio_scsi_cmd *cmd = buf;
> @@ -579,6 +589,18 @@ static int virtscsi_tmf(struct virtio_scsi *vscsi,
> struct virtio_scsi_cmd *cmd)
> cmd->resp.tmf.response == VIRTIO_SCSI_S_FUNCTION_SUCCEEDED)
> ret = SUCCESS;
>
> + /*
> + * The spec guarantees that all requests related to the TMF have
> + * been completed, but the callback might not have run yet if
> + * we're using independent interrupts (e.g. MSI). Poll the
> + * virtqueues once.
> + *
> + * In the abort case, sc->scsi_done will do nothing, because
> + * the block layer must have detected a timeout and as a result
> + * REQ_ATOM_COMPLETE has been set.
> + */
> + virtscsi_poll_requests(vscsi);
Do you really want to poll the request VQs for completions if the TMF
was rejected?
TMF ABORT may return FUNCTION REJECTED if the command to abort
completed before the device saw the TMF ABORT message, for example. In
such cases, this would
unnecessarily lengthen the EH path.
> +
> out:
> mempool_free(cmd, virtscsi_cmd_pool);
> return ret;
> --
> 1.8.3.1
Thanks for looking into this,
-- vs;
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 5/6] virtio-scsi: fix various bad behavior on aborted requests
Even though the virtio-scsi spec guarantees that all requests related
to the TMF will have been completed by the time the TMF itself completes,
the request queue's callback might not have run yet. This causes requests
to be completed more than once, and as a result triggers a variety of
BUGs or oopses.
Cc: sta...@vger.kernel.org
Signed-off-by: Paolo Bonzini
---
drivers/scsi/virtio_scsi.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index d66c4ee2c774..fda9fb35 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -235,6 +235,16 @@ static void virtscsi_req_done(struct virtqueue *vq)
virtscsi_vq_done(vscsi, req_vq, virtscsi_complete_cmd);
};
+static void virtscsi_poll_requests(struct virtio_scsi *vscsi)
+{
+ int i, num_vqs;
+
+ num_vqs = vscsi->num_queues;
+ for (i = 0; i < num_vqs; i++)
+ virtscsi_vq_done(vscsi, &vscsi->req_vqs[i],
+virtscsi_complete_cmd);
+}
+
static void virtscsi_complete_free(struct virtio_scsi *vscsi, void *buf)
{
struct virtio_scsi_cmd *cmd = buf;
@@ -579,6 +589,18 @@ static int virtscsi_tmf(struct virtio_scsi *vscsi, struct
virtio_scsi_cmd *cmd)
cmd->resp.tmf.response == VIRTIO_SCSI_S_FUNCTION_SUCCEEDED)
ret = SUCCESS;
+ /*
+* The spec guarantees that all requests related to the TMF have
+* been completed, but the callback might not have run yet if
+* we're using independent interrupts (e.g. MSI). Poll the
+* virtqueues once.
+*
+* In the abort case, sc->scsi_done will do nothing, because
+* the block layer must have detected a timeout and as a result
+* REQ_ATOM_COMPLETE has been set.
+*/
+ virtscsi_poll_requests(vscsi);
+
out:
mempool_free(cmd, virtscsi_cmd_pool);
return ret;
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
