Re: [Qemu-devel] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitmaps

[Vladimir Sementsov-Ogievskiy]

07.10.2016 22:25, Max Reitz пишет:
> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
> > Auto loading bitmaps are bitmaps in Qcow2, with AUTO flag set. They are
> > loaded at image open and becomes BdrvDirtyBitmap's for corresponding
> > drive. These bitmaps are deleted from Qcow2 image after loading to avoid
> > conflicts.
> >
> > Extra data in bitmaps is not supported for now.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > ---
> >   block/qcow2-bitmap.c | 518 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> >   block/qcow2.c        |   5 +
> >   block/qcow2.h        |   3 +
> >   3 files changed, 525 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> > index cd18b07..760a047 100644
> > --- a/block/qcow2-bitmap.c
> > +++ b/block/qcow2-bitmap.c
> [...]
>
> > +static int directory_update(BlockDriverState *bs, uint8_t *new_dir,
> > +                            size_t new_size, uint32_t new_nb_bitmaps)
> > +{
> > +    BDRVQcow2State *s = bs->opaque;
> > +    int ret;
> > +    int64_t new_offset = 0;
> > +    uint64_t old_offset = s->bitmap_directory_offset;
> > +    uint64_t old_size = s->bitmap_directory_size;
> > +    uint32_t old_nb_bitmaps = s->nb_bitmaps;
> > +    uint64_t old_autocl = s->autoclear_features;
> > +
> > +    if (new_size > QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE) {
> > +        return -EINVAL;
> > +    }
> > +
> > +    if ((new_size == 0) != (new_nb_bitmaps == 0)) {
> > +        return -EINVAL;
> > +    }
> > +
> > +    if (new_nb_bitmaps > 0) {
> > +        new_offset = directory_write(bs, new_dir, new_size);
> > +        if (new_offset < 0) {
> > +            return new_offset;
> > +        }
> > +
> > +        ret = bdrv_flush(bs);
> > +        if (ret < 0) {
> > +            goto fail;
> > +        }
> > +    }
> > +    s->bitmap_directory_offset = new_offset;
> > +    s->bitmap_directory_size = new_size;
> > +    s->nb_bitmaps = new_nb_bitmaps;
> > +
> > +    ret = update_header_sync(bs);
> > +    if (ret < 0) {
> > +        goto fail;
> > +    }
> > +
> > +    if (old_size) {
> > +        qcow2_free_clusters(bs, old_offset, old_size, QCOW2_DISCARD_ALWAYS);
> > +    }
> > +
> > +    return 0;
> > +
> > +fail:
> > +    if (new_offset > 0) {
> > +        qcow2_free_clusters(bs, new_offset, new_size, QCOW2_DISCARD_ALWAYS);
> > +        s->bitmap_directory_offset = old_offset;
> > +        s->bitmap_directory_size = old_size;
> > +        s->nb_bitmaps = old_nb_bitmaps;
> > +        s->autoclear_features = old_autocl;
> What if bdrv_flush() in update_header_sync() failed? Then you cannot be
> sure what bitmap directory the header is actually pointing to. In that
> case, it would be wrong to assume it's still pointing to the old one and
> freeing the new one.
>
> Max

Hmm.. Good point. Any suggestions?

I'm trying to achieve some kind of transaction - if function failed file 
is not changed.. But now I understand that because of flush ability to 
fail I can't achieve this..

Has write and flush any guaranties in case of fail? Would it be 
old-or-new state, or some mixed state is possible?

> > +    }
> > +
> > +    return ret;
> > +}


--
Best regards,
Vladimir