[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v15 05/14] block: Add bdrv_set_backing_hd()
From: |
Jeff Cody |
Subject: |
Re: [Qemu-devel] [PATCH v15 05/14] block: Add bdrv_set_backing_hd() |
Date: |
Wed, 26 Feb 2014 17:35:11 -0500 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Sun, Feb 23, 2014 at 09:54:46AM +0800, Fam Zheng wrote:
> This is the common but non-trivial steps to assign or change the
> backing_hd of BDS.
>
> Signed-off-by: Fam Zheng <address@hidden>
> ---
> block.c | 46 ++++++++++++++++++++++++++++++++++++++--------
> include/block/block.h | 1 +
> 2 files changed, 39 insertions(+), 8 deletions(-)
>
> diff --git a/block.c b/block.c
> index 684b9d6..9caade9 100644
> --- a/block.c
> +++ b/block.c
> @@ -1041,6 +1041,32 @@ fail:
> return ret;
> }
>
> +void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
> +{
> + if (backing_hd) {
> + /* Grab the reference before unref original backing_hd, so we are
> safe
> + * when rebasing in the backing chain.
> + */
> + bdrv_ref(backing_hd);
I think the problem is performing this bdrv_ref() makes the
assumptions that:
A) bs->backing_hd is non-NULL, and
B) backing_hd is currently a backing file, at some level, of
bs->backing_chain.
The above conditions are not always true, which is what led to my
concerns in my previous email. I think we could avoid the spurious
bdrv_ref() if we check for both conditions A and B before calling
bdrv_ref(backing_hd).
But I think there could still be a problem...
> + }
> +
> + if (bs->backing_hd) {
> + bdrv_unref(bs->backing_hd);
Only if conditions A and B are true would this bdrv_unref()
potentially lead to a bdrv_unref() being called on backing_hd.
But what if the refcnt on bs->backing_hd is > 1? Then even if
conditions A and B are met, we still won't eventually unref
backing_hd, making the bdrv_ref(backing_hd) spurious.
But as I mentioned before, manually checking refcnt, or making
assumptions on refcnt, seems very wrong.
It is almost like what is needed, are some conditional refcnt
implementations. Something like:
void bdrv_cond_ref(BlockDriverState *bs_cond, BlockDriverState *bs)
That would increase the refcnt on bs_cond IFF:
1) bs is non-NULL
2) bs_cond is in the backing chain of bs
3) bs is at risk of deletion on the next unref
> + }
> +
> + bs->backing_hd = backing_hd;
> + if (!backing_hd) {
> + bs->backing_file[0] = '\0';
> + bs->backing_format[0] = '\0';
> + goto out;
> + }
> + pstrcpy(bs->backing_file, sizeof(bs->backing_file),
> backing_hd->filename);
> + pstrcpy(bs->backing_format, sizeof(bs->backing_format),
> + backing_hd->drv ? backing_hd->drv->format_name : "");
> +out:
> + bdrv_refresh_limits(bs);
> +}
> +
> /*
> * Opens the backing file for a BlockDriverState if not yet open
> *
> @@ -1054,6 +1080,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict
> *options, Error **errp)
> char backing_filename[PATH_MAX];
> int back_flags, ret;
> BlockDriver *back_drv = NULL;
> + BlockDriverState *backing_hd;
> Error *local_err = NULL;
>
> if (bs->backing_hd != NULL) {
> @@ -1077,6 +1104,8 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict
> *options, Error **errp)
> sizeof(backing_filename));
> }
>
> + backing_hd = bdrv_new("");
> +
> if (bs->backing_format[0] != '\0') {
> back_drv = bdrv_find_format(bs->backing_format);
> }
> @@ -1085,23 +1114,24 @@ int bdrv_open_backing_file(BlockDriverState *bs,
> QDict *options, Error **errp)
> back_flags = bs->open_flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT |
> BDRV_O_COPY_ON_READ);
>
> - assert(bs->backing_hd == NULL);
> - ret = bdrv_open(&bs->backing_hd,
> + ret = bdrv_open(&backing_hd,
> *backing_filename ? backing_filename : NULL, NULL,
> options,
> back_flags, back_drv, &local_err);
> if (ret < 0) {
> bs->backing_hd = NULL;
> + bdrv_unref(backing_hd);
> + backing_hd = NULL;
> bs->open_flags |= BDRV_O_NO_BACKING;
> error_setg(errp, "Could not open backing file: %s",
> error_get_pretty(local_err));
> error_free(local_err);
> return ret;
> }
> -
> - if (bs->backing_hd->file) {
> - pstrcpy(bs->backing_file, sizeof(bs->backing_file),
> - bs->backing_hd->file->filename);
> - }
> + bdrv_set_backing_hd(bs, backing_hd);
> + /* Now we have refcnt = 2 on backing_hd by bdrv_new and
> + * bdrv_set_backing_hd, while we only need 1 */
> + assert(backing_hd->refcnt == 2);
> + bdrv_unref(backing_hd);
>
> /* Recalculate the BlockLimits with the backing file */
> bdrv_refresh_limits(bs);
> @@ -1923,7 +1953,7 @@ void bdrv_append(BlockDriverState *bs_new,
> BlockDriverState *bs_top)
>
> /* The contents of 'tmp' will become bs_top, as we are
> * swapping bs_new and bs_top contents. */
> - bs_top->backing_hd = bs_new;
> + bdrv_set_backing_hd(bs_top, bs_new);
> bs_top->open_flags &= ~BDRV_O_NO_BACKING;
> pstrcpy(bs_top->backing_file, sizeof(bs_top->backing_file),
> bs_new->filename);
> diff --git a/include/block/block.h b/include/block/block.h
> index a46f70a..ee1582d 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -208,6 +208,7 @@ int bdrv_parse_discard_flags(const char *mode, int
> *flags);
> int bdrv_open_image(BlockDriverState **pbs, const char *filename,
> QDict *options, const char *bdref_key, int flags,
> bool allow_none, Error **errp);
> +void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd);
> int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error
> **errp);
> int bdrv_open(BlockDriverState **pbs, const char *filename,
> const char *reference, QDict *options, int flags,
> --
> 1.8.5.4
>
>
- [Qemu-devel] [PATCH v15 00/14] Drop in_use from BlockDriverState and enable point-in-time snapshot exporting over NBD, Fam Zheng, 2014/02/22
- [Qemu-devel] [PATCH v15 01/14] block: Add BlockOpType enum, Fam Zheng, 2014/02/22
- [Qemu-devel] [PATCH v15 02/14] block: Introduce op_blockers to BlockDriverState, Fam Zheng, 2014/02/22
- [Qemu-devel] [PATCH v15 03/14] block: Replace in_use with operation blocker, Fam Zheng, 2014/02/22
- [Qemu-devel] [PATCH v15 04/14] block: Move op_blocker check from block_job_create to its caller, Fam Zheng, 2014/02/22
- [Qemu-devel] [PATCH v15 05/14] block: Add bdrv_set_backing_hd(), Fam Zheng, 2014/02/22
- [Qemu-devel] [PATCH v15 06/14] block: Add backing_blocker in BlockDriverState, Fam Zheng, 2014/02/22
- [Qemu-devel] [PATCH v15 07/14] block: Parse "backing" option to reference existing BDS, Fam Zheng, 2014/02/22
- [Qemu-devel] [PATCH v15 08/14] block: Support dropping active in bdrv_drop_intermediate, Fam Zheng, 2014/02/22
- [Qemu-devel] [PATCH v15 09/14] stream: Use bdrv_drop_intermediate and drop close_unused_images, Fam Zheng, 2014/02/22
- [Qemu-devel] [PATCH v15 10/14] qmp: Add command 'blockdev-backup', Fam Zheng, 2014/02/22
- [Qemu-devel] [PATCH v15 11/14] block: Allow backup on referenced named BlockDriverState, Fam Zheng, 2014/02/22
- [Qemu-devel] [PATCH v15 12/14] block: Add blockdev-backup to transaction, Fam Zheng, 2014/02/22
- [Qemu-devel] [PATCH v15 13/14] qemu-iotests: Test blockdev-backup in 055, Fam Zheng, 2014/02/22