Re: [PATCH v2 12/19] qcow2: Fix cache_clean_timer

[Kevin Wolf <kwolf@redhat.com>]

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