Re: [win-pv-devel] [PATCH 2/2] Fix race calculating SrbExt->Count

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: win-pv-devel [mailto:win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On
> Behalf Of owen.smith@xxxxxxxxxx
> Sent: 16 February 2017 10:12
> To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Owen Smith <owen.smith@xxxxxxxxxx>
> Subject: [win-pv-devel] [PATCH 2/2] Fix race calculating SrbExt->Count
> 
> From: Owen Smith <owen.smith@xxxxxxxxxx>
> 
> It is possible under heavy loads for the backend to
> start completing sub-requests of a Srb before the
> SrbExt Count is set. This would leave the count unable
> to reach 0 (as 1 or more requests are skipped by count
> being overridden)
> 
> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxxx>

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

> ---
>  src/xenvbd/pdo.c | 31 ++++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/src/xenvbd/pdo.c b/src/xenvbd/pdo.c
> index dd5b6ea..ee2a3cc 100644
> --- a/src/xenvbd/pdo.c
> +++ b/src/xenvbd/pdo.c
> @@ -1169,6 +1169,7 @@ PrepareReadWrite(
>      ULONG           SectorsLeft = Cdb_TransferBlock(Srb);
>      LIST_ENTRY      List;
>      XENVBD_SG_LIST  SGList;
> +    ULONG           DebugCount;
> 
>      InitializeListHead(&List);
>      SrbExt->Count = 0;
> @@ -1185,6 +1186,7 @@ PrepareReadWrite(
>          if (Request == NULL)
>              goto fail1;
>          InsertTailList(&List, &Request->Entry);
> +        InterlockedIncrement(&SrbExt->Count);
> 
>          Request->Srb    = Srb;
>          MaxSegments = UseIndirect(Pdo, SectorsLeft);
> @@ -1207,7 +1209,10 @@ PrepareReadWrite(
>          SectorStart += SectorsDone;
>      }
> 
> -    SrbExt->Count = PdoQueueRequestList(Pdo, &List);
> +    DebugCount = PdoQueueRequestList(Pdo, &List);
> +    if (DebugCount != (ULONG)SrbExt->Count) {
> +        Trace("[%u] %d != %u\n", PdoGetTargetId(Pdo), SrbExt->Count,
> DebugCount);
> +    }
>      Srb->SrbStatus = SRB_STATUS_PENDING;
>      return TRUE;
> 
> @@ -1215,6 +1220,7 @@ fail3:
>  fail2:
>  fail1:
>      PdoCancelRequestList(Pdo, &List);
> +    SrbExt->Count = 0;
>      Srb->SrbStatus = SRB_STATUS_ERROR;
>      return FALSE;
>  }
> @@ -1230,6 +1236,7 @@ PrepareSyncCache(
>      PXENVBD_REQUEST     Request;
>      LIST_ENTRY          List;
>      UCHAR               Operation;
> +    ULONG               DebugCount;
> 
>      if (FrontendGetDiskInfo(Pdo->Frontend)->FlushCache)
>          Operation = BLKIF_OP_FLUSH_DISKCACHE;
> @@ -1243,17 +1250,22 @@ PrepareSyncCache(
>      if (Request == NULL)
>          goto fail1;
>      InsertTailList(&List, &Request->Entry);
> +    InterlockedIncrement(&SrbExt->Count);
> 
>      Request->Srb        = Srb;
>      Request->Operation  = Operation;
>      Request->FirstSector = Cdb_LogicalBlock(Srb);
> 
> -    SrbExt->Count = PdoQueueRequestList(Pdo, &List);
> +    DebugCount = PdoQueueRequestList(Pdo, &List);
> +    if (DebugCount != (ULONG)SrbExt->Count) {
> +        Trace("[%u] %d != %u\n", PdoGetTargetId(Pdo), SrbExt->Count,
> DebugCount);
> +    }
>      Srb->SrbStatus = SRB_STATUS_PENDING;
>      return TRUE;
> 
>  fail1:
>      PdoCancelRequestList(Pdo, &List);
> +    SrbExt->Count = 0;
>      Srb->SrbStatus = SRB_STATUS_ERROR;
>      return FALSE;
>  }
> @@ -1270,6 +1282,7 @@ PrepareUnmap(
>       ULONG               Count = _byteswap_ushort(*(PUSHORT)Unmap-
> >BlockDescrDataLength) / sizeof(UNMAP_BLOCK_DESCRIPTOR);
>      ULONG               Index;
>      LIST_ENTRY          List;
> +    ULONG               DebugCount;
> 
>      InitializeListHead(&List);
>      SrbExt->Count = 0;
> @@ -1282,6 +1295,7 @@ PrepareUnmap(
>          if (Request == NULL)
>              goto fail1;
>          InsertTailList(&List, &Request->Entry);
> +        InterlockedIncrement(&SrbExt->Count);
> 
>          Request->Srb            = Srb;
>          Request->Operation      = BLKIF_OP_DISCARD;
> @@ -1290,12 +1304,16 @@ PrepareUnmap(
>          Request->Flags          = 0;
>      }
> 
> -    SrbExt->Count = PdoQueueRequestList(Pdo, &List);
> +    DebugCount = PdoQueueRequestList(Pdo, &List);
> +    if (DebugCount != (ULONG)SrbExt->Count) {
> +        Trace("[%u] %d != %u\n", PdoGetTargetId(Pdo), SrbExt->Count,
> DebugCount);
> +    }
>      Srb->SrbStatus = SRB_STATUS_PENDING;
>      return TRUE;
> 
>  fail1:
>      PdoCancelRequestList(Pdo, &List);
> +    SrbExt->Count = 0;
>      Srb->SrbStatus = SRB_STATUS_ERROR;
>      return FALSE;
>  }
> @@ -1429,6 +1447,13 @@ PdoSubmitPrepared(
>      )
>  {
>      PXENVBD_BLOCKRING   BlockRing = FrontendGetBlockRing(Pdo-
> >Frontend);
> +    if (PdoIsPaused(Pdo)) {
> +        if (QueueCount(&Pdo->PreparedReqs))
> +            Warning("Target[%d] : Paused, not submitting new requests 
> (%u)\n",
> +                    PdoGetTargetId(Pdo),
> +                    QueueCount(&Pdo->PreparedReqs));
> +        return FALSE;
> +    }
> 
>      for (;;) {
>          PXENVBD_REQUEST Request;
> --
> 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