Re: [PATCH 23/24] block: Take graph lock for most of .bdrv_open

[Kevin Wolf <kwolf@redhat.com>]

Am 30.10.2023 um 22:34 hat Eric Blake geschrieben:
> On Fri, Oct 27, 2023 at 05:53:32PM +0200, Kevin Wolf wrote:
> > Most implementations of .bdrv_open first open their file child (which is
> > an operation that internally takes the write lock and therefore we
> > shouldn't hold the graph lock while calling it), and afterwards many
> > operations that require holding the graph lock, e.g. for accessing
> > bs->file.
> > 
> > This changes block drivers that follow this pattern to take the graph
> > lock after opening the child node.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/blkdebug.c          | 16 ++++++++++------
> >  block/bochs.c             |  4 ++++
> >  block/cloop.c             |  4 ++++
> >  block/copy-before-write.c |  2 ++
> >  block/copy-on-read.c      |  4 ++--
> >  block/crypto.c            |  4 ++++
> >  block/dmg.c               |  5 +++++
> >  block/filter-compress.c   |  2 ++
> >  block/parallels.c         |  4 ++--
> >  block/preallocate.c       |  4 ++++
> >  block/qcow.c              | 11 +++++++----
> >  block/raw-format.c        |  6 ++++--
> >  block/snapshot-access.c   |  3 +++
> >  block/throttle.c          |  3 +++
> >  block/vdi.c               |  4 ++--
> >  block/vpc.c               |  4 ++--
> >  16 files changed, 60 insertions(+), 20 deletions(-)
> > 
> 
> > +++ b/block/qcow.c
> > @@ -124,9 +124,11 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
> >  
> >      ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
> >      if (ret < 0) {
> > -        goto fail;
> > +        goto fail_unlocked;
> >      }
> >  
> > +    bdrv_graph_rdlock_main_loop();
> > +
> >      ret = bdrv_pread(bs->file, 0, sizeof(header), &header, 0);
> >      if (ret < 0) {
> >          goto fail;
> > @@ -301,11 +303,9 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
> >      }
> >  
> >      /* Disable migration when qcow images are used */
> > -    bdrv_graph_rdlock_main_loop();
> >      error_setg(&s->migration_blocker, "The qcow format used by node '%s' "
> >                 "does not support live migration",
> >                 bdrv_get_device_or_node_name(bs));
> > -    bdrv_graph_rdunlock_main_loop();
> >  
> >      ret = migrate_add_blocker(s->migration_blocker, errp);
> >      if (ret < 0) {
> > @@ -316,9 +316,12 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
> >      qobject_unref(encryptopts);
> >      qapi_free_QCryptoBlockOpenOptions(crypto_opts);
> >      qemu_co_mutex_init(&s->lock);
> > +    bdrv_graph_rdunlock_main_loop();
> >      return 0;
> >  
> > - fail:
> > +fail:
> 
> Why the change in indentation?  At least emacs intentionally prefers
> to indent top-level labels 1 column in, so that they cannot be
> confused with function declarations in the first column.  But that's cosmetic.

The coding style document isn't explicit about it, but the overwhelming
majority of code both in the block layer and QEMU in general prefers the
style without indentation:

$ git grep '^ [A-Za-z_]\+:$' | wc -l
392
$ git grep '^[A-Za-z_]\+:$' | wc -l
2739

$ git grep '^ [A-Za-z_]\+:$' block | wc -l
27
$ git grep '^[A-Za-z_]\+:$' block | wc -l
332

So I wanted to add the new label with the usual convention, but also not
make the function inconsistent, which is why I reindented the existing
label.

Kevin