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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5644D973.9000809@citrix.com>
Date:	Thu, 12 Nov 2015 19:24:51 +0100
From:	Roger Pau Monné <roger.pau@...rix.com>
To:	Julien Grall <julien.grall@...rix.com>,
	<xen-devel@...ts.xenproject.org>, <linux-kernel@...r.kernel.org>
CC:	Boris Ostrovsky <boris.ostrovsky@...cle.com>,
	David Vrabel <david.vrabel@...rix.com>
Subject: Re: [Xen-devel] [PATCH v2 2/2] block/xen-blkfront: Handle
 non-indirect grant with 64KB pages

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@...rix.com>
>>>>
>>>> LGTM, only a couple of typos and a simplification:
>>>>
>>>> Signed-off-by: Roger Pau Monné <roger.pau@...rix.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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/