[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/5] block: convert bdrv_invalidate_cache callba
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH 3/5] block: convert bdrv_invalidate_cache callback to coroutine_fn |
Date: |
Tue, 11 Jul 2017 10:33:42 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 |
On 11/07/2017 10:24, Kevin Wolf wrote:
> Am 10.07.2017 um 18:58 hat Paolo Bonzini geschrieben:
>> QED's bdrv_invalidate_cache implementation would like to reuse functions
>> that acquire/release the metadata locks. Call it from coroutine context
>> to simplify the logic.
>>
>> Signed-off-by: Paolo Bonzini <address@hidden>
>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index b5de67d113..4ff7e89009 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1854,7 +1854,8 @@ static void qcow2_close(BlockDriverState *bs)
>> qcow2_free_snapshots(bs);
>> }
>>
>> -static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
>> +static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
>> + Error **errp)
>> {
>> BDRVQcow2State *s = bs->opaque;
>> int flags = s->flags;
>> @@ -1877,7 +1878,9 @@ static void qcow2_invalidate_cache(BlockDriverState
>> *bs, Error **errp)
>> options = qdict_clone_shallow(bs->options);
>>
>> flags &= ~BDRV_O_INACTIVE;
>> + qemu_co_mutex_lock(&s->lock);
>> ret = qcow2_do_open(bs, options, flags, &local_err);
>> + qemu_co_mutex_unlock(&s->lock);
>> QDECREF(options);
>> if (local_err) {
>> error_propagate(errp, local_err);
>
> How useful is this lock really? If we expect any requests while this
> function is running (we don't, it would break horribly), it should
> probably cover qcow2_close(), too.
In the case of QED, this was needed because qed_read_table drops
s->table_lock. Here I just copied the idiom.
However, I think it's cleaner to define which functions are called with
which lock held. Sometimes it leads to taking a lock unnecessarily, but
it's clearer and it can also help once we annotate the source and let
the compiler do static analysis (see clang's -Wthread-safety).
Paolo
> Or maybe the cleaner way would be bdrv_drained_begin/end() around
> everything directly in bdrv_co_invalidate_cache().
>
> Kevin
>
[Qemu-devel] [PATCH 3/5] block: convert bdrv_invalidate_cache callback to coroutine_fn, Paolo Bonzini, 2017/07/10
[Qemu-devel] [PATCH 4/5] qcow2: introduce qcow2_write_caches and qcow2_flush_caches, Paolo Bonzini, 2017/07/10
[Qemu-devel] [PATCH 2/5] qed: make bdrv_qed_do_open a coroutine_fn, Paolo Bonzini, 2017/07/10
[Qemu-devel] [PATCH 5/5] block: convert bdrv_check callback to coroutine_fn, Paolo Bonzini, 2017/07/10