Refactor BuildIo and StartIo calls

Signed-off-by: Owen Smith <owen.smith@citrix.com>

diff --git a/src/xenvbd/adapter.c b/src/xenvbd/adapter.c
index 14e9ae662c5a183e0e5a1fbbd66efb2aafd4f788..c8c07c5f4935d5f91ed7dd917eed3f794e0ca196 100644 (file)
--- a/src/xenvbd/adapter.c
+++ b/src/xenvbd/adapter.c
@@ -1404,7 +1404,7 @@ AdapterCompleteSrb(
 
     ASSERT3U(Srb->SrbStatus, !=, SRB_STATUS_PENDING);
 
-    ++Adapter->Completed;
+    InterlockedIncrement((PLONG)&Adapter->Completed);
 
     StorPortNotification(RequestComplete, Adapter, Srb);
 }
@@ -1753,7 +1753,6 @@ AdapterHwResetBus(
     return TRUE;
 }
 
-
 static FORCEINLINE VOID
 __AdapterSrbPnp(
     IN  PXENVBD_ADAPTER         Adapter,
@@ -1780,27 +1779,40 @@ AdapterHwBuildIo(
 {
     PXENVBD_ADAPTER         Adapter = DevExt;
     PXENVBD_SRBEXT          SrbExt = Srb->SrbExtension;
+    PXENVBD_TARGET          Target;
 
     InitSrbExt(Srb);
 
+    InterlockedIncrement((PLONG)&Adapter->BuildIo);
     switch (Srb->Function) {
     case SRB_FUNCTION_EXECUTE_SCSI:
         AdapterPullupSrb(Adapter, Srb);
-        // Intentional fall through
+        Target = AdapterGetTarget(Adapter, Srb->TargetId);
+        if (Target == NULL)
+            Srb->SrbStatus = SRB_STATUS_NO_DEVICE;
+        else
+            TargetPrepareIo(Target, SrbExt);
+        break;
+
     case SRB_FUNCTION_RESET_DEVICE:
     case SRB_FUNCTION_FLUSH:
     case SRB_FUNCTION_SHUTDOWN:
-        ++Adapter->BuildIo;
-        return TRUE;
+        Target = AdapterGetTarget(Adapter, Srb->TargetId);
+        if (Target == NULL)
+            Srb->SrbStatus = SRB_STATUS_NO_DEVICE;
+        else
+            Srb->SrbStatus = SRB_STATUS_PENDING;
+        break;
 
-        // dont pass to StartIo
     case SRB_FUNCTION_PNP:
         __AdapterSrbPnp(Adapter, (PSCSI_PNP_REQUEST_BLOCK)Srb);
         Srb->SrbStatus = SRB_STATUS_SUCCESS;
         break;
+
     case SRB_FUNCTION_ABORT_COMMAND:
         Srb->SrbStatus = SRB_STATUS_ABORT_FAILED;
         break;
+
     case SRB_FUNCTION_RESET_BUS:
         AdapterHwResetBus(Adapter, Srb->PathId);
         Srb->SrbStatus = SRB_STATUS_SUCCESS;
@@ -1811,6 +1823,9 @@ AdapterHwBuildIo(
         break;
     }
 
+    if (Srb->SrbStatus == SRB_STATUS_PENDING)
+        return TRUE;
+
     AdapterCompleteSrb(Adapter, SrbExt);
     return FALSE;
 }
@@ -1825,21 +1840,46 @@ AdapterHwStartIo(
 {
     PXENVBD_ADAPTER         Adapter = DevExt;
     PXENVBD_SRBEXT          SrbExt = Srb->SrbExtension;
+    BOOLEAN                 WasQueued = FALSE;
     PXENVBD_TARGET          Target;
 
     Target = AdapterGetTarget(Adapter, Srb->TargetId);
-    if (Target == NULL)
-        goto fail1;
+    if (Target == NULL) {
+        Srb->SrbStatus = SRB_STATUS_NO_DEVICE;
+        goto done;
+    }
 
-    ++Adapter->StartIo;
-    if (TargetStartIo(Target, SrbExt))
-        AdapterCompleteSrb(Adapter, SrbExt);
+    switch (Srb->Function) {
+    case SRB_FUNCTION_EXECUTE_SCSI:
+        WasQueued = TargetStartIo(Target, SrbExt);
+        break;
 
-    return TRUE;
+    case SRB_FUNCTION_RESET_DEVICE:
+        TargetReset(Target);
+        Srb->SrbStatus = SRB_STATUS_SUCCESS;
+        break;
 
-fail1:
-    Srb->SrbStatus = SRB_STATUS_INVALID_TARGET_ID;
-    AdapterCompleteSrb(Adapter, SrbExt);
+    case SRB_FUNCTION_FLUSH:
+        TargetFlush(Target, SrbExt);
+        WasQueued = TRUE;
+        break;
+
+    case SRB_FUNCTION_SHUTDOWN:
+        TargetShutdown(Target, SrbExt);
+        WasQueued = TRUE;
+        break;
+
+    default:
+        ASSERT(FALSE);
+        break;
+    }
+
+done:
+    // if SRB WasQueued, the SRB will be completed when the Queues are processed
+    // Note: there could be a race in updating the Srb->SrbStatus field if the
+    //       processing of the queue completes before returning from TargetStartIo
+    if (!WasQueued)
+        AdapterCompleteSrb(Adapter, SrbExt);
     return TRUE;
 }
 

diff --git a/src/xenvbd/target.c b/src/xenvbd/target.c
index c9cb803a6e0768c567b3f21aca94cf694b8bdad3..dcd87c9188745e24f32f3f4dbd7876e61bdf8ae9 100644 (file)
--- a/src/xenvbd/target.c
+++ b/src/xenvbd/target.c
@@ -1906,60 +1906,127 @@ TargetReadCapacity16(
     Srb->SrbStatus = SRB_STATUS_SUCCESS;
 }
 
-//=============================================================================
-// StorPort Methods
-__checkReturn
 static FORCEINLINE BOOLEAN
-__TargetExecuteScsi(
-    __in PXENVBD_TARGET             Target,
-    __in PSCSI_REQUEST_BLOCK     Srb
+__ValidateSrbForTarget(
+    IN  PXENVBD_TARGET      Target,
+    IN  PSCSI_REQUEST_BLOCK Srb
     )
 {
-    const UCHAR Operation = Cdb_OperationEx(Srb);
-    PXENVBD_DISKINFO    DiskInfo = FrontendGetDiskInfo(Target->Frontend);
+    const UCHAR             Operation = Cdb_OperationEx(Srb);
 
-    if (DiskInfo->DiskInfo & VDISK_READONLY) {
-        Trace("Target[%d] : (%08x) Read-Only, fail SRB (%02x:%s)\n", TargetGetTargetId(Target),
-                DiskInfo->DiskInfo, Operation, Cdb_OperationName(Operation));
-        Srb->ScsiStatus = 0x40; // SCSI_ABORT
-        return TRUE;
+    if (Target == NULL) {
+        Error("Invalid Target(NULL) (%02x:%s)\n",
+              Operation,
+              Cdb_OperationName(Operation));
+        Srb->SrbStatus = SRB_STATUS_INVALID_TARGET_ID;
+        return FALSE;
+    }
+
+    if (Srb->PathId != 0) {
+        Error("Target[%d] : Invalid PathId(%d) (%02x:%s)\n",
+              TargetGetTargetId(Target),
+              Srb->PathId,
+              Operation,
+              Cdb_OperationName(Operation));
+        Srb->SrbStatus = SRB_STATUS_INVALID_PATH_ID;
+        return FALSE;
     }
 
-    // idea: check pdo state here. still push to freshsrbs
+    if (Srb->Lun != 0) {
+        Error("Target[%d] : Invalid Lun(%d) (%02x:%s)\n",
+              TargetGetTargetId(Target),
+              Srb->Lun,
+              Operation,
+              Cdb_OperationName(Operation));
+        Srb->SrbStatus = SRB_STATUS_INVALID_LUN;
+        return FALSE;
+    }
+
+    if (TargetIsMissing(Target)) {
+        Error("Target[%d] : %s (%s) (%02x:%s)\n",
+              TargetGetTargetId(Target),
+              Target->Missing ? "MISSING" : "NOT_MISSING",
+              Target->Reason,
+              Operation,
+              Cdb_OperationName(Operation));
+        Srb->SrbStatus = SRB_STATUS_NO_DEVICE;
+        return FALSE;
+    }
+
+    return TRUE;
+}
+
+VOID
+TargetPrepareIo(
+    IN  PXENVBD_TARGET  Target,
+    IN  PXENVBD_SRBEXT  SrbExt
+    )
+{
+    PSCSI_REQUEST_BLOCK Srb = SrbExt->Srb;
+
+    if (!__ValidateSrbForTarget(Target, Srb))
+        return;
+
+    Srb->SrbStatus = SRB_STATUS_PENDING;
+}
+
+BOOLEAN
+TargetStartIo(
+    IN  PXENVBD_TARGET  Target,
+    IN  PXENVBD_SRBEXT  SrbExt
+    )
+{
+    PSCSI_REQUEST_BLOCK Srb = SrbExt->Srb;
+    const UCHAR         Operation = Cdb_OperationEx(Srb);
+    BOOLEAN             WasQueued = FALSE;
+
+    ASSERT(__ValidateSrbForTarget(Target, Srb));
+
     switch (Operation) {
     case SCSIOP_READ:
     case SCSIOP_WRITE:
-        return TargetReadWrite(Target, Srb);
+        if (!TargetReadWrite(Target, Srb))
+            WasQueued = TRUE;
         break;
 
-    case SCSIOP_SYNCHRONIZE_CACHE:
-        return TargetSyncCache(Target, Srb);
+    case SCSIOP_UNMAP:
+        if (!TargetUnmap(Target, Srb))
+            WasQueued = TRUE;
         break;
 
-    case SCSIOP_UNMAP:
-        return TargetUnmap(Target, Srb);
+    case SCSIOP_SYNCHRONIZE_CACHE:
+        if (!TargetSyncCache(Target, Srb))
+            WasQueued = TRUE;
         break;
 
     case SCSIOP_INQUIRY:
         AdapterSetDeviceQueueDepth(TargetGetAdapter(Target),
                                    TargetGetTargetId(Target));
-        PdoInquiry(TargetGetTargetId(Target), FrontendGetInquiry(Target->Frontend), Srb);
+        PdoInquiry(TargetGetTargetId(Target),
+                   FrontendGetInquiry(Target->Frontend),
+                   Srb);
         break;
+
     case SCSIOP_MODE_SENSE:
         TargetModeSense(Target, Srb);
         break;
+
     case SCSIOP_REQUEST_SENSE:
         TargetRequestSense(Target, Srb);
         break;
+
     case SCSIOP_REPORT_LUNS:
         TargetReportLuns(Target, Srb);
         break;
+
     case SCSIOP_READ_CAPACITY:
         TargetReadCapacity(Target, Srb);
         break;
+
     case SCSIOP_READ_CAPACITY16:
         TargetReadCapacity16(Target, Srb);
         break;
+
     case SCSIOP_MEDIUM_REMOVAL:
     case SCSIOP_TEST_UNIT_READY:
     case SCSIOP_RESERVE_UNIT:
@@ -1968,136 +2035,61 @@ __TargetExecuteScsi(
     case SCSIOP_RELEASE_UNIT10:
     case SCSIOP_VERIFY:
     case SCSIOP_VERIFY16:
-        Srb->SrbStatus = SRB_STATUS_SUCCESS;
-        break;
     case SCSIOP_START_STOP_UNIT:
-        Trace("Target[%d] : Start/Stop Unit (%02X)\n", TargetGetTargetId(Target), Srb->Cdb[4]);
         Srb->SrbStatus = SRB_STATUS_SUCCESS;
         break;
+
     default:
-        Trace("Target[%d] : Unsupported CDB (%02x:%s)\n", TargetGetTargetId(Target), Operation, Cdb_OperationName(Operation));
+        Trace("Target[%d] : Unsupported CDB (%02x:%s)\n",
+              TargetGetTargetId(Target),
+              Operation,
+              Cdb_OperationName(Operation));
+        Srb->SrbStatus = SRB_STATUS_INVALID_REQUEST;
         break;
     }
-    return TRUE;
-}
-
-static FORCEINLINE BOOLEAN
-__TargetQueueShutdown(
-    __in PXENVBD_TARGET             Target,
-    __in PSCSI_REQUEST_BLOCK     Srb
-    )
-{
-    PXENVBD_SRBEXT      SrbExt = GetSrbExt(Srb);
-    PXENVBD_NOTIFIER    Notifier = FrontendGetNotifier(Target->Frontend);
-
-    QueueAppend(&Target->ShutdownSrbs, &SrbExt->Entry);
-    NotifierKick(Notifier);
-
-    return FALSE;
-}
-
-static FORCEINLINE BOOLEAN
-__TargetReset(
-    __in PXENVBD_TARGET             Target,
-    __in PSCSI_REQUEST_BLOCK     Srb
-    )
-{
-    Verbose("Target[%u] ====>\n", TargetGetTargetId(Target));
-
-    TargetReset(Target);
-    Srb->SrbStatus = SRB_STATUS_SUCCESS;
-
-    Verbose("Target[%u] <====\n", TargetGetTargetId(Target));
-    return TRUE;
+    return WasQueued;
 }
 
-__checkReturn
-static FORCEINLINE BOOLEAN
-__ValidateSrbForTarget(
-    __in PXENVBD_TARGET             Target,
-    __in PSCSI_REQUEST_BLOCK     Srb
+VOID
+TargetReset(
+    IN  PXENVBD_TARGET  Target
     )
 {
-    const UCHAR Operation = Cdb_OperationEx(Srb);
-
-    if (Target == NULL) {
-        Error("Invalid Target(NULL) (%02x:%s)\n",
-                Operation, Cdb_OperationName(Operation));
-        Srb->SrbStatus = SRB_STATUS_INVALID_TARGET_ID;
-        return FALSE;
-    }
+    Verbose("[%u] =====>\n", TargetGetTargetId(Target));
 
-    if (Srb->PathId != 0) {
-        Error("Target[%d] : Invalid PathId(%d) (%02x:%s)\n",
-                TargetGetTargetId(Target), Srb->PathId, Operation, Cdb_OperationName(Operation));
-        Srb->SrbStatus = SRB_STATUS_INVALID_PATH_ID;
-        return FALSE;
-    }
+    __TargetPauseDataPath(Target, TRUE);
 
-    if (Srb->Lun != 0) {
-        Error("Target[%d] : Invalid Lun(%d) (%02x:%s)\n",
-                TargetGetTargetId(Target), Srb->Lun, Operation, Cdb_OperationName(Operation));
-        Srb->SrbStatus = SRB_STATUS_INVALID_LUN;
-        return FALSE;
+    if (QueueCount(&Target->SubmittedReqs)) {
+        Error("Target[%d] : backend has %u outstanding requests after a TargetReset\n",
+              TargetGetTargetId(Target),
+              QueueCount(&Target->SubmittedReqs));
     }
 
-    if (TargetIsMissing(Target)) {
-        Error("Target[%d] : %s (%s) (%02x:%s)\n",
-                TargetGetTargetId(Target), Target->Missing ? "MISSING" : "NOT_MISSING", Target->Reason, Operation, Cdb_OperationName(Operation));
-        Srb->SrbStatus = SRB_STATUS_NO_DEVICE;
-        return FALSE;
-    }
+    __TargetUnpauseDataPath(Target);
 
-    return TRUE;
+    Verbose("[%u] <=====\n", TargetGetTargetId(Target));
 }
 
-BOOLEAN
-TargetStartIo(
+VOID
+TargetFlush(
     IN  PXENVBD_TARGET  Target,
     IN  PXENVBD_SRBEXT  SrbExt
     )
 {
-    PSCSI_REQUEST_BLOCK Srb = SrbExt->Srb;
-
-    if (!__ValidateSrbForTarget(Target, Srb))
-        return TRUE;
-
-    switch (Srb->Function) {
-    case SRB_FUNCTION_EXECUTE_SCSI:
-        return __TargetExecuteScsi(Target, Srb);
-
-    case SRB_FUNCTION_RESET_DEVICE:
-        return __TargetReset(Target, Srb);
-
-    case SRB_FUNCTION_FLUSH:
-    case SRB_FUNCTION_SHUTDOWN:
-        return __TargetQueueShutdown(Target, Srb);
-
-    default:
-        return TRUE;
-    }
+    QueueAppend(&Target->ShutdownSrbs, &SrbExt->Entry);
+    NotifierKick(FrontendGetNotifier(Target->Frontend));
 }
 
 VOID
-TargetReset(
-    __in PXENVBD_TARGET             Target
+TargetShutdown(
+    IN  PXENVBD_TARGET  Target,
+    IN  PXENVBD_SRBEXT  SrbExt
     )
 {
-    Trace("Target[%d] ====> (Irql=%d)\n", TargetGetTargetId(Target), KeGetCurrentIrql());
-
-    __TargetPauseDataPath(Target, TRUE);
-
-    if (QueueCount(&Target->SubmittedReqs)) {
-        Error("Target[%d] : backend has %u outstanding requests after a TargetReset\n",
-                TargetGetTargetId(Target), QueueCount(&Target->SubmittedReqs));
-    }
-
-    __TargetUnpauseDataPath(Target);
-
-    Trace("Target[%d] <==== (Irql=%d)\n", TargetGetTargetId(Target), KeGetCurrentIrql());
+    QueueAppend(&Target->ShutdownSrbs, &SrbExt->Entry);
+    NotifierKick(FrontendGetNotifier(Target->Frontend));
 }
 
-
 VOID
 TargetSrbPnp(
     __in PXENVBD_TARGET             Target,

diff --git a/src/xenvbd/target.h b/src/xenvbd/target.h
index 67b6319e4e353a9f636b8ff24054ecafda818b55..8808460b5531f7ee57c23c31ae5d3127256bc385 100644 (file)
--- a/src/xenvbd/target.h
+++ b/src/xenvbd/target.h
@@ -156,10 +156,10 @@ TargetPostResume(
     __in PXENVBD_TARGET             Target
     );
 
-// StorPort Methods
 extern VOID
-TargetReset(
-    __in PXENVBD_TARGET             Target
+TargetPrepareIo(
+    IN  PXENVBD_TARGET  Target,
+    IN  PXENVBD_SRBEXT  SrbExt
     );
 
 extern BOOLEAN
@@ -168,6 +168,23 @@ TargetStartIo(
     IN  PXENVBD_SRBEXT  SrbExt
     );
 
+extern VOID
+TargetReset(
+    IN  PXENVBD_TARGET  Target
+    );
+
+extern VOID
+TargetFlush(
+    IN  PXENVBD_TARGET  Target,
+    IN  PXENVBD_SRBEXT  SrbExt
+    );
+
+extern VOID
+TargetShutdown(
+    IN  PXENVBD_TARGET  Target,
+    IN  PXENVBD_SRBEXT  SrbExt
+    );
+
 extern VOID
 TargetSrbPnp(
     __in PXENVBD_TARGET             Target,