Re: [Qemu-block] [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 :|