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

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

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here is a suggestion to enable keepalive option to track server availablity.
We suggest to enable it by default. If we need, we'll be able to add option
to specify timeout by hand later.

v2: 01 - Fix io channel returned errors to be -1 [Daniel]
    02 - Fix typo in commit message [Eric]

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

 include/io/channel.h | 15 +++++++++++++++
 block/nbd-client.c   |  1 +
 io/channel-socket.c  | 20 ++++++++++++++++++++
 io/channel.c         | 14 ++++++++++++++
 4 files changed, 50 insertions(+)

-- 
2.18.0

[Qemu-devel] [PATCH v2 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 | 15 +++++++++++++++
 io/channel-socket.c  | 20 ++++++++++++++++++++
 io/channel.c         | 14 ++++++++++++++
 3 files changed, 49 insertions(+)

diff --git a/include/io/channel.h b/include/io/channel.h
index 59460cb1ec..b8789ea08a 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,18 @@ 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
+ *
+ * Returns 0 on success, -1 on error.
+ */
+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..84b474851b 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -656,6 +656,25 @@ 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 -1;
+    }
+
+    return 0;
+}
+
+
 static void
 qio_channel_socket_set_delay(QIOChannel *ioc,
                              bool enabled)
@@ -762,6 +781,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..a9e62fdbdc 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 -1;
+    }
+
+    return klass->io_set_keepalive(ioc, enabled, errp);
+}
+
 
 int qio_channel_close(QIOChannel *ioc,
                       Error **errp)
-- 
2.18.0

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

[Daniel P. Berrangé]

On Wed, Jun 05, 2019 at 07:18:03PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/io/channel.h | 15 +++++++++++++++
>  io/channel-socket.c  | 20 ++++++++++++++++++++
>  io/channel.c         | 14 ++++++++++++++
>  3 files changed, 49 insertions(+)

Acked-by: Daniel P. Berrangé <berrange@redhat.com>


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

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

[Vladimir Sementsov-Ogievskiy]

Enable keepalive option to track server availability.

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] [Qemu-block] [PATCH v2 0/2] nbd: enable keepalive

[John Snow]

On 6/5/19 12:18 PM, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> Here is a suggestion to enable keepalive option to track server availablity.
> We suggest to enable it by default. If we need, we'll be able to add option
> to specify timeout by hand later.
> 
> v2: 01 - Fix io channel returned errors to be -1 [Daniel]
>     02 - Fix typo in commit message [Eric]
> 
> Vladimir Sementsov-Ogievskiy (2):
>   io/channel: add qio_channel_set_keepalive
>   nbd-client: enable TCP keepalive
> 
>  include/io/channel.h | 15 +++++++++++++++
>  block/nbd-client.c   |  1 +
>  io/channel-socket.c  | 20 ++++++++++++++++++++
>  io/channel.c         | 14 ++++++++++++++
>  4 files changed, 50 insertions(+)
> 

Ping -- I think this was good to go with Dan's ACK, based on the
discussion from v1.

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

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 1317 bytes --]

On 6/26/19 4:18 PM, John Snow wrote:
> 
> 
> On 6/5/19 12:18 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> Here is a suggestion to enable keepalive option to track server availablity.
>> We suggest to enable it by default. If we need, we'll be able to add option
>> to specify timeout by hand later.
>>
>> v2: 01 - Fix io channel returned errors to be -1 [Daniel]
>>     02 - Fix typo in commit message [Eric]
>>
>> Vladimir Sementsov-Ogievskiy (2):
>>   io/channel: add qio_channel_set_keepalive
>>   nbd-client: enable TCP keepalive
>>
>>  include/io/channel.h | 15 +++++++++++++++
>>  block/nbd-client.c   |  1 +
>>  io/channel-socket.c  | 20 ++++++++++++++++++++
>>  io/channel.c         | 14 ++++++++++++++
>>  4 files changed, 50 insertions(+)
>>
> 
> Ping -- I think this was good to go with Dan's ACK, based on the
> discussion from v1.

Actually, I thought that we changed tactics, and that the latest version is:

https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg01989.html

[PATCH v2] qapi: InitSocketAddress: add keepalive option

to make the setting conditional based on blockdev parameters rather than
unconditional.

-- 
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] [Qemu-block] [PATCH v2 0/2] nbd: enable keepalive

[John Snow]

On 6/26/19 5:23 PM, Eric Blake wrote:
> On 6/26/19 4:18 PM, John Snow wrote:
>>
>>
>> On 6/5/19 12:18 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> Hi all!
>>>
>>> Here is a suggestion to enable keepalive option to track server availablity.
>>> We suggest to enable it by default. If we need, we'll be able to add option
>>> to specify timeout by hand later.
>>>
>>> v2: 01 - Fix io channel returned errors to be -1 [Daniel]
>>>     02 - Fix typo in commit message [Eric]
>>>
>>> Vladimir Sementsov-Ogievskiy (2):
>>>   io/channel: add qio_channel_set_keepalive
>>>   nbd-client: enable TCP keepalive
>>>
>>>  include/io/channel.h | 15 +++++++++++++++
>>>  block/nbd-client.c   |  1 +
>>>  io/channel-socket.c  | 20 ++++++++++++++++++++
>>>  io/channel.c         | 14 ++++++++++++++
>>>  4 files changed, 50 insertions(+)
>>>
>>
>> Ping -- I think this was good to go with Dan's ACK, based on the
>> discussion from v1.
> 
> Actually, I thought that we changed tactics, and that the latest version is:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg01989.html
> 
> [PATCH v2] qapi: InitSocketAddress: add keepalive option
> 
> to make the setting conditional based on blockdev parameters rather than
> unconditional.
> 

OK, thanks for the pointer! I had my head buried for a little bit and I
am playing catchup with discussions, and it looked like this one needed
attention, but I missed this.

Thank you!

--js

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

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 1585 bytes --]

On 6/26/19 4:23 PM, Eric Blake wrote:
> On 6/26/19 4:18 PM, John Snow wrote:
>>
>>
>> On 6/5/19 12:18 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> Hi all!
>>>
>>> Here is a suggestion to enable keepalive option to track server availablity.
>>> We suggest to enable it by default. If we need, we'll be able to add option
>>> to specify timeout by hand later.
>>>
>>> v2: 01 - Fix io channel returned errors to be -1 [Daniel]
>>>     02 - Fix typo in commit message [Eric]
>>>
>>> Vladimir Sementsov-Ogievskiy (2):
>>>   io/channel: add qio_channel_set_keepalive
>>>   nbd-client: enable TCP keepalive
>>>
>>>  include/io/channel.h | 15 +++++++++++++++
>>>  block/nbd-client.c   |  1 +
>>>  io/channel-socket.c  | 20 ++++++++++++++++++++
>>>  io/channel.c         | 14 ++++++++++++++
>>>  4 files changed, 50 insertions(+)
>>>
>>
>> Ping -- I think this was good to go with Dan's ACK, based on the
>> discussion from v1.
> 
> Actually, I thought that we changed tactics, and that the latest version is:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg01989.html
> 
> [PATCH v2] qapi: InitSocketAddress: add keepalive option
> 
> to make the setting conditional based on blockdev parameters rather than
> unconditional.

Or even v3, which still had review comments pending:

https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg05508.html

[PATCH v3] qapi: Add InetSocketAddress member keep-alive

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