From 4a9ddf42788d3f924bdad7746f7aca615f03d7c1 Mon Sep 17 00:00:00 2001

From: Eric Blake <eblake@redhat.com>

Date: Wed, 11 May 2022 19:49:24 -0500

Subject: [PATCH 2/2] nbd/server: Allow MULTI_CONN for shared writable exports

MIME-Version: 1.0

Content-Type: text/plain; charset=UTF-8

Content-Transfer-Encoding: 8bit

RH-Author: Eric Blake <eblake@redhat.com>

RH-MergeRequest: 90: Advertise MULTI_CONN on writeable NBD servers

RH-Commit: [2/2] 53f0e885a5ed7f6e4bb14e74fe8e7957e6afe90f (ebblake/centos-qemu-kvm)

RH-Bugzilla: 1708300

RH-Acked-by: Nir Soffer <nsoffer@redhat.com>

RH-Acked-by: Kevin Wolf <kwolf@redhat.com>

RH-Acked-by: Daniel P. Berrangé <berrange@redhat.com>

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 always satisfy these conditions in qemu - even when we support

multiple clients, ALL clients go through a single point of reference

into the block layer, with no local caching.  The effect of one client

is instantly visible to the next client.  Even if our backend were a

network device, we argue that any multi-path caching effects that

would cause inconsistencies in back-to-back actions not seeing the

effect of previous actions would be a bug in that backend, and not the

fault of caching in qemu.  As such, it is safe to unconditionally

advertise CAN_MULTI_CONN for any qemu NBD server situation that

supports parallel clients.

Note, however, that we don't want to advertise CAN_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).  This is visible by no longer advertising MULTI_CONN for

'qemu-nbd -r' without -e, as in the iotest nbd-qemu-allocation.

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@mail.ru>

Message-Id: <20220512004924.417153-3-eblake@redhat.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

(cherry picked from commit 58a6fdcc9efb2a7c1ef4893dca4aa5e8020ca3dc)

Conflicts:

	nbd/server.c - context, e5fb29d5 not backported

Signed-off-by: Eric Blake <eblake@redhat.com>

---

 MAINTAINERS                                   |   1 +

 blockdev-nbd.c                                |   5 +

 docs/interop/nbd.txt                          |   1 +

 docs/tools/qemu-nbd.rst                       |   3 +-

 include/block/nbd.h                           |   3 +-

 nbd/server.c                                  |  10 +-

 qapi/block-export.json                        |   8 +-

 tests/qemu-iotests/tests/nbd-multiconn        | 145 ++++++++++++++++++

 tests/qemu-iotests/tests/nbd-multiconn.out    |   5 +

 .../tests/nbd-qemu-allocation.out             |   2 +-

 10 files changed, 172 insertions(+), 11 deletions(-)

 create mode 100755 tests/qemu-iotests/tests/nbd-multiconn

 create mode 100644 tests/qemu-iotests/tests/nbd-multiconn.out

diff --git a/MAINTAINERS b/MAINTAINERS

index 4ad2451e03..2fe20a49ab 100644

--- a/MAINTAINERS

+++ b/MAINTAINERS

@@ -3370,6 +3370,7 @@ F: qemu-nbd.*

 F: blockdev-nbd.c

 F: docs/interop/nbd.txt

 F: docs/tools/qemu-nbd.rst

