[Qemu-devel] [PATCH 0/4] nbd: non-blocking negotiation

[Qemu-devel] [PATCH 0/4] nbd: non-blocking negotiation

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here is a try of moving to non-blocking negotiation in nbd
code, proposed by Deniel in
https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg03817.html

in thread "[PATCH v4 00/10] NBD reconnect"
https://lists.gnu.org/archive/html/qemu-devel/2018-07/msg05973.html

Vladimir Sementsov-Ogievskiy (4):
  io/channel: add qio_channel_get_attached_aio_context()
  nbd/client: do negotiation in coroutine
  nbd: do qemu_coroutine_yield during tls handshake
  block/nbd-client: use non-blocking io channel for nbd negotiation

 include/io/channel.h |   9 +++
 block/nbd-client.c   |  10 +--
 io/channel.c         |   5 ++
 nbd/client.c         | 149 ++++++++++++++++++++++++++++++++++---------
 nbd/common.c         |   6 +-
 nbd/server.c         |  45 +++++--------
 nbd/trace-events     |  15 ++---
 7 files changed, 161 insertions(+), 78 deletions(-)

-- 
2.18.0

[Qemu-devel] [PATCH 1/4] io/channel: add qio_channel_get_attached_aio_context()

[Vladimir Sementsov-Ogievskiy]

Expose attached aio context. It will be used in nbd code, to
understand, in which aio context negotiation should be done.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/io/channel.h | 9 +++++++++
 io/channel.c         | 5 +++++
 2 files changed, 14 insertions(+)

