| Message ID | 20161129105013.7419-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
>>> On 29.11.16 at 11:50, <JGross@suse.com> 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(). Jan -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 29/11/16 12:14, Jan Beulich wrote: >>>> On 29.11.16 at 11:50, <JGross@suse.com> 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. As an alternative I could have decremented req_prod_pvt in case of an error, but I like my current solution better. Juergen -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 29/11/16 11:19, Juergen Gross wrote: > On 29/11/16 12:14, Jan Beulich wrote: >>>>> On 29.11.16 at 11:50, <JGross@suse.com> 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. > > As an alternative I could have decremented req_prod_pvt in case of an > error, but I like my current solution better. FWIW, I found the commit message a bit misleading and also came to the same conclusion as Jan initially. Perhaps, "When adding a new request to the ring, an error may cause the (partially constructed) request to be discarded and used for the next. Thus ring->req_prod_pvt should not be advanced until we know the request will be successfully added to the ring." Or similar. David -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 29/11/16 12:28, David Vrabel wrote: > On 29/11/16 11:19, Juergen Gross wrote: >> On 29/11/16 12:14, Jan Beulich wrote: >>>>>> On 29.11.16 at 11:50, <JGross@suse.com> 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. >> >> As an alternative I could have decremented req_prod_pvt in case of an >> error, but I like my current solution better. > > FWIW, I found the commit message a bit misleading and also came to the > same conclusion as Jan initially. > > Perhaps, > > "When adding a new request to the ring, an error may cause the > (partially constructed) request to be discarded and used for the next. > Thus ring->req_prod_pvt should not be advanced until we know the request > will be successfully added to the ring." This is indeed much better, thanks. In case there are no other objections I'll fix this up when committing. Juergen -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> On 29.11.16 at 12:19, <JGross@suse.com> wrote: > On 29/11/16 12:14, Jan Beulich wrote: >>>>> On 29.11.16 at 11:50, <JGross@suse.com> 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 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 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). Jan -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 29/11/16 12:40, Jan Beulich wrote: >>>> On 29.11.16 at 12:19, <JGross@suse.com> wrote: >> On 29/11/16 12:14, Jan Beulich wrote: >>>>>> On 29.11.16 at 11:50, <JGross@suse.com> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/scsi/xen-scsifront.c b/drivers/scsi/xen-scsifront.c index c01316c..33805f6 100644 --- 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++; - ring_req->rqid = (uint16_t)id; return ring_req; @@ -196,6 +194,8 @@ static void scsifront_do_request(struct vscsifrnt_info *info) struct vscsiif_front_ring *ring = &(info->ring); int notify; + ring->req_prod_pvt++; + RING_PUSH_REQUESTS_AND_CHECK_NOTIFY(ring, notify); if (notify) notify_remote_via_irq(info->irq); @@ -626,6 +626,7 @@ static int scsifront_action_handler(struct scsi_cmnd *sc, uint8_t act) } if (scsifront_enter(info)) { + scsifront_put_rqid(info, shadow->rqid); spin_unlock_irq(host->host_lock); kfree(shadow); return FAILED;
When allocating a new ring slot for a request don't advance the producer index until the request is really ready to go to the backend. Otherwise only partially filled requests will be visible to the backend in case of errors on the frontend side. In scsifront_action_handler() free the request id in case of an error. Signed-off-by: Juergen Gross <jgross@suse.com> --- In case accepted I'll take this patch through the Xen tree as it is based on another patch by lambert.quentin@gmail.com which is going through Xen, too. --- drivers/scsi/xen-scsifront.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)