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.
Kevin