diff --git a/include/io/channel.h b/include/io/channel.h
index da2f138200..1a1e4a01b0 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -718,6 +718,15 @@ GSource *qio_channel_add_watch_source(QIOChannel *ioc,
 void qio_channel_attach_aio_context(QIOChannel *ioc,
                                     AioContext *ctx);
 
+/*
+ * qio_channel_get_aio_context
+ * @ioc: the channel object
+ *
+ * Returns channel AioContext if any attached by
+ * qio_channel_attach_aio_context(), otherwise NULL.
+ */
+AioContext *qio_channel_get_attached_aio_context(QIOChannel *ioc);
+
 /**
  * qio_channel_detach_aio_context:
  * @ioc: the channel object
diff --git a/io/channel.c b/io/channel.c
index 8dd0684f5d..a1b937bb6b 100644
--- a/io/channel.c
+++ b/io/channel.c
@@ -454,6 +454,11 @@ void qio_channel_detach_aio_context(QIOChannel *ioc)
     ioc->ctx = NULL;
 }
 
+AioContext *qio_channel_get_attached_aio_context(QIOChannel *ioc)
+{
+    return ioc->ctx;
+}
+
 void coroutine_fn qio_channel_yield(QIOChannel *ioc,
                                     GIOCondition condition)
 {
-- 
2.18.0

Re: [Qemu-devel] [PATCH 1/4] io/channel: add qio_channel_get_attached_aio_context()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1592 bytes --]

On 2/11/19 6:55 AM, Vladimir Sementsov-Ogievskiy wrote:
> Expose attached aio context. It will be used in nbd code, to
> understand, in which aio context negotiation should be done.

s/, in/ in/

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/io/channel.h | 9 +++++++++
>  io/channel.c         | 5 +++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/include/io/channel.h b/include/io/channel.h
> index da2f138200..1a1e4a01b0 100644
> --- a/include/io/channel.h
> +++ b/include/io/channel.h
> @@ -718,6 +718,15 @@ GSource *qio_channel_add_watch_source(QIOChannel *ioc,
>  void qio_channel_attach_aio_context(QIOChannel *ioc,
>                                      AioContext *ctx);
>  
> +/*
> + * qio_channel_get_aio_context

Mismatch between comments...

> + * @ioc: the channel object
> + *
> + * Returns channel AioContext if any attached by
> + * qio_channel_attach_aio_context(), otherwise NULL.
> + */
> +AioContext *qio_channel_get_attached_aio_context(QIOChannel *ioc);

and function name.

Otherwise, looks straightforward, but I'd rather get Daniel's opinion on
whether we need it or if something else is better.

> +AioContext *qio_channel_get_attached_aio_context(QIOChannel *ioc)
> +{
> +    return ioc->ctx;
> +}
> +
>  void coroutine_fn qio_channel_yield(QIOChannel *ioc,
>                                      GIOCondition condition)
>  {
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 1/4] io/channel: add qio_channel_get_attached_aio_context()

[Daniel P. Berrangé]

On Mon, Feb 11, 2019 at 03:55:58PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Expose attached aio context. It will be used in nbd code, to
> understand, in which aio context negotiation should be done.

I'm not especially objecting to the idea of adding the API to the
QIOChannel class, but I'm surprised that NBD needs this. Surely it
already knows what AIO context it assigned to the channel in the
first place. IOW, it feels like this is papering over a limitation
in NBD not keeping track of what AIO context it should be using.

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/io/channel.h | 9 +++++++++
>  io/channel.c         | 5 +++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/include/io/channel.h b/include/io/channel.h
> index da2f138200..1a1e4a01b0 100644
> --- a/include/io/channel.h
> +++ b/include/io/channel.h
> @@ -718,6 +718,15 @@ GSource *qio_channel_add_watch_source(QIOChannel *ioc,
>  void qio_channel_attach_aio_context(QIOChannel *ioc,
>                                      AioContext *ctx);
>  
> +/*
> + * qio_channel_get_aio_context
> + * @ioc: the channel object
> + *
> + * Returns channel AioContext if any attached by
> + * qio_channel_attach_aio_context(), otherwise NULL.
> + */
> +AioContext *qio_channel_get_attached_aio_context(QIOChannel *ioc);
> +
>  /**
>   * qio_channel_detach_aio_context:
>   * @ioc: the channel object
> diff --git a/io/channel.c b/io/channel.c
> index 8dd0684f5d..a1b937bb6b 100644
> --- a/io/channel.c
> +++ b/io/channel.c
> @@ -454,6 +454,11 @@ void qio_channel_detach_aio_context(QIOChannel *ioc)
>      ioc->ctx = NULL;
>  }
>  
> +AioContext *qio_channel_get_attached_aio_context(QIOChannel *ioc)
> +{
> +    return ioc->ctx;
> +}
> +
>  void coroutine_fn qio_channel_yield(QIOChannel *ioc,
>                                      GIOCondition condition)
>  {
> -- 
> 2.18.0
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 1/4] io/channel: add qio_channel_get_attached_aio_context()

[Vladimir Sementsov-Ogievskiy]

12.02.2019 13:33, Daniel P. Berrangé wrote:
> On Mon, Feb 11, 2019 at 03:55:58PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Expose attached aio context. It will be used in nbd code, to
>> understand, in which aio context negotiation should be done.
> 
> I'm not especially objecting to the idea of adding the API to the
> QIOChannel class, but I'm surprised that NBD needs this. Surely it
> already knows what AIO context it assigned to the channel in the
> first place.

Actually not, nbd/client.c don't know it, it gets @ioc as a paramter of nbd_receive_negotiate
and nbd_receive_export_list, which may be called from block/nbd-client.c and qemu-nbd.c..

for example, nbd-client.c do

nbd_client_attach_aio_context(bs, bdrv_get_aio_context(bs));

where nbd_client_attach_aio_context is

void nbd_client_attach_aio_context(BlockDriverState *bs,
                                    AioContext *new_context)
{
     NBDClientSession *client = nbd_get_client_session(bs);
     qio_channel_attach_aio_context(QIO_CHANNEL(client->ioc), new_context);
     aio_co_schedule(new_context, client->connection_co);
}

It does it after nbd_receive_negotiate(), but I think, it should be split in these series in nbd_client_connect(),
so that we attach aio context before negotiation and schedule connection_co after it.

and NBDClientSession is block/* related object. So, nbd/client.c sees only resulting ioc.
And it seems better to get aio context from ioc, than pass additional parameter to nbd_receive_negotiate()
together with ioc.

  IOW, it feels like this is papering over a limitation
> in NBD not keeping track of what AIO context it should be using.
> 
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   include/io/channel.h | 9 +++++++++
>>   io/channel.c         | 5 +++++
>>   2 files changed, 14 insertions(+)
>>
>> diff --git a/include/io/channel.h b/include/io/channel.h
>> index da2f138200..1a1e4a01b0 100644
>> --- a/include/io/channel.h
>> +++ b/include/io/channel.h
>> @@ -718,6 +718,15 @@ GSource *qio_channel_add_watch_source(QIOChannel *ioc,
>>   void qio_channel_attach_aio_context(QIOChannel *ioc,
>>                                       AioContext *ctx);
>>   
>> +/*
>> + * qio_channel_get_aio_context
>> + * @ioc: the channel object
>> + *
>> + * Returns channel AioContext if any attached by
>> + * qio_channel_attach_aio_context(), otherwise NULL.
>> + */
>> +AioContext *qio_channel_get_attached_aio_context(QIOChannel *ioc);
>> +
>>   /**
>>    * qio_channel_detach_aio_context:
>>    * @ioc: the channel object
>> diff --git a/io/channel.c b/io/channel.c
>> index 8dd0684f5d..a1b937bb6b 100644
>> --- a/io/channel.c
>> +++ b/io/channel.c
>> @@ -454,6 +454,11 @@ void qio_channel_detach_aio_context(QIOChannel *ioc)
>>       ioc->ctx = NULL;
>>   }
>>   
>> +AioContext *qio_channel_get_attached_aio_context(QIOChannel *ioc)
>> +{
>> +    return ioc->ctx;
>> +}
>> +
>>   void coroutine_fn qio_channel_yield(QIOChannel *ioc,
>>                                       GIOCondition condition)
>>   {
>> -- 
>> 2.18.0
>>
> 
> Regards,
> Daniel
> 


-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH 2/4] nbd/client: do negotiation in coroutine

[Vladimir Sementsov-Ogievskiy]

As a first step to non-blocking negotiation, move it to coroutine.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/client.c | 123 +++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 109 insertions(+), 14 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c
index 10a52ad7d0..2ba2220a4a 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -859,13 +859,14 @@ static int nbd_list_meta_contexts(QIOChannel *ioc,
  *          2: server is newstyle, but lacks structured replies
  *          3: server is newstyle and set up for structured replies
  */
-static int nbd_start_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
-                               const char *hostname, QIOChannel **outioc,
-                               bool structured_reply, bool *zeroes,
-                               Error **errp)
+static int coroutine_fn nbd_co_start_negotiate(
+        QIOChannel *ioc, QCryptoTLSCreds *tlscreds, const char *hostname,
+        QIOChannel **outioc, bool structured_reply, bool *zeroes, Error **errp)
 {
     uint64_t magic;
 
+    assert(qemu_in_coroutine());
+
     trace_nbd_start_negotiate(tlscreds, hostname ? hostname : "<null>");
 
     if (zeroes) {
@@ -990,19 +991,20 @@ static int nbd_negotiate_finish_oldstyle(QIOChannel *ioc, NBDExportInfo *info,
  * Returns: negative errno: failure talking to server
  *          0: server is connected
  */
-int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
-                          const char *hostname, QIOChannel **outioc,
-                          NBDExportInfo *info, Error **errp)
+static int coroutine_fn nbd_co_receive_negotiate(
+        QIOChannel *ioc, QCryptoTLSCreds *tlscreds, const char *hostname,
+        QIOChannel **outioc, NBDExportInfo *info, Error **errp)
 {
     int result;
     bool zeroes;
     bool base_allocation = info->base_allocation;
 
+    assert(qemu_in_coroutine());
     assert(info->name);
     trace_nbd_receive_negotiate_name(info->name);
 
-    result = nbd_start_negotiate(ioc, tlscreds, hostname, outioc,
-                                 info->structured_reply, &zeroes, errp);
+    result = nbd_co_start_negotiate(ioc, tlscreds, hostname, outioc,
+                                   info->structured_reply, &zeroes, errp);
 
     info->structured_reply = false;
     info->base_allocation = false;
@@ -1108,9 +1110,9 @@ void nbd_free_export_list(NBDExportInfo *info, int count)
  * in @info by the server, or -1 on error. Caller must free @info using
  * nbd_free_export_list().
  */
-int nbd_receive_export_list(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
-                            const char *hostname, NBDExportInfo **info,
-                            Error **errp)
+static int coroutine_fn nbd_co_receive_export_list(
+        QIOChannel *ioc, QCryptoTLSCreds *tlscreds, const char *hostname,
+        NBDExportInfo **info, Error **errp)
 {
     int result;
     int count = 0;
@@ -1120,9 +1122,10 @@ int nbd_receive_export_list(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
     NBDExportInfo *array = NULL;
     QIOChannel *sioc = NULL;
 
+    assert(qemu_in_coroutine());
     *info = NULL;
-    result = nbd_start_negotiate(ioc, tlscreds, hostname, &sioc, true, NULL,
-                                 errp);
+    result = nbd_co_start_negotiate(ioc, tlscreds, hostname, &sioc, true, NULL,
+                                    errp);
     if (tlscreds && sioc) {
         ioc = sioc;
     }
@@ -1212,6 +1215,98 @@ int nbd_receive_export_list(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
     return ret;
 }
 
+typedef struct NbdReceiveNegotiateCo {
+    QIOChannel *ioc;
+    QCryptoTLSCreds *tlscreds;
+    const char *hostname;
+    union {
+        struct {
+            QIOChannel **outioc;
+            NBDExportInfo *info;
+        } negotiate;
+        struct {
+            NBDExportInfo **info;
+        } export_list;
+    };
+    Error **errp;
+    int ret;
+} NbdReceiveNegotiateCo;
+
+static void coroutine_fn nbd_receive_negotiate_entry(void *opaque)
+{
+    NbdReceiveNegotiateCo *s = opaque;
+
+    s->ret = nbd_co_receive_negotiate(s->ioc, s->tlscreds, s->hostname,
+                                      s->negotiate.outioc, s->negotiate.info,
+                                      s->errp);
+}
+
+static void coroutine_fn nbd_receive_export_list_entry(void *opaque)
+{
+    NbdReceiveNegotiateCo *s = opaque;
+
+    s->ret = nbd_co_receive_export_list(s->ioc, s->tlscreds, s->hostname,
+                                        s->export_list.info, s->errp);
+}
+
+static int nbd_receive_common(CoroutineEntry *entry,
+                              NbdReceiveNegotiateCo *data)
+{
+    data->ret = -EINPROGRESS;
+
+    if (qemu_in_coroutine()) {
+        entry(data);
+    } else {
+        AioContext *ctx = qio_channel_get_attached_aio_context(data->ioc);
+        bool attach = !ctx;
+
+        if (attach) {
+            ctx = qemu_get_current_aio_context();
+            qio_channel_attach_aio_context(data->ioc, ctx);
+        }
+
+        qemu_aio_coroutine_enter(ctx, qemu_coroutine_create(entry, data));
+        AIO_WAIT_WHILE(ctx, data->ret == -EINPROGRESS);
+
+        if (attach) {
+            qio_channel_detach_aio_context(data->ioc);
+        }
+    }
+
+    return data->ret;
+}
+
+int nbd_receive_negotiate(
+        QIOChannel *ioc, QCryptoTLSCreds *tlscreds, const char *hostname,
+        QIOChannel **outioc, NBDExportInfo *info, Error **errp)
+{
+    NbdReceiveNegotiateCo data = {
+        .ioc = ioc,
+        .tlscreds = tlscreds,
+        .hostname = hostname,
+        .negotiate.outioc = outioc,
+        .negotiate.info = info,
+        .errp = errp,
+    };
+
+    return nbd_receive_common(nbd_receive_negotiate_entry, &data);
+}
+
+int nbd_receive_export_list(
+        QIOChannel *ioc, QCryptoTLSCreds *tlscreds, const char *hostname,
+        NBDExportInfo **info, Error **errp)
+{
+    NbdReceiveNegotiateCo data = {
+        .ioc = ioc,
+        .tlscreds = tlscreds,
+        .hostname = hostname,
+        .export_list.info = info,
+        .errp = errp,
+    };
+
+    return nbd_receive_common(nbd_receive_export_list_entry, &data);
+}
+
 #ifdef __linux__
 int nbd_init(int fd, QIOChannelSocket *sioc, NBDExportInfo *info,
              Error **errp)
-- 
2.18.0

Re: [Qemu-devel] [PATCH 2/4] nbd/client: do negotiation in coroutine

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 4550 bytes --]

On 2/11/19 6:55 AM, Vladimir Sementsov-Ogievskiy wrote:
> As a first step to non-blocking negotiation, move it to coroutine.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/client.c | 123 +++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 109 insertions(+), 14 deletions(-)
> 
> diff --git a/nbd/client.c b/nbd/client.c
> index 10a52ad7d0..2ba2220a4a 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -859,13 +859,14 @@ static int nbd_list_meta_contexts(QIOChannel *ioc,
>   *          2: server is newstyle, but lacks structured replies
>   *          3: server is newstyle and set up for structured replies
>   */
> -static int nbd_start_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
> -                               const char *hostname, QIOChannel **outioc,
> -                               bool structured_reply, bool *zeroes,
> -                               Error **errp)
> +static int coroutine_fn nbd_co_start_negotiate(
> +        QIOChannel *ioc, QCryptoTLSCreds *tlscreds, const char *hostname,
> +        QIOChannel **outioc, bool structured_reply, bool *zeroes, Error **errp)
>  {
>      uint64_t magic;
>  
> +    assert(qemu_in_coroutine());
> +

Do we need this assert, since this function is static, and only called by:

>      trace_nbd_start_negotiate(tlscreds, hostname ? hostname : "<null>");
>  
>      if (zeroes) {
> @@ -990,19 +991,20 @@ static int nbd_negotiate_finish_oldstyle(QIOChannel *ioc, NBDExportInfo *info,
>   * Returns: negative errno: failure talking to server
>   *          0: server is connected
>   */
> -int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
> -                          const char *hostname, QIOChannel **outioc,
> -                          NBDExportInfo *info, Error **errp)
> +static int coroutine_fn nbd_co_receive_negotiate(
> +        QIOChannel *ioc, QCryptoTLSCreds *tlscreds, const char *hostname,
> +        QIOChannel **outioc, NBDExportInfo *info, Error **errp)
>  {
>      int result;
>      bool zeroes;
>      bool base_allocation = info->base_allocation;
>  
> +    assert(qemu_in_coroutine());
>      assert(info->name);
>      trace_nbd_receive_negotiate_name(info->name);
>  
> -    result = nbd_start_negotiate(ioc, tlscreds, hostname, outioc,
> -                                 info->structured_reply, &zeroes, errp);
> +    result = nbd_co_start_negotiate(ioc, tlscreds, hostname, outioc,
> +                                   info->structured_reply, &zeroes, errp);

other functions in the same file that have also made the same assertion?
 For that matter, does the assert() add any value over the fact that we
already annotated things as coroutine_fn?

I know that at some point, there was a proposal on the list for having
the compiler enforce coroutine_fn annotations (ensuring that a
coroutine_fn only calls other coroutine_fns, and that coroutines can
only be started on an entry point properly marked), but that's a
different task for another day.


> +static int nbd_receive_common(CoroutineEntry *entry,
> +                              NbdReceiveNegotiateCo *data)
> +{
> +    data->ret = -EINPROGRESS;
> +
> +    if (qemu_in_coroutine()) {
> +        entry(data);
> +    } else {
> +        AioContext *ctx = qio_channel_get_attached_aio_context(data->ioc);
> +        bool attach = !ctx;

There's where you used the function added in 1/4.

> +
> +        if (attach) {
> +            ctx = qemu_get_current_aio_context();
> +            qio_channel_attach_aio_context(data->ioc, ctx);
> +        }
> +
> +        qemu_aio_coroutine_enter(ctx, qemu_coroutine_create(entry, data));
> +        AIO_WAIT_WHILE(ctx, data->ret == -EINPROGRESS);
> +
> +        if (attach) {
> +            qio_channel_detach_aio_context(data->ioc);
> +        }
> +    }
> +
> +    return data->ret;
> +}

Looks reasonable.

> +
> +int nbd_receive_negotiate(
> +        QIOChannel *ioc, QCryptoTLSCreds *tlscreds, const char *hostname,
> +        QIOChannel **outioc, NBDExportInfo *info, Error **errp)
> +{

Why the reflowed parameter list, compared to its existing listing (as
shown by the - lines where you added nbd_receive_co_negotiate)? That's
only cosmetic, so I can live with it, but it seems like it makes the
diff slightly larger.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 2/4] nbd/client: do negotiation in coroutine

[Vladimir Sementsov-Ogievskiy]

12.02.2019 0:38, Eric Blake wrote:
> On 2/11/19 6:55 AM, Vladimir Sementsov-Ogievskiy wrote:
>> As a first step to non-blocking negotiation, move it to coroutine.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   nbd/client.c | 123 +++++++++++++++++++++++++++++++++++++++++++++------
>>   1 file changed, 109 insertions(+), 14 deletions(-)
>>
>> diff --git a/nbd/client.c b/nbd/client.c
>> index 10a52ad7d0..2ba2220a4a 100644
>> --- a/nbd/client.c
>> +++ b/nbd/client.c
>> @@ -859,13 +859,14 @@ static int nbd_list_meta_contexts(QIOChannel *ioc,
>>    *          2: server is newstyle, but lacks structured replies
>>    *          3: server is newstyle and set up for structured replies
>>    */
>> -static int nbd_start_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
>> -                               const char *hostname, QIOChannel **outioc,
>> -                               bool structured_reply, bool *zeroes,
>> -                               Error **errp)
>> +static int coroutine_fn nbd_co_start_negotiate(
>> +        QIOChannel *ioc, QCryptoTLSCreds *tlscreds, const char *hostname,
>> +        QIOChannel **outioc, bool structured_reply, bool *zeroes, Error **errp)
>>   {
>>       uint64_t magic;
>>   
>> +    assert(qemu_in_coroutine());
>> +
> 
> Do we need this assert, since this function is static, and only called by:

OK, I think this can be dropped

> 
>>       trace_nbd_start_negotiate(tlscreds, hostname ? hostname : "<null>");
>>   
>>       if (zeroes) {
>> @@ -990,19 +991,20 @@ static int nbd_negotiate_finish_oldstyle(QIOChannel *ioc, NBDExportInfo *info,
>>    * Returns: negative errno: failure talking to server
>>    *          0: server is connected
>>    */
>> -int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
>> -                          const char *hostname, QIOChannel **outioc,
>> -                          NBDExportInfo *info, Error **errp)
>> +static int coroutine_fn nbd_co_receive_negotiate(
>> +        QIOChannel *ioc, QCryptoTLSCreds *tlscreds, const char *hostname,
>> +        QIOChannel **outioc, NBDExportInfo *info, Error **errp)
>>   {
>>       int result;
>>       bool zeroes;
>>       bool base_allocation = info->base_allocation;
>>   
>> +    assert(qemu_in_coroutine());
>>       assert(info->name);
>>       trace_nbd_receive_negotiate_name(info->name);
>>   
>> -    result = nbd_start_negotiate(ioc, tlscreds, hostname, outioc,
>> -                                 info->structured_reply, &zeroes, errp);
>> +    result = nbd_co_start_negotiate(ioc, tlscreds, hostname, outioc,
>> +                                   info->structured_reply, &zeroes, errp);
> 
> other functions in the same file that have also made the same assertion?
>   For that matter, does the assert() add any value over the fact that we
> already annotated things as coroutine_fn?
> 
> I know that at some point, there was a proposal on the list for having
> the compiler enforce coroutine_fn annotations (ensuring that a
> coroutine_fn only calls other coroutine_fns, and that coroutines can
> only be started on an entry point properly marked), but that's a
> different task for another day.
> 

I thought, that converting function to be "coroutine_fn" (not creating a new one)
is a good reason to add an assertion.

> 
>> +static int nbd_receive_common(CoroutineEntry *entry,
>> +                              NbdReceiveNegotiateCo *data)
>> +{
>> +    data->ret = -EINPROGRESS;
>> +
>> +    if (qemu_in_coroutine()) {
>> +        entry(data);
>> +    } else {
>> +        AioContext *ctx = qio_channel_get_attached_aio_context(data->ioc);
>> +        bool attach = !ctx;
> 
> There's where you used the function added in 1/4.
> 
>> +
>> +        if (attach) {
>> +            ctx = qemu_get_current_aio_context();
>> +            qio_channel_attach_aio_context(data->ioc, ctx);
>> +        }
>> +
>> +        qemu_aio_coroutine_enter(ctx, qemu_coroutine_create(entry, data));
>> +        AIO_WAIT_WHILE(ctx, data->ret == -EINPROGRESS);
>> +
>> +        if (attach) {
>> +            qio_channel_detach_aio_context(data->ioc);
>> +        }
>> +    }
>> +
>> +    return data->ret;
>> +}
> 
> Looks reasonable.
> 
>> +
>> +int nbd_receive_negotiate(
>> +        QIOChannel *ioc, QCryptoTLSCreds *tlscreds, const char *hostname,
>> +        QIOChannel **outioc, NBDExportInfo *info, Error **errp)
>> +{
> 
> Why the reflowed parameter list, compared to its existing listing (as
> shown by the - lines where you added nbd_receive_co_negotiate)? That's
> only cosmetic, so I can live with it, but it seems like it makes the
> diff slightly larger.
> 

Hmm, maybe, will change it back.

-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH 3/4] nbd: do qemu_coroutine_yield during tls handshake

[Vladimir Sementsov-Ogievskiy]

We always call qio_channel_tls_handshake in nbd from couroutine. Take
benefit of it and just yield instead of creating personal main loop.

Mark and rename the function and it's callers correspondingly and
trace-points too.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/client.c     | 26 +++++++++-----------------
 nbd/common.c     |  6 ++----
 nbd/server.c     | 45 +++++++++++++++++----------------------------
 nbd/trace-events | 15 +++++++--------
 4 files changed, 35 insertions(+), 57 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c
index 2ba2220a4a..e3919be30e 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -578,13 +578,14 @@ static int nbd_request_simple_option(QIOChannel *ioc, int opt, Error **errp)
     return 1;
 }
 
-static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
-                                        QCryptoTLSCreds *tlscreds,
-                                        const char *hostname, Error **errp)
+static QIOChannel *nbd_co_receive_starttls(
+        QIOChannel *ioc, QCryptoTLSCreds *tlscreds, const char *hostname,
+        Error **errp)
 {
     int ret;
     QIOChannelTLS *tioc;
-    struct NBDTLSHandshakeData data = { 0 };
+
+    assert(qemu_in_coroutine());
 
     ret = nbd_request_simple_option(ioc, NBD_OPT_STARTTLS, errp);
     if (ret <= 0) {
@@ -601,23 +602,13 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
         return NULL;
     }
     qio_channel_set_name(QIO_CHANNEL(tioc), "nbd-client-tls");
-    data.loop = g_main_loop_new(g_main_context_default(), FALSE);
     trace_nbd_receive_starttls_tls_handshake();
     qio_channel_tls_handshake(tioc,
                               nbd_tls_handshake,
-                              &data,
+                              qemu_coroutine_self(),
                               NULL,
                               NULL);
-
-    if (!data.complete) {
-        g_main_loop_run(data.loop);
-    }
-    g_main_loop_unref(data.loop);
-    if (data.error) {
-        error_propagate(errp, data.error);
-        object_unref(OBJECT(tioc));
-        return NULL;
-    }
+    qemu_coroutine_yield();
 
     return QIO_CHANNEL(tioc);
 }
@@ -922,7 +913,8 @@ static int coroutine_fn nbd_co_start_negotiate(
         }
         if (tlscreds) {
             if (fixedNewStyle) {
-                *outioc = nbd_receive_starttls(ioc, tlscreds, hostname, errp);
+                *outioc = nbd_co_receive_starttls(ioc, tlscreds, hostname,
+                                                  errp);
                 if (!*outioc) {
                     return -EINVAL;
                 }
diff --git a/nbd/common.c b/nbd/common.c
index cc8b278e54..8cff29ec33 100644
--- a/nbd/common.c
+++ b/nbd/common.c
@@ -50,11 +50,9 @@ int nbd_drop(QIOChannel *ioc, size_t size, Error **errp)
 void nbd_tls_handshake(QIOTask *task,
                        void *opaque)
 {
-    struct NBDTLSHandshakeData *data = opaque;
+    Coroutine *co = opaque;
 
-    qio_task_propagate_error(task, &data->error);
-    data->complete = true;
-    g_main_loop_quit(data->loop);
+    aio_co_wake(co);
 }
 
 
diff --git a/nbd/server.c b/nbd/server.c
index 838c150d8c..4463e8d792 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -668,16 +668,15 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags,
 
 /* Handle NBD_OPT_STARTTLS. Return NULL to drop connection, or else the
  * new channel for all further (now-encrypted) communication. */
-static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
-                                                 Error **errp)
+static QIOChannel coroutine_fn *nbd_co_negotiate_handle_starttls(
+        NBDClient *client, Error **errp)
 {
     QIOChannel *ioc;
     QIOChannelTLS *tioc;
-    struct NBDTLSHandshakeData data = { 0 };
 
     assert(client->opt == NBD_OPT_STARTTLS);
 
-    trace_nbd_negotiate_handle_starttls();
+    trace_nbd_co_negotiate_handle_starttls();
     ioc = client->ioc;
 
     if (nbd_negotiate_send_rep(client, NBD_REP_ACK, errp) < 0) {
@@ -693,23 +692,13 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
     }
 
     qio_channel_set_name(QIO_CHANNEL(tioc), "nbd-server-tls");
-    trace_nbd_negotiate_handle_starttls_handshake();
-    data.loop = g_main_loop_new(g_main_context_default(), FALSE);
+    trace_nbd_co_negotiate_handle_starttls_handshake();
     qio_channel_tls_handshake(tioc,
                               nbd_tls_handshake,
-                              &data,
+                              qemu_coroutine_self(),
                               NULL,
                               NULL);
-
-    if (!data.complete) {
-        g_main_loop_run(data.loop);
-    }
-    g_main_loop_unref(data.loop);
-    if (data.error) {
-        object_unref(OBJECT(tioc));
-        error_propagate(errp, data.error);
-        return NULL;
-    }
+    qemu_coroutine_yield();
 
     return QIO_CHANNEL(tioc);
 }
@@ -1023,8 +1012,8 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
  * 1       if client sent NBD_OPT_ABORT, i.e. on valid disconnect,
  *         errp is not set
  */
-static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
-                                 Error **errp)
+static int coroutine_fn nbd_co_negotiate_options(
+        NBDClient *client, uint16_t myflags, Error **errp)
 {
     uint32_t flags;
     bool fixedNewstyle = false;
@@ -1048,7 +1037,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
     if (nbd_read32(client->ioc, &flags, "flags", errp) < 0) {
         return -EIO;
     }
-    trace_nbd_negotiate_options_flags(flags);
+    trace_nbd_co_negotiate_options_flags(flags);
     if (flags & NBD_FLAG_C_FIXED_NEWSTYLE) {
         fixedNewstyle = true;
         flags &= ~NBD_FLAG_C_FIXED_NEWSTYLE;
@@ -1070,7 +1059,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
         if (nbd_read64(client->ioc, &magic, "opts magic", errp) < 0) {
             return -EINVAL;
         }
-        trace_nbd_negotiate_options_check_magic(magic);
+        trace_nbd_co_negotiate_options_check_magic(magic);
         if (magic != NBD_OPTS_MAGIC) {
             error_setg(errp, "Bad magic received");
             return -EINVAL;
@@ -1093,7 +1082,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
             return -EINVAL;
         }
 
-        trace_nbd_negotiate_options_check_option(option,
+        trace_nbd_co_negotiate_options_check_option(option,
                                                  nbd_opt_lookup(option));
         if (client->tlscreds &&
             client->ioc == (QIOChannel *)client->sioc) {
@@ -1109,7 +1098,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
                      * can't start a TLS negotiation correctly */
                     return nbd_reject_length(client, true, errp);
                 }
-                tioc = nbd_negotiate_handle_starttls(client, errp);
+                tioc = nbd_co_negotiate_handle_starttls(client, errp);
                 if (!tioc) {
                     return -EIO;
                 }
@@ -1241,7 +1230,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
  * 1       if client sent NBD_OPT_ABORT, i.e. on valid disconnect,
  *         errp is not set
  */
-static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
+static coroutine_fn int nbd_co_negotiate(NBDClient *client, Error **errp)
 {
     char buf[NBD_OLDSTYLE_NEGOTIATE_SIZE] = "";
     int ret;
@@ -1265,7 +1254,7 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
 
     qio_channel_set_blocking(client->ioc, false, NULL);
 
-    trace_nbd_negotiate_begin();
+    trace_nbd_co_negotiate_begin();
     memcpy(buf, "NBDMAGIC", 8);
 
     stq_be_p(buf + 8, NBD_OPTS_MAGIC);
@@ -1275,7 +1264,7 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
         error_prepend(errp, "write failed: ");
         return -EINVAL;
     }
-    ret = nbd_negotiate_options(client, myflags, errp);
+    ret = nbd_co_negotiate_options(client, myflags, errp);
     if (ret != 0) {
         if (ret < 0) {
             error_prepend(errp, "option negotiation failed: ");
@@ -1284,7 +1273,7 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
     }
 
     assert(!client->optlen);
-    trace_nbd_negotiate_success();
+    trace_nbd_co_negotiate_success();
 
     return 0;
 }
@@ -2408,7 +2397,7 @@ static coroutine_fn void nbd_co_client_start(void *opaque)
 
     qemu_co_mutex_init(&client->send_lock);
 
-    if (nbd_negotiate(client, &local_err)) {
+    if (nbd_co_negotiate(client, &local_err)) {
         if (local_err) {
             error_report_err(local_err);
         }
diff --git a/nbd/trace-events b/nbd/trace-events
index 7f10ebd4e0..0316ba0c71 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -46,19 +46,18 @@ nbd_negotiate_send_info(int info, const char *name, uint32_t length) "Sending NB
 nbd_negotiate_handle_info_requests(int requests) "Client requested %d items of info"
 nbd_negotiate_handle_info_request(int request, const char *name) "Client requested info %d (%s)"
 nbd_negotiate_handle_info_block_size(uint32_t minimum, uint32_t preferred, uint32_t maximum) "advertising minimum 0x%" PRIx32 ", preferred 0x%" PRIx32 ", maximum 0x%" PRIx32
-nbd_negotiate_handle_starttls(void) "Setting up TLS"
-nbd_negotiate_handle_starttls_handshake(void) "Starting TLS handshake"
+nbd_co_negotiate_handle_starttls(void) "Setting up TLS"
+nbd_co_negotiate_handle_starttls_handshake(void) "Starting TLS handshake"
 nbd_negotiate_meta_context(const char *optname, const char *export, uint32_t queries) "Client requested %s for export %s, with %" PRIu32 " queries"
 nbd_negotiate_meta_query_skip(const char *reason) "Skipping meta query: %s"
 nbd_negotiate_meta_query_parse(const char *query) "Parsed meta query '%s'"
 nbd_negotiate_meta_query_reply(const char *context, uint32_t id) "Replying with meta context '%s' id %" PRIu32
-nbd_negotiate_options_flags(uint32_t flags) "Received client flags 0x%" PRIx32
-nbd_negotiate_options_check_magic(uint64_t magic) "Checking opts magic 0x%" PRIx64
-nbd_negotiate_options_check_option(uint32_t option, const char *name) "Checking option %" PRIu32 " (%s)"
-nbd_negotiate_begin(void) "Beginning negotiation"
-nbd_negotiate_old_style(uint64_t size, unsigned flags) "advertising size %" PRIu64 " and flags 0x%x"
+nbd_co_negotiate_options_flags(uint32_t flags) "Received client flags 0x%" PRIx32
+nbd_co_negotiate_options_check_magic(uint64_t magic) "Checking opts magic 0x%" PRIx64
+nbd_co_negotiate_options_check_option(uint32_t option, const char *name) "Checking option %" PRIu32 " (%s)"
+nbd_co_negotiate_begin(void) "Beginning negotiation"
 nbd_negotiate_new_style_size_flags(uint64_t size, unsigned flags) "advertising size %" PRIu64 " and flags 0x%x"
-nbd_negotiate_success(void) "Negotiation succeeded"
+nbd_co_negotiate_success(void) "Negotiation succeeded"
 nbd_receive_request(uint32_t magic, uint16_t flags, uint16_t type, uint64_t from, uint32_t len) "Got request: { magic = 0x%" PRIx32 ", .flags = 0x%" PRIx16 ", .type = 0x%" PRIx16 ", from = %" PRIu64 ", len = %" PRIu32 " }"
 nbd_blk_aio_attached(const char *name, void *ctx) "Export %s: Attaching clients to AIO context %p\n"
 nbd_blk_aio_detach(const char *name, void *ctx) "Export %s: Detaching clients from AIO context %p\n"
-- 
2.18.0

Re: [Qemu-devel] [PATCH 3/4] nbd: do qemu_coroutine_yield during tls handshake

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 4023 bytes --]

On 2/11/19 6:56 AM, Vladimir Sementsov-Ogievskiy wrote:
> We always call qio_channel_tls_handshake in nbd from couroutine. Take
> benefit of it and just yield instead of creating personal main loop.
> 
> Mark and rename the function and it's callers correspondingly and
> trace-points too.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/client.c     | 26 +++++++++-----------------
>  nbd/common.c     |  6 ++----
>  nbd/server.c     | 45 +++++++++++++++++----------------------------
>  nbd/trace-events | 15 +++++++--------
>  4 files changed, 35 insertions(+), 57 deletions(-)
> 
> diff --git a/nbd/client.c b/nbd/client.c
> index 2ba2220a4a..e3919be30e 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -578,13 +578,14 @@ static int nbd_request_simple_option(QIOChannel *ioc, int opt, Error **errp)
>      return 1;
>  }
>  
> -static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
> -                                        QCryptoTLSCreds *tlscreds,
> -                                        const char *hostname, Error **errp)
> +static QIOChannel *nbd_co_receive_starttls(

Missing coroutine_fn ?

> +        QIOChannel *ioc, QCryptoTLSCreds *tlscreds, const char *hostname,
> +        Error **errp)
>  {
>      int ret;
>      QIOChannelTLS *tioc;
> -    struct NBDTLSHandshakeData data = { 0 };
> +
> +    assert(qemu_in_coroutine());

Again, I'm not sure these assertions add much.

>  
>      ret = nbd_request_simple_option(ioc, NBD_OPT_STARTTLS, errp);

Should we also be marking these helper functions as coroutine_fn by the
end of the series, once all callers are marked that way?

>      if (ret <= 0) {
> @@ -601,23 +602,13 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
>          return NULL;
>      }
>      qio_channel_set_name(QIO_CHANNEL(tioc), "nbd-client-tls");
> -    data.loop = g_main_loop_new(g_main_context_default(), FALSE);
>      trace_nbd_receive_starttls_tls_handshake();
>      qio_channel_tls_handshake(tioc,
>                                nbd_tls_handshake,
> -                              &data,
> +                              qemu_coroutine_self(),
>                                NULL,
>                                NULL);
> -
> -    if (!data.complete) {
> -        g_main_loop_run(data.loop);
> -    }
> -    g_main_loop_unref(data.loop);
> -    if (data.error) {
> -        error_propagate(errp, data.error);
> -        object_unref(OBJECT(tioc));
> -        return NULL;
> -    }
> +    qemu_coroutine_yield();

Nice.

> +++ b/nbd/server.c
> @@ -668,16 +668,15 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags,
>  
>  /* Handle NBD_OPT_STARTTLS. Return NULL to drop connection, or else the
>   * new channel for all further (now-encrypted) communication. */
> -static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
> -                                                 Error **errp)
> +static QIOChannel coroutine_fn *nbd_co_negotiate_handle_starttls(

Awkward split of the return type; the coroutine_fn should instead be
placed after the *, as in:

block/mirror.c:static MirrorOp *coroutine_fn active_write_prepare(...

> +        NBDClient *client, Error **errp)
>  {
>      QIOChannel *ioc;
>      QIOChannelTLS *tioc;
> -    struct NBDTLSHandshakeData data = { 0 };

All uses of this type have been deleted; you should also remove it from
nbd-internal.h.


> @@ -1093,7 +1082,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
>              return -EINVAL;
>          }
>  
> -        trace_nbd_negotiate_options_check_option(option,
> +        trace_nbd_co_negotiate_options_check_option(option,
>                                                   nbd_opt_lookup(option));

Indentation looks off.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 3/4] nbd: do qemu_coroutine_yield during tls handshake

[Vladimir Sementsov-Ogievskiy]

12.02.2019 0:55, Eric Blake wrote:
> On 2/11/19 6:56 AM, Vladimir Sementsov-Ogievskiy wrote:
>> We always call qio_channel_tls_handshake in nbd from couroutine. Take
>> benefit of it and just yield instead of creating personal main loop.
>>
>> Mark and rename the function and it's callers correspondingly and
>> trace-points too.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   nbd/client.c     | 26 +++++++++-----------------
>>   nbd/common.c     |  6 ++----
>>   nbd/server.c     | 45 +++++++++++++++++----------------------------
>>   nbd/trace-events | 15 +++++++--------
>>   4 files changed, 35 insertions(+), 57 deletions(-)
>>
>> diff --git a/nbd/client.c b/nbd/client.c
>> index 2ba2220a4a..e3919be30e 100644
>> --- a/nbd/client.c
>> +++ b/nbd/client.c
>> @@ -578,13 +578,14 @@ static int nbd_request_simple_option(QIOChannel *ioc, int opt, Error **errp)
>>       return 1;
>>   }
>>   
>> -static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
>> -                                        QCryptoTLSCreds *tlscreds,
>> -                                        const char *hostname, Error **errp)
>> +static QIOChannel *nbd_co_receive_starttls(
> 
> Missing coroutine_fn ?

hmm, yes.

> 
>> +        QIOChannel *ioc, QCryptoTLSCreds *tlscreds, const char *hostname,
>> +        Error **errp)
>>   {
>>       int ret;
>>       QIOChannelTLS *tioc;
>> -    struct NBDTLSHandshakeData data = { 0 };
>> +
>> +    assert(qemu_in_coroutine());
> 
> Again, I'm not sure these assertions add much.
> 
>>   
>>       ret = nbd_request_simple_option(ioc, NBD_OPT_STARTTLS, errp);
> 
> Should we also be marking these helper functions as coroutine_fn by the
> end of the series, once all callers are marked that way?

I think, not. It still may be called from non-coroutine context.

> 
>>       if (ret <= 0) {
>> @@ -601,23 +602,13 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
>>           return NULL;
>>       }
>>       qio_channel_set_name(QIO_CHANNEL(tioc), "nbd-client-tls");
>> -    data.loop = g_main_loop_new(g_main_context_default(), FALSE);
>>       trace_nbd_receive_starttls_tls_handshake();
>>       qio_channel_tls_handshake(tioc,
>>                                 nbd_tls_handshake,
>> -                              &data,
>> +                              qemu_coroutine_self(),
>>                                 NULL,
>>                                 NULL);
>> -
>> -    if (!data.complete) {
>> -        g_main_loop_run(data.loop);
>> -    }
>> -    g_main_loop_unref(data.loop);
>> -    if (data.error) {
>> -        error_propagate(errp, data.error);
>> -        object_unref(OBJECT(tioc));
>> -        return NULL;
>> -    }
>> +    qemu_coroutine_yield();
> 
> Nice.
> 
>> +++ b/nbd/server.c
>> @@ -668,16 +668,15 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags,
>>   
>>   /* Handle NBD_OPT_STARTTLS. Return NULL to drop connection, or else the
>>    * new channel for all further (now-encrypted) communication. */
>> -static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
>> -                                                 Error **errp)
>> +static QIOChannel coroutine_fn *nbd_co_negotiate_handle_starttls(
> 
> Awkward split of the return type; the coroutine_fn should instead be
> placed after the *, as in:

Oops, agree.

> 
> block/mirror.c:static MirrorOp *coroutine_fn active_write_prepare(...
> 
>> +        NBDClient *client, Error **errp)
>>   {
>>       QIOChannel *ioc;
>>       QIOChannelTLS *tioc;
>> -    struct NBDTLSHandshakeData data = { 0 };
> 
> All uses of this type have been deleted; you should also remove it from
> nbd-internal.h.

OK

> 
> 
>> @@ -1093,7 +1082,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
>>               return -EINVAL;
>>           }
>>   
>> -        trace_nbd_negotiate_options_check_option(option,
>> +        trace_nbd_co_negotiate_options_check_option(option,
>>                                                    nbd_opt_lookup(option));
> 
> Indentation looks off.
> 


-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH 4/4] block/nbd-client: use non-blocking io channel for nbd negotiation

[Vladimir Sementsov-Ogievskiy]

Now negotiation is done in coroutine, so to take benefit of it let's
use non-blocking model.

Note that QIOChannel handle synchronous io calls correctly anyway, so
it's not a problem to send final NBD_CMD_DISC to non-blocking channel
but using sync qio interface, even not in coroutine.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/nbd-client.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index f0ad54ce21..79bc6b7e29 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -1029,7 +1029,7 @@ static int nbd_client_connect(BlockDriverState *bs,
 
     /* NBD handshake */
     logout("session init %s\n", export);
-    qio_channel_set_blocking(QIO_CHANNEL(sioc), true, NULL);
+    qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
 
     client->info.request_sizes = true;
     client->info.structured_reply = true;
@@ -1072,9 +1072,6 @@ static int nbd_client_connect(BlockDriverState *bs,
         object_ref(OBJECT(client->ioc));
     }
 
-    /* Now that we're connected, set the socket to be non-blocking and
-     * kick the reply mechanism.  */
-    qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
     client->connection_co = qemu_coroutine_create(nbd_connection_entry, client);
     nbd_client_attach_aio_context(bs, bdrv_get_aio_context(bs));
 
@@ -1083,9 +1080,8 @@ static int nbd_client_connect(BlockDriverState *bs,
 
  fail:
     /*
-     * We have connected, but must fail for other reasons. The
-     * connection is still blocking; send NBD_CMD_DISC as a courtesy
-     * to the server.
+     * We have connected, but must fail for other reasons.
+     * Send NBD_CMD_DISC as a courtesy to the server.
      */
     {
         NBDRequest request = { .type = NBD_CMD_DISC };
-- 
2.18.0

Re: [Qemu-devel] [PATCH 4/4] block/nbd-client: use non-blocking io channel for nbd negotiation

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1969 bytes --]

On 2/11/19 6:56 AM, Vladimir Sementsov-Ogievskiy wrote:
> Now negotiation is done in coroutine, so to take benefit of it let's
> use non-blocking model.
> 
> Note that QIOChannel handle synchronous io calls correctly anyway, so

s/handle/handles/

> it's not a problem to send final NBD_CMD_DISC to non-blocking channel
> but using sync qio interface, even not in coroutine.

s/not in/when not in a/

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/nbd-client.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)

This fixes qemu as NBD client for use in block devices, but what about
qemu as NBD client in qemu-nbd?  Does that need any updates?  Then
again, your observation about QIO doing the right thing for both
blocking (qemu-nbd) and non-blocking (block layer) channels seems to
cover that.

> @@ -1072,9 +1072,6 @@ static int nbd_client_connect(BlockDriverState *bs,
>          object_ref(OBJECT(client->ioc));
>      }
>  
> -    /* Now that we're connected, set the socket to be non-blocking and
> -     * kick the reply mechanism.  */
> -    qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
>      client->connection_co = qemu_coroutine_create(nbd_connection_entry, client);
>      nbd_client_attach_aio_context(bs, bdrv_get_aio_context(bs));
>  
> @@ -1083,9 +1080,8 @@ static int nbd_client_connect(BlockDriverState *bs,
>  
>   fail:
>      /*
> -     * We have connected, but must fail for other reasons. The
> -     * connection is still blocking; send NBD_CMD_DISC as a courtesy
> -     * to the server.
> +     * We have connected, but must fail for other reasons.
> +     * Send NBD_CMD_DISC as a courtesy to the server.
>       */
>      {
>          NBDRequest request = { .type = NBD_CMD_DISC };
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 4/4] block/nbd-client: use non-blocking io channel for nbd negotiation

[Vladimir Sementsov-Ogievskiy]

12.02.2019 1:02, Eric Blake wrote:
> On 2/11/19 6:56 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Now negotiation is done in coroutine, so to take benefit of it let's
>> use non-blocking model.
>>
>> Note that QIOChannel handle synchronous io calls correctly anyway, so
> 
> s/handle/handles/
> 
>> it's not a problem to send final NBD_CMD_DISC to non-blocking channel
>> but using sync qio interface, even not in coroutine.
> 
> s/not in/when not in a/
> 
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/nbd-client.c | 10 +++-------
>>   1 file changed, 3 insertions(+), 7 deletions(-)
> 
> This fixes qemu as NBD client for use in block devices, but what about
> qemu as NBD client in qemu-nbd?  Does that need any updates?  Then
> again, your observation about QIO doing the right thing for both
> blocking (qemu-nbd) and non-blocking (block layer) channels seems to
> cover that.

In qemu-nbd client works in a separate thread, so, I think, we don't need
nonblocking: we have a thread anyway.

On the other hand, is my code in nbd_reaceive_common safe, keeping in mind
that we call it from separate thread?

Are qio_channel_attach_aio_context, qemu_aio_coroutine_enter, AIO_WAIT_WHILE
thread-safe?

> 
>> @@ -1072,9 +1072,6 @@ static int nbd_client_connect(BlockDriverState *bs,
>>           object_ref(OBJECT(client->ioc));
>>       }
>>   
>> -    /* Now that we're connected, set the socket to be non-blocking and
>> -     * kick the reply mechanism.  */
>> -    qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
>>       client->connection_co = qemu_coroutine_create(nbd_connection_entry, client);
>>       nbd_client_attach_aio_context(bs, bdrv_get_aio_context(bs));
>>   
>> @@ -1083,9 +1080,8 @@ static int nbd_client_connect(BlockDriverState *bs,
>>   
>>    fail:
>>       /*
>> -     * We have connected, but must fail for other reasons. The
>> -     * connection is still blocking; send NBD_CMD_DISC as a courtesy
>> -     * to the server.
>> +     * We have connected, but must fail for other reasons.
>> +     * Send NBD_CMD_DISC as a courtesy to the server.
>>        */
>>       {
>>           NBDRequest request = { .type = NBD_CMD_DISC };
>>
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 4/4] block/nbd-client: use non-blocking io channel for nbd negotiation

[Vladimir Sementsov-Ogievskiy]

19.02.2019 16:18, Vladimir Sementsov-Ogievskiy wrote:
> 12.02.2019 1:02, Eric Blake wrote:
>> On 2/11/19 6:56 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Now negotiation is done in coroutine, so to take benefit of it let's
>>> use non-blocking model.
>>>
>>> Note that QIOChannel handle synchronous io calls correctly anyway, so
>>
>> s/handle/handles/
>>
>>> it's not a problem to send final NBD_CMD_DISC to non-blocking channel
>>> but using sync qio interface, even not in coroutine.
>>
>> s/not in/when not in a/
>>
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block/nbd-client.c | 10 +++-------
>>>   1 file changed, 3 insertions(+), 7 deletions(-)
>>
>> This fixes qemu as NBD client for use in block devices, but what about
>> qemu as NBD client in qemu-nbd?  Does that need any updates?  Then
>> again, your observation about QIO doing the right thing for both
>> blocking (qemu-nbd) and non-blocking (block layer) channels seems to
>> cover that.
> 
> In qemu-nbd client works in a separate thread, so, I think, we don't need
> nonblocking: we have a thread anyway.
> 
> On the other hand, is my code in nbd_receive_common safe, keeping in mind
> that we call it from separate thread?
> 
> Are qio_channel_attach_aio_context, qemu_aio_coroutine_enter, AIO_WAIT_WHILE
> thread-safe?

Could somebody shed a bit of light here, please?

> 
>>
>>> @@ -1072,9 +1072,6 @@ static int nbd_client_connect(BlockDriverState *bs,
>>>           object_ref(OBJECT(client->ioc));
>>>       }
>>> -    /* Now that we're connected, set the socket to be non-blocking and
>>> -     * kick the reply mechanism.  */
>>> -    qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
>>>       client->connection_co = qemu_coroutine_create(nbd_connection_entry, client);
>>>       nbd_client_attach_aio_context(bs, bdrv_get_aio_context(bs));
>>> @@ -1083,9 +1080,8 @@ static int nbd_client_connect(BlockDriverState *bs,
>>>    fail:
>>>       /*
>>> -     * We have connected, but must fail for other reasons. The
>>> -     * connection is still blocking; send NBD_CMD_DISC as a courtesy
>>> -     * to the server.
>>> +     * We have connected, but must fail for other reasons.
>>> +     * Send NBD_CMD_DISC as a courtesy to the server.
>>>        */
>>>       {
>>>           NBDRequest request = { .type = NBD_CMD_DISC };
>>>
>>
> 
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 0/4] nbd: non-blocking negotiation

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1116 bytes --]

On 2/11/19 6:55 AM, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> Here is a try of moving to non-blocking negotiation in nbd
> code, proposed by Deniel in
> https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg03817.html
> 
> in thread "[PATCH v4 00/10] NBD reconnect"
> https://lists.gnu.org/archive/html/qemu-devel/2018-07/msg05973.html
> 
> Vladimir Sementsov-Ogievskiy (4):
>   io/channel: add qio_channel_get_attached_aio_context()
>   nbd/client: do negotiation in coroutine
>   nbd: do qemu_coroutine_yield during tls handshake
>   block/nbd-client: use non-blocking io channel for nbd negotiation

Will there be a v2 of this series?  Soft freeze for 4.0 is coming up
very rapidly, so I'm trying to gauge if this is still 4.0 material (and
I should hold off my NBD queue for a chance to include it) or if it is
more likely to slip into 4.1 (and I should post my NBD pull request
sooner to avoid burdening Peter with a last-minute rush next week).

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 0/4] nbd: non-blocking negotiation

[Vladimir Sementsov-Ogievskiy]

06.03.2019 19:11, Eric Blake wrote:
> On 2/11/19 6:55 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> Here is a try of moving to non-blocking negotiation in nbd
>> code, proposed by Deniel in
>> https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg03817.html
>>
>> in thread "[PATCH v4 00/10] NBD reconnect"
>> https://lists.gnu.org/archive/html/qemu-devel/2018-07/msg05973.html
>>
>> Vladimir Sementsov-Ogievskiy (4):
>>    io/channel: add qio_channel_get_attached_aio_context()
>>    nbd/client: do negotiation in coroutine
>>    nbd: do qemu_coroutine_yield during tls handshake
>>    block/nbd-client: use non-blocking io channel for nbd negotiation
> 
> Will there be a v2 of this series?  Soft freeze for 4.0 is coming up
> very rapidly, so I'm trying to gauge if this is still 4.0 material (and
> I should hold off my NBD queue for a chance to include it) or if it is
> more likely to slip into 4.1 (and I should post my NBD pull request
> sooner to avoid burdening Peter with a last-minute rush next week).
> 

I decided to go another way as I really unsure about using all this staff
in a context of the thread in nbd server. And it'll be 1-2 patches.
Possibly, they'll be ready on March 9.

On the other hand, the most interest part is nbd-reconnect series and it
doesn't seem feasible to finish it for 4.0. (it's possible that March 9
would be the only my working day from 7-11)

So, I think, let's move this all to 4.1.


-- 
Best regards,
Vladimir