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:55 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 17.02.2017 15:21, Kevin Wolf wrote:
> 
>     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.
> 
> 
> It was my first option. The problem is that resulting state is absolutely
> inconsistent - it is not guaranteed  to be the old one or new.. However with
> current approach it may be inconsistent too, but without an error..
> 
> So, error message should contain information that all dirty bitmaps may be
> inconsistent even if in the image all flags says that bitmaps are OK.

After a failing flush, we're always in a state that is hard to predict
and where we can make little guarantees about what the image actually
contains on disk and what not. The only thing we can really do is to
make sure the user sees an error and can respond to it, rather than
being silent about a potential corruption.

>                     +    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.
> 
> 
> No problem with that, I'm always for better wording)
> 
> s/containing/of/ looks better for me, as it should be covered by bitmap data,
> not by cluster.

That works for me, too.

Kevin