[prev in list] [next in list] [prev in thread] [next in thread]
List: win-pv-devel
Subject: Re: [win-pv-devel] [PATCH 06/10] Refactor BuildIo and StartIo calls
From: Paul Durrant <Paul.Durrant () citrix ! com>
Date: 2017-06-27 13:47:03
Message-ID: ca836ee5d5a94842aca5235376781e85 () AMSPEX02CL03 ! citrite ! net
[Download RAW message or body]
> -----Original Message-----
> From: win-pv-devel [mailto:win-pv-devel-bounces@lists.xenproject.org] On
> Behalf Of owen.smith@citrix.com
> Sent: 23 June 2017 13:49
> To: win-pv-devel@lists.xenproject.org
> Cc: Owen Smith <owen.smith@citrix.com>
> Subject: [win-pv-devel] [PATCH 06/10] Refactor BuildIo and StartIo calls
>
> From: Owen Smith <owen.smith@citrix.com>
>
> Signed-off-by: Owen Smith <owen.smith@citrix.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
> ---
> src/xenvbd/adapter.c | 70 +++++++++++----
> src/xenvbd/target.c | 236 +++++++++++++++++++++++++---------------------
> -----
> src/xenvbd/target.h | 23 ++++-
> 3 files changed, 189 insertions(+), 140 deletions(-)
>
> diff --git a/src/xenvbd/adapter.c b/src/xenvbd/adapter.c
> index 14e9ae6..b0a7365 100644
> --- 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 c9cb803..dcd87c9 100644
> --- 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 67b6319..8808460 100644
> --- 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
> @@ -169,6 +169,23 @@ TargetStartIo(
> );
>
> 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,
> __in PSCSI_PNP_REQUEST_BLOCK Srb
> --
> 2.8.3
>
>
> _______________________________________________
> win-pv-devel mailing list
> win-pv-devel@lists.xenproject.org
> https://lists.xenproject.org/cgi-bin/mailman/listinfo/win-pv-devel
_______________________________________________
win-pv-devel mailing list
win-pv-devel@lists.xenproject.org
https://lists.xenproject.org/cgi-bin/mailman/listinfo/win-pv-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic