[Qemu-devel] [PATCH 05/16] block: Convert bs->file to BdrvChild
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, - &child_file, true, &local_err); +file = bdrv_open_child(filename, options, "file", bs, + &child_file, true, &local_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, &drv, &local_err); +ret = find_image_format(file->bs, filename, &drv, &local_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, &bs->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_fil
Re: [Qemu-devel] [PATCH 05/16] block: Convert bs->file to BdrvChild
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, &bs->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
Re: [Qemu-devel] [PATCH 05/16] block: Convert bs->file to BdrvChild
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, &bs->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) { > pgpMzb_ObEBe8.pgp Description: PGP signature
Re: [Qemu-devel] [PATCH 05/16] block: Convert bs->file to BdrvChild
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; > +} > + With Max's suggestion of using bdrv_unref_child(bs->file) instead, Reviewed-by: Alberto Garcia Berto
Re: [Qemu-devel] [PATCH 05/16] block: Convert bs->file to BdrvChild
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, &bs->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