volume: Be more careful with create=False

Recently we started to use create=False when converting images to block
storage. This was needed as a workaround for qemu-img bug that is now
fixed.

Then we found that qemu-img preallocation is inefficient and cause
trouble with legacy NFS storage, and now we preallocate images with out
fallocate helper.

These changes revealed the fact that we always create the target image
when running qemuimg.convert(). This does not make sense for our use
case, so we switch to create=False for all cases.

Unfortunately, this does not work. We have 2 cases that require
create=True:

1. Raw sparse images when the file system does not support punching
   holes. When qemu-img convert is trying to punch holes in unallocated
   areas it falls back to writing zeroes, which is very slow, and fully
   allocates sparse images.

2. qcow2 with compat=0.10 when volume does not have a parent. Since this
   older qcow2 format does not support zero clusters, qemu-img fall back
   to writing zeroes which is very slow and allocates the entire image.

When qemu-img convert creates a new image, it knows that the image is
zeroed so it can skip unallocated areas.

Here are few examples showing the problem cases:

Coping sparse image to NFS 3

    $ truncate -s 10g /var/tmp/src.raw
    $ truncate -s 10g dst.raw

    $ time qemu-img convert -f raw -O raw -t none -T none -n /var/tmp/src.raw dst.raw

    real    0m50.684s
    user    0m0.034s
    sys     0m0.711s

    $ du -sh dst.raw
    10G     dst.raw

The image became fully allocated and the operation was very slow. If
we use create=True:

    $ time qemu-img convert -f raw -O raw -t none -T none /var/tmp/src.raw dst.raw

    real    0m0.222s
    user    0m0.005s
    sys     0m0.003s

    $ du -sh dst.raw
    4.0K    dst.raw

More correct and 250 times faster.

Copying image to qcow2 compat=0.10:

    $ qemu-img create -f qcow2 -o compat=0.10 dst.qcow2 10g

    $ time qemu-img convert -f raw -O qcow2 -t none -T none -n /var/tmp/src.raw dst.qcow2

    real    0m58.734s
    user    0m0.049s
    sys     0m0.673s

    # du -sh dst.qcow2
    11G     dst.qcow2

Again the image was fully allocated, slowly. If we use create=True:

    $ time qemu-img convert -f raw -O qcow2 -t none -T none /var/tmp/src.raw dst.qcow2

    real    0m0.224s
    user    0m0.003s
    sys     0m0.006s

    $ du -sh dst.qcow2
    196K    dst.qcow2

The creation logic is needed in the 3 callers of qemuimg.convert(), and
is mostly about the volume properties. Add Volume.requires_create()
method returning True if using the volume as target image in
qemuimg.convert() requires create=True.

The issue with qcow2 compat=0.10 format is fixed upstream. Once
qemu-5.1.0 is available we can remove this check and handle only raw
sparse images.

Change-Id: Ia456006262376d2cc6174a7849133b77b2322a4a
Bug-Url: https://bugzilla.redhat.com/1850267
Related-To: https://bugzilla.redhat.com/1858632
Signed-off-by: Nir Soffer <nsoffer@redhat.com>

lib/vdsm/storage/image.py

@@ -441,18 +441,22 @@ def _interImagesCopy(self, destDom, srcSdUUID, imgUUID, chains):
                     if parentVol is not None:
                         backing = volume.getBackingVolumePath(
                             imgUUID, parentVol.volUUID)
+                        backingFormat = sc.fmt2str(parentVol.getFormat())
                     else:
                         backing = None
