[Qemu-devel] [PATCH for-2.8] megasas: clean up and fix request completion/cancellation

[Qemu-devel] [PATCH for-2.8] megasas: clean up and fix request completion/cancellation

[Paolo Bonzini]

megasas_command_cancel is a callback; it should report the abort in
the frame, not try another abort!  Compare for instance with
mptsas_request_cancelled.

So extract the common bits for request completion in a new function
megasas_complete_command, call it from both the .complete and .cancel
callbacks, and remove duplicate pieces from the DCMD path.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/megasas.c | 53 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 28 insertions(+), 25 deletions(-)

diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index ca62952..67fc1e7 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -300,12 +300,6 @@ unmap:
     return iov_count - i;
 }
 
-static void megasas_unmap_sgl(MegasasCmd *cmd)
-{
-    qemu_sglist_destroy(&cmd->qsg);
-    cmd->iov_offset = 0;
-}
-
 /*
  * passthrough sense and io sense are at the same offset
  */
@@ -580,6 +574,20 @@ static void megasas_complete_frame(MegasasState *s, uint64_t context)
     }
 }
 
+static void megasas_complete_command(MegasasCmd *cmd)
+{
+    qemu_sglist_destroy(&cmd->qsg);
+    cmd->iov_size = 0;
+    cmd->iov_offset = 0;
+
+    cmd->req->hba_private = NULL;
+    scsi_req_unref(cmd->req);
+    cmd->req = NULL;
+
+    megasas_unmap_frame(cmd->state, cmd);
+    megasas_complete_frame(cmd->state, cmd->context);
+}
+
 static void megasas_reset_frames(MegasasState *s)
 {
     int i;
@@ -596,9 +604,9 @@ static void megasas_reset_frames(MegasasState *s)
 
 static void megasas_abort_command(MegasasCmd *cmd)
 {
-    if (cmd->req) {
+    /* Never abort internal commands.  */
+    if (cmd->req != NULL) {
         scsi_req_cancel(cmd->req);
-        cmd->req = NULL;
     }
 }
 
@@ -689,9 +697,6 @@ static void megasas_finish_dcmd(MegasasCmd *cmd, uint32_t iov_size)
 {
     trace_megasas_finish_dcmd(cmd->index, iov_size);
 
-    if (cmd->frame->header.sge_count) {
-        qemu_sglist_destroy(&cmd->qsg);
-    }
     if (iov_size > cmd->iov_size) {
         if (megasas_frame_is_ieee_sgl(cmd)) {
             cmd->frame->dcmd.sgl.sg_skinny->len = cpu_to_le32(iov_size);
@@ -701,7 +706,6 @@ static void megasas_finish_dcmd(MegasasCmd *cmd, uint32_t iov_size)
             cmd->frame->dcmd.sgl.sg32->len = cpu_to_le32(iov_size);
         }
     }
-    cmd->iov_size = 0;
 }
 
 static int megasas_ctrl_get_info(MegasasState *s, MegasasCmd *cmd)
@@ -1589,7 +1593,6 @@ static int megasas_finish_internal_dcmd(MegasasCmd *cmd,
     int lun = req->lun;
 
     opcode = le32_to_cpu(cmd->frame->dcmd.opcode);
-    scsi_req_unref(req);
     trace_megasas_dcmd_internal_finish(cmd->index, opcode, lun);
     switch (opcode) {
     case MFI_DCMD_PD_GET_INFO:
@@ -1860,7 +1863,11 @@ static void megasas_command_complete(SCSIRequest *req, uint32_t status,
 
     trace_megasas_command_complete(cmd->index, status, resid);
 
-    if (cmd->req != req) {
+    if (req->io_canceled) {
+        return;
+    }
+
+    if (cmd->req == NULL) {
         /*
          * Internal command complete
          */
@@ -1879,25 +1886,21 @@ static void megasas_command_complete(SCSIRequest *req, uint32_t status,
             megasas_copy_sense(cmd);
         }
 
-        megasas_unmap_sgl(cmd);
         cmd->frame->header.scsi_status = req->status;
-        scsi_req_unref(cmd->req);
-        cmd->req = NULL;
     }
     cmd->frame->header.cmd_status = cmd_status;
-    megasas_unmap_frame(cmd->state, cmd);
-    megasas_complete_frame(cmd->state, cmd->context);
+    megasas_complete_command(cmd);
 }
 
-static void megasas_command_cancel(SCSIRequest *req)
+static void megasas_command_cancelled(SCSIRequest *req)
 {
     MegasasCmd *cmd = req->hba_private;
 
-    if (cmd) {
-        megasas_abort_command(cmd);
-    } else {
-        scsi_req_unref(req);
+    if (!cmd) {
+        return;
     }
+    cmd->frame->header.cmd_status = MFI_STAT_SCSI_IO_FAILED;
+    megasas_complete_command(cmd);
 }
 
 static int megasas_handle_abort(MegasasState *s, MegasasCmd *cmd)
@@ -2316,7 +2319,7 @@ static const struct SCSIBusInfo megasas_scsi_info = {
     .transfer_data = megasas_xfer_complete,
     .get_sg_list = megasas_get_sg_list,
     .complete = megasas_command_complete,
-    .cancel = megasas_command_cancel,
+    .cancel = megasas_command_cancelled,
 };
 
 static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
-- 
2.9.3

Re: [Qemu-devel] [PATCH for-2.8] megasas: clean up and fix request completion/cancellation

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1048 bytes --]

On Thu, Nov 10, 2016 at 04:27:51PM +0100, Paolo Bonzini wrote:
> megasas_command_cancel is a callback; it should report the abort in
> the frame, not try another abort!  Compare for instance with
> mptsas_request_cancelled.
> 
> So extract the common bits for request completion in a new function
> megasas_complete_command, call it from both the .complete and .cancel
> callbacks, and remove duplicate pieces from the DCMD path.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/scsi/megasas.c | 53 ++++++++++++++++++++++++++++-------------------------
>  1 file changed, 28 insertions(+), 25 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

> @@ -2316,7 +2319,7 @@ static const struct SCSIBusInfo megasas_scsi_info = {
>      .transfer_data = megasas_xfer_complete,
>      .get_sg_list = megasas_get_sg_list,
>      .complete = megasas_command_complete,
> -    .cancel = megasas_command_cancel,
> +    .cancel = megasas_command_cancelled,

Should .cancel be renamed in a separate patch?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH for-2.8] megasas: clean up and fix request completion/cancellation

[Paolo Bonzini]

On 11/11/2016 13:50, Stefan Hajnoczi wrote:
> On Thu, Nov 10, 2016 at 04:27:51PM +0100, Paolo Bonzini wrote:
>> megasas_command_cancel is a callback; it should report the abort in
>> the frame, not try another abort!  Compare for instance with
>> mptsas_request_cancelled.
>>
>> So extract the common bits for request completion in a new function
>> megasas_complete_command, call it from both the .complete and .cancel
>> callbacks, and remove duplicate pieces from the DCMD path.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  hw/scsi/megasas.c | 53 ++++++++++++++++++++++++++++-------------------------
>>  1 file changed, 28 insertions(+), 25 deletions(-)
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
>> @@ -2316,7 +2319,7 @@ static const struct SCSIBusInfo megasas_scsi_info = {
>>      .transfer_data = megasas_xfer_complete,
>>      .get_sg_list = megasas_get_sg_list,
>>      .complete = megasas_command_complete,
>> -    .cancel = megasas_command_cancel,
>> +    .cancel = megasas_command_cancelled,
> 
> Should .cancel be renamed in a separate patch?

Good idea, or even .request_completed/.request_cancelled.

Paolo