Re: [PATCH v3 3/6] block/qcow2: introduce inflight writes counters: fix discard

[Max Reitz <mreitz@redhat.com>]

On 12.03.21 13:42, Vladimir Sementsov-Ogievskiy wrote:
> 12.03.2021 15:32, Vladimir Sementsov-Ogievskiy wrote:
>> 12.03.2021 14:17, Max Reitz wrote:
>>> On 12.03.21 10:09, Vladimir Sementsov-Ogievskiy wrote:
>>>> 11.03.2021 22:58, Max Reitz wrote:
>>>>> On 05.03.21 18:35, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> There is a bug in qcow2: host cluster can be discarded (refcount
>>>>>> becomes 0) and reused during data write. In this case data write may
>>
>> [..]
>>
>>>>>> @@ -885,6 +1019,13 @@ static int QEMU_WARN_UNUSED_RESULT 
>>>>>> update_refcount(BlockDriverState *bs,
>>>>>>           if (refcount == 0) {
>>>>>>               void *table;
>>>>>> +            Qcow2InFlightRefcount *infl = find_infl_wr(s, 
>>>>>> cluster_index);
>>>>>> +
>>>>>> +            if (infl) {
>>>>>> +                infl->refcount_zero = true;
>>>>>> +                infl->type = type;
>>>>>> +                continue;
>>>>>> +            }
>>>>>
>>>>> I don’t understand what this is supposed to do exactly.  It seems 
>>>>> like it wants to keep metadata structures in the cache that are 
>>>>> still in use (because dropping them from the caches is what happens 
>>>>> next), but users of metadata structures won’t set in-flight 
>>>>> counters for those metadata structures, will they?
>>>>
>>>> Don't follow.
>>>>
>>>> We want the code in "if (refcount == 0)" to be triggered only when 
>>>> full reference count of the host cluster becomes 0, including 
>>>> inflight-write-cnt. So, if at this point inflight-write-cnt is not 
>>>> 0, we postpone freeing the host cluster, it will be done later from 
>>>> "slow path" in update_inflight_write_cnt().
>>>
>>> But the code under “if (refcount == 0)” doesn’t free anything, does 
>>> it?  All I can see is code to remove metadata structures from the 
>>> metadata caches (if the discarded cluster was an L2 table or a 
>>> refblock), and finally the discard on the underlying file.  I don’t 
>>> see how that protocol-level discard has anything to do with our 
>>> problem, though.
>>
>> Hmm. Still, if we do this discard, and then our in-flight write, we'll 
>> have data instead of a hole. Not a big deal, but seems better to 
>> postpone discard.
>>
>> On the other hand, clearing caches is OK, as its related only to 
>> qcow2-refcount, not to inflight-write-cnt
>>
>>>
>>> As far as I understand, the freeing happens immediately above the “if 
>>> (refcount == 0)” block by s->set_refcount() setting the refcount to 
>>> 0. (including updating s->free_cluster_index if the refcount is 0).
>>
>> Hmm.. And that (setting s->free_cluster_index) what I should actually 
>> prevent until total reference count becomes zero.
>>
>> And about s->set_refcount(): it only update a refcount itself, and 
>> don't free anything.
>>
>>
> 
> 
> Looking now at this place:
> 
> 
>          if (refcount == 0 && cluster_index < s->free_cluster_index) {
>              s->free_cluster_index = cluster_index;
>          }
>          s->set_refcount(refcount_block, block_index, refcount);
>          if (refcount == 0) {
>              void *table;
>              Qcow2InFlightRefcount *infl = find_infl_wr(s, cluster_index);
>              if (infl) {
>                  infl->refcount_zero = true;
>                  infl->type = type;
>                  continue;
>              }
>              table = qcow2_cache_is_table_offset(s->refcount_block_cache,
>                                                  offset);
>              if (table != NULL) {
>                  qcow2_cache_put(s->refcount_block_cache, &refcount_block);
>                  old_table_index = -1;
>                  qcow2_cache_discard(s->refcount_block_cache, table);
>              }
>              table = qcow2_cache_is_table_offset(s->l2_table_cache, 
> offset);
>              if (table != NULL) {
>                  qcow2_cache_discard(s->l2_table_cache, table);
>              }
>              if (s->compressed_cache) {
>                  seqcache_discard_cluster(s->compressed_cache, 
> cluster_offset);
>              }
>              if (s->discard_passthrough[type]) {
>                  update_refcount_discard(bs, cluster_offset, 
> s->cluster_size);
>              }
>          }
> 
> 
> Hmm. Is it OK that we use "offset" to discard qcow2 metadata caches? 
> offset is the start of the whole loop, and is not updated on iterations. 
> Isn't it more correct to use cluster_offset here? Or we are sure that 
> refcount and l2 metadata is always discarded by exactly one cluster? 
> Than we are OK, but still worth an assertion that offset == cluster_offset.

Spontaneously, I think it’s a bug that hasn’t made any problems yet, 
because I suppose L2 tables and refblocks are indeed always discarded 
one by one (i.e., cluster by cluster).  It definitely looks like this 
should be cluster_offset, yes.

Max