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

[Hanna Czenczek <hreitz@redhat.com>]

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