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

[Owen Smith <owen.smith@xxxxxxxxxx>]

> -----Original Message-----
> From: Paul Durrant
> Sent: 17 September 2019 16:02
> To: Owen Smith <owen.smith@xxxxxxxxxx>; win-pv-
> devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Owen Smith <owen.smith@xxxxxxxxxx>
> Subject: RE: [win-pv-devel] [PATCH 4/4] Rework BlkifRingDisable
> 
> > -----Original Message-----
> > From: win-pv-devel <win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On
> > Behalf Of Owen Smith
> > Sent: 16 September 2019 16:18
> > To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> > Cc: Owen Smith <owen.smith@xxxxxxxxxx>
> > Subject: [win-pv-devel] [PATCH 4/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.
> >
> > Signed-off-by: Owen Smith <owen.smith@xxxxxxxxxx>
> > ---
> >  src/xenvbd/ring.c | 62
> > +++++++++++++++++++++----------------------------------
> >  1 file changed, 23 insertions(+), 39 deletions(-)
> >
> > diff --git a/src/xenvbd/ring.c b/src/xenvbd/ring.c index
> > d5db1da..5323243 100644
> > --- a/src/xenvbd/ring.c
> > +++ b/src/xenvbd/ring.c
> > @@ -1242,16 +1242,17 @@ BlkifRingPoll(
> >
> >              rsp = RING_GET_RESPONSE(&BlkifRing->Front, rsp_cons);
> >              rsp_cons++;
> > -            BlkifRing->ResponsesProcessed++;
> >
> >              BlkifRing->Stopped = FALSE;
> >
> >              Request = __BlkifRingGetSubmittedRequest(BlkifRing,
> >                                                       rsp->id);
> > -            if (Request != NULL)
> > +            if (Request != NULL) {
> > +                BlkifRing->ResponsesProcessed++;
> >                  __BlkifRingCompleteResponse(BlkifRing,
> >                                              Request,
> >                                              rsp->status);
> > +            }
> 
> Still don't understand why this is conditional, but if it needs to be then 
> then
> moving the counter bump should surely be in the previous patch.
> 
>   Paul

This counter bump should be combined with the previous patch
Owen

> 
> >
> >              if (rsp_cons - BlkifRing->Front.rsp_cons > 
> > XENVBD_BATCH(BlkifRing))
> >                  Retry = TRUE;
> > @@ -2043,58 +2044,41 @@ 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
> > +    BlkifRing->Enabled = FALSE;
> > +
> >      for (;;) {
> > -        PLIST_ENTRY         ListEntry;
> > -        PXENVBD_REQUEST     Request;
> > -        PXENVBD_SRBEXT      SrbExt;
> > -        PSCSI_REQUEST_BLOCK Srb;
> > +        PLIST_ENTRY ListEntry;
> > +        PXENVBD_REQUEST Request;
> >
> > -        ListEntry = RemoveHeadList(&BlkifRing->PreparedQueue);
> > -        if (ListEntry  == &BlkifRing->PreparedQueue)
> > +        ListEntry = RemoveHeadList(&BlkifRing->SubmittedList);
> > +        if (ListEntry == &BlkifRing->SubmittedList)
> >              break;
> >
> > -        Request = CONTAINING_RECORD(ListEntry,
> > -                                    XENVBD_REQUEST,
> > -                                    ListEntry);
> > -        SrbExt = Request->SrbExt;
> > -        Srb = SrbExt->Srb;
> > -        Srb->SrbStatus = SRB_STATUS_ABORTED;
> > -        Srb->ScsiStatus = 0x40; // SCSI_ABORTED
> > -
> > -        BlkifRingPutRequest(BlkifRing, Request);
> > -
> > -        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);
> > +    for (;;) {
> > +        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);
> > +        if (ListEntry == &BlkifRing->PreparedQueue)
> > +            break;
> >
> > -        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