Re: [Qemu-block] [PATCH v2 2/6] block/dirty-bitmaps: rename frozen predicate helper

[John Snow]

On 2/19/19 5:17 AM, Vladimir Sementsov-Ogievskiy wrote:
> 19.02.2019 1:32, John Snow wrote:
>>
>>
>> On 2/18/19 8:57 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> 14.02.2019 2:23, John Snow wrote:
>>>> "Frozen" was a good description a long time ago, but it isn't adequate now.
>>>> Rename the frozen predicate to has_successor to make the semantics of the
>>>> predicate more clear to outside callers.
>>>>
>>>> In the process, remove some calls to frozen() that no longer semantically
>>>> make sense. For enabled and disabled in particular, it's actually okay for
>>>> the internals to do this but only forbidden for users to invoke them, and
>>>
>>> I'm a bit lost in this paragraph.. to this - what?, to invoke them - whom?
>>> I think, it would be simpler for me to read patch itself :)
>>>
>>
>> Touched this up. I meant enable and disable, not enabled and disabled.
>>
>>>> all of the QMP entry uses already check against qmp_locked.
>>>>
>>>> Several other assertions really want to check that the bitmap isn't in-use
>>>> by another operation -- use the qmp_locked function for this instead, which
>>>> presently also checks for has_successor.
>>>
>>> hm, you mean user_locked, not qmp_locked.
>>>
>>
>> Yes.
>>
>> [...]
>>
>>>>    /**
>>>>     * Create a successor bitmap destined to replace this bitmap after an 
>>>> operation.
>>>> - * Requires that the bitmap is not frozen and has no successor.
>>>> + * Requires that the bitmap is not locked and has no successor.
>>>
>>> I think, user_locked, to not interfere with bitmaps mutex. And you use 
>>> user_locked in
>>> other comments in this patch.
>>>
>>
>> You're right. It gets changed again later, but I didn't make this easy
>> to read.
>>
>>>>     * Called with BQL taken.
>>>>     */
>>>>    int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
>>>> @@ -244,12 +244,16 @@ int 
>>>> bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
>>>>        uint64_t granularity;
>>>>        BdrvDirtyBitmap *child;
>>>>    
>>>> -    if (bdrv_dirty_bitmap_frozen(bitmap)) {
>>>> -        error_setg(errp, "Cannot create a successor for a bitmap that is "
>>>> -                   "currently frozen");
>>>> +    if (bdrv_dirty_bitmap_user_locked(bitmap)) {
>>>> +        error_setg(errp, "Cannot create a successor for a bitmap that is 
>>>> in-use "
>>>> +                   "by an operation");
>>>> +        return -1;
>>>> +    }
>>>> +    if (bdrv_dirty_bitmap_has_successor(bitmap)) {
>>>> +        error_setg(errp, "Cannot create a successor for a bitmap that 
>>>> already "
>>>> +                   "has one");
>>>
>>>
>>> Amm, dead code? _user_locked() implies no successor, so we instead can keep 
>>> an assertion..
>>>
>>
>> It gets changed later in the series, but I didn't do a great job of
>> explaining that in advance. I'll amend the commit message to explain
>> what I'm trying to do.
>>
>> I tried to hint at this with: "which presently also checks for
>> has_successor" as an admission that it was redundant, but I need to call
>> it out in stronger language.
>>
> 
> Hmm, isn't it better to keep an assertion, than add dead code, to be fixed in 
> later commits?
> 

Eh. I wrote code that looked semantically correct without worrying about
what the calls actually do:

- We want to make sure this bitmap is not in use (user_locked,
qmp_locked, or busy -- however you want to spell it), and

- We want to make sure this bitmap doesn't have a successor.

Now, you and I happen to know that these two conditions aren't actually
independent, but that's not necessarily obvious from this one function
to a new reader. Adding the second check gives some assurance to the reader.

In my mind, the concept of having a successor is now distinct from that
of being busy, and create_successor actually wants to check both things:
(1) That is able to create a successor, because it doesn't have one, and
(2) That it is not modifying a bitmap in use by some operation.

But, you're right, there's no way to have a bitmap with a successor that
isn't busy, so an assertion will suffice for the instructional purpose.

I'll change it.

--js