Re: [Qemu-block] [PATCH 05/16] block: Convert bs->file to BdrvChild

[Alberto Garcia]

On Thu 17 Sep 2015 03:48:09 PM CEST, Kevin Wolf  wrote:

> @@ -1929,6 +1925,11 @@ void bdrv_close(BlockDriverState *bs)
>  bdrv_unref(backing_hd);
>  }
>  
> +if (bs->file != NULL) {
> +bdrv_unref(bs->file->bs);
> +bs->file = NULL;
> +}
> +
>  QLIST_FOREACH_SAFE(child, >children, next, next) {
>  /* TODO Remove bdrv_unref() from drivers' close function and use
>   * bdrv_unref_child() here */
> @@ -1953,11 +1954,6 @@ void bdrv_close(BlockDriverState *bs)
>  bs->options = NULL;
>  QDECREF(bs->full_open_options);
>  bs->full_open_options = NULL;
> -
> -if (bs->file != NULL) {
> -bdrv_unref(bs->file);
> -bs->file = NULL;
> -}
>  }

You are moving bdrv_unref(bs->file) up in the function and this seems to
be causing a problem, by turning this:

bs->drv->bdrv_close(bs);
bs->drv = NULL;
bdrv_unref(bs->file);

into this:

bs->drv->bdrv_close(bs);
bdrv_unref(bs->file);
bs->drv = NULL;

In the latter case, closing bs->file calls aio_poll(). This can trigger
new requests on bs, which at that point has a valid pointer to a
BlockDriver that has already been closed. If throttling is enabled on bs
those requests might be queued for later. At the point in which those
requests are scheduled bs->drv is already NULL, crashing QEMU. I can
reproduce this easily with x-data-plane=on.

I sent a separate patch that moves the bdrv_io_limits_disable() call to
the beginning of bdrv_close(). That solves the crash, but I guess that
this patch should also be changed.

Berto

Re: [Qemu-block] [PATCH 05/16] block: Convert bs->file to BdrvChild

[Kevin Wolf]

