nbd_wr_syncv is called either from coroutine or from client negotiation
code, when socket is in blocking mode. So, -EAGAIN is impossible.
Furthermore, EAGAIN is confusing, as, what to read/write again? With
EAGAIN as a return code we don't know how much data is already
read or written by the function, so in case of EAGAIN the whole
communication is broken.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
Hi all!
If I understand right, nbd_wr_syncv is called either from coroutine
or from client negotiation code, when socket is in blocking mode.
So, some thoughts
1. let's establish this with an assert, because returning EAGAIN is
confusing (see above)
2. should we in case of non-coroutine context start this coroutine in
nbd_wr_syncv, like in bdrv_prwv_co, and use non-blocking mode?
3. is there any problems or disadvantages of moving client negotiation
to coroutine too?
nbd/common.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/nbd/common.c b/nbd/common.c
index dccbb8e9de..4db45b3ede 100644
--- a/nbd/common.c
+++ b/nbd/common.c
@@ -20,6 +20,10 @@
#include "qapi/error.h"
#include "nbd-internal.h"
+/* nbd_wr_syncv
+ * The function may be called from coroutine or from non-coroutine context.
+ * When called from non-coroutine context @ioc must be in blocking mode.
+ */
ssize_t nbd_wr_syncv(QIOChannel *ioc,
struct iovec *iov,
size_t niov,
@@ -42,11 +46,8 @@ ssize_t nbd_wr_syncv(QIOChannel *ioc,
len = qio_channel_writev(ioc, local_iov, nlocal_iov, &local_err);
}
if (len == QIO_CHANNEL_ERR_BLOCK) {
- if (qemu_in_coroutine()) {
- qio_channel_yield(ioc, do_read ? G_IO_IN : G_IO_OUT);
- } else {
- return -EAGAIN;
- }
+ assert(qemu_in_coroutine());
+ qio_channel_yield(ioc, do_read ? G_IO_IN : G_IO_OUT);
continue;
}
if (len < 0) {
--
2.11.1
On 12/05/2017 16:17, Vladimir Sementsov-Ogievskiy wrote:
> nbd_wr_syncv is called either from coroutine or from client negotiation
> code, when socket is in blocking mode. So, -EAGAIN is impossible.
>
> Furthermore, EAGAIN is confusing, as, what to read/write again? With
> EAGAIN as a return code we don't know how much data is already
> read or written by the function, so in case of EAGAIN the whole
> communication is broken.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>
> Hi all!
>
> If I understand right, nbd_wr_syncv is called either from coroutine
> or from client negotiation code, when socket is in blocking mode.
> So, some thoughts
>
> 1. let's establish this with an assert, because returning EAGAIN is
> confusing (see above)
Yes, this seems like a good idea.
> 2. should we in case of non-coroutine context start this coroutine in
> nbd_wr_syncv, like in bdrv_prwv_co, and use non-blocking mode?
> 3. is there any problems or disadvantages of moving client negotiation
> to coroutine too?
When you move code to coroutines you need to be aware of what code can
now run concurrently, for example the monitor. I'm not sure that it's
possible to do this.
Paolo
> nbd/common.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/nbd/common.c b/nbd/common.c
> index dccbb8e9de..4db45b3ede 100644
> --- a/nbd/common.c
> +++ b/nbd/common.c
> @@ -20,6 +20,10 @@
> #include "qapi/error.h"
> #include "nbd-internal.h"
>
> +/* nbd_wr_syncv
> + * The function may be called from coroutine or from non-coroutine context.
> + * When called from non-coroutine context @ioc must be in blocking mode.
> + */
> ssize_t nbd_wr_syncv(QIOChannel *ioc,
> struct iovec *iov,
> size_t niov,
> @@ -42,11 +46,8 @@ ssize_t nbd_wr_syncv(QIOChannel *ioc,
> len = qio_channel_writev(ioc, local_iov, nlocal_iov, &local_err);
> }
> if (len == QIO_CHANNEL_ERR_BLOCK) {
> - if (qemu_in_coroutine()) {
> - qio_channel_yield(ioc, do_read ? G_IO_IN : G_IO_OUT);
> - } else {
> - return -EAGAIN;
> - }
> + assert(qemu_in_coroutine());
> + qio_channel_yield(ioc, do_read ? G_IO_IN : G_IO_OUT);
> continue;
> }
> if (len < 0) {
>
12.05.2017 18:46, Paolo Bonzini wrote:
>
> On 12/05/2017 16:17, Vladimir Sementsov-Ogievskiy wrote:
>> nbd_wr_syncv is called either from coroutine or from client negotiation
>> code, when socket is in blocking mode. So, -EAGAIN is impossible.
>>
>> Furthermore, EAGAIN is confusing, as, what to read/write again? With
>> EAGAIN as a return code we don't know how much data is already
>> read or written by the function, so in case of EAGAIN the whole
>> communication is broken.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>
>> Hi all!
>>
>> If I understand right, nbd_wr_syncv is called either from coroutine
>> or from client negotiation code, when socket is in blocking mode.
>> So, some thoughts
>>
>> 1. let's establish this with an assert, because returning EAGAIN is
>> confusing (see above)
> Yes, this seems like a good idea.
>
>> 2. should we in case of non-coroutine context start this coroutine in
>> nbd_wr_syncv, like in bdrv_prwv_co, and use non-blocking mode?
>> 3. is there any problems or disadvantages of moving client negotiation
>> to coroutine too?
> When you move code to coroutines you need to be aware of what code can
> now run concurrently, for example the monitor. I'm not sure that it's
> possible to do this.
Hmm, can you please give some example of a problem? qcow2_open starts
coroutines to read it's header, why nbd_open can't start
coroutine/coroutines to read/write some negotiation data?
>
> Paolo
>
>> nbd/common.c | 11 ++++++-----
>> 1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/nbd/common.c b/nbd/common.c
>> index dccbb8e9de..4db45b3ede 100644
>> --- a/nbd/common.c
>> +++ b/nbd/common.c
>> @@ -20,6 +20,10 @@
>> #include "qapi/error.h"
>> #include "nbd-internal.h"
>>
>> +/* nbd_wr_syncv
>> + * The function may be called from coroutine or from non-coroutine context.
>> + * When called from non-coroutine context @ioc must be in blocking mode.
>> + */
>> ssize_t nbd_wr_syncv(QIOChannel *ioc,
>> struct iovec *iov,
>> size_t niov,
>> @@ -42,11 +46,8 @@ ssize_t nbd_wr_syncv(QIOChannel *ioc,
>> len = qio_channel_writev(ioc, local_iov, nlocal_iov, &local_err);
>> }
>> if (len == QIO_CHANNEL_ERR_BLOCK) {
>> - if (qemu_in_coroutine()) {
>> - qio_channel_yield(ioc, do_read ? G_IO_IN : G_IO_OUT);
>> - } else {
>> - return -EAGAIN;
>> - }
>> + assert(qemu_in_coroutine());
>> + qio_channel_yield(ioc, do_read ? G_IO_IN : G_IO_OUT);
>> continue;
>> }
>> if (len < 0) {
>>
--
Best regards,
Vladimir
On 12/05/2017 17:57, Vladimir Sementsov-Ogievskiy wrote: > 12.05.2017 18:46, Paolo Bonzini wrote: >> >> On 12/05/2017 16:17, Vladimir Sementsov-Ogievskiy wrote: >>> nbd_wr_syncv is called either from coroutine or from client negotiation >>> code, when socket is in blocking mode. So, -EAGAIN is impossible. >>> >>> Furthermore, EAGAIN is confusing, as, what to read/write again? With >>> EAGAIN as a return code we don't know how much data is already >>> read or written by the function, so in case of EAGAIN the whole >>> communication is broken. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> --- >>> >>> Hi all! >>> >>> If I understand right, nbd_wr_syncv is called either from coroutine >>> or from client negotiation code, when socket is in blocking mode. >>> So, some thoughts >>> >>> 1. let's establish this with an assert, because returning EAGAIN is >>> confusing (see above) >> Yes, this seems like a good idea. >> >>> 2. should we in case of non-coroutine context start this coroutine in >>> nbd_wr_syncv, like in bdrv_prwv_co, and use non-blocking mode? >>> 3. is there any problems or disadvantages of moving client negotiation >>> to coroutine too? >> When you move code to coroutines you need to be aware of what code can >> now run concurrently, for example the monitor. I'm not sure that it's >> possible to do this. > > Hmm, can you please give some example of a problem? qcow2_open starts > coroutines to read it's header, why nbd_open can't start > coroutine/coroutines to read/write some negotiation data? Ah, it's not a problem if you use synchronous I/O (aio_poll) within the coroutines. But then it's still blocking I/O in every way (except you've fcntl-ed the descriptor to make it non-blocking); it's simply hidden underneath coroutines and aio_poll. Paolo
12.05.2017 19:30, Paolo Bonzini wrote: > > On 12/05/2017 17:57, Vladimir Sementsov-Ogievskiy wrote: >> 12.05.2017 18:46, Paolo Bonzini wrote: >>> On 12/05/2017 16:17, Vladimir Sementsov-Ogievskiy wrote: >>>> nbd_wr_syncv is called either from coroutine or from client negotiation >>>> code, when socket is in blocking mode. So, -EAGAIN is impossible. >>>> >>>> Furthermore, EAGAIN is confusing, as, what to read/write again? With >>>> EAGAIN as a return code we don't know how much data is already >>>> read or written by the function, so in case of EAGAIN the whole >>>> communication is broken. >>>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>> --- >>>> >>>> Hi all! >>>> >>>> If I understand right, nbd_wr_syncv is called either from coroutine >>>> or from client negotiation code, when socket is in blocking mode. >>>> So, some thoughts >>>> >>>> 1. let's establish this with an assert, because returning EAGAIN is >>>> confusing (see above) >>> Yes, this seems like a good idea. >>> >>>> 2. should we in case of non-coroutine context start this coroutine in >>>> nbd_wr_syncv, like in bdrv_prwv_co, and use non-blocking mode? >>>> 3. is there any problems or disadvantages of moving client negotiation >>>> to coroutine too? >>> When you move code to coroutines you need to be aware of what code can >>> now run concurrently, for example the monitor. I'm not sure that it's >>> possible to do this. >> Hmm, can you please give some example of a problem? qcow2_open starts >> coroutines to read it's header, why nbd_open can't start >> coroutine/coroutines to read/write some negotiation data? > Ah, it's not a problem if you use synchronous I/O (aio_poll) within the > coroutines. But then it's still blocking I/O in every way (except > you've fcntl-ed the descriptor to make it non-blocking); it's simply > hidden underneath coroutines and aio_poll. I mean, make negotiation behave like normal nbd communication, non-blocking socket + yield.. So, some other coroutines may do their work, while nbd-negotiation coroutine waits for io.. > > Paolo -- Best regards, Vladimir
15.05.2017 12:43, Vladimir Sementsov-Ogievskiy wrote: > 12.05.2017 19:30, Paolo Bonzini wrote: >> >> On 12/05/2017 17:57, Vladimir Sementsov-Ogievskiy wrote: >>> 12.05.2017 18:46, Paolo Bonzini wrote: >>>> On 12/05/2017 16:17, Vladimir Sementsov-Ogievskiy wrote: >>>>> nbd_wr_syncv is called either from coroutine or from client >>>>> negotiation >>>>> code, when socket is in blocking mode. So, -EAGAIN is impossible. >>>>> >>>>> Furthermore, EAGAIN is confusing, as, what to read/write again? With >>>>> EAGAIN as a return code we don't know how much data is already >>>>> read or written by the function, so in case of EAGAIN the whole >>>>> communication is broken. >>>>> >>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy >>>>> <vsementsov@virtuozzo.com> >>>>> --- >>>>> >>>>> Hi all! >>>>> >>>>> If I understand right, nbd_wr_syncv is called either from coroutine >>>>> or from client negotiation code, when socket is in blocking mode. >>>>> So, some thoughts >>>>> >>>>> 1. let's establish this with an assert, because returning EAGAIN is >>>>> confusing (see above) >>>> Yes, this seems like a good idea. >>>> >>>>> 2. should we in case of non-coroutine context start this coroutine in >>>>> nbd_wr_syncv, like in bdrv_prwv_co, and use non-blocking mode? >>>>> 3. is there any problems or disadvantages of moving client >>>>> negotiation >>>>> to coroutine too? >>>> When you move code to coroutines you need to be aware of what code can >>>> now run concurrently, for example the monitor. I'm not sure that it's >>>> possible to do this. >>> Hmm, can you please give some example of a problem? qcow2_open starts >>> coroutines to read it's header, why nbd_open can't start >>> coroutine/coroutines to read/write some negotiation data? >> Ah, it's not a problem if you use synchronous I/O (aio_poll) within the >> coroutines. But then it's still blocking I/O in every way (except >> you've fcntl-ed the descriptor to make it non-blocking); it's simply >> hidden underneath coroutines and aio_poll. > > I mean, make negotiation behave like normal nbd communication, > non-blocking socket + yield.. So, some other coroutines may do their > work, while nbd-negotiation coroutine waits for io.. > >> >> Paolo > > Also, one more question here: in nbd_negotiate_write(), why do we need qio_channel_add_watch? write_sync will yield with qio_channel_yield() until io complete, why to use 2 mechanisms to wake up a coroutine? -- Best regards, Vladimir
16.05.2017 12:10, Vladimir Sementsov-Ogievskiy wrote: > 15.05.2017 12:43, Vladimir Sementsov-Ogievskiy wrote: >> 12.05.2017 19:30, Paolo Bonzini wrote: >>> >>> On 12/05/2017 17:57, Vladimir Sementsov-Ogievskiy wrote: >>>> 12.05.2017 18:46, Paolo Bonzini wrote: >>>>> On 12/05/2017 16:17, Vladimir Sementsov-Ogievskiy wrote: >>>>>> nbd_wr_syncv is called either from coroutine or from client >>>>>> negotiation >>>>>> code, when socket is in blocking mode. So, -EAGAIN is impossible. >>>>>> >>>>>> Furthermore, EAGAIN is confusing, as, what to read/write again? With >>>>>> EAGAIN as a return code we don't know how much data is already >>>>>> read or written by the function, so in case of EAGAIN the whole >>>>>> communication is broken. >>>>>> >>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy >>>>>> <vsementsov@virtuozzo.com> >>>>>> --- >>>>>> >>>>>> Hi all! >>>>>> >>>>>> If I understand right, nbd_wr_syncv is called either from coroutine >>>>>> or from client negotiation code, when socket is in blocking mode. >>>>>> So, some thoughts >>>>>> >>>>>> 1. let's establish this with an assert, because returning EAGAIN is >>>>>> confusing (see above) >>>>> Yes, this seems like a good idea. >>>>> >>>>>> 2. should we in case of non-coroutine context start this >>>>>> coroutine in >>>>>> nbd_wr_syncv, like in bdrv_prwv_co, and use non-blocking mode? >>>>>> 3. is there any problems or disadvantages of moving client >>>>>> negotiation >>>>>> to coroutine too? >>>>> When you move code to coroutines you need to be aware of what code >>>>> can >>>>> now run concurrently, for example the monitor. I'm not sure that >>>>> it's >>>>> possible to do this. >>>> Hmm, can you please give some example of a problem? qcow2_open starts >>>> coroutines to read it's header, why nbd_open can't start >>>> coroutine/coroutines to read/write some negotiation data? >>> Ah, it's not a problem if you use synchronous I/O (aio_poll) within the >>> coroutines. But then it's still blocking I/O in every way (except >>> you've fcntl-ed the descriptor to make it non-blocking); it's simply >>> hidden underneath coroutines and aio_poll. >> >> I mean, make negotiation behave like normal nbd communication, >> non-blocking socket + yield.. So, some other coroutines may do their >> work, while nbd-negotiation coroutine waits for io.. >> >>> >>> Paolo >> >> > > Also, one more question here: in nbd_negotiate_write(), why do we need > qio_channel_add_watch? write_sync will yield with qio_channel_yield() > until io complete, why to use 2 mechanisms to wake up a coroutine? > > Hmm, these nbd_negotiate_* functions was introduced in 1a6245a5b, when nbd_wr_syncv was working through qemu_co_sendv_recvv, which just yields, without setting any handlers. But now, nbd_wr_syncv works through qio_channel_yield() which sets handlers, so the code with extra watchers looks wrong. -- Best regards, Vladimir
On 16/05/2017 11:32, Vladimir Sementsov-Ogievskiy wrote: > 16.05.2017 12:10, Vladimir Sementsov-Ogievskiy wrote: >> 15.05.2017 12:43, Vladimir Sementsov-Ogievskiy wrote: >>> I mean, make negotiation behave like normal nbd communication, >>> non-blocking socket + yield.. So, some other coroutines may do their >>> work, while nbd-negotiation coroutine waits for io.. Some callers of bdrv_open may not allow reentrancy. For example: handle_qmp_command -> qmp_dispatch -> do_qmp_dispatch -> qmp_marshal_blockdev_add -> qmp_blockdev_add -> bds_tree_init -> bdrv_open You cannot return to the monitor before qmp_blockdev_add is done, otherwise you don't have a return value for handle_qmp_command to pass to monitor_json_emitter. >> Also, one more question here: in nbd_negotiate_write(), why do we need >> qio_channel_add_watch? write_sync will yield with qio_channel_yield() >> until io complete, why to use 2 mechanisms to wake up a coroutine? > > Hmm, these nbd_negotiate_* functions was introduced in 1a6245a5b, when > nbd_wr_syncv was working through qemu_co_sendv_recvv, which just yields, > without setting any handlers. But now, nbd_wr_syncv works through > qio_channel_yield() which sets handlers, so the code with extra watchers > looks wrong. Yes, I think you're right about that. Paolo
16.05.2017 12:51, Paolo Bonzini wrote: > > On 16/05/2017 11:32, Vladimir Sementsov-Ogievskiy wrote: >> 16.05.2017 12:10, Vladimir Sementsov-Ogievskiy wrote: >>> 15.05.2017 12:43, Vladimir Sementsov-Ogievskiy wrote: >>>> I mean, make negotiation behave like normal nbd communication, >>>> non-blocking socket + yield.. So, some other coroutines may do their >>>> work, while nbd-negotiation coroutine waits for io.. > Some callers of bdrv_open may not allow reentrancy. For example: > > handle_qmp_command > -> qmp_dispatch > -> do_qmp_dispatch > -> qmp_marshal_blockdev_add > -> qmp_blockdev_add > -> bds_tree_init > -> bdrv_open > > You cannot return to the monitor before qmp_blockdev_add is done, > otherwise you don't have a return value for handle_qmp_command to pass > to monitor_json_emitter. Hmm. What about something like bdrv_pread (finally, bdrv_prwv_co) for non-coroutine, ie, calling aio_poll in a while loop, until coroutine finishes? > >>> Also, one more question here: in nbd_negotiate_write(), why do we need >>> qio_channel_add_watch? write_sync will yield with qio_channel_yield() >>> until io complete, why to use 2 mechanisms to wake up a coroutine? >> Hmm, these nbd_negotiate_* functions was introduced in 1a6245a5b, when >> nbd_wr_syncv was working through qemu_co_sendv_recvv, which just yields, >> without setting any handlers. But now, nbd_wr_syncv works through >> qio_channel_yield() which sets handlers, so the code with extra watchers >> looks wrong. > Yes, I think you're right about that. Ok, I'll make a patch for it and finish LOG->errp conversion. > > Paolo -- Best regards, Vladimir
On 16/05/2017 12:16, Vladimir Sementsov-Ogievskiy wrote: > 16.05.2017 12:51, Paolo Bonzini wrote: >> >> On 16/05/2017 11:32, Vladimir Sementsov-Ogievskiy wrote: >>> 16.05.2017 12:10, Vladimir Sementsov-Ogievskiy wrote: >>>> 15.05.2017 12:43, Vladimir Sementsov-Ogievskiy wrote: >>>>> I mean, make negotiation behave like normal nbd communication, >>>>> non-blocking socket + yield.. So, some other coroutines may do their >>>>> work, while nbd-negotiation coroutine waits for io.. >> Some callers of bdrv_open may not allow reentrancy. For example: >> >> handle_qmp_command >> -> qmp_dispatch >> -> do_qmp_dispatch >> -> qmp_marshal_blockdev_add >> -> qmp_blockdev_add >> -> bds_tree_init >> -> bdrv_open >> >> You cannot return to the monitor before qmp_blockdev_add is done, >> otherwise you don't have a return value for handle_qmp_command to pass >> to monitor_json_emitter. > > Hmm. What about something like bdrv_pread (finally, bdrv_prwv_co) for > non-coroutine, ie, calling aio_poll in a while loop, until coroutine > finishes? Yes, of course that would work: you create a coroutine, and wrap it with aio_poll until it completes. Paolo >> >>>> Also, one more question here: in nbd_negotiate_write(), why do we need >>>> qio_channel_add_watch? write_sync will yield with qio_channel_yield() >>>> until io complete, why to use 2 mechanisms to wake up a coroutine? >>> Hmm, these nbd_negotiate_* functions was introduced in 1a6245a5b, when >>> nbd_wr_syncv was working through qemu_co_sendv_recvv, which just yields, >>> without setting any handlers. But now, nbd_wr_syncv works through >>> qio_channel_yield() which sets handlers, so the code with extra watchers >>> looks wrong. >> Yes, I think you're right about that. > > Ok, I'll make a patch for it and finish LOG->errp conversion. > >> >> Paolo > >
© 2016 - 2024 Red Hat, Inc.