[PATCH 0/3] block: Keep auto_backing_file post-migration

[PATCH 0/3] block: Keep auto_backing_file post-migration

[Hanna Reitz]

Hi,

https://gitlab.com/qemu-project/qemu/-/issues/1117 reports the following
issue:

Say you have a VM with a backing chain of images where the image
metadata contains json:{} backing file strings, which however will be
resolved to simple plain filenames when opened[1].

So when these images are opened, bs->auto_backing_file is first read
directly from the image header, and will thus contain a json:{}
filename.  The backing image is opened based off of this filename, and
bdrv_refresh_filename() simplfies the filename as shown[1].  We then
update bs->auto_backing_file from bs->backing->bs->filename, so both are
equal.

It is quite important that both are equal, because
bdrv_backing_overridden() checks whether the backing file has been
changed from the default by comparing bs->auto_backing_file to
bs->backing->bs->filename.

Because we did set bs->auto_backing_file from bs->backing->bs->filename,
both are equal, the backing file is not considered overridden, and
bdrv_refresh_filename(bs) will not consider it necessary to generate a
json:{} filename for the overlay.

Then the VM is migrated.

The destination side invokes bdrv_invalidate_cache(), which by qcow2 and
qed is implemented by closing the image and opening it.  This re-reads
the backing file string from disk, resetting bs->auto_backing_file.
Now, it will contains the json:{} filename again and thus differ from
bs->backing->bs->filename.

Consequentially, a subsequent bdrv_refresh_filename(bs) will find that
the overlay’s backing file has been overridden and generate a json:{}
filename, which isn’t great.

This series fixes that by having qcow2’s and qed’s image-open operations
not overwrite bs->auto_backing_file unless something has changed since
the last time we read the backing filename from the metadata.


Now, generating a json:{} filename can be a nuisance but shouldn’t be a
real problem.  The actual problem reported in 1117 comes later, namely
when creating a snapshot overlay post-migration.  This overlay image
will have a json:{} backing filename in its image metadata, which
contains a 'backing' key[2].

'qemu-img info' uses the BDRV_O_NO_BACKING flag to open images, which
conflicts with those backing options: With that flag, nobody processes
those options, and that’s an error.  Therefore, you can’t run 'qemu-img
info --backing-chain' on that overlay image.

That part of the issue is not fixed in this series, however.  I’ll send
a separate RFC series for it, because I’m honstly not quite certain how
it should be fixed.


[1] Example:
        json:{"driver": "qcow2",
              "file": {"driver": "file", "filename": "img.qcow2"}}
    Will generally be “resolved” by bdrv_refresh_filename() to
        "img.qcow2"

[2] That it contains a 'backing' key is only natural, because the reason
    why bdrv_refresh_filename() decided to generate a json:{} filename
    for the image is because it considered the backing file overridden.
    Hence it must put the actual backing file options into a 'backing'
    object in the json:{} filename.


Hanna Reitz (3):
  block/qcow2: Keep auto_backing_file if possible
  block/qed: Keep auto_backing_file if possible
  iotests/backing-file-invalidation: Add new test

 block/qcow2.c                                 |  21 ++-
 block/qed.c                                   |  15 +-
 .../tests/backing-file-invalidation           | 152 ++++++++++++++++++
 .../tests/backing-file-invalidation.out       |   5 +
 4 files changed, 184 insertions(+), 9 deletions(-)
 create mode 100755 tests/qemu-iotests/tests/backing-file-invalidation
 create mode 100644 tests/qemu-iotests/tests/backing-file-invalidation.out

-- 
2.36.1

[PATCH 1/3] block/qcow2: Keep auto_backing_file if possible

[Hanna Reitz]

qcow2_do_open() is used by qcow2_co_invalidate_cache(), i.e. may be run
on an image that has been opened before.  When reading the backing file
string from the image header, compare it against the existing
bs->backing_file, and update bs->auto_backing_file only if they differ.

auto_backing_file should ideally contain the filename the backing BDS
will actually have after opening, i.e. a post-bdrv_refresh_filename()
version of what is in the image header.  So for example, if the image
header reports the following backing file string:

    json:{"driver": "qcow2", "file": {
        "driver": "file", "filename": "/tmp/backing.qcow2"
    }}

Then auto_backing_file should contain simply "/tmp/backing.qcow2".

Because bdrv_refresh_filename() only works on existing BDSs, though, the
way how we get this auto_backing_file value is to have the format driver
set it to whatever is in the image header, and when the backing BDS is
opened based on that, we update it with the filename the backing BDS
actually got.

