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 <address@hidden>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>> ---
>>>   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