Re: [win-pv-devel] [PATCH 06/10] Refactor BuildIo and StartIo calls

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: win-pv-devel [mailto:win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On
> Behalf Of owen.smith@xxxxxxxxxx
> Sent: 23 June 2017 13:49
> To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Owen Smith <owen.smith@xxxxxxxxxx>
> Subject: [win-pv-devel] [PATCH 06/10] Refactor BuildIo and StartIo calls
> 
> From: Owen Smith <owen.smith@xxxxxxxxxx>
> 
> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxxx>

Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx>

> ---
>  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@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/cgi-bin/mailman/listinfo/win-pv-devel
_______________________________________________
win-pv-devel mailing list
win-pv-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/cgi-bin/mailman/listinfo/win-pv-devel