Re: [PATCH v3 1/2] qemu-nbd: Use SOMAXCONN for socket listen() backlog

[Eric Blake]

On 2/10/21 10:58 AM, Nir Soffer wrote:
> On Tue, Feb 9, 2021 at 5:28 PM Eric Blake <eblake@redhat.com> wrote:
>>
>> Our default of a backlog of 1 connection is rather puny; it gets in
>> the way when we are explicitly allowing multiple clients (such as
>> qemu-nbd -e N [--shared], or nbd-server-start with its default
>> "max-connections":0 for unlimited), but is even a problem when we
>> stick to qemu-nbd's default of only 1 active client but use -t
>> [--persistent] where a second client can start using the server once
>> the first finishes.  While the effects are less noticeable on TCP
>> sockets (since the client can poll() to learn when the server is ready
>> again), it is definitely observable on Unix sockets, where on Unix, a
>> client will fail with EAGAIN and no recourse but to sleep an arbitrary
>> amount of time before retrying if the server backlog is already full.
>>
>> Since QMP nbd-server-start is always persistent, it now always
>> requests a backlog of SOMAXCONN;
> 
> This makes sense since we don't limit the number of connections.
> 
>> meanwhile, qemu-nbd will request
>> SOMAXCONN if persistent, otherwise its backlog should be based on the
>> expected number of clients.
> 
> If --persistent is used without --shared, we allow only one concurrent
> connection, so not clear why we need maximum backlog.

We only allow one active connection, but other clients can queue up to
also take advantage of the server once the first client disconnects.  A
larger backlog allows those additional clients to reach the point where
they can poll() for activity, rather than getting EAGAIN failures.

> 
> I think that separating --persistent and --shared would be easier to
> understand and use. The backlog will always be based on shared value.
> 

>> +++ b/qemu-nbd.c
>> @@ -964,8 +964,16 @@ int main(int argc, char **argv)
>>
>>      server = qio_net_listener_new();
>>      if (socket_activation == 0) {
>> +        int backlog;
>> +
>> +        if (persistent) {
>> +            backlog = SOMAXCONN;
> 
> This increases the backlog, but since default shared is still 1, we will
> not accept more than 1 connection, so not clear why SOMAXCONN
> is better.

While we aren't servicing the next client yet, we are at least allowing
them to make it further in their connection by supporting a backlog.


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