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

[Eric Blake]

On Mon, May 15, 2023 at 06:19:41PM +0200, Kevin Wolf wrote:
> > > @@ -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.

Yeah, LOTS of people are waiting on clang __attribute__((cleanup))
analysis sanity ;)

> 
> 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.

I'm sold!  The very reason you can't use a cleanup scope-guard
(because clang can't see through the annotation) is also the reason
why clang is able to flag your error if you don't properly clean up by
hand.  So while it is more tedious to maintain, we've finally got
compiler assistance to something that used to be human-only, which is
a step forwards even if it is more to type in the short term.

Thanks for testing that.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org