| Message ID | 1400565880-13409-7-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, May 20, 2014 at 02:04:31PM +0800, Fam Zheng wrote: > diff --git a/block/mirror.c b/block/mirror.c > index 1c38aa8..6a53d79 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -499,6 +499,7 @@ immediate_exit: > * trigger the unref from the top one */ > BlockDriverState *p = s->base->backing_hd; > s->base->backing_hd = NULL; > + bdrv_op_unblock_all(p, s->base->backing_blocker); > bdrv_unref(p); > } > } Would be cleaner to call bdrv_set_backing_hd(s->base, NULL) here instead of open coding it. Stefan
On Tue, May 20, 2014 at 02:04:31PM +0800, Fam Zheng wrote: > This makes use of op_blocker and blocks all the operations except for > commit target, on each BlockDriverState->backing_hd. > > The asserts for op_blocker in bdrv_swap are removed because with this > change, the target of block commit has at least the backing blocker of > its child, so the assertion is not true. Callers should do their check. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block.c | 22 ++++++++++++++++++---- > block/mirror.c | 1 + > include/block/block_int.h | 3 +++ > 3 files changed, 22 insertions(+), 4 deletions(-) IMO it would be nice to split the series after this patch. The op_blocker stuff looks solid and the series is at v20. Let's handle the rest of the changes in separate, smaller series that can be easily digested. Stefan
On Wed, May 21, 2014 at 04:03:03PM +0200, Stefan Hajnoczi wrote: > On Tue, May 20, 2014 at 02:04:31PM +0800, Fam Zheng wrote: > > diff --git a/block/mirror.c b/block/mirror.c > > index 1c38aa8..6a53d79 100644 > > --- a/block/mirror.c > > +++ b/block/mirror.c > > @@ -499,6 +499,7 @@ immediate_exit: > > * trigger the unref from the top one */ > > BlockDriverState *p = s->base->backing_hd; > > s->base->backing_hd = NULL; > > + bdrv_op_unblock_all(p, s->base->backing_blocker); > > bdrv_unref(p); > > } > > } > > Would be cleaner to call bdrv_set_backing_hd(s->base, NULL) here instead > of open coding it. > Patch 10 gets rid of essentially this whole chunk of code, and replaces it with bdrv_drop_intermediate(). So it does get cleaned up, just later in the series.
On Wed, 05/21 10:24, Jeff Cody wrote: > On Wed, May 21, 2014 at 04:03:03PM +0200, Stefan Hajnoczi wrote: > > On Tue, May 20, 2014 at 02:04:31PM +0800, Fam Zheng wrote: > > > diff --git a/block/mirror.c b/block/mirror.c > > > index 1c38aa8..6a53d79 100644 > > > --- a/block/mirror.c > > > +++ b/block/mirror.c > > > @@ -499,6 +499,7 @@ immediate_exit: > > > * trigger the unref from the top one */ > > > BlockDriverState *p = s->base->backing_hd; > > > s->base->backing_hd = NULL; > > > + bdrv_op_unblock_all(p, s->base->backing_blocker); > > > bdrv_unref(p); > > > } > > > } > > > > Would be cleaner to call bdrv_set_backing_hd(s->base, NULL) here instead > > of open coding it. > > > > Patch 10 gets rid of essentially this whole chunk of code, and > replaces it with bdrv_drop_intermediate(). So it does get cleaned up, > just later in the series. Thanks for pointing out, Jeff. Stefan, if there are other reasons to respin, I'll take your suggestion and update this, and then split the series, so you can merge 1-6. Fam
Stefan Hajnoczi <stefanha@redhat.com> writes: > On Tue, May 20, 2014 at 02:04:31PM +0800, Fam Zheng wrote: >> This makes use of op_blocker and blocks all the operations except for >> commit target, on each BlockDriverState->backing_hd. >> >> The asserts for op_blocker in bdrv_swap are removed because with this >> change, the target of block commit has at least the backing blocker of >> its child, so the assertion is not true. Callers should do their check. >> >> Signed-off-by: Fam Zheng <famz@redhat.com> >> --- >> block.c | 22 ++++++++++++++++++---- >> block/mirror.c | 1 + >> include/block/block_int.h | 3 +++ >> 3 files changed, 22 insertions(+), 4 deletions(-) > > IMO it would be nice to split the series after this patch. > > The op_blocker stuff looks solid and the series is at v20. Let's handle > the rest of the changes in separate, smaller series that can be easily > digested. Yes, please!
On Wed, May 21, 2014 at 10:37:50PM +0800, Fam Zheng wrote: > On Wed, 05/21 10:24, Jeff Cody wrote: > > On Wed, May 21, 2014 at 04:03:03PM +0200, Stefan Hajnoczi wrote: > > > On Tue, May 20, 2014 at 02:04:31PM +0800, Fam Zheng wrote: > > > > diff --git a/block/mirror.c b/block/mirror.c > > > > index 1c38aa8..6a53d79 100644 > > > > --- a/block/mirror.c > > > > +++ b/block/mirror.c > > > > @@ -499,6 +499,7 @@ immediate_exit: > > > > * trigger the unref from the top one */ > > > > BlockDriverState *p = s->base->backing_hd; > > > > s->base->backing_hd = NULL; > > > > + bdrv_op_unblock_all(p, s->base->backing_blocker); > > > > bdrv_unref(p); > > > > } > > > > } > > > > > > Would be cleaner to call bdrv_set_backing_hd(s->base, NULL) here instead > > > of open coding it. > > > > > > > Patch 10 gets rid of essentially this whole chunk of code, and > > replaces it with bdrv_drop_intermediate(). So it does get cleaned up, > > just later in the series. > > Thanks for pointing out, Jeff. > > Stefan, if there are other reasons to respin, I'll take your suggestion and > update this, and then split the series, so you can merge 1-6. > Unfortunately, it will need a respin. I've been doing some more testing, and with just patches 1-6, block-commit asserts. The culprits are the open coded backing_hd assignments, like above, in bdrv_drop_intermediate() (there are two, and either will cause an assert): ret = bdrv_change_backing_file(new_top_bs, backing_file_str, base_bs->drv ? base_bs->drv->format_name : ""); if (ret) { goto exit; } >> new_top_bs->backing_hd = base_bs; And: QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, next) { /* so that bdrv_close() does not recursively close the chain */ >> intermediate_state->bs->backing_hd = NULL; bdrv_unref(intermediate_state->bs); ^^^^^^^^^^^ This will assert Without the call to bdrv_set_backing_hd(), the backing_blocker does not get cleared, and this asserts in bdrv_delete(): assert(bdrv_op_blocker_is_empty(bs)); So all instances of open coded assignment of backing_hd should be replaced, either in this patch, or (preferably) in a new patch after patch 5.
diff --git a/block.c b/block.c index a47118b..efec10a 100644 --- a/block.c +++ b/block.c @@ -1097,14 +1097,29 @@ fail: void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) { + if (bs->backing_hd) { + assert(bs->backing_blocker); + bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker); + } else if (backing_hd) { + error_setg(&bs->backing_blocker, + "device is used as backing hd of '%s'", + bs->device_name); + } + bs->backing_hd = backing_hd; if (!backing_hd) { + error_free(bs->backing_blocker); goto out; } bs->open_flags &= ~BDRV_O_NO_BACKING; 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 : ""); + + bdrv_op_block_all(bs->backing_hd, bs->backing_blocker); + /* Otherwise we won't be able to commit due to check in bdrv_commit */ + bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_COMMIT, + bs->backing_blocker); out: bdrv_refresh_limits(bs); } @@ -1762,8 +1777,9 @@ void bdrv_close(BlockDriverState *bs) if (bs->drv) { if (bs->backing_hd) { - bdrv_unref(bs->backing_hd); - bs->backing_hd = NULL; + BlockDriverState *backing_hd = bs->backing_hd; + bdrv_set_backing_hd(bs, NULL); + bdrv_unref(backing_hd); } bs->drv->bdrv_close(bs); g_free(bs->opaque); @@ -1965,7 +1981,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old) assert(QLIST_EMPTY(&bs_new->dirty_bitmaps)); assert(bs_new->job == NULL); assert(bs_new->dev == NULL); - assert(bdrv_op_blocker_is_empty(bs_new)); assert(bs_new->io_limits_enabled == false); assert(!throttle_have_timer(&bs_new->throttle_state)); @@ -1984,7 +1999,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old) /* Check a few fields that should remain attached to the device */ assert(bs_new->dev == NULL); assert(bs_new->job == NULL); - assert(bdrv_op_blocker_is_empty(bs_new)); assert(bs_new->io_limits_enabled == false); assert(!throttle_have_timer(&bs_new->throttle_state)); diff --git a/block/mirror.c b/block/mirror.c index 1c38aa8..6a53d79 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -499,6 +499,7 @@ immediate_exit: * trigger the unref from the top one */ BlockDriverState *p = s->base->backing_hd; s->base->backing_hd = NULL; + bdrv_op_unblock_all(p, s->base->backing_blocker); bdrv_unref(p); } } diff --git a/include/block/block_int.h b/include/block/block_int.h index 3518076..49e5824 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -368,6 +368,9 @@ struct BlockDriverState { BlockJob *job; QDict *options; + + /* The error object in use for blocking operations on backing_hd */ + Error *backing_blocker; }; int get_tmp_filename(char *filename, int size);
This makes use of op_blocker and blocks all the operations except for commit target, on each BlockDriverState->backing_hd. The asserts for op_blocker in bdrv_swap are removed because with this change, the target of block commit has at least the backing blocker of its child, so the assertion is not true. Callers should do their check. Signed-off-by: Fam Zheng <famz@redhat.com> --- block.c | 22 ++++++++++++++++++---- block/mirror.c | 1 + include/block/block_int.h | 3 +++ 3 files changed, 22 insertions(+), 4 deletions(-)