On 29/11/16 12:40, Jan Beulich wrote: >>>> On 29.11.16 at 12:19, <JGross@xxxxxxxx> wrote: >> On 29/11/16 12:14, Jan Beulich wrote: >>>>>> On 29.11.16 at 11:50, <JGross@xxxxxxxx> wrote: >>>> --- a/drivers/scsi/xen-scsifront.c >>>> +++ b/drivers/scsi/xen-scsifront.c >>>> @@ -184,8 +184,6 @@ static struct vscsiif_request *scsifront_pre_req(struct >> vscsifrnt_info *info) >>>> >>>> ring_req = RING_GET_REQUEST(&(info->ring), ring->req_prod_pvt); >>>> >>>> - ring->req_prod_pvt++; >>> >>> Please note the "_pvt" suffix, which stands for "private": This field is >>> not visible to the backend. Only ring->sring fields are shared, and >>> the updating of the shared field happens in RING_PUSH_REQUESTS() >>> and RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(). >> >> Sure, but RING_PUSH_REQUESTS() will copy req_prod_pvt to req_prod. In >> the case corrected this would advance req_prod by two after the error >> case before, even if only one request would have made it to the ring. > > Okay, then I may have been mislead by the patch description: I > understood it to say that you want to avoid the backend seeing > requests which haven't been filled fully, but it looks like you're > instead saying that for these requests the filling will never be > completed (because of some unrelated(?) error). Iirc other > frontend drivers behave similarly to the unpatched scsifront, and blkfront and netfront seem to be okay. > incrementing req_prod_pvt late has possible (perhaps just > theoretical) other issues, like parallel retrieval and filling of them > on mor than one CPU. Wouldn't it be better to obtain a request In scsifront the complete critical path is guarded by a lock. > structure only when everything else is ready (and hence no further > errors can occur)? After all you also need to deal with the acquired > ID upon errors, and seems odd to me to deal with the two parts of > cleanup in different places (and even in different ways). Hmm, I can see your point. I'll have a look how intrusive such a change would be. Juergen -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
