[Qemu-devel] [PATCH 0/2] nbd: enable keepalive

[Qemu-devel] [PATCH 0/2] nbd: enable keepalive

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here is a suggestion to enable keepalive option to track server availablity.

Vladimir Sementsov-Ogievskiy (2):
  io/channel: add qio_channel_set_keepalive
  nbd-client: enable TCP keepalive

 include/io/channel.h | 13 +++++++++++++
 block/nbd-client.c   |  1 +
 io/channel-socket.c  | 19 +++++++++++++++++++
 io/channel.c         | 14 ++++++++++++++
 4 files changed, 47 insertions(+)

-- 
2.18.0

[Qemu-devel] [PATCH 1/2] io/channel: add qio_channel_set_keepalive

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/io/channel.h | 13 +++++++++++++
 io/channel-socket.c  | 19 +++++++++++++++++++
 io/channel.c         | 14 ++++++++++++++
 3 files changed, 46 insertions(+)

diff --git a/include/io/channel.h b/include/io/channel.h
index 59460cb1ec..34d871a414 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -124,6 +124,9 @@ struct QIOChannelClass {
     int (*io_set_blocking)(QIOChannel *ioc,
                            bool enabled,
                            Error **errp);
+    int (*io_set_keepalive)(QIOChannel *ioc,
+                            bool enabled,
+                            Error **errp);
 
     /* Optional callbacks */
     int (*io_shutdown)(QIOChannel *ioc,
@@ -490,6 +493,16 @@ int qio_channel_set_blocking(QIOChannel *ioc,
                              bool enabled,
                              Error **errp);
 
+/*
+ * qio_channel_set_keepalive:
+ * @ioc: the channel object
+ * @enabled: the keepalive flag state
+ * @errp: pointer to a NULL-initialized error object
+ */
+int qio_channel_set_keepalive(QIOChannel *ioc,
+                              bool enabled,
+                              Error **errp);
+
 /**
  * qio_channel_close:
  * @ioc: the channel object
diff --git a/io/channel-socket.c b/io/channel-socket.c
index bc5f80e780..5c1ea08660 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -656,6 +656,24 @@ qio_channel_socket_set_blocking(QIOChannel *ioc,
 }
 
 
+static int
+qio_channel_socket_set_keepalive(QIOChannel *ioc,
+                                 bool enabled,
+                                 Error **errp)
+{
+    QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc);
+    int val = enabled;
+    int ret = qemu_setsockopt(sioc->fd, SOL_SOCKET, SO_KEEPALIVE,
+                              &val, sizeof(val));
+
+    if (ret < 0) {
+        error_setg_errno(errp, errno, "Unable to set KEEPALIVE");
+    }
+
+    return ret;
+}
+
+
 static void
 qio_channel_socket_set_delay(QIOChannel *ioc,
                              bool enabled)
@@ -762,6 +780,7 @@ static void qio_channel_socket_class_init(ObjectClass *klass,
     ioc_klass->io_writev = qio_channel_socket_writev;
     ioc_klass->io_readv = qio_channel_socket_readv;
     ioc_klass->io_set_blocking = qio_channel_socket_set_blocking;
+    ioc_klass->io_set_keepalive = qio_channel_socket_set_keepalive;
     ioc_klass->io_close = qio_channel_socket_close;
     ioc_klass->io_shutdown = qio_channel_socket_shutdown;
     ioc_klass->io_set_cork = qio_channel_socket_set_cork;
diff --git a/io/channel.c b/io/channel.c
index 2a26c2a2c0..0f0b2b7b65 100644
--- a/io/channel.c
+++ b/io/channel.c
@@ -265,6 +265,20 @@ int qio_channel_set_blocking(QIOChannel *ioc,
     return klass->io_set_blocking(ioc, enabled, errp);
 }
 
+int qio_channel_set_keepalive(QIOChannel *ioc,
+                              bool enabled,
+                              Error **errp)
+{
+    QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc);
+
+    if (!klass->io_set_keepalive) {
+        error_setg(errp, "KEEPALIVE is not supported by IO channel");
+        return -ENOTSUP;
+    }
+
+    return klass->io_set_keepalive(ioc, enabled, errp);
+}
+
 
 int qio_channel_close(QIOChannel *ioc,
                       Error **errp)
-- 
2.18.0

Re: [Qemu-devel] [PATCH 1/2] io/channel: add qio_channel_set_keepalive

[Eric Blake]

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

On 6/5/19 5:09 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/io/channel.h | 13 +++++++++++++
>  io/channel-socket.c  | 19 +++++++++++++++++++
>  io/channel.c         | 14 ++++++++++++++
>  3 files changed, 46 insertions(+)

Dan, if you'd like, I can take this through my NBD tree.

Reviewed-by: Eric Blake <eblake@redhat.com

-- 
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/2] io/channel: add qio_channel_set_keepalive

[Daniel P. Berrangé]

On Wed, Jun 05, 2019 at 09:38:06AM -0500, Eric Blake wrote:
> On 6/5/19 5:09 AM, Vladimir Sementsov-Ogievskiy wrote:
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >  include/io/channel.h | 13 +++++++++++++
> >  io/channel-socket.c  | 19 +++++++++++++++++++
> >  io/channel.c         | 14 ++++++++++++++
> >  3 files changed, 46 insertions(+)
> 
> Dan, if you'd like, I can take this through my NBD tree.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com

There's a couple of small tweaks needed, but if you apply those then
consider it to have my Acked-by: Daniel P. Berrangé <berrange@redhat.com>
for you to merge directly.

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/2] io/channel: add qio_channel_set_keepalive

[Daniel P. Berrangé]

On Wed, Jun 05, 2019 at 01:09:12PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/io/channel.h | 13 +++++++++++++
>  io/channel-socket.c  | 19 +++++++++++++++++++
>  io/channel.c         | 14 ++++++++++++++
>  3 files changed, 46 insertions(+)
> 
> diff --git a/include/io/channel.h b/include/io/channel.h
> index 59460cb1ec..34d871a414 100644
> --- a/include/io/channel.h
> +++ b/include/io/channel.h
> @@ -124,6 +124,9 @@ struct QIOChannelClass {
>      int (*io_set_blocking)(QIOChannel *ioc,
>                             bool enabled,
>                             Error **errp);
> +    int (*io_set_keepalive)(QIOChannel *ioc,
> +                            bool enabled,
> +                            Error **errp);
>  
>      /* Optional callbacks */
>      int (*io_shutdown)(QIOChannel *ioc,
> @@ -490,6 +493,16 @@ int qio_channel_set_blocking(QIOChannel *ioc,
>                               bool enabled,
>                               Error **errp);
>  
> +/*
> + * qio_channel_set_keepalive:
> + * @ioc: the channel object
> + * @enabled: the keepalive flag state
> + * @errp: pointer to a NULL-initialized error object
> + */

Missing docs for the return value. SHould be

  "Returns 0 on success, -1 on error."

note we do *not* return "-errno" values in QIOChannel APIs

> +int qio_channel_set_keepalive(QIOChannel *ioc,
> +                              bool enabled,
> +                              Error **errp);
> +

> diff --git a/io/channel-socket.c b/io/channel-socket.c
> index bc5f80e780..5c1ea08660 100644
> --- a/io/channel-socket.c
> +++ b/io/channel-socket.c
> @@ -656,6 +656,24 @@ qio_channel_socket_set_blocking(QIOChannel *ioc,
>  }
>  
>  
> +static int
> +qio_channel_socket_set_keepalive(QIOChannel *ioc,
> +                                 bool enabled,
> +                                 Error **errp)
> +{
> +    QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc);
> +    int val = enabled;
> +    int ret = qemu_setsockopt(sioc->fd, SOL_SOCKET, SO_KEEPALIVE,
> +                              &val, sizeof(val));
> +
> +    if (ret < 0) {
> +        error_setg_errno(errp, errno, "Unable to set KEEPALIVE");

Add 'return -1' there to be explicit, avoiding need to read up on
whether qemu_setsockopt returns -1 or -errno.  (It returns -1)

> +    }
> +
> +    return ret;
> +}
> +
> +
>  static void
>  qio_channel_socket_set_delay(QIOChannel *ioc,
>                               bool enabled)
> @@ -762,6 +780,7 @@ static void qio_channel_socket_class_init(ObjectClass *klass,
>      ioc_klass->io_writev = qio_channel_socket_writev;
>      ioc_klass->io_readv = qio_channel_socket_readv;
>      ioc_klass->io_set_blocking = qio_channel_socket_set_blocking;
> +    ioc_klass->io_set_keepalive = qio_channel_socket_set_keepalive;
>      ioc_klass->io_close = qio_channel_socket_close;
>      ioc_klass->io_shutdown = qio_channel_socket_shutdown;
>      ioc_klass->io_set_cork = qio_channel_socket_set_cork;
> diff --git a/io/channel.c b/io/channel.c
> index 2a26c2a2c0..0f0b2b7b65 100644
> --- a/io/channel.c
> +++ b/io/channel.c
> @@ -265,6 +265,20 @@ int qio_channel_set_blocking(QIOChannel *ioc,
>      return klass->io_set_blocking(ioc, enabled, errp);
>  }
>  
> +int qio_channel_set_keepalive(QIOChannel *ioc,
> +                              bool enabled,
> +                              Error **errp)
> +{
> +    QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc);
> +
> +    if (!klass->io_set_keepalive) {
> +        error_setg(errp, "KEEPALIVE is not supported by IO channel");
> +        return -ENOTSUP;

return -1;

> +    }
> +
> +    return klass->io_set_keepalive(ioc, enabled, errp);
> +}
> +
>  
>  int qio_channel_close(QIOChannel *ioc,
>                        Error **errp)

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/2] io/channel: add qio_channel_set_keepalive

[Vladimir Sementsov-Ogievskiy]

05.06.2019 19:02, Daniel P. Berrangé wrote:
> On Wed, Jun 05, 2019 at 01:09:12PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   include/io/channel.h | 13 +++++++++++++
>>   io/channel-socket.c  | 19 +++++++++++++++++++
>>   io/channel.c         | 14 ++++++++++++++
>>   3 files changed, 46 insertions(+)
>>
>> diff --git a/include/io/channel.h b/include/io/channel.h
>> index 59460cb1ec..34d871a414 100644
>> --- a/include/io/channel.h
>> +++ b/include/io/channel.h
>> @@ -124,6 +124,9 @@ struct QIOChannelClass {
>>       int (*io_set_blocking)(QIOChannel *ioc,
>>                              bool enabled,
>>                              Error **errp);
>> +    int (*io_set_keepalive)(QIOChannel *ioc,
>> +                            bool enabled,
>> +                            Error **errp);
>>   
>>       /* Optional callbacks */
>>       int (*io_shutdown)(QIOChannel *ioc,
>> @@ -490,6 +493,16 @@ int qio_channel_set_blocking(QIOChannel *ioc,
>>                                bool enabled,
>>                                Error **errp);
>>   
>> +/*
>> + * qio_channel_set_keepalive:
>> + * @ioc: the channel object
>> + * @enabled: the keepalive flag state
>> + * @errp: pointer to a NULL-initialized error object
>> + */
> 
> Missing docs for the return value. SHould be
> 
>    "Returns 0 on success, -1 on error."
> 
> note we do *not* return "-errno" values in QIOChannel APIs
> 
>> +int qio_channel_set_keepalive(QIOChannel *ioc,
>> +                              bool enabled,
>> +                              Error **errp);
>> +
> 
>> diff --git a/io/channel-socket.c b/io/channel-socket.c
>> index bc5f80e780..5c1ea08660 100644
>> --- a/io/channel-socket.c
>> +++ b/io/channel-socket.c
>> @@ -656,6 +656,24 @@ qio_channel_socket_set_blocking(QIOChannel *ioc,
>>   }
>>   
>>   
>> +static int
>> +qio_channel_socket_set_keepalive(QIOChannel *ioc,
>> +                                 bool enabled,
>> +                                 Error **errp)
>> +{
>> +    QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc);
>> +    int val = enabled;
>> +    int ret = qemu_setsockopt(sioc->fd, SOL_SOCKET, SO_KEEPALIVE,
>> +                              &val, sizeof(val));
>> +
>> +    if (ret < 0) {
>> +        error_setg_errno(errp, errno, "Unable to set KEEPALIVE");
> 
> Add 'return -1' there to be explicit, avoiding need to read up on
> whether qemu_setsockopt returns -1 or -errno.  (It returns -1)
> 
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +
>>   static void
>>   qio_channel_socket_set_delay(QIOChannel *ioc,
>>                                bool enabled)
>> @@ -762,6 +780,7 @@ static void qio_channel_socket_class_init(ObjectClass *klass,
>>       ioc_klass->io_writev = qio_channel_socket_writev;
>>       ioc_klass->io_readv = qio_channel_socket_readv;
>>       ioc_klass->io_set_blocking = qio_channel_socket_set_blocking;
>> +    ioc_klass->io_set_keepalive = qio_channel_socket_set_keepalive;
>>       ioc_klass->io_close = qio_channel_socket_close;
>>       ioc_klass->io_shutdown = qio_channel_socket_shutdown;
>>       ioc_klass->io_set_cork = qio_channel_socket_set_cork;
>> diff --git a/io/channel.c b/io/channel.c
>> index 2a26c2a2c0..0f0b2b7b65 100644
>> --- a/io/channel.c
>> +++ b/io/channel.c
>> @@ -265,6 +265,20 @@ int qio_channel_set_blocking(QIOChannel *ioc,
>>       return klass->io_set_blocking(ioc, enabled, errp);
>>   }
>>   
>> +int qio_channel_set_keepalive(QIOChannel *ioc,
>> +                              bool enabled,
>> +                              Error **errp)
>> +{
>> +    QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc);
>> +
>> +    if (!klass->io_set_keepalive) {
>> +        error_setg(errp, "KEEPALIVE is not supported by IO channel");
>> +        return -ENOTSUP;
> 
> return -1;
> 
>> +    }
>> +
>> +    return klass->io_set_keepalive(ioc, enabled, errp);
>> +}
>> +
>>   
>>   int qio_channel_close(QIOChannel *ioc,
>>                         Error **errp)
> 
> Regards,
> Daniel
> 

Thank you, I'll resend

-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH 2/2] nbd-client: enable TCP keepalive

[Vladimir Sementsov-Ogievskiy]

Enable keepalive option to track server availablity.

Requested-by: Denis V. Lunev <den@openvz.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/nbd-client.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 790ecc1ee1..b57cea8482 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -1137,6 +1137,7 @@ static int nbd_client_connect(BlockDriverState *bs,
 
     /* NBD handshake */
     logout("session init %s\n", export);
+    qio_channel_set_keepalive(QIO_CHANNEL(sioc), true, NULL);
     qio_channel_set_blocking(QIO_CHANNEL(sioc), true, NULL);
 
     client->info.request_sizes = true;
-- 
2.18.0

Re: [Qemu-devel] [PATCH 2/2] nbd-client: enable TCP keepalive

[Eric Blake]

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

On 6/5/19 5:09 AM, Vladimir Sementsov-Ogievskiy wrote:
> Enable keepalive option to track server availablity.

s/availablity/availability/

Do we want this unconditionally, or should it be an option (and hence
exposed over QMP)?

> 
> Requested-by: Denis V. Lunev <den@openvz.org>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/nbd-client.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index 790ecc1ee1..b57cea8482 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -1137,6 +1137,7 @@ static int nbd_client_connect(BlockDriverState *bs,
>  
>      /* NBD handshake */
>      logout("session init %s\n", export);
> +    qio_channel_set_keepalive(QIO_CHANNEL(sioc), true, NULL);
>      qio_channel_set_blocking(QIO_CHANNEL(sioc), true, NULL);
>  
>      client->info.request_sizes = true;
> 

-- 
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/2] nbd-client: enable TCP keepalive

[Denis V. Lunev]

On 6/5/19 5:39 PM, Eric Blake wrote:
> On 6/5/19 5:09 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Enable keepalive option to track server availablity.
> s/availablity/availability/
>
> Do we want this unconditionally, or should it be an option (and hence
> exposed over QMP)?
That is good question, if we would expose it, we should specify
timeout duration as an option. Though IMHO it would be safe
to get this unconditional.


>> Requested-by: Denis V. Lunev <den@openvz.org>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>  block/nbd-client.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/block/nbd-client.c b/block/nbd-client.c
>> index 790ecc1ee1..b57cea8482 100644
>> --- a/block/nbd-client.c
>> +++ b/block/nbd-client.c
>> @@ -1137,6 +1137,7 @@ static int nbd_client_connect(BlockDriverState *bs,
>>  
>>      /* NBD handshake */
>>      logout("session init %s\n", export);
>> +    qio_channel_set_keepalive(QIO_CHANNEL(sioc), true, NULL);
>>      qio_channel_set_blocking(QIO_CHANNEL(sioc), true, NULL);
>>  
>>      client->info.request_sizes = true;
>>

Re: [Qemu-devel] [PATCH 2/2] nbd-client: enable TCP keepalive

[Daniel P. Berrangé]

On Wed, Jun 05, 2019 at 09:39:10AM -0500, Eric Blake wrote:
> On 6/5/19 5:09 AM, Vladimir Sementsov-Ogievskiy wrote:
> > Enable keepalive option to track server availablity.
> 
> s/availablity/availability/
> 
> Do we want this unconditionally, or should it be an option (and hence
> exposed over QMP)?

I guess this is really a question about what our intended connection
reliability policy should be.

By enabling TCP keepalives we are explicitly making the connection
less reliable by forcing it to be terminated when keepalive
threshold triggers, instead of waiting longer for TCP to recover.

The rationale s that once a connection has been in a hung state for
so long that keepalive triggers, its (hopefully) not useful to the
mgmt app to carry on waiting anyway.

If the connection is terminated by keepalive & the mgmt app then
spawns a new client to carry on with the work, what are the risks
involved ? eg Could packets from the stuck, terminated, connection
suddenly arrive later and trigger I/O with outdated data payload ?

I guess this is no different a situation from an app explicitly
killing the QEMU NBD client process instead & spawning a new one.

I'm still feeling a little uneasy about enabling it unconditionally
though, since pretty much everything I know which supports keepalives
has a way to turn them on/off at least, even if you can't tune the
individual timer settings.

> > Requested-by: Denis V. Lunev <den@openvz.org>
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >  block/nbd-client.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/block/nbd-client.c b/block/nbd-client.c
> > index 790ecc1ee1..b57cea8482 100644
> > --- a/block/nbd-client.c
> > +++ b/block/nbd-client.c
> > @@ -1137,6 +1137,7 @@ static int nbd_client_connect(BlockDriverState *bs,
> >  
> >      /* NBD handshake */
> >      logout("session init %s\n", export);
> > +    qio_channel_set_keepalive(QIO_CHANNEL(sioc), true, NULL);
> >      qio_channel_set_blocking(QIO_CHANNEL(sioc), true, NULL);
> >  
> >      client->info.request_sizes = true;
> > 
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
> 




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 2/2] nbd-client: enable TCP keepalive

[Vladimir Sementsov-Ogievskiy]

05.06.2019 19:37, Daniel P. Berrangé wrote:
> On Wed, Jun 05, 2019 at 09:39:10AM -0500, Eric Blake wrote:
>> On 6/5/19 5:09 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Enable keepalive option to track server availablity.
>>
>> s/availablity/availability/
>>
>> Do we want this unconditionally, or should it be an option (and hence
>> exposed over QMP)?
> 
> I guess this is really a question about what our intended connection
> reliability policy should be.
> 
> By enabling TCP keepalives we are explicitly making the connection
> less reliable by forcing it to be terminated when keepalive
> threshold triggers, instead of waiting longer for TCP to recover.
> 
> The rationale s that once a connection has been in a hung state for
> so long that keepalive triggers, its (hopefully) not useful to the
> mgmt app to carry on waiting anyway.
> 
> If the connection is terminated by keepalive & the mgmt app then
> spawns a new client to carry on with the work, what are the risks
> involved ? eg Could packets from the stuck, terminated, connection
> suddenly arrive later and trigger I/O with outdated data payload ?

Hmm, I believe that tcp guarantees isolation between different connections

> 
> I guess this is no different a situation from an app explicitly
> killing the QEMU NBD client process instead & spawning a new one.
> 
> I'm still feeling a little uneasy about enabling it unconditionally
> though, since pretty much everything I know which supports keepalives
> has a way to turn them on/off at least, even if you can't tune the
> individual timer settings.

Hm. So, I can add bool keepalive parameter for nbd format with default to true.
And if needed, it may be later extended to be qapi 'alternate' of bool or struct with
three numeric parameters, corresponding to TCP_KEEPCNT, TCP_KEEPIDLE and TCP_KEEPINTVL .

Opinions?


> 
>>> Requested-by: Denis V. Lunev <den@openvz.org>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block/nbd-client.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/block/nbd-client.c b/block/nbd-client.c
>>> index 790ecc1ee1..b57cea8482 100644
>>> --- a/block/nbd-client.c
>>> +++ b/block/nbd-client.c
>>> @@ -1137,6 +1137,7 @@ static int nbd_client_connect(BlockDriverState *bs,
>>>   
>>>       /* NBD handshake */
>>>       logout("session init %s\n", export);
>>> +    qio_channel_set_keepalive(QIO_CHANNEL(sioc), true, NULL);
>>>       qio_channel_set_blocking(QIO_CHANNEL(sioc), true, NULL);
>>>   
>>>       client->info.request_sizes = true;
>>>
>>
>> -- 
>> Eric Blake, Principal Software Engineer
>> Red Hat, Inc.           +1-919-301-3226
>> Virtualization:  qemu.org | libvirt.org
>>
> 
> 
> 
> 
> Regards,
> Daniel
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 2/2] nbd-client: enable TCP keepalive

[Eric Blake]

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

On 6/5/19 12:05 PM, Vladimir Sementsov-Ogievskiy wrote:

>> By enabling TCP keepalives we are explicitly making the connection
>> less reliable by forcing it to be terminated when keepalive
>> threshold triggers, instead of waiting longer for TCP to recover.
>>
>> The rationale s that once a connection has been in a hung state for
>> so long that keepalive triggers, its (hopefully) not useful to the
>> mgmt app to carry on waiting anyway.
>>
>> If the connection is terminated by keepalive & the mgmt app then
>> spawns a new client to carry on with the work, what are the risks
>> involved ? eg Could packets from the stuck, terminated, connection
>> suddenly arrive later and trigger I/O with outdated data payload ?
> 
> Hmm, I believe that tcp guarantees isolation between different connections
> 
>>
>> I guess this is no different a situation from an app explicitly
>> killing the QEMU NBD client process instead & spawning a new one.
>>
>> I'm still feeling a little uneasy about enabling it unconditionally
>> though, since pretty much everything I know which supports keepalives
>> has a way to turn them on/off at least, even if you can't tune the
>> individual timer settings.
> 
> Hm. So, I can add bool keepalive parameter for nbd format with default to true.
> And if needed, it may be later extended to be qapi 'alternate' of bool or struct with
> three numeric parameters, corresponding to TCP_KEEPCNT, TCP_KEEPIDLE and TCP_KEEPINTVL .
> 
> Opinions?

Adding a bool that could later turn into a qapi 'alternate' for
fine-tuning seems reasonable. Defaulting the bool to true is not
backwards-compatible; better would be defaulting it to false and letting
users opt-in; introspection will also work to let you know whether the
feature is present.

-- 
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/2] nbd-client: enable TCP keepalive

[Vladimir Sementsov-Ogievskiy]

05.06.2019 20:12, Eric Blake wrote:
> On 6/5/19 12:05 PM, Vladimir Sementsov-Ogievskiy wrote:
> 
>>> By enabling TCP keepalives we are explicitly making the connection
>>> less reliable by forcing it to be terminated when keepalive
>>> threshold triggers, instead of waiting longer for TCP to recover.
>>>
>>> The rationale s that once a connection has been in a hung state for
>>> so long that keepalive triggers, its (hopefully) not useful to the
>>> mgmt app to carry on waiting anyway.
>>>
>>> If the connection is terminated by keepalive & the mgmt app then
>>> spawns a new client to carry on with the work, what are the risks
>>> involved ? eg Could packets from the stuck, terminated, connection
>>> suddenly arrive later and trigger I/O with outdated data payload ?
>>
>> Hmm, I believe that tcp guarantees isolation between different connections
>>
>>>
>>> I guess this is no different a situation from an app explicitly
>>> killing the QEMU NBD client process instead & spawning a new one.
>>>
>>> I'm still feeling a little uneasy about enabling it unconditionally
>>> though, since pretty much everything I know which supports keepalives
>>> has a way to turn them on/off at least, even if you can't tune the
>>> individual timer settings.
>>
>> Hm. So, I can add bool keepalive parameter for nbd format with default to true.
>> And if needed, it may be later extended to be qapi 'alternate' of bool or struct with
>> three numeric parameters, corresponding to TCP_KEEPCNT, TCP_KEEPIDLE and TCP_KEEPINTVL .
>>
>> Opinions?
> 
> Adding a bool that could later turn into a qapi 'alternate' for
> fine-tuning seems reasonable. Defaulting the bool to true is not
> backwards-compatible; better would be defaulting it to false and letting
> users opt-in; introspection will also work to let you know whether the
> feature is present.
> 

Ok.

One more thing to discuss then. Should I add keepalive directly to BlockdevOptionsNbd?

Seems more useful to put it into SocketAddress, to be reused by other socket users..
But "SocketAddress" sounds like address, not like address+connection-options. On
the other hand, structure names are not part of API. So, finally, is InetSocketAddress
a good place for such thing?

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 2/2] nbd-client: enable TCP keepalive

[Daniel P. Berrangé]

On Wed, Jun 05, 2019 at 05:28:05PM +0000, Vladimir Sementsov-Ogievskiy wrote:
> 05.06.2019 20:12, Eric Blake wrote:
> > On 6/5/19 12:05 PM, Vladimir Sementsov-Ogievskiy wrote:
> > 
> >>> By enabling TCP keepalives we are explicitly making the connection
> >>> less reliable by forcing it to be terminated when keepalive
> >>> threshold triggers, instead of waiting longer for TCP to recover.
> >>>
> >>> The rationale s that once a connection has been in a hung state for
> >>> so long that keepalive triggers, its (hopefully) not useful to the
> >>> mgmt app to carry on waiting anyway.
> >>>
> >>> If the connection is terminated by keepalive & the mgmt app then
> >>> spawns a new client to carry on with the work, what are the risks
> >>> involved ? eg Could packets from the stuck, terminated, connection
> >>> suddenly arrive later and trigger I/O with outdated data payload ?
> >>
> >> Hmm, I believe that tcp guarantees isolation between different connections
> >>
> >>>
> >>> I guess this is no different a situation from an app explicitly
> >>> killing the QEMU NBD client process instead & spawning a new one.
> >>>
> >>> I'm still feeling a little uneasy about enabling it unconditionally
> >>> though, since pretty much everything I know which supports keepalives
> >>> has a way to turn them on/off at least, even if you can't tune the
> >>> individual timer settings.
> >>
> >> Hm. So, I can add bool keepalive parameter for nbd format with default to true.
> >> And if needed, it may be later extended to be qapi 'alternate' of bool or struct with
> >> three numeric parameters, corresponding to TCP_KEEPCNT, TCP_KEEPIDLE and TCP_KEEPINTVL .
> >>
> >> Opinions?
> > 
> > Adding a bool that could later turn into a qapi 'alternate' for
> > fine-tuning seems reasonable. Defaulting the bool to true is not
> > backwards-compatible; better would be defaulting it to false and letting
> > users opt-in; introspection will also work to let you know whether the
> > feature is present.
> > 
> 
> Ok.
> 
> One more thing to discuss then. Should I add keepalive directly to BlockdevOptionsNbd?
> 
> Seems more useful to put it into SocketAddress, to be reused by other socket users..
> But "SocketAddress" sounds like address, not like address+connection-options. On
> the other hand, structure names are not part of API. So, finally, is InetSocketAddress
> a good place for such thing?

That's an interesting idea. Using InetSocketAddress would mean that we could
get support for this enabled "for free" everywhere in QEMU that uses an
InetSocketAddress as its master config format.

Of course there's plenty of places not using InetSocketAddress that would
still require some glue to wire up the code which converts the custom
format into InetSocketAddress


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 2/2] nbd-client: enable TCP keepalive

[Eric Blake]

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

On 6/5/19 12:36 PM, Daniel P. Berrangé wrote:

>>
>> Ok.
>>
>> One more thing to discuss then. Should I add keepalive directly to BlockdevOptionsNbd?
>>
>> Seems more useful to put it into SocketAddress, to be reused by other socket users..
>> But "SocketAddress" sounds like address, not like address+connection-options. On
>> the other hand, structure names are not part of API. So, finally, is InetSocketAddress
>> a good place for such thing?
> 
> That's an interesting idea. Using InetSocketAddress would mean that we could
> get support for this enabled "for free" everywhere in QEMU that uses an
> InetSocketAddress as its master config format.

I like the idea as well.

> 
> Of course there's plenty of places not using InetSocketAddress that would
> still require some glue to wire up the code which converts the custom
> format into InetSocketAddress

Hmm - how many places are we using InetSocketAddress (which allows an
optional 'to' port value) when we really meant InetSocketAddressBase?
There may be some interesting hierarchy decisions to consider on where
we stick a keepalive option.

This also made me wonder if we should start a deprecation clock to
improve the nbd-server-start command to use SocketAddress instead of
SocketAddressLegacy.  If we revive Max's work on implementing a default
branch for a union discriminator
(https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01682.html),
we could have something like:

{ 'enum': 'NbdSocketAddressHack',
  'data': [ 'legacy', 'inet', 'unix' ] }
{ 'struct': 'NbdServerAddrLegacy',
  'data': { 'addr', 'SocketAddressLegacy' } }
{ 'union': 'NbdServerAddr',
  'base': { 'type': 'NbdSocketAddressHack',
            '*tls-creds': 'str',
            '*tls-authz': 'str' },
  'discriminator': 'type',
  'default-variant': 'legacy',
  'data': { 'legacy': 'NbdServerAddrLegacy',
            'inet', 'InetSocketAddress',
            'unix', 'UnixSocketAddress' } }
{ 'command', 'nbd-server-start', 'data': 'NbdServerAddr' }

which should be backwards compatible with the existing:

{ "execute": "nbd-server-start", "arguments": {
    "tls-authz": "authz0",
    "addr": { "type": "inet", "data": {
      "host": "localhost", "port": "10809" } } } }

by relying on the discriminator's default expansion to:

{ "execute": "nbd-server-start", "arguments": {
    "tls-authz": "authz0",
    "type": "legacy",
    "addr": { "type": "inet", "data": {
      "host": "localhost", "port": "10809" } } } }

but also permit the flatter:

{ "execute": "nbd-server-start", "arguments": {
    "tls-authz": "authz0",
    "type": "inet", "host": "localhost", "port": "10809" } }

and let us start a deprecation clock to get rid of the "legacy" branch
(especially if coupled with Kevin's work on adding introspectable
deprecation annotations in QAPI).

-- 
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 --]

[Qemu-devel] nbd-server-add [was: [PATCH 2/2] nbd-client: enable TCP keepalive]

[Eric Blake]

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

On 6/5/19 2:48 PM, Eric Blake wrote:

> This also made me wonder if we should start a deprecation clock to
> improve the nbd-server-start command to use SocketAddress instead of
> SocketAddressLegacy.  If we revive Max's work on implementing a default
> branch for a union discriminator
> (https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01682.html),
> we could have something like:

Re-reading that thread, I see that Markus was arguing for a slightly
different QAPI syntax than Max's proposal, basically:

> 
> { 'enum': 'NbdSocketAddressHack',
>   'data': [ 'legacy', 'inet', 'unix' ] }
> { 'struct': 'NbdServerAddrLegacy',
>   'data': { 'addr', 'SocketAddressLegacy' } }
> { 'union': 'NbdServerAddr',
>   'base': { 'type': 'NbdSocketAddressHack',
>             '*tls-creds': 'str',
>             '*tls-authz': 'str' },
>   'discriminator': 'type',
>   'default-variant': 'legacy',
>   'data': { 'legacy': 'NbdServerAddrLegacy',
>             'inet', 'InetSocketAddress',
>             'unix', 'UnixSocketAddress' } }
> { 'command', 'nbd-server-start', 'data': 'NbdServerAddr' }

{ 'union': 'NbdServerAddr',
  'base': { '*type': { 'type': 'NbdSocketAddressHack',
                       'default': 'legacy' },
            '*tls-creds', 'str', '*tls-authz', 'str' },
  'discriminator': 'type',
  'data': { 'legacy': 'NbdServerAddrLegacy',
            'inet', 'InetSocketAddress',
            'unix', 'UnixSocketAddress' } }

> 
> which should be backwards compatible with the existing:
> 
> { "execute": "nbd-server-start", "arguments": {
>     "tls-authz": "authz0",
>     "addr": { "type": "inet", "data": {
>       "host": "localhost", "port": "10809" } } } }
> 
> by relying on the discriminator's default expansion to:
> 
> { "execute": "nbd-server-start", "arguments": {
>     "tls-authz": "authz0",
>     "type": "legacy",
>     "addr": { "type": "inet", "data": {
>       "host": "localhost", "port": "10809" } } } }

But this part remains true - if a flat union has an optional
discriminator, then the discriminator must include a default value,
where omitting the discriminator then results in sane expansion, and
where a careful choice of discriminator default allows legacy syntax to
co-exist with new preferred syntax.

-- 
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 --]