Am 22.09.2015 um 20:36 hat Max Reitz geschrieben:
> On 17.09.2015 15:48, Kevin Wolf wrote:
> > This patch removes the temporary duplication between bs->file and
> > bs->file_child by converting everything to BdrvChild.
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >  block.c   | 61 
> > ++
> >  block/blkdebug.c  | 32 
> >  block/blkverify.c | 33 ++---
> >  block/bochs.c |  8 +++---
> >  block/cloop.c | 10 
> >  block/dmg.c   | 20 +++
> >  block/io.c| 50 +++---
> >  block/parallels.c | 38 +++--
> >  block/qapi.c  |  2 +-
> >  block/qcow.c  | 42 +---
> >  block/qcow2-cache.c   | 11 +
> >  block/qcow2-cluster.c | 38 +
> >  block/qcow2-refcount.c| 45 ++
> >  block/qcow2-snapshot.c| 30 ---
> >  block/qcow2.c | 62 
> > ---
> >  block/qed-table.c |  4 +--
> >  block/qed.c   | 32 
> >  block/raw_bsd.c   | 40 +++---
> >  block/snapshot.c  | 12 -
> >  block/vdi.c   | 17 +++--
> >  block/vhdx-log.c  | 25 ++-
> >  block/vhdx.c  | 36 ++-
> >  block/vmdk.c  | 27 +++--
> >  block/vpc.c   | 34 ++
> >  include/block/block.h |  8 +-
> >  include/block/block_int.h |  3 +--
> >  26 files changed, 377 insertions(+), 343 deletions(-)
> 
> Reviewed-by: Max Reitz 
> 
> > diff --git a/block.c b/block.c
> > index 2e9e5e2..93d713b 100644
> > --- a/block.c
> > +++ b/block.c
> 
> [...]
> 
> > @@ -1929,6 +1925,11 @@ void bdrv_close(BlockDriverState *bs)
> >  bdrv_unref(backing_hd);
> >  }
> >  
> > +if (bs->file != NULL) {
> > +bdrv_unref(bs->file->bs);
> 
> I think we can use bdrv_unref_child(bs->file) here. I'd personally like
> it more, at least (because using bdrv_unref() and relying on the loop
> below is basically what the comment inside of the loop advises against).
> 
> Yes, I know, eventually, we want to drop this block anyway and let the
> loop below handle everything using bdrv_unref_child(). But when we do
> that conversion, it'll be obvious to drop a bdrv_unref_child() call,
> whereas dropping bdrv_unref() alone isn't so obvious.

Seems safe to do, and reads a bit nicer, so I'll change the patch
accordingly. Thanks.

Kevin

> > +bs->file = NULL;
> > +}
> > +
> >  QLIST_FOREACH_SAFE(child, >children, next, next) {
> >  /* TODO Remove bdrv_unref() from drivers' close function and 
> > use
> >   * bdrv_unref_child() here */
> > @@ -1953,11 +1954,6 @@ void bdrv_close(BlockDriverState *bs)
> >  bs->options = NULL;
> >  QDECREF(bs->full_open_options);
> >  bs->full_open_options = NULL;
> > -
> > -if (bs->file != NULL) {
> > -bdrv_unref(bs->file);
> > -bs->file = NULL;
> > -}
> >  }
> >  
> >  if (bs->blk) {
> 




pgprNUNzQR4Gl.pgp
Description: PGP signature

Re: [Qemu-block] [PATCH 05/16] block: Convert bs->file to BdrvChild

[Max Reitz]

On 17.09.2015 15:48, Kevin Wolf wrote:
> This patch removes the temporary duplication between bs->file and
> bs->file_child by converting everything to BdrvChild.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c   | 61 ++
>  block/blkdebug.c  | 32 
>  block/blkverify.c | 33 ++---
>  block/bochs.c |  8 +++---
>  block/cloop.c | 10 
>  block/dmg.c   | 20 +++
>  block/io.c| 50 +++---
>  block/parallels.c | 38 +++--
>  block/qapi.c  |  2 +-
>  block/qcow.c  | 42 +---
>  block/qcow2-cache.c   | 11 +
>  block/qcow2-cluster.c | 38 +
>  block/qcow2-refcount.c| 45 ++
>  block/qcow2-snapshot.c| 30 ---
>  block/qcow2.c | 62 
> ---
>  block/qed-table.c |  4 +--
>  block/qed.c   | 32 
>  block/raw_bsd.c   | 40 +++---
>  block/snapshot.c  | 12 -
>  block/vdi.c   | 17 +++--
>  block/vhdx-log.c  | 25 ++-
>  block/vhdx.c  | 36 ++-
>  block/vmdk.c  | 27 +++--
>  block/vpc.c   | 34 ++
>  include/block/block.h |  8 +-
>  include/block/block_int.h |  3 +--
>  26 files changed, 377 insertions(+), 343 deletions(-)

Reviewed-by: Max Reitz 

> diff --git a/block.c b/block.c
> index 2e9e5e2..93d713b 100644
> --- a/block.c
> +++ b/block.c

[...]

> @@ -1929,6 +1925,11 @@ void bdrv_close(BlockDriverState *bs)
>  bdrv_unref(backing_hd);
>  }
>  
> +if (bs->file != NULL) {
> +bdrv_unref(bs->file->bs);

I think we can use bdrv_unref_child(bs->file) here. I'd personally like
it more, at least (because using bdrv_unref() and relying on the loop
below is basically what the comment inside of the loop advises against).

Yes, I know, eventually, we want to drop this block anyway and let the
loop below handle everything using bdrv_unref_child(). But when we do
that conversion, it'll be obvious to drop a bdrv_unref_child() call,
whereas dropping bdrv_unref() alone isn't so obvious.

Max

> +bs->file = NULL;
> +}
> +
>  QLIST_FOREACH_SAFE(child, >children, next, next) {
>  /* TODO Remove bdrv_unref() from drivers' close function and use
>   * bdrv_unref_child() here */
> @@ -1953,11 +1954,6 @@ void bdrv_close(BlockDriverState *bs)
>  bs->options = NULL;
>  QDECREF(bs->full_open_options);
>  bs->full_open_options = NULL;
> -
> -if (bs->file != NULL) {
> -bdrv_unref(bs->file);
> -bs->file = NULL;
> -}
>  }
>  
>  if (bs->blk) {



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH 05/16] block: Convert bs->file to BdrvChild

[Kevin Wolf]

This patch removes the temporary duplication between bs->file and
bs->file_child by converting everything to BdrvChild.

