Re: [Qemu-devel] [PATCH 1/3] nbd-client: enter read_reply_co during init to avoid crash

[Eric Blake]

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature