From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: Paolo Bonzini <pbonzini@redhat.com>,
Eric Blake <eblake@redhat.com>,
Stefan Hajnoczi <stefanha@redhat.com>,
qemu-devel@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>, qemu-block@nongnu.org
Subject: Re: [Qemu-devel] [PATCH 1/3] nbd-client: enter read_reply_co during init to avoid crash
Date: Fri, 25 Aug 2017 18:57:45 +0300 [thread overview]
Message-ID: <d49450b1-9ef1-60ed-91e3-16ff163c6c80@virtuozzo.com> (raw)
In-Reply-To: <26fee110-ce53-6c54-275c-5550dab6d3c4@redhat.com>
24.08.2017 20:42, Paolo Bonzini wrote:
> 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
>
>
I'll be on vocation for the next two weeks and will continue this work
after it.
I think we have a lot of conflicts already with my series after
different fixes,
so I'll rebase on them all, no problem.
PS: I think recent problems and bugs in nbd are related to not very good
separation between
block/nbd-client.c and nbd/client.c. Ideally, I think, block/nbd-client
should not
directly access the ioc, it should just call one or two high level
functions of
nbd/client. The big problem of this separation is CMD_READ reply - only
real client
knows, should we read a payload or not. I have two ideas on it:
1. We can add this information to request handle, then nbd/client will
now by handle,
that it should read the payload.. The length of payload should be in
handle too. It looks
possible, as handle is 64bit when request len is 32bit.
2. Move part of NBDClientSession to nbd/client, with NBDClientRequest
requests[MAX_NBD_REQUESTS];
to nbd/client, to implement one public function nbd_request(state,
*request, *reply), which will do all the
work for block/nbd-client..
--
Best regards,
Vladimir
next prev parent reply other threads:[~2017-08-25 15:57 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-24 15:33 [Qemu-devel] [PATCH 0/3] nbd-client: enter read_reply_co during init to avoid crash Stefan Hajnoczi
2017-08-24 15:33 ` [Qemu-devel] [PATCH 1/3] " Stefan Hajnoczi
2017-08-24 16:21 ` Paolo Bonzini
2017-08-24 17:37 ` Eric Blake
2017-08-24 17:42 ` Paolo Bonzini
2017-08-25 15:57 ` Vladimir Sementsov-Ogievskiy [this message]
2017-08-25 10:40 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2017-08-24 15:33 ` [Qemu-devel] [PATCH 2/3] qemu-iotests: improve nbd-fault-injector.py startup protocol Stefan Hajnoczi
2017-08-24 17:39 ` Eric Blake
2017-08-24 15:33 ` [Qemu-devel] [PATCH 3/3] qemu-iotests: test NBD over UNIX domain sockets in 083 Stefan Hajnoczi
2017-08-24 17:45 ` Eric Blake
2017-08-24 15:52 ` [Qemu-devel] [PATCH 0/3] nbd-client: enter read_reply_co during init to avoid crash Eric Blake
2017-08-24 16:05 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2017-08-25 15:08 ` Eric Blake
2017-08-25 20:33 ` Paolo Bonzini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d49450b1-9ef1-60ed-91e3-16ff163c6c80@virtuozzo.com \
--to=vsementsov@virtuozzo.com \
--cc=eblake@redhat.com \
--cc=kwolf@redhat.com \
--cc=pbonzini@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=stefanha@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.