Signed-off-by: Kevin Wolf 
---
 block.c   | 61 ++
 block/blkdebug.c  | 32 
 block/blkverify.c | 33 ++---
 block/bochs.c |  8 +++---
 block/cloop.c | 10 
 block/dmg.c   | 20 +++
 block/io.c| 50 +++---
 block/parallels.c | 38 +++--
 block/qapi.c  |  2 +-
 block/qcow.c  | 42 +---
 block/qcow2-cache.c   | 11 +
 block/qcow2-cluster.c | 38 +
 block/qcow2-refcount.c| 45 ++
 block/qcow2-snapshot.c| 30 ---
 block/qcow2.c | 62 ---
 block/qed-table.c |  4 +--
 block/qed.c   | 32 
 block/raw_bsd.c   | 40 +++---
 block/snapshot.c  | 12 -
 block/vdi.c   | 17 +++--
 block/vhdx-log.c  | 25 ++-
 block/vhdx.c  | 36 ++-
 block/vmdk.c  | 27 +++--
 block/vpc.c   | 34 ++
 include/block/block.h |  8 +-
 include/block/block_int.h |  3 +--
 26 files changed, 377 insertions(+), 343 deletions(-)

diff --git a/block.c b/block.c
index 2e9e5e2..93d713b 100644
--- a/block.c
+++ b/block.c
@@ -809,7 +809,7 @@ static QemuOptsList bdrv_runtime_opts = {
  *
  * Removes all processed options from *options.
  */
-static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
+static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
 QDict *options, int flags, BlockDriver *drv, Error **errp)
 {
 int ret, open_flags;
@@ -823,7 +823,7 @@ static int bdrv_open_common(BlockDriverState *bs, 
BlockDriverState *file,
 assert(options != NULL && bs->options != options);
 
 if (file != NULL) {
-filename = file->filename;
+filename = file->bs->filename;
 } else {
 filename = qdict_get_try_str(options, "filename");
 }
@@ -1401,7 +1401,8 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
const char *filename,
  const BdrvChildRole *child_role, Error **errp)
 {
 int ret;
-BlockDriverState *file = NULL, *bs;
+BdrvChild *file = NULL;
+BlockDriverState *bs;
 BlockDriver *drv = NULL;
 const char *drvname;
 Error *local_err = NULL;
@@ -1485,25 +1486,20 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
const char *filename,
 flags = bdrv_backing_flags(flags);
 }
 
-assert(file == NULL);
 bs->open_flags = flags;
 
-bs->file_child = bdrv_open_child(filename, options, "file", bs,
- _file, true, _err);
+file = bdrv_open_child(filename, options, "file", bs,
+   _file, true, _err);
 if (local_err) {
 ret = -EINVAL;
 goto fail;
 }
-
-if (bs->file_child) {
-file = bs->file_child->bs;
-}
 }
 
 /* Image format probing */
 bs->probed = !drv;
 if (!drv && file) {
-ret = find_image_format(file, filename, , _err);
+ret = find_image_format(file->bs, filename, , _err);
 if (ret < 0) {
 goto fail;
 }
@@ -1526,7 +1522,7 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
const char *filename,
 }
 
 if (file && (bs->file != file)) {
-bdrv_unref(file);
+bdrv_unref_child(bs, file);
 file = NULL;
 }
 
@@ -1587,7 +1583,7 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
const char *filename,
 
 fail:
 if (file != NULL) {
-bdrv_unref(file);
+bdrv_unref_child(bs, file);
 }
 QDECREF(bs->options);
 QDECREF(options);
@@ -1929,6 +1925,11 @@ void bdrv_close(BlockDriverState *bs)
 bdrv_unref(backing_hd);
 }
 
+if (bs->file != NULL) {
+bdrv_unref(bs->file->bs);
+bs->file = NULL;
+}
+
 QLIST_FOREACH_SAFE(child, >children, next, next) {
 /* TODO Remove bdrv_unref() from drivers' close function and use
  * bdrv_unref_child() here */
@@ -1953,11 +1954,6 @@ void bdrv_close(BlockDriverState *bs)
 bs->options = NULL;
 QDECREF(bs->full_open_options);
 bs->full_open_options = NULL;
-
-if (bs->file != NULL) {
-bdrv_unref(bs->file);
-bs->file = NULL;
-}
 }
 
 if (bs->blk) {
@@ -2565,7 +2561,7 @@ int64_t bdrv_get_allocated_file_size(BlockDriverState *bs)