qemu-devel
[Top][All Lists]
Advanced

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

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


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v3 3/6] block/qcow2: introduce inflight writes counters: fix discard
Date: Fri, 12 Mar 2021 15:42:42 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1

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.

--
Best regards,
Vladimir



reply via email to

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