Re: [PATCH v2] nbd/server: Allow MULTI_CONN for shared writable exports

[Vladimir Sementsov-Ogievskiy]

15.02.2022 20:18, Eric Blake wrote:
> According to the NBD spec, a server advertising
> NBD_FLAG_CAN_MULTI_CONN promises that multiple client connections will
> not see any cache inconsistencies: when properly separated by a single
> flush, actions performed by one client will be visible to another
> client, regardless of which client did the flush.  We satisfy these
> conditions in qemu when our block layer is backed by the local
> filesystem (by virtue of the semantics of fdatasync(), and the fact
> that qemu itself is not buffering writes beyond flushes).  It is
> harder to state whether we satisfy these conditions for network-based
> protocols, so the safest course of action is to allow users to opt-in
> to advertising multi-conn.  We may later tweak defaults to advertise
> by default when the block layer can confirm that the underlying
> protocol driver is cache consistent between multiple writers, but for
> now, this at least allows savvy users (such as virt-v2v or nbdcopy) to
> explicitly start qemu-nbd or qemu-storage-daemon with multi-conn
> advertisement in a known-safe setup where the client end can then
> benefit from parallel clients.
>
> Note, however, that we don't want to advertise MULTI_CONN when we know

Here we change existing default behavior. Pre-patch MULTI_CONN is still 
advertised for readonly export even when connections number is limited to 1.

Still may be it's a good change? Let's then note it here.. Could it break some 
existing clients? Hard to imagine.

Otherwise patch looks good to me, nonsignificant notes below.

> that a second client cannot connect (for historical reasons, qemu-nbd
> defaults to a single connection while nbd-server-add and QMP commands
> default to unlimited connections; but we already have existing means
> to let either style of NBD server creation alter those defaults).  The
> harder part of this patch is setting up an iotest to demonstrate
> behavior of multiple NBD clients to a single server.  It might be
> possible with parallel qemu-io processes, but concisely managing that
> in shell is painful.  I found it easier to do by relying on the libnbd
> project's nbdsh, which means this test will be skipped on platforms
> where that is not available.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Fixes: https://bugzilla.redhat.com/1708300
> ---

[..]

> +##
> +# @NbdExportMultiConn:
> +#
> +# Possible settings for advertising NBD multiple client support.
> +#
> +# @off: Do not advertise multiple clients.
> +#
> +# @on: Allow multiple clients (for writable clients, this is only safe
> +#      if the underlying BDS is cache-consistent, such as when backed
> +#      by the raw file driver); ignored if the NBD server was set up
> +#      with max-connections of 1.
> +#
> +# @auto: Behaves like @off if the export is writable, and @on if the
> +#        export is read-only.
> +#
> +# Since: 7.0
> +##
> +{ 'enum': 'NbdExportMultiConn',
> +  'data': ['off', 'on', 'auto'] }

Probably we should use generic OnOffAuto type that we already have.. But in 
your way documentation looks better.

> +
>   ##
>   # @BlockExportOptionsNbd:
>   #

[..]

> +
> +_make_test_img 4M
> +$QEMU_IO -c 'w -P 1 0 2M' -c 'w -P 2 2M 2M' "$TEST_IMG" | _filter_qemu_io
> +_launch_qemu 2> >(_filter_nbd)
> +_send_qemu_cmd $QEMU_HANDLE '{"execute":"qmp_capabilities"}' "return"
> +_send_qemu_cmd $QEMU_HANDLE '{"execute":"blockdev-add",
> +  "arguments":{"driver":"qcow2", "node-name":"n",
> +    "file":{"driver":"file", "filename":"'"$TEST_IMG"'"}}}' "return"
> +export nbd_unix_socket
> +

[..]

> +nbdsh -c '
> +import os
> +sock = os.getenv("nbd_unix_socket")

Just interested why you pass it through environment and no simply do '... sock = 
"'"$nbd_unix_socket"'"... '

> +h = []
> +
> +for i in range(3):
> +  h.append(nbd.NBD())

Looks like Python:) And if something looks like Python, it's Python, we know. 
Should I say about PEP8 that recommends 4 whitespaces?)

> +  h[i].connect_unix(sock)
> +  assert h[i].can_multi_conn()
> +
> +buf1 = h[0].pread(1024 * 1024, 0)
> +if buf1 != b"\x01" * 1024 * 1024:
> +  print("Unexpected initial read")
> +buf2 = b"\x03" * 1024 * 1024
> +h[1].pwrite(buf2, 0)
> +h[2].flush()
> +buf1 = h[0].pread(1024 * 1024, 0)
> +if buf1 == buf2:
> +  print("Flush appears to be consistent across connections")
> +
> +for i in range(3):
> +  h[i].shutdown()
> +'

--
Best regards,
Vladimir