Re: [win-pv-devel] commit "Make sure we don't leave SRBs in state PENDING" introduces race

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andreas Kinzler [mailto:ml-ak@xxxxxxxxx]
> Sent: 17 February 2017 19:47
> To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx; Paul Durrant
> <Paul.Durrant@xxxxxxxxxx>; Owen Smith <owen.smith@xxxxxxxxxx>
> Subject: commit "Make sure we don't leave SRBs in state PENDING"
> introduces race
> 
> Hello Paul,
> 
>       SrbExt->Count = PdoQueueRequestList(Pdo, &List);
> +    Srb->SrbStatus = SRB_STATUS_PENDING;
>       return TRUE;
> 
> doesn't everything you do after PdoQueueRequestList introduce a race, be it
> writing to SrbExt->Count or setting Srb->SrbStatus?
> 
> In PdoCompleteResponse
> 
>      if (InterlockedDecrement(&SrbExt->Count) == 0) {
> --->race
>           if (Srb->SrbStatus == SRB_STATUS_PENDING) {
>               // SRB has not hit a failure condition (BLKIF_RSP_ERROR |
> BLKIF_RSP_EOPNOTSUPP)
>               // from any of its responses. SRB must have succeeded
>               Srb->SrbStatus = SRB_STATUS_SUCCESS;
>               Srb->ScsiStatus = 0x00; // SCSI_GOOD
>           } else {
>               // Srb->SrbStatus has already been set by 1 or more requests 
> with
> Status != BLKIF_RSP_OKAY
>               Srb->ScsiStatus = 0x40; // SCSI_ABORTED
>           }
> 
> So you may overwrite an already existing error condition.

Yes, you are correct. That does need to be fixed.

> 
> I would really suggest to use my patch. So far load tests are finally stable 
> with
> it.
> 

Owen had already done some fairly lengthy soak testing on his patch too. I'll 
fix the race by moving the status change up.

Thanks for noticing that,

  Paul

> Regards Andreas
_______________________________________________
win-pv-devel mailing list
win-pv-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/cgi-bin/mailman/listinfo/win-pv-devel