Re: [win-pv-devel] [PATCH v2 3/4] Rework BlkifRingDisable

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: win-pv-devel <win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of 
> Owen Smith
> Sent: 18 September 2019 14:25
> To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Owen Smith <owen.smith@xxxxxxxxxx>
> Subject: [win-pv-devel] [PATCH v2 3/4] Rework BlkifRingDisable
> 
> Clean up all prepared and submitted requests when the ring is disabled,
> so that outstanding SRBs are returned to storport for queueing. This is
> especially important on the return from suspend path, as the ring is no
> longer valid, and any submitted requests would be lost and trigger a
> storport target reset.
> Also ignores missing requests for responses.
> 
> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxxx>
> ---
>  src/xenvbd/ring.c | 68 
> ++++++++++++++++++++-----------------------------------
>  1 file changed, 25 insertions(+), 43 deletions(-)
> 
> diff --git a/src/xenvbd/ring.c b/src/xenvbd/ring.c
> index 6edee5c..8dcdee3 100644
> --- a/src/xenvbd/ring.c
> +++ b/src/xenvbd/ring.c
> @@ -1242,17 +1242,17 @@ BlkifRingPoll(
> 
>              rsp = RING_GET_RESPONSE(&BlkifRing->Front, rsp_cons);
>              rsp_cons++;
> -            BlkifRing->ResponsesProcessed++;
> 
>              BlkifRing->Stopped = FALSE;
> 
>              Request = __BlkifRingGetSubmittedRequest(BlkifRing,
>                                                       rsp->id);
> -            ASSERT3P(Request, != , NULL);
> -
> -            __BlkifRingCompleteResponse(BlkifRing,
> -                                        Request,
> -                                        rsp->status);
> +            if (Request != NULL) {
> +                BlkifRing->ResponsesProcessed++;
> +                __BlkifRingCompleteResponse(BlkifRing,
> +                                            Request,
> +                                            rsp->status);
> +            }

This really should not happen, but I guess it may avoid a non-obvious crash in 
the non-debug case (where the assert is compiled out).

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

> 
>              if (rsp_cons - BlkifRing->Front.rsp_cons > 
> XENVBD_BATCH(BlkifRing))
>                  Retry = TRUE;
> @@ -2044,57 +2044,39 @@ BlkifRingDisable(
>      IN  PXENVBD_BLKIF_RING  BlkifRing
>      )
>  {
> -    PXENVBD_RING            Ring = BlkifRing->Ring;
> -    ULONG                   Attempt;
> -
>      Trace("====> %u\n", BlkifRing->Index);
> 
>      __BlkifRingAcquireLock(BlkifRing);
>      ASSERT(BlkifRing->Enabled);
> 
> -    // Discard any pending requests
> -    while (!IsListEmpty(&BlkifRing->PreparedQueue)) {
> -        PLIST_ENTRY         ListEntry;
> -        PXENVBD_REQUEST     Request;
> -        PXENVBD_SRBEXT      SrbExt;
> -        PSCSI_REQUEST_BLOCK Srb;
> -
> -        ListEntry = RemoveHeadList(&BlkifRing->PreparedQueue);
> -        ASSERT3P(ListEntry, !=, &BlkifRing->PreparedQueue);
> +    BlkifRing->Enabled = FALSE;
> 
> -        Request = CONTAINING_RECORD(ListEntry,
> -                                    XENVBD_REQUEST,
> -                                    ListEntry);
> -        SrbExt = Request->SrbExt;
> -        Srb = SrbExt->Srb;
> -        Srb->SrbStatus = SRB_STATUS_ABORTED;
> -        Srb->ScsiStatus = 0x40; // SCSI_ABORTED
> +    while (!IsListEmpty(&BlkifRing->SubmittedList)) {
> +        PLIST_ENTRY ListEntry;
> +        PXENVBD_REQUEST Request;
> 
> -        BlkifRingPutRequest(BlkifRing, Request);
> +        ListEntry = RemoveHeadList(&BlkifRing->SubmittedList);
> +        ASSERT3P(ListEntry, !=, &BlkifRing->SubmittedList);
> 
> -        if (InterlockedDecrement(&SrbExt->RequestCount) == 0)
> -            __BlkifRingCompleteSrb(BlkifRing, SrbExt);
> +        Request = CONTAINING_RECORD(ListEntry, XENVBD_REQUEST, ListEntry);
> +        BlkifRing->ResponsesProcessed++;
> +        __BlkifRingCompleteResponse(BlkifRing, Request, BLKIF_RSP_ERROR);
>      }
> 
> -    Attempt = 0;
> -    ASSERT3U(BlkifRing->RequestsPushed, == , BlkifRing->RequestsPosted);
> -    while (BlkifRing->ResponsesProcessed != BlkifRing->RequestsPushed) {
> -        Attempt++;
> -        ASSERT(Attempt < 100);
> -
> -        // Try to move things along
> -        __BlkifRingSend(BlkifRing);
> -        (VOID)BlkifRingPoll(BlkifRing);
> +    while (!IsListEmpty(&BlkifRing->PreparedQueue)) {
> +        PLIST_ENTRY ListEntry;
> +        PXENVBD_REQUEST Request;
> 
> -        // We are waiting for a watch event at DISPATCH_LEVEL so
> -        // it is our responsibility to poll the store ring.
> -        XENBUS_STORE(Poll,
> -                     &Ring->StoreInterface);
> +        ListEntry = RemoveHeadList(&BlkifRing->PreparedQueue);
> +        ASSERT3P(ListEntry, !=, &BlkifRing->PreparedQueue);
> 
> -        KeStallExecutionProcessor(1000);    // 1ms
> +        Request = CONTAINING_RECORD(ListEntry, XENVBD_REQUEST, ListEntry);
> +        // Dont increment ResponsesProcessed, as this is a faked response
> +        __BlkifRingCompleteResponse(BlkifRing, Request, BLKIF_RSP_ERROR);
>      }
> 
> -    BlkifRing->Enabled = FALSE;
> +    BlkifRing->Stopped = FALSE;
> +
>      __BlkifRingReleaseLock(BlkifRing);
> 
>      Trace("<==== %u\n", BlkifRing->Index);
> --
> 2.16.2.windows.1
> 
> 
> _______________________________________________
> win-pv-devel mailing list
> win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/win-pv-devel
_______________________________________________
win-pv-devel mailing list
win-pv-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/win-pv-devel