| Message ID | 1393120495-13815-6-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <famz@redhat.com> > --- > 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); > + } > + > + if (bs->backing_hd) { > + bdrv_unref(bs->backing_hd); > + } > + > + 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; Assigning bs->backing_hd = NULL is no longer needed now, so this should be removed. > + 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); It is a bit worrisome to manually track refcnt like this, and I think it might be red flag that bdrv_set_backing_hd() isn't entirely correct. It seems wrong (and a bit fragile) to have to compensate for what is essentially a spurious bdrv_ref() call by another function. Also, this relies on implicit knowledge that bdrv_set_backing_hd() increases the refcnt, and that the refcnt is increased only once in bdrv_set_backing_fd(). > > /* 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); If the bs_top->backing_hd is NULL, this means the refcnt on bs_new is off by one now, right? And I think in the current usage, bs_top->backing_hd will always be NULL here. > bs_top->open_flags &= ~BDRV_O_NO_BACKING; Hmm, this should probably be in bdrv_set_backing_hd() for any non-NULL backing_hd value. > pstrcpy(bs_top->backing_file, sizeof(bs_top->backing_file), > bs_new->filename); This line, and the one below it (not in the diff context) are now redundant, and should be removed. > 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 > >
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 <famz@redhat.com> > --- > 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 > >
On Wed, 02/26 17:35, Jeff Cody wrote: > 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 <famz@redhat.com> > > --- > > 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 > I see the problem, however these rules (bdrv_cond_ref) still look hard to infer. To keep it simple, I prefer to remove bdrv_ref/bdrv_unref in bdrv_set_backing_hd and leave it to caller, which is the most readable I think. Fam
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); + } + + if (bs->backing_hd) { + bdrv_unref(bs->backing_hd); + } + + 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,
This is the common but non-trivial steps to assign or change the backing_hd of BDS. Signed-off-by: Fam Zheng <famz@redhat.com> --- block.c | 46 ++++++++++++++++++++++++++++++++++++++-------- include/block/block.h | 1 + 2 files changed, 39 insertions(+), 8 deletions(-)