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

[Kevin Wolf]

Am 14.03.2022 um 21:38 hat Eric Blake geschrieben:
> According to the NBD spec, a server that advertises
> 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.

Do you have an example of how this could be unsafe?

As I understand it, the NBD server has a single BlockBackend and
therefore is a single client for the backend, be it file-posix or any
network-based protocol. It doesn't really make a difference for the
storage from how many different NBD clients the requests are coming.

I would have expected that cache coherency of the protocol level driver
would only matter if you had two QEMU processes accessing the same file
concurrently.

In fact, I don't think we even need the flush restriction from the NBD
spec. All clients see the same state (that of the NBD server
BlockBackend) even without anyone issuing any flush. The flush is only
needed to make sure that cached data is written to the backing storage
when writeback caches are involved.

Please correct me if I'm misunderstanding something here.

> 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 (new -m command-line option) or
> qemu-storage-daemon (new qapi field 'multi-conn') 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
> 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 I found it easier to do
> in python with the help of libnbd, and help from Nir and Vladimir in
> writing the test.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Suggested-by: Nir Soffer <nsoffer@redhat.com>
> Suggested-by: Vladimir Sementsov-Ogievskiy <v.sementsov-og@ya.ru>

> @@ -709,6 +714,17 @@ int main(int argc, char **argv)
>                  exit(EXIT_FAILURE);
>              }
>              break;
> +        case 'm':
> +        {
> +            Error *err = NULL;
> +            multi_conn = qapi_enum_parse(&OnOffAuto_lookup, optarg,
> +                                         ON_OFF_AUTO_AUTO, &err);
> +            if (err) {
> +                error_report_err(err);
> +                exit(EXIT_FAILURE);
> +            }

I think this is the same as passing &error_fatal.

> +            break;
> +        }
>          case 'f':
>              fmt = optarg;
>              break;
> diff --git a/tests/qemu-iotests/tests/nbd-multiconn 
> b/tests/qemu-iotests/tests/nbd-multiconn
> new file mode 100755
> index 000000000000..7d1179b33b05
> --- /dev/null
> +++ b/tests/qemu-iotests/tests/nbd-multiconn
> @@ -0,0 +1,157 @@
> +#!/usr/bin/env python3
> +# group: rw auto quick
> +#
> +# Test cases for NBD multi-conn advertisement
> +#
> +# Copyright (C) 2022 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +import os
> +from contextlib import contextmanager
> +import iotests
> +from iotests import qemu_img_create, qemu_io_silent

qemu_io_silent() doesn't exist any more, commit 72cfb937 removed it.

Kevin