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 <pbonz...@redhat.com> > >> 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 >