Re: [PATCH 1/8] block: Call .bdrv_co_create(_opts) unlocked

[Kevin Wolf <kwolf@redhat.com>]

Am 12.05.2023 um 18:12 hat Eric Blake geschrieben:
> 
> 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 <kwolf@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?

TSA doesn't understand these guards, which is why they are only
annotated as assertions (I think we talked about this in my previous
series), at the cost of leaving unlocking unchecked. So in cases where
the scope isn't the full function, individual calls are better at the
moment. Once clang implements support for __attribute__((cleanup)), we
can maybe change this.

Of course, TSA solves the very maintenance problem you're concerned
about: With a 'goto out' added, compilation on clang fails because it
sees that there is a code path that doesn't unlock. So at least it makes
the compromise not terrible.

For example, if I comment out the unlock in the error case in the first,
this is what I get:

../block/qcow2.c:3825:5: error: mutex 'graph_lock' is not held on every path through here [-Werror,-Wthread-safety-analysis]
    blk_co_unref(blk);
    ^
../block/qcow2.c:3735:5: note: mutex acquired here
    bdrv_graph_co_rdlock();
    ^
1 error generated.

Kevin