Re: [RFC PATCH 08/22] nbd: Add max-connections to nbd-server-start

From: Kevin Wolf <kwolf@redhat.com>
To: Eric Blake <eblake@redhat.com>
Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, mreitz@redhat.com
Subject: Re: [RFC PATCH 08/22] nbd: Add max-connections to nbd-server-start
Date: Thu, 20 Aug 2020 13:12:44 +0200	[thread overview]
Message-ID: <20200820111244.GC99531@linux.fritz.box> (raw)
In-Reply-To: <973cb8f5-b042-fa70-da45-0aefeaad2fcd@redhat.com>

Am 19.08.2020 um 22:00 hat Eric Blake geschrieben:
> On 8/13/20 11:29 AM, Kevin Wolf wrote:
> > This is a QMP equivalent of qemu-nbd's --share option, limiting the
> > maximum number of clients that can attach at the same time.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >   qapi/block-export.json         | 10 ++++++++--
> >   include/block/nbd.h            |  3 ++-
> >   block/monitor/block-hmp-cmds.c |  2 +-
> >   blockdev-nbd.c                 | 33 ++++++++++++++++++++++++++-------
> >   qemu-storage-daemon.c          |  4 ++--
> >   5 files changed, 39 insertions(+), 13 deletions(-)
> > 
> > diff --git a/qapi/block-export.json b/qapi/block-export.json
> > index 40369814b4..1fdc55c53a 100644
> > --- a/qapi/block-export.json
> > +++ b/qapi/block-export.json
> > @@ -14,6 +14,8 @@
> >   #             is only resolved at time of use, so can be deleted and
> >   #             recreated on the fly while the NBD server is active.
> >   #             If missing, it will default to denying access (since 4.0).
> > +# @max-connections: The maximum number of connections to allow at the same
> > +#                   time, 0 for unlimited. (since 5.2; default: 0)
> 
> Nice way to add feature parity.
> 
> Limiting the number of connections (particularly for a writable client,
> where we cannot guarantee cache consistency between the connections), seems
> like a worthwhile feature to have; I've always found it odd that qemu-nbd
> and QMP nbd-server-add defaulted to different limits (1 vs. unlimited).  For
> reference, nbdkit defaults to unlimited, and I'm happy if
> qemu-storage-daemon does likewise; but changing qemu-nbd's default of 1
> would be backwards incompatible and may cause surprises (there's always
> 'qemu-nbd -e' when needed).  I also wonder if we should change 'qemu-nbd -e
> 0' to mean unlimited rather than an error (right now, qemu-iotests/common.rc
> uses -e 42 for all nbd-based tests for a saner limit than just 1, but it
> smells of being arbitrary compared to unlimited).

I think eventually the actual NBD server implementation in qemu-nbd
should go away and it should just reuse the QMP one. Changing the
default would remove one more difference between them, which can only be
helpful in this process. (Though of course having a different default is
still simple enough for a simple wrapper.)

Kevin