Re: [PATCH 2/2] Clear SRB queue and Shutdown queue on reset

[Tu Dinh <ngoc-tu.dinh@xxxxxxxxxx>]

On 08/10/2026 08:31, Owen Smith wrote:
> Fix a potential hang during TargetReset.
> When the ring is disabled, any SRBs queued, but not submitted were left
> in the queues. These SRBs would only be recovered when StorPort's internal
> timeout triggered.
> When the ring is disabled, abort any SRBs that were not sent to the backend,
> so that StorPort is responsible for requeueing any when the ring is 
> re-enabled.
> SRB_STATUS_ABORTED will inform StorPort to re-issue the requests when the
> target has recovered, rather than failing them.
> 
> Assisted-by: Claude:claude-opus-4.8
> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxxx>

There's still a gap because __BlkifRingQueueSrb and BlkifRingDisable 
don't wait for each other. So the following sequence is still possible:

- __BlkifRingQueueSrb observes BlkifRing->Enabled = TRUE and enters the 
submission code
- BlkifRingDisable acquires the ring lock, sets Enabled = FALSE and swizzles
- __BlkifRingQueueSrb queues yet another SRB to the atomic list
- BlkifRingDisable exits without aborting that SRB

I think it's possible to implement this in the !Enabled case of 
BlkifRingSchedule instead. Then instead of having to duplicate the 
cleanup code, RingDisable only needs to acquire the lock, set Enabled = 
FALSE and drop the lock. This also handles the above case if RingDisable 
is the lock holder.

Still, I'm not sure if this is sufficient to drain all RingQueueSrb 
attempts either, since RingDisable does not wait for all RingQueueSrb 
callers to complete first. If RingQueueSrb is instead the lock holder, 
it could attempt to access an already destroyed ring during unlock.

> ---
>   src/xenvbd/ring.c | 40 ++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 40 insertions(+)
> 
> diff --git a/src/xenvbd/ring.c b/src/xenvbd/ring.c
> index ffbd18a..4ad707f 100644
> --- a/src/xenvbd/ring.c
> +++ b/src/xenvbd/ring.c
> @@ -2271,6 +2271,46 @@ BlkifRingDisable(
>           __BlkifRingCompleteResponse(BlkifRing, Request, BLKIF_RSP_ERROR);
>       }
>   
> +    // Complete any SRBs that were queued but never made it as far as the
> +    // backend. Now that the ring is disabled the schedule thread will not
> +    // run again, so without this these requests would be left stuck until
> +    // StorPort times them out. First drain the atomic packet list into the
> +    // SRB queue so that nothing accepted from a concurrent submitter is
> +    // missed, then fail everything still pending. SRB_STATUS_ABORTED lets
> +    // StorPort re-issue them once the target has recovered.
> +
> +    BlkifRingSwizzle(BlkifRing);
> +
> +    while (!IsListEmpty(&BlkifRing->SrbQueue)) {
> +        PLIST_ENTRY ListEntry;
> +        PXENVBD_SRBEXT SrbExt;
> +        PSCSI_REQUEST_BLOCK Srb;
> +
> +        ListEntry = RemoveHeadList(&BlkifRing->SrbQueue);
> +        ASSERT3P(ListEntry, !=, &BlkifRing->SrbQueue);
> +
> +        RtlZeroMemory(ListEntry, sizeof(LIST_ENTRY));
> +
> +        SrbExt = CONTAINING_RECORD(ListEntry, XENVBD_SRBEXT, ListEntry);
> +        Srb = SrbExt->Srb;
> +        Srb->SrbStatus = SRB_STATUS_ABORTED;
> +        __BlkifRingCompleteSrb(BlkifRing, SrbExt);
> +    }
> +
> +    while (!IsListEmpty(&BlkifRing->ShutdownQueue)) {
> +        PLIST_ENTRY ListEntry;
> +        PXENVBD_SRBEXT SrbExt;
> +        PSCSI_REQUEST_BLOCK Srb;
> +
> +        ListEntry = RemoveHeadList(&BlkifRing->ShutdownQueue);
> +        ASSERT3P(ListEntry, !=, &BlkifRing->ShutdownQueue);
> +
> +        SrbExt = CONTAINING_RECORD(ListEntry, XENVBD_SRBEXT, ListEntry);
> +        Srb = SrbExt->Srb;
> +        Srb->SrbStatus = SRB_STATUS_ABORTED;
> +        __BlkifRingCompleteSrb(BlkifRing, SrbExt);
> +    }
> +
>       BlkifRing->Stopped = FALSE;
>   
>       __BlkifRingReleaseLock(BlkifRing);



--
Ngoc Tu Dinh | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech