qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [Qemu-devel] [PATCH v5 03/11] blockdev: n-ary bitmap me


From: John Snow
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH v5 03/11] blockdev: n-ary bitmap merge
Date: Thu, 20 Dec 2018 16:03:19 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.1


On 12/19/18 9:48 PM, Eric Blake wrote:
> On 12/19/18 8:29 PM, John Snow wrote:
>> Especially outside of transactions, it is helpful to provide
>> all-or-nothing semantics for bitmap merges. This facilitates
>> the coalescing of multiple bitmaps into a single target for
>> the "checkpoint" interpretation when assembling bitmaps that
>> represent arbitrary points in time from component bitmaps.
>>
>> This is an incompatible change from the preliminary version
>> of the API.
> 
> but that doesn't matter because it was in the x- namespace, and we're
> about to rename it anyway.
> 

Yes, just an "FYI".

>>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>>   blockdev.c           | 75 ++++++++++++++++++++++++++++++--------------
>>   qapi/block-core.json | 22 ++++++-------
>>   2 files changed, 62 insertions(+), 35 deletions(-)
>>
> 
>> +static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(const char *node,
>> +                                                    const char *target,
>> +                                                    strList *bitmaps,
>> +                                                    HBitmap **backup,
>> +                                                    Error **errp)
>>   {
> 
>> -    bdrv_merge_dirty_bitmap(dst, src, NULL, errp);
>> +    for (lst = bitmaps; lst; lst = lst->next) {
>> +        src = bdrv_find_dirty_bitmap(bs, lst->value);
>> +        if (!src) {
>> +            error_setg(errp, "Dirty bitmap '%s' not found", lst->value);
>> +            dst = NULL;
>> +            goto out;
>> +        }
>> +
>> +        bdrv_merge_dirty_bitmap(anon, src, NULL, &local_err);
>> +        if (local_err) {
>> +            error_propagate(errp, local_err);
>> +            dst = NULL;
>> +            goto out;
>> +        }
>> +    }
> 
> Appears to be a silent no-op when given "bitmaps":[] as the source.  An
> alternative would be requiring at least one source in the list, but I
> don't see it as worth changing the patch to special-case an empty list
> differently from a no-op.
> >> @@ -1943,23 +1943,23 @@
>>   ##
>>   # @x-block-dirty-bitmap-merge:
>>   #
>> -# FIXME: Rename @src_name and @dst_name to src-name and dst-name.
>> -#
>> -# Merge @src_name dirty bitmap to @dst_name dirty bitmap. @src_name
>> dirty
>> -# bitmap is unchanged. On error, @dst_name is unchanged.
>> +# Merge dirty bitmaps listed in @bitmaps to the @target dirty bitmap.
>> +# The @bitmaps dirty bitmaps are unchanged.
>> +# On error, @target is unchanged.
>>   #
>>   # Returns: nothing on success
>>   #          If @node is not a valid block device, DeviceNotFound
>> -#          If @dst_name or @src_name is not found, GenericError
>> -#          If bitmaps has different sizes or granularities, GenericError
>> +#          If any bitmap in @bitmaps or @target is not found,
>> GenericError
>> +#          If any of the bitmaps have different sizes or granularities,
>> +#              GenericError
>>   #
>>   # Since: 3.0
> 
> Could do s/3.0/4.0/ to match the incompatible change here, but you do it
> in the later patch where your remove the x-.
> 
> Reviewed-by: Eric Blake <address@hidden>
> 

Yeah, I think I'll just leave it this way, so all the version
graduations are in the same patch.

Thank you!



reply via email to

[Prev in Thread] Current Thread [Next in Thread]