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

[Kevin Wolf <kwolf@redhat.com>]

Am 17.02.2017 um 13:07 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 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.

Pass an error up and let the calling operation fail, because we can't
promise that it actually executed correctly.

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

Please don't use the word "disk sectors", it's misleading because it's
not a sector size of any specific disk. It's best to say just "sectors",
which is a fixed qemu block layer unit of 512 bytes.

> 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.

Aha! I completely misunderstood what a bitmap cluster is supposed to
be. I expected it to be a chunk of bitmap data whose size corresponds to
the bitmap granularity, whereas it's really about qcow2 clusters.

I wonder if we can rephrase the comment to make this more obvious. Maybe
"...covered by a single qcow2 cluster containing bitmap data"? And the
function could be called sectors_covered_by_bitmap_cluster() or
something.

Kevin