+                        backingFormat = None
 
                     operation = qemuimg.convert(
                         srcVol.getVolumePath(),
                         dstVol.getVolumePath(),
                         srcFormat=srcFormat,
                         dstFormat=dstFormat,
+                        dstQcow2Compat=destDom.qcow2_compat(),
                         backing=backing,
+                        backingFormat=backingFormat,
                         unordered_writes=destDom.recommends_unordered_writes(
                             dstVol.getFormat()),
-                        create=False,
+                        create=dstVol.requires_create(),
                     )
                     with utils.stopwatch(
                             "Copy volume {}".format(srcVol.volUUID),
@@ -735,9 +739,10 @@ def copyCollapsed(self, sdUUID, vmUUID, srcImgUUID, srcVolUUID, dstImgUUID,
                         dstVol.getVolumePath(),
                         srcFormat=sc.fmt2str(volParams['volFormat']),
                         dstFormat=sc.fmt2str(dstVolFormat),
+                        dstQcow2Compat=destDom.qcow2_compat(),
                         unordered_writes=destDom.recommends_unordered_writes(
                             dstVolFormat),
-                        create=False,
+                        create=dstVol.requires_create(),
                     )
                     with utils.stopwatch(
                             "Copy volume {}".format(srcVol.volUUID),

lib/vdsm/storage/sdm/api/copy_data.py

@@ -79,10 +79,12 @@ def _run(self):
                         self._dest.path,
                         srcFormat=src_format,
                         dstFormat=dst_format,
+                        dstQcow2Compat=self._dest.qcow2_compat,
                         backing=self._dest.backing_path,
+                        backingFormat=self._dest.backing_qemu_format,
                         unordered_writes=self._dest
                             .recommends_unordered_writes,
-                        create=False)
+                        create=self._dest.requires_create)
                     with utils.stopwatch(
                             "Copy volume {}".format(self._source.path),
                             level=logging.INFO,
@@ -145,11 +147,27 @@ def backing_path(self):
             return None
         return volume.getBackingVolumePath(self.img_id, parent_vol.volUUID)
 
+    @property
+    def qcow2_compat(self):
+        dom = sdCache.produce_manifest(self.sd_id)
+        return dom.qcow2_compat()
+
+    @property
+    def backing_qemu_format(self):
+        parent_vol = self.volume.getParentVolume()
+        if not parent_vol:
+            return None
+        return sc.fmt2str(parent_vol.getFormat())
+
     @property
     def recommends_unordered_writes(self):
         dom = sdCache.produce_manifest(self.sd_id)
         return dom.recommends_unordered_writes(self.volume.getFormat())
 
+    @property
+    def requires_create(self):
+        return self.volume.requires_create()
+
     @property
     def volume(self):
         if self._vol is None:

lib/vdsm/storage/volume.py

@@ -798,6 +798,38 @@ def setParentTag(self, puuid):
     def getMetaSlot(self):
         raise NotImplementedError
 
+    # Copy volume helpers.
+
+    def requires_create(self):
+        """
+        Return True if we need to use qemuimg.convert(create=True) when this
+        volume is the target image.
+
+        We have 2 cases:
+
+        1. Raw sparse image on filesystem not supporting punching holes (e.g.
+           NFS < 4.2). qemu-img convert will fully allocate the entire image
+           when trying to punch holes in the unallocated areas.
+
+        2. qcow2 compat=0.10 when volume does not have a parent. qemu-img
+           convert will fully allocate the image instead of skipping the
+           unallocated areas.
+           TODO: Remove when qemu-5.1.0 is available.
+           https://bugzilla.redhat.com/1858632
+
+        When qemu-img convert creates the target image it knows that the image
+        is zeroed so it can skip the unallocated areas.
+        """
+        if self.getFormat() == sc.RAW_FORMAT:
+            return self.isSparse()
+        else:
+            puuid = self.getParent()
+            if puuid and puuid != sc.BLANK_UUID:
+                return False
+
+            dom = sdCache.produce(self.sdUUID)
+            return dom.qcow2_compat() == "0.10"
+
 
 class Volume(object):
     log = logging.getLogger('storage.Volume')
@@ -1486,6 +1518,9 @@ def setParentMeta(self, puuid):
     def setParentTag(self, puuid):
         raise NotImplementedError
 
+    def requires_create(self):
+        return self._manifest.requires_create()
+
 
 class VolumeLease(guarded.AbstractLock):
     """

tests/storage/sdm_copy_data_test.py

@@ -45,7 +45,6 @@
     write_qemu_chain,
 )
 
-import testing
 from . import qemuio
 from . import userstorage
 
@@ -70,10 +69,6 @@
 DEFAULT_SIZE = MiB
 
 
-xfail_block = pytest.mark.xfail(
-    reason="Doesn't work when -n flag is used by qemu-img")
-
-
 @pytest.fixture(
     scope="module",
     params=[
@@ -272,18 +267,8 @@ def test_wrong_generation(self):
 @pytest.mark.parametrize("env_type,qcow2_compat,sd_version", [
     # env_type, src_compat, sd_version
     # Old storage domain, we supported only 0.10
-    pytest.param(
-        'file', '0.10', 3,
-        marks=pytest.mark.xfail(
-            reason="qemu-img allocates entire image",
-            # Test passes on oVirt CI.
-            strict=not testing.on_ovirt_ci())),
-    pytest.param(
-        'block', '0.10', 3,
-        marks=pytest.mark.xfail(
-            reason="qemu-img allocates entire image",
-            # Test passes on oVirt CI.
-            strict=not testing.on_ovirt_ci())),
+    ('file', '0.10', 3),
+    ('block', '0.10', 3),
     # New domain old volume
     ('file', '0.10', 4),
     ('block', '0.10', 4),
@@ -333,7 +318,7 @@ def test_qcow2_compat(
     pytest.param('file', 'cow', 'raw'),
     pytest.param('file', 'cow', 'cow'),
     pytest.param('block', 'raw', 'raw'),
-    pytest.param('block', 'raw', 'cow', marks=xfail_block),
+    pytest.param('block', 'raw', 'cow'),
     pytest.param('block', 'cow', 'raw'),
     pytest.param('block', 'cow', 'cow'),
 ])
@@ -368,13 +353,8 @@ def test_intra_domain_copy(env_type, src_fmt, dst_fmt):
 
 
 @pytest.mark.parametrize("dest_format,sd_version", [
-    (sc.COW_FORMAT, 5),    # compat=1.1.
-    pytest.param(
-        sc.COW_FORMAT, 3,  # compat=0.10.
-        marks=pytest.mark.xfail(
-            reason="qemu-img allocates entire image",
-            # Test passes on oVirt CI.
-            strict=not testing.on_ovirt_ci())),
+    (sc.COW_FORMAT, 5),  # compat=1.1.
+    (sc.COW_FORMAT, 3),  # compat=0.10.
     (sc.RAW_FORMAT, 5),
 ])
 def test_copy_data_collapse(

tests/storage/storagetestlib.py

@@ -237,6 +237,9 @@ def extendVolume(self, volumeUUID, size, isShuttingDown=None):
         if self.lvm:
             self.lvm.extendLV(self._manifest.sdUUID, volumeUUID, size)
 
+    def qcow2_compat(self):
+        return self._manifest.qcow2_compat()
+
 
 def make_sd_metadata(sduuid, version=3, dom_class=sd.DATA_DOMAIN, pools=None):
     md = FakeMetadata()