[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/3] nbd-client: enter read_reply_co during init
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH 1/3] nbd-client: enter read_reply_co during init to avoid crash |
Date: |
Thu, 24 Aug 2017 19:42:20 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 |
On 24/08/2017 19:37, Eric Blake wrote:
> On 08/24/2017 11:21 AM, Paolo Bonzini wrote:
>> On 24/08/2017 17:33, Stefan Hajnoczi wrote:
>>> This patch enters read_reply_co directly in
>>> nbd_client_attach_aio_context(). This is safe because new_context is
>>> acquired by the caller. This ensures that read_reply_co reaches its
>>> first yield point and its ctx is set up.
>>
>> I'm not very confident with this patch. aio_context_acquire/release is
>> going to go away, and this then becomes possible
>>
>> main context new_context
>> qemu_aio_coroutine_enter
>> send request
>> wait for reply
>> read first reply
>> wake coroutine
>>
>> where the "wake coroutine" part thinks it's running in new_context, and
>> thus simply enters the coroutine instead of using the bottom half.
>>
>> But blk_co_preadv() should need the read_reply_co itself, in order to be
>> woken up after reading the reply header. The core issue here is that
>> nbd_co_receive_reply was never called, I suspect. And if it was never
>> called, read_reply_co should not be woken up by nbd_coroutine_end.
>>
>> So the fix is:
>>
>> 1) assign NULL to s->recv_coroutine[i] when nbd_co_send_request fails
>>
>> 2) move this to nbd_co_receive_reply:
>>
>> s->recv_coroutine[i] = NULL;
>>
>> /* Kick the read_reply_co to get the next reply. */
>> if (s->read_reply_co) {
>> aio_co_wake(s->read_reply_co);
>> }
>>
>> Does this make sense? (Note that the read_reply_co idea actually came
>> from you, or from my recollections of your proposed design :)).
>
> How much of this overlaps with Vladimir's proposal?
> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg00846.html
The above should be about 15 lines added, 10 removed. :)
Paolo
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH 2/3] qemu-iotests: improve nbd-fault-injector.py startup protocol, Stefan Hajnoczi, 2017/08/24
[Qemu-devel] [PATCH 3/3] qemu-iotests: test NBD over UNIX domain sockets in 083, Stefan Hajnoczi, 2017/08/24
Re: [Qemu-devel] [PATCH 0/3] nbd-client: enter read_reply_co during init to avoid crash, Eric Blake, 2017/08/24