However, qcow2's qcow2_co_invalidate_cache() implementation breaks this
because it just resets auto_backing_file to whatever is in the image
file without opening a BDS based on it, so we never get
auto_backing_file back to the "refreshed" version, and in the example
above, it would stay "json:{...}".

Then, bs->backing->bs->filename will differ from bs->auto_backing_file,
making bdrv_backing_overridden(bs) return true, which will lead
bdrv_refresh_filename(bs) to generate a json:{} filename for bs, even
though that may not have been necessary.  This is reported in the issue
linked below.

Therefore, skip updating auto_backing_file if nothing has changed in the
image header since we last read it.

Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1117
Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 block/qcow2.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index c6c6692fb7..a43fee2067 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1696,16 +1696,27 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
             ret = -EINVAL;
             goto fail;
         }
+
+        s->image_backing_file = g_malloc(len + 1);
         ret = bdrv_pread(bs->file, header.backing_file_offset, len,
-                         bs->auto_backing_file, 0);
+                         s->image_backing_file, 0);
         if (ret < 0) {
             error_setg_errno(errp, -ret, "Could not read backing file name");
             goto fail;
         }
-        bs->auto_backing_file[len] = '\0';
-        pstrcpy(bs->backing_file, sizeof(bs->backing_file),
-                bs->auto_backing_file);
-        s->image_backing_file = g_strdup(bs->auto_backing_file);
+        s->image_backing_file[len] = '\0';
+
+        /*
+         * Update only when something has changed.  This function is called by
+         * qcow2_co_invalidate_cache(), and we do not want to reset
+         * auto_backing_file unless necessary.
+         */
+        if (!g_str_equal(s->image_backing_file, bs->backing_file)) {
+            pstrcpy(bs->backing_file, sizeof(bs->backing_file),
+                    s->image_backing_file);
+            pstrcpy(bs->auto_backing_file, sizeof(bs->auto_backing_file),
+                    s->image_backing_file);
+        }
     }
 
     /*
-- 
2.36.1

[PATCH 2/3] block/qed: Keep auto_backing_file if possible

[Hanna Reitz]

Just like qcow2, qed invokes its open function in its
.bdrv_co_invalidate_cache() implementation.  Therefore, just like done
for qcow2 in HEAD^, update auto_backing_file only if the backing file
string in the image header differs from the one we have read before.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 block/qed.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index 40943e679b..324ca0e95a 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -445,6 +445,8 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
     }
 
     if ((s->header.features & QED_F_BACKING_FILE)) {
+        g_autofree char *backing_file_str = NULL;
+
         if ((uint64_t)s->header.backing_filename_offset +
             s->header.backing_filename_size >
             s->header.cluster_size * s->header.header_size) {
@@ -452,16 +454,21 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
             return -EINVAL;
         }
 
+        backing_file_str = g_malloc(sizeof(bs->backing_file));
         ret = qed_read_string(bs->file, s->header.backing_filename_offset,
                               s->header.backing_filename_size,
-                              bs->auto_backing_file,
-                              sizeof(bs->auto_backing_file));
+                              backing_file_str, sizeof(bs->backing_file));
         if (ret < 0) {
             error_setg(errp, "Failed to read backing filename");
             return ret;
         }
-        pstrcpy(bs->backing_file, sizeof(bs->backing_file),
-                bs->auto_backing_file);
+
+        if (!g_str_equal(backing_file_str, bs->backing_file)) {
+            pstrcpy(bs->backing_file, sizeof(bs->backing_file),
+                    backing_file_str);
+            pstrcpy(bs->auto_backing_file, sizeof(bs->auto_backing_file),
+                    backing_file_str);
+        }
 
         if (s->header.features & QED_F_BACKING_FORMAT_NO_PROBE) {
             pstrcpy(bs->backing_format, sizeof(bs->backing_format), "raw");
-- 
2.36.1

[PATCH 3/3] iotests/backing-file-invalidation: Add new test

[Hanna Reitz]

Add a new test to see what happens when you migrate a VM with a backing
chain that has json:{} backing file strings, which, when opened, will be
resolved to plain filenames.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 .../tests/backing-file-invalidation           | 152 ++++++++++++++++++
 .../tests/backing-file-invalidation.out       |   5 +
 2 files changed, 157 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/backing-file-invalidation
 create mode 100644 tests/qemu-iotests/tests/backing-file-invalidation.out

diff --git a/tests/qemu-iotests/tests/backing-file-invalidation b/tests/qemu-iotests/tests/backing-file-invalidation
new file mode 100755
index 0000000000..4eccc80153
--- /dev/null
+++ b/tests/qemu-iotests/tests/backing-file-invalidation
@@ -0,0 +1,152 @@
+#!/usr/bin/env python3
+# group: rw migration
+#
+# Migrate a VM with a BDS with backing nodes, which runs
+# bdrv_invalidate_cache(), which for qcow2 and qed triggers reading the
+# backing file string from the image header.  Check whether this
+# interferes with bdrv_backing_overridden().
+#
+# 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 json
+import os
+from typing import Optional
+
+import iotests
+from iotests import qemu_img_create, qemu_img_info
+
+
+image_size = 1 * 1024 * 1024
+imgs = [os.path.join(iotests.test_dir, f'{i}.img') for i in range(0, 4)]
+
+mig_sock = os.path.join(iotests.sock_dir, 'mig.sock')
+
+
+class TestPostMigrateFilename(iotests.QMPTestCase):
+    vm_s: Optional[iotests.VM] = None
+    vm_d: Optional[iotests.VM] = None
+
+    def setUp(self) -> None:
+        # Create backing chain of three images, where the backing file strings
+        # are json:{} filenames
+        qemu_img_create('-f', iotests.imgfmt, imgs[0], str(image_size))
+        for i in range(1, 3):
+            backing = {
+                'driver': iotests.imgfmt,
+                'file': {
+                    'driver': 'file',
+                    'filename': imgs[i - 1]
+                }
+            }
+            qemu_img_create('-f', iotests.imgfmt, '-F', iotests.imgfmt,
+                            '-b', 'json:' + json.dumps(backing),
+                            imgs[i], str(image_size))
+
+    def tearDown(self) -> None:
+        if self.vm_s is not None:
+            self.vm_s.shutdown()
+        if self.vm_d is not None:
+            self.vm_d.shutdown()
+
+        for img in imgs:
+            try:
+                os.remove(img)
+            except OSError:
+                pass
+        try:
+            os.remove(mig_sock)
+        except OSError:
+            pass
+
+    def test_migration(self) -> None:
+        """
+        Migrate a VM with the backing chain created in setUp() attached.  At
+        the end of the migration process, the destination will run
+        bdrv_invalidate_cache(), which for some image formats (qcow2 and qed)
+        means the backing file string is re-read from the image header.  If
+        this overwrites bs->auto_backing_file, doing so may cause
+        bdrv_backing_overridden() to become true: The image header reports a
+        json:{} filename, but when opening it, bdrv_refresh_filename() will
+        simplify it to a plain simple filename; and when bs->auto_backing_file
+        and bs->backing->bs->filename differ, bdrv_backing_overridden() becomes
+        true.
+        If bdrv_backing_overridden() is true, the BDS will be forced to get a
+        json:{} filename, which in general is not the end of the world, but not
+        great.  Check whether that happens, i.e. whether migration changes the
+        node's filename.
+        """
+
+        blockdev = {
+            'node-name': 'node0',
+            'driver': iotests.imgfmt,
+            'file': {
+                'driver': 'file',
+                'filename': imgs[2]
+            }
+        }
+
+        self.vm_s = iotests.VM(path_suffix='a') \
+                           .add_blockdev(json.dumps(blockdev))
+        self.vm_d = iotests.VM(path_suffix='b') \
+                           .add_blockdev(json.dumps(blockdev)) \
+                           .add_incoming(f'unix:{mig_sock}')
+
+        assert self.vm_s is not None
+        assert self.vm_d is not None
+
+        self.vm_s.launch()
+        self.vm_d.launch()
+
+        pre_mig_filename = self.vm_s.node_info('node0')['file']
+
+        self.vm_s.qmp('migrate', uri=f'unix:{mig_sock}')
+
+        # Wait for migration to be done
+        self.vm_s.event_wait('STOP')
+        self.vm_d.event_wait('RESUME')
+
+        post_mig_filename = self.vm_d.node_info('node0')['file']
+
+        # Verify that the filename hasn't changed from before the migration
+        self.assertEqual(pre_mig_filename, post_mig_filename)
+
+        self.vm_s.shutdown()
+        self.vm_s = None
+
+        # For good measure, try creating an overlay and check its backing
+        # chain below.  This is how the issue was originally found.
+        result = self.vm_d.qmp('blockdev-snapshot-sync',
+                               format=iotests.imgfmt,
+                               snapshot_file=imgs[3],
+                               node_name='node0',
+                               snapshot_node_name='node0-overlay')
+        self.assert_qmp(result, 'return', {})
+
+        self.vm_d.shutdown()
+        self.vm_d = None
+
+        # Check the newly created overlay's backing chain
+        chain = qemu_img_info('--backing-chain', imgs[3])
+        for index, image in enumerate(chain):
+            self.assertEqual(image['filename'], imgs[3 - index])
+
+
+if __name__ == '__main__':
+    # These are the image formats that run their open() function from their
+    # .bdrv_co_invaliate_cache() implementations, so test them
+    iotests.main(supported_fmts=['qcow2', 'qed'],
+                 supported_protocols=['file'])
diff --git a/tests/qemu-iotests/tests/backing-file-invalidation.out b/tests/qemu-iotests/tests/backing-file-invalidation.out
new file mode 100644
index 0000000000..ae1213e6f8
--- /dev/null
+++ b/tests/qemu-iotests/tests/backing-file-invalidation.out
@@ -0,0 +1,5 @@
+.
+----------------------------------------------------------------------
+Ran 1 tests
+
+OK
-- 
2.36.1

Re: [PATCH 0/3] block: Keep auto_backing_file post-migration

[Kevin Wolf]

Am 03.08.2022 um 16:44 hat Hanna Reitz geschrieben:
> Hi,
> 
> https://gitlab.com/qemu-project/qemu/-/issues/1117 reports the following
> issue:
> 
> Say you have a VM with a backing chain of images where the image
> metadata contains json:{} backing file strings, which however will be
> resolved to simple plain filenames when opened[1].
> 
> So when these images are opened, bs->auto_backing_file is first read
> directly from the image header, and will thus contain a json:{}
> filename.  The backing image is opened based off of this filename, and
> bdrv_refresh_filename() simplfies the filename as shown[1].  We then
> update bs->auto_backing_file from bs->backing->bs->filename, so both are
> equal.
> 
> It is quite important that both are equal, because
> bdrv_backing_overridden() checks whether the backing file has been
> changed from the default by comparing bs->auto_backing_file to
> bs->backing->bs->filename.
> 
> Because we did set bs->auto_backing_file from bs->backing->bs->filename,
> both are equal, the backing file is not considered overridden, and
> bdrv_refresh_filename(bs) will not consider it necessary to generate a
> json:{} filename for the overlay.
> 
> Then the VM is migrated.
> 
> The destination side invokes bdrv_invalidate_cache(), which by qcow2 and
> qed is implemented by closing the image and opening it.  This re-reads
> the backing file string from disk, resetting bs->auto_backing_file.
> Now, it will contains the json:{} filename again and thus differ from
> bs->backing->bs->filename.
> 
> Consequentially, a subsequent bdrv_refresh_filename(bs) will find that
> the overlay’s backing file has been overridden and generate a json:{}
> filename, which isn’t great.
> 
> This series fixes that by having qcow2’s and qed’s image-open operations
> not overwrite bs->auto_backing_file unless something has changed since
> the last time we read the backing filename from the metadata.
> 
> 
> Now, generating a json:{} filename can be a nuisance but shouldn’t be a
> real problem.  The actual problem reported in 1117 comes later, namely
> when creating a snapshot overlay post-migration.  This overlay image
> will have a json:{} backing filename in its image metadata, which
> contains a 'backing' key[2].
> 
> 'qemu-img info' uses the BDRV_O_NO_BACKING flag to open images, which
> conflicts with those backing options: With that flag, nobody processes
> those options, and that’s an error.  Therefore, you can’t run 'qemu-img
> info --backing-chain' on that overlay image.
> 
> That part of the issue is not fixed in this series, however.  I’ll send
> a separate RFC series for it, because I’m honstly not quite certain how
> it should be fixed.
> 
> 
> [1] Example:
>         json:{"driver": "qcow2",
>               "file": {"driver": "file", "filename": "img.qcow2"}}
>     Will generally be “resolved” by bdrv_refresh_filename() to
>         "img.qcow2"
> 
> [2] That it contains a 'backing' key is only natural, because the reason
>     why bdrv_refresh_filename() decided to generate a json:{} filename
>     for the image is because it considered the backing file overridden.
>     Hence it must put the actual backing file options into a 'backing'
>     object in the json:{} filename.
> 
> 
> Hanna Reitz (3):
>   block/qcow2: Keep auto_backing_file if possible
>   block/qed: Keep auto_backing_file if possible
>   iotests/backing-file-invalidation: Add new test

Thanks, applied to the block branch.

Kevin