[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH v15 09/25] qcow2: add .bdrv_load_autoloading_dir

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [Qemu-block] [PATCH v15 09/25] qcow2: add .bdrv_load_autoloading_dirty_bitmaps
Date: Fri, 17 Feb 2017 15:07:30 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1

16.02.2017 15:47, Kevin Wolf wrote:
Sorry, this was sent too early. Next attempt...

Am 16.02.2017 um 12:45 hat Kevin Wolf geschrieben:
Am 15.02.2017 um 11:10 hat Vladimir Sementsov-Ogievskiy geschrieben:
Auto loading bitmaps are bitmaps in Qcow2, with the AUTO flag set. They
are loaded when the image is opened and become BdrvDirtyBitmaps for the
corresponding drive.

Extra data in bitmaps is not supported for now.


hdx.o vhdx-endian.o vhdx-log.o
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
new file mode 100644
index 0000000..e08e46e
--- /dev/null
+++ b/block/qcow2-bitmap.c


+static int update_header_sync(BlockDriverState *bs)
+    int ret;
+    ret = qcow2_update_header(bs);
+    if (ret < 0) {
+        return ret;
+    }
+    /* We don't return bdrv_flush error code. Even if it fails, write was
+     * successful and it is more logical to consider that header is in the new
+     * state than in the old.
+     */
+    ret = bdrv_flush(bs);
+    if (ret < 0) {
+        fprintf(stderr, "Failed to flush qcow2 header");
+    }
I don't think I can agree with this one. If you don't care whether the
new header is actually on disk, don't call bdrv_flush(). But if you do
care, then bdrv_flush() failure means that most likely the new header
has not made it to the disk, but is just sitting in some volatile cache.

And what should be done on bdrv_flush fail? Current solution was proposed by Max.

+    return 0;


+/* This function returns the number of disk sectors covered by a single cluster
+ * of bitmap data. */
+static uint64_t disk_sectors_in_bitmap_cluster(const BDRVQcow2State *s,
+                                               const BdrvDirtyBitmap *bitmap)
+    uint32_t sector_granularity =
+            bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS;
+    return (uint64_t)sector_granularity * (s->cluster_size << 3);
This has nothing to do with disk sectors, neither of the guest disk nor
of the host disk. It's just using a funny 512 bytes unit. Is there a
good reason for introducing this funny unit in new code?

I'm also not sure what this function calculates, but it's not what the
comment says. The unit of the result is something like sectors * bytes,
and even when normalising it to a single base unit, I've never seen a
use for square bytes so far.

sector granularity is number of disk sectors, corresponding to one bit of the dirty bitmap, (disk-sectors/bitmap-bit)

cluster_size << 3 is a number of bits in one cluster, (bitmap-bit)

so, we have
sector_granularity (disk-sector/bitmap-bit) * <cluster size in bits> (bitmapbit) = some disk sectors, corresponding to one cluster of bitmap data.

Best regards,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]