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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3943 bytes --]

On 21.10.2016 13:59, Vladimir Sementsov-Ogievskiy wrote:
> 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 <vsementsov@virtuozzo.com>
>>> ---
>>>   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?

Anything is possible, I think. However, if flushing the data fails, it
still means writing it was successful -- you just don't know whether it
has been written to the disk successfully.

The most correct way out would be to signal image corruption, but that
isn't what we should do, I think.

If the write operation was successful but flushing was not, I'd always
assume the data hopefully gets flushed later and continue as if it had
been written to disk successfully. Therefore, I think it would be best
for update_header_sync() to just ignore bdrv_flush()'s return value.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]