On Wed, May 10, 2023 at 10:35:54PM +0200, Kevin Wolf wrote: > > These are functions that modify the graph, so they must be able to take > a writer lock. This is impossible if they already hold the reader lock. > If they need a reader lock for some of their operations, they should > take it internally. > > Many of them go through blk_*(), which will always take the lock itself. > Direct calls of bdrv_*() need to take the reader lock. Note that while > locking for bdrv_co_*() calls is checked by TSA, this is not the case > for the mixed_coroutine_fns bdrv_*(). Holding the lock is still required > when they are called from coroutine context like here! > > This effectively reverts 4ec8df0183, but adds some internal locking > instead. > > Signed-off-by: Kevin Wolf <kw...@redhat.com> > ---
> +++ b/block/qcow2.c > -static int coroutine_fn > +static int coroutine_fn GRAPH_UNLOCKED > qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp) > { > BlockdevCreateOptionsQcow2 *qcow2_opts; > @@ -3724,8 +3726,10 @@ qcow2_co_create(BlockdevCreateOptions *create_options, > Error **errp) > goto out; > } > > + bdrv_graph_co_rdlock(); > ret = qcow2_alloc_clusters(blk_bs(blk), 3 * cluster_size); > if (ret < 0) { > + bdrv_graph_co_rdunlock(); > error_setg_errno(errp, -ret, "Could not allocate clusters for qcow2 " > "header and refcount table"); > goto out; > @@ -3743,6 +3747,8 @@ qcow2_co_create(BlockdevCreateOptions *create_options, > Error **errp) > > /* Create a full header (including things like feature table) */ > ret = qcow2_update_header(blk_bs(blk)); > + bdrv_graph_co_rdunlock(); > + If we ever inject any 'goto out' in the elided lines, we're in trouble. Would this be any safer by wrapping the intervening statements in a scope-guarded lock? But what you have is correct, despite my worries about potential maintenance concerns. Reviewed-by: Eric Blake <ebl...@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org