On 18.11.25 18:06, Kevin Wolf wrote:
Am 18.11.2025 um 12:01 hat Hanna Czenczek geschrieben:
On 17.11.25 15:50, Kevin Wolf wrote:
Am 10.11.2025 um 16:48 hat Hanna Czenczek geschrieben:
+/* s_locked specifies whether s->lock is held or not */
static void qcow2_update_options_commit(BlockDriverState *bs,
- Qcow2ReopenState *r)
+ Qcow2ReopenState *r,
+ bool s_locked)
{
BDRVQcow2State *s = bs->opaque;
int i;
+ /*
+ * We need to stop the cache-clean-timer before destroying the metadata
+ * table caches
+ */
+ if (s_locked) {
+ cache_clean_timer_co_locked_del_and_wait(bs);
+ } else {
+ cache_clean_timer_del_and_wait(bs);
+ }
+
if (s->l2_table_cache) {
qcow2_cache_destroy(s->l2_table_cache);
}
@@ -1228,6 +1312,10 @@ static void qcow2_update_options_commit(BlockDriverState
*bs,
}
s->l2_table_cache = r->l2_table_cache;
s->refcount_block_cache = r->refcount_block_cache;
+
+ s->cache_clean_interval = r->cache_clean_interval;
+ cache_clean_timer_init(bs, bdrv_get_aio_context(bs));
+
s->l2_slice_size = r->l2_slice_size;
s->overlap_check = r->overlap_check;
@@ -1239,12 +1327,6 @@ static void qcow2_update_options_commit(BlockDriverState
*bs,
s->discard_no_unref = r->discard_no_unref;
- if (s->cache_clean_interval != r->cache_clean_interval) {
- cache_clean_timer_del(bs);
- s->cache_clean_interval = r->cache_clean_interval;
- cache_clean_timer_init(bs, bdrv_get_aio_context(bs));
- }
-
qapi_free_QCryptoBlockOpenOptions(s->crypto_opts);
s->crypto_opts = r->crypto_opts;
}
Is there any specific reason why you move cache_clean_timer_init()
earlier? I don't see an actual problem with the code as it is after this
change, but s->l2_slice_size is related to the cache in a way, so it
would feel safer if the cache cleaner were only started once all the
settings are updated and not only those that it actually happens to
access at the moment.
Oh. I don’t think there’s a good reason. I think it makes sense to keep
the set-up in the old place. Can you do that in your tree?
Yes, I changed it back and applied the series to my block branch.
Thanks.
Thanks a lot!
Hanna