+F: tests/qemu-iotests/tests/*nbd*

 T: git https://repo.or.cz/qemu/ericb.git nbd

 T: git https://src.openvz.org/scm/~vsementsov/qemu.git nbd

diff --git a/blockdev-nbd.c b/blockdev-nbd.c

index add41a23af..c6d9b0324c 100644

--- a/blockdev-nbd.c

+++ b/blockdev-nbd.c

@@ -44,6 +44,11 @@ bool nbd_server_is_running(void)

     return nbd_server || qemu_nbd_connections >= 0;

 }

+int nbd_server_max_connections(void)

+{

+    return nbd_server ? nbd_server->max_connections : qemu_nbd_connections;

+}

+

 static void nbd_blockdev_client_closed(NBDClient *client, bool ignored)

 {

     nbd_client_put(client);

diff --git a/docs/interop/nbd.txt b/docs/interop/nbd.txt

index bdb0f2a41a..f5ca25174a 100644

--- a/docs/interop/nbd.txt

+++ b/docs/interop/nbd.txt

@@ -68,3 +68,4 @@ NBD_CMD_BLOCK_STATUS for "qemu:dirty-bitmap:", NBD_CMD_CACHE

 * 4.2: NBD_FLAG_CAN_MULTI_CONN for shareable read-only exports,

 NBD_CMD_FLAG_FAST_ZERO

 * 5.2: NBD_CMD_BLOCK_STATUS for "qemu:allocation-depth"

+* 7.1: NBD_FLAG_CAN_MULTI_CONN for shareable writable exports

diff --git a/docs/tools/qemu-nbd.rst b/docs/tools/qemu-nbd.rst

index 4c950f6199..8e08a29e89 100644

--- a/docs/tools/qemu-nbd.rst

+++ b/docs/tools/qemu-nbd.rst

@@ -139,8 +139,7 @@ driver options if :option:`--image-opts` is specified.

 .. option:: -e, --shared=NUM

   Allow up to *NUM* clients to share the device (default

-  ``1``), 0 for unlimited. Safe for readers, but for now,

-  consistency is not guaranteed between multiple writers.

+  ``1``), 0 for unlimited.

 .. option:: -t, --persistent

diff --git a/include/block/nbd.h b/include/block/nbd.h

index c5a29ce1c6..c74b7a9d2e 100644

--- a/include/block/nbd.h

+++ b/include/block/nbd.h

@@ -1,5 +1,5 @@

 /*

- *  Copyright (C) 2016-2020 Red Hat, Inc.

+ *  Copyright (C) 2016-2022 Red Hat, Inc.

  *  Copyright (C) 2005  Anthony Liguori <anthony@codemonkey.ws>

  *

  *  Network Block Device

@@ -346,6 +346,7 @@ void nbd_client_put(NBDClient *client);

 void nbd_server_is_qemu_nbd(int max_connections);

 bool nbd_server_is_running(void);

+int nbd_server_max_connections(void);

 void nbd_server_start(SocketAddress *addr, const char *tls_creds,

                       const char *tls_authz, uint32_t max_connections,

                       Error **errp);

diff --git a/nbd/server.c b/nbd/server.c

index c5644fd3f6..6e2157acfa 100644

--- a/nbd/server.c

+++ b/nbd/server.c

@@ -1,5 +1,5 @@

 /*

- *  Copyright (C) 2016-2021 Red Hat, Inc.

+ *  Copyright (C) 2016-2022 Red Hat, Inc.

  *  Copyright (C) 2005  Anthony Liguori <anthony@codemonkey.ws>

  *

  *  Network Block Device Server Side

@@ -1642,7 +1642,6 @@ static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args,

     int64_t size;

     uint64_t perm, shared_perm;

     bool readonly = !exp_args->writable;

-    bool shared = !exp_args->writable;

     strList *bitmaps;

     size_t i;

     int ret;

@@ -1693,11 +1692,12 @@ static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args,

     exp->description = g_strdup(arg->description);

     exp->nbdflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_FLUSH |

                      NBD_FLAG_SEND_FUA | NBD_FLAG_SEND_CACHE);

+

+    if (nbd_server_max_connections() != 1) {

+        exp->nbdflags |= NBD_FLAG_CAN_MULTI_CONN;

+    }

     if (readonly) {

         exp->nbdflags |= NBD_FLAG_READ_ONLY;

-        if (shared) {

-            exp->nbdflags |= NBD_FLAG_CAN_MULTI_CONN;

-        }

     } else {

         exp->nbdflags |= (NBD_FLAG_SEND_TRIM | NBD_FLAG_SEND_WRITE_ZEROES |

                           NBD_FLAG_SEND_FAST_ZERO);

diff --git a/qapi/block-export.json b/qapi/block-export.json

index 1e34927f85..755ccc89b1 100644

--- a/qapi/block-export.json

+++ b/qapi/block-export.json

@@ -21,7 +21,9 @@

 #             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)

+#                   time, 0 for unlimited. Setting this to 1 also stops

+#                   the server from advertising multiple client support

+#                   (since 5.2; default: 0)

 #

 # Since: 4.2

 ##

@@ -50,7 +52,9 @@

 #             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)

+#                   time, 0 for unlimited. Setting this to 1 also stops

+#                   the server from advertising multiple client support

+#                   (since 5.2; default: 0).

 #

 # Returns: error if the server is already running.

 #

diff --git a/tests/qemu-iotests/tests/nbd-multiconn b/tests/qemu-iotests/tests/nbd-multiconn

new file mode 100755

index 0000000000..b121f2e363

--- /dev/null

+++ b/tests/qemu-iotests/tests/nbd-multiconn

@@ -0,0 +1,145 @@

+#!/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

+

+

+disk = os.path.join(iotests.test_dir, 'disk')

+size = '4M'

+nbd_sock = os.path.join(iotests.sock_dir, 'nbd_sock')

+nbd_uri = 'nbd+unix:///{}?socket=' + nbd_sock

+

+

+@contextmanager

+def open_nbd(export_name):

+    h = nbd.NBD()

+    try:

+        h.connect_uri(nbd_uri.format(export_name))

+        yield h

+    finally:

+        h.shutdown()

+

+class TestNbdMulticonn(iotests.QMPTestCase):

+    def setUp(self):

+        qemu_img_create('-f', iotests.imgfmt, disk, size)

+        qemu_io('-c', 'w -P 1 0 2M', '-c', 'w -P 2 2M 2M', disk)

+

+        self.vm = iotests.VM()

+        self.vm.launch()

+        result = self.vm.qmp('blockdev-add', {

+            'driver': 'qcow2',

+            'node-name': 'n',

+            'file': {'driver': 'file', 'filename': disk}

+        })

+        self.assert_qmp(result, 'return', {})

+

+    def tearDown(self):

+        self.vm.shutdown()

+        os.remove(disk)

+        try:

+            os.remove(nbd_sock)

+        except OSError:

+            pass

+

+    @contextmanager

+    def run_server(self, max_connections=None):

+        args = {

+            'addr': {

+                'type': 'unix',

+                'data': {'path': nbd_sock}

+            }

+        }

+        if max_connections is not None:

+            args['max-connections'] = max_connections

+

+        result = self.vm.qmp('nbd-server-start', args)

+        self.assert_qmp(result, 'return', {})

+        yield

+

+        result = self.vm.qmp('nbd-server-stop')

+        self.assert_qmp(result, 'return', {})

+

+    def add_export(self, name, writable=None):

+        args = {

+            'type': 'nbd',

+            'id': name,

+            'node-name': 'n',

+            'name': name,

+        }

+        if writable is not None:

+            args['writable'] = writable

+

+        result = self.vm.qmp('block-export-add', args)

+        self.assert_qmp(result, 'return', {})

+

+    def test_default_settings(self):

+        with self.run_server():

+            self.add_export('r')

+            self.add_export('w', writable=True)

+            with open_nbd('r') as h:

+                self.assertTrue(h.can_multi_conn())

+            with open_nbd('w') as h:

+                self.assertTrue(h.can_multi_conn())

+

+    def test_limited_connections(self):

+        with self.run_server(max_connections=1):

+            self.add_export('r')

+            self.add_export('w', writable=True)

+            with open_nbd('r') as h:

+                self.assertFalse(h.can_multi_conn())

+            with open_nbd('w') as h:

+                self.assertFalse(h.can_multi_conn())

+

+    def test_parallel_writes(self):

+        with self.run_server():

+            self.add_export('w', writable=True)

+

+            clients = [nbd.NBD() for _ in range(3)]

+            for c in clients:

+                c.connect_uri(nbd_uri.format('w'))

+                self.assertTrue(c.can_multi_conn())

+

+            initial_data = clients[0].pread(1024 * 1024, 0)

+            self.assertEqual(initial_data, b'\x01' * 1024 * 1024)

+

+            updated_data = b'\x03' * 1024 * 1024

+            clients[1].pwrite(updated_data, 0)

+            clients[2].flush()

+            current_data = clients[0].pread(1024 * 1024, 0)

+

+            self.assertEqual(updated_data, current_data)

+

+            for i in range(3):

+                clients[i].shutdown()

+

+

+if __name__ == '__main__':

+    try:

+        # Easier to use libnbd than to try and set up parallel

+        # 'qemu-nbd --list' or 'qemu-io' processes, but not all systems

+        # have libnbd installed.

+        import nbd  # type: ignore

+

+        iotests.main(supported_fmts=['qcow2'])

+    except ImportError:

+        iotests.notrun('libnbd not installed')

diff --git a/tests/qemu-iotests/tests/nbd-multiconn.out b/tests/qemu-iotests/tests/nbd-multiconn.out

new file mode 100644

index 0000000000..8d7e996700

--- /dev/null

+++ b/tests/qemu-iotests/tests/nbd-multiconn.out

@@ -0,0 +1,5 @@

+...

+----------------------------------------------------------------------

+Ran 3 tests

+

+OK

diff --git a/tests/qemu-iotests/tests/nbd-qemu-allocation.out b/tests/qemu-iotests/tests/nbd-qemu-allocation.out

index 0bf1abb063..9d938db24e 100644

--- a/tests/qemu-iotests/tests/nbd-qemu-allocation.out

+++ b/tests/qemu-iotests/tests/nbd-qemu-allocation.out

@@ -17,7 +17,7 @@ wrote 2097152/2097152 bytes at offset 1048576

 exports available: 1

  export: ''

   size:  4194304

-  flags: 0x58f ( readonly flush fua df multi cache )

+  flags: 0x48f ( readonly flush fua df cache )

   min block: 1

   opt block: 4096

   max block: 33554432

-- 

2.31.1