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

[Vladimir Sementsov-Ogievskiy]

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,
Vladimir