Re: [Xen-devel] [PATCH v2 2/2] block/xen-blkfront: Handle non-indirect grant with 64KB pages

["Roger Pau Monné" <roger.pau@citrix.com>]

El 12/11/15 a les 19.04, Julien Grall ha escrit:
> On 12/11/15 17:51, Roger Pau Monné wrote:
>> El 12/11/15 a les 18.30, Julien Grall ha escrit:
>>> Hi,
>>>
>>> On 12/11/15 16:40, Roger Pau Monné wrote:
>>>>> [1] http://lists.xen.org/archives/html/xen-devel/2015-08/msg02200.html
>>>>>
>>>>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
>>>>
>>>> LGTM, only a couple of typos and a simplification:
>>>>
>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>
>>> Do you mean Acked-by? ;)
>>
>> Yes, I also had problems with smtp, so I thought this one was actually
>> not sent. You have another one with a proper Ack :).
>>
>>>>> +
>>>>> +	/* The I/O request may be split in two */
>>>>> +	if (unlikely(s->associated_id != NO_ASSOCIATED_ID)) {
>>>>> +		struct blk_shadow *s2 = &info->shadow[s->associated_id];
>>>>> +
>>>>> +		/* Keep the status of the current response in shadow */
>>>>> +		s->status = (bret->status == BLKIF_RSP_OKAY) ?
>>>>> +			REQ_DONE : REQ_FAIL;
>>>>> +
>>>>> +		/* Wait the second response if not yet here */
>>>>> +		if (s2->status == REQ_WAITING)
>>>>> +			return 0;
>>>>> +
>>>>> +		/*
>>>>> +		 * The status of the current response will be used in
>>>>> +		 * order to know if the request has failed.
>>>>> +		 * Update the current response status only if has not
>>>>> +		 * failed.
>>>>> +		 */
>>>>> +		if (bret->status == BLKIF_RSP_OKAY && s2->status == REQ_FAIL)
>>>>
>>>> This could be simplified by only checking if s2->status == REQ_FAIL.
>>>
>>> I didn't do it because bret->status may be different than
>>> BLKIF_RSP_ERROR (for instance BLKIF_RSP_EOPNOTSUPP).
>>
>> I think this is not actually possible in practice, but what if
>> bret->status == BLKIF_RSP_OKAY and the bret from s2 actually had
>> BLKIF_RSP_EOPNOTSUPP, wouldn't we loose the EOPNOTSUPP by
>> unconditionally setting BLKIF_RSP_ERROR?
> 
> No because EOPNOTSUPP are used when an operation is not supported. As
> the 2 ring request is coming from the same I/O request, it will always
> have the same operation.
> 
> So if one get EOPNOTSUPP the other will get too.

That's why I said that I think it's not currently possible. IMHO, it's
fine as it is now.

The only scenario I can think of that can lead to that combination is
that we migrate the guest and one request gets processed by one backend
that supports the operation, while the other request get processed by a
backend that doesn't support it.

With your current implementation we would return an error code anyway,
which is not that bad I guess.

>>
>> Should s->status be able to store all the possible return codes from the
>> response (OK/ERROR/NOTSUPP)?
> 
> That could would work. However, how do you decide which will be the
> final status?

It should be the most restrictive one, for example if we have ERROR and
NOTSUPP we should return ERROR, while if we have OK and NOTSUPP we
should return NOTSUPP.

Roger.