On 29/11/16 12:14, Jan Beulich wrote:
>>>> On 29.11.16 at 11:50, <[email protected]> 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
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, <[email protected]> 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
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, <[email protected]> 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
>>> On 29.11.16 at 12:19, <[email protected]> wrote:
> On 29/11/16 12:14, Jan Beulich wrote:
>>>>> On 29.11.16 at 11:50, <[email protected]> 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