From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([209.51.188.92]:35146) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ghUXE-0000ZN-Mi for qemu-devel@nongnu.org; Thu, 10 Jan 2019 02:13:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ghUXD-0001k8-8i for qemu-devel@nongnu.org; Thu, 10 Jan 2019 02:13:48 -0500 From: Eric Blake Date: Thu, 10 Jan 2019 01:13:25 -0600 Message-Id: <20190110071330.28136-2-eblake@redhat.com> In-Reply-To: <20190110071330.28136-1-eblake@redhat.com> References: <20190110071330.28136-1-eblake@redhat.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Subject: [Qemu-devel] [PATCH v2 1/6] nbd: Only require disabled bitmap for read-only exports List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: jsnow@redhat.com, qemu-block@nongnu.org, vsementsov@virtuozzo.com, Kevin Wolf , Max Reitz Our initial implementation of x-nbd-server-add-bitmap put in a restriction because of incremental backups: in that usage, we are exporting one qcow2 file (the temporary overlay target of a blockdev-backup sync:none job) and a dirty bitmap owned by a second qcow2 file (the source of the blockdev-backup, which is the backing file of the temporary). While both qcow2 files are still writable (the target capture copy-on-write of old contents, the source to track live guest writes in the meantime), the NBD client expects to see constant data, including the dirty bitmap. An enabled bitmap in the source would be modified by guest writes, which is at odds with the NBD export being a read-only constant view, hence the initial code choice of enforcing a disabled bitmap (the intent is that the exposed bitmap was disabled in the same transaction that started the blockdev-backup job, although we don't want to actually track enough state to actually enforce that). However, in the case of image migration, where we WANT a writable export, it makes total sense that the NBD client could expect writes to change the status visible through the exposed dirty bitmap, and thus permitting an enabled bitmap for an export that is not read-only makes sense; although the current restriction prevents that usage. Alternatively, consider the case when the active layer does not contain the bitmap but the backing layer does. If the backing layer is read-only (which is different from the backing layer of a blockdev-backup job being writable), we know the backing layer cannot be modified, and thus the bitmap will not change contents even if it is still marked enabled (for that matter, since the backing file is read-only, we couldn't change the bitmap to be disabled in the first place). Again, the current restriction prevents this usage. Solve both issues by gating the restriction against a disabled bitmap to only happen when the caller has requested a read-only export, and where the BDS that owns the bitmap (which might be a backing file of the BDS handed to nbd_export_new) is still writable. Update iotest 223 to add some tests of the error paths. Signed-off-by: Eric Blake --- v2: new patch --- nbd/server.c | 7 +++++-- tests/qemu-iotests/223 | 14 ++++++++++++-- tests/qemu-iotests/223.out | 4 ++++ 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/nbd/server.c b/nbd/server.c index 7af0ddffb20..98327088cb4 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -2456,8 +2456,11 @@ void nbd_export_bitmap(NBDExport *exp, const char = *bitmap, return; } - if (bdrv_dirty_bitmap_enabled(bm)) { - error_setg(errp, "Bitmap '%s' is enabled", bitmap); + if ((exp->nbdflags & NBD_FLAG_READ_ONLY) && bdrv_is_writable(bs) && + bdrv_dirty_bitmap_enabled(bm)) { + error_setg(errp, + "Enabled bitmap '%s' incompatible with readonly expor= t", + bitmap); return; } diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223 index 5513dc62159..f1fbb9bc1c6 100755 --- a/tests/qemu-iotests/223 +++ b/tests/qemu-iotests/223 @@ -61,6 +61,8 @@ echo "=3D=3D=3D Create partially sparse image, then add= dirty bitmaps =3D=3D=3D" echo # Two bitmaps, to contrast granularity issues +# Also note that b will be disabled, while b2 is left enabled, to +# check for read-only interactions _make_test_img -o cluster_size=3D4k 4M $QEMU_IO -c 'w -P 0x11 1M 2M' "$TEST_IMG" | _filter_qemu_io run_qemu <