On 2017-06-27 17:06, Pavel Butsykin wrote: > On 26.06.2017 20:47, Max Reitz wrote: >> On 2017-06-26 17:23, Pavel Butsykin wrote: > [] >>> >>> Is there any guarantee that in the future this will not change? Because >>> in this case it can be a potential danger. >> >> Since this behavior is not documented anywhere, there is no guarantee. >> >>> I can add a comment... Or add a new variable with the size of >>> reftable_tmp, and every time count min(s->refcount_table_size, >>> reftable_tmp_size) >>> before accessing to s->refcount_table[]/reftable_tmp[] >> >> Or (1) you add an assertion that refcount_table_size doesn't change >> along with a comment why that is the case, which also explains in detail >> why the call to qcow2_free_clusters() should be safe: The on-disk >> reftable differs from the one in memory. qcow2_free_clusters()and >> update_refcount() themselves do not access the reftable, so they are >> safe. However, update_refcount() calls alloc_refcount_block(), and that >> function does access the reftable: Now, as long as >> s->refcount_table_size does not shrink (which I can't see why it would), >> refcount_table_index should always be smaller. Now we're accessing >> s->refcount_table: This will always return an existing refblock because >> this will either be the refblock itself (for self-referencing refblocks) >> or another one that is not going to be freed by qcow2_shrink_reftable() >> because this function will not free refblocks which cover other clusters >> than themselves. >> We will then proceed to update the refblock which is either right (if it >> is not the refblock to be freed) or won't do anything (if it is the one >> to be freed). >> In any case, we will never write to the reftable and reading from the >> basically outdated cached version will never do anything bad. > > OK, SGTM. > >> Or (2) you copy reftable_tmp into s->refcount_table[] *before* any call >> to qcow2_free_clusters(). To make this work, you would need to also >> discard all refblocks from the cache in this function here (and not in >> update_refcount()) and then only call qcow2_free_clusters() on refblocks >> which were not self-referencing. An alternative hack would be to simply >> mark the image dirty and just not do any qcow2_free_clusters() call... > > The main purpose of qcow2_reftable_shrink() function is discard all > unnecessary refblocks from the file. If we do only rewrite > refcount_table and discard non-self-referencing refblocks (which are > actually very rare), then the meaning of the function is lost.
It would do exactly the same. The idea is that you do not need to call qcow2_free_clusters() on self-referencing refblocks at all, since they are freed automatically when their reftable entry is overwritten with 0. >> Or (3) of course it would be possible to not clean up refcount >> structures at all... > > Nice solution :) It is, because as I said refcount structures only have a small overhead. Max
signature.asc
Description: OpenPGP digital signature