| Message ID | 2da7b388a740d1f1b7f40e1182fc821f8abb8fbb.1547739122.git.berto@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add a 'x-blockdev-reopen' QMP command | expand |
Am 17.01.2019 um 16:33 hat Alberto Garcia geschrieben: > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > block/commit.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/block/commit.c b/block/commit.c > index 53148e610b..8824d135e0 100644 > --- a/block/commit.c > +++ b/block/commit.c > @@ -73,6 +73,8 @@ static int commit_prepare(Job *job) > { > CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); > > + bdrv_unfreeze_backing_chain(s->commit_top_bs, s->base_bs); > + > /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before > * the normal backing chain can be restored. */ > blk_unref(s->base); > @@ -89,6 +91,8 @@ static void commit_abort(Job *job) > CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); > BlockDriverState *top_bs = blk_bs(s->top); > > + bdrv_unfreeze_backing_chain(s->commit_top_bs, s->base_bs); commit_abort() will run if commit_prepare() returned an error, so we may end up unfreezing twice. Maybe move it into the 'if (s->base)' block a few lines down. > /* Make sure commit_top_bs and top stay around until bdrv_replace_node() */ > bdrv_ref(top_bs); > bdrv_ref(s->commit_top_bs); > @@ -336,6 +340,10 @@ void commit_start(const char *job_id, BlockDriverState *bs, > } > } > > + if (bdrv_freeze_backing_chain(commit_top_bs, base, errp) < 0) { > + goto fail; > + } Don't error paths need to unfreeze after this? Kevin
On Tue 12 Feb 2019 03:54:15 PM CET, Kevin Wolf wrote: >> @@ -336,6 +340,10 @@ void commit_start(const char *job_id, BlockDriverState *bs, >> } >> } >> >> + if (bdrv_freeze_backing_chain(commit_top_bs, base, errp) < 0) { >> + goto fail; >> + } > > Don't error paths need to unfreeze after this? Yes, and while debugging this I realized that the error path is wrong: if (commit_top_bs) { bdrv_replace_node(commit_top_bs, top, &error_abort); } job_early_fail(&s->common.job); Doing bdrv_replace_node() here before job_early_fail() fails with Unexpected error in bdrv_check_update_perm() at block.c:1920: Conflicts with use by commit job 'virtio0' as 'intermediate node', which does not allow 'consistent read' on <node-name> Aborted I'll write a separate patch for this problem. Berto
Am 14.02.2019 um 16:21 hat Alberto Garcia geschrieben: > On Tue 12 Feb 2019 03:54:15 PM CET, Kevin Wolf wrote: > >> @@ -336,6 +340,10 @@ void commit_start(const char *job_id, BlockDriverState *bs, > >> } > >> } > >> > >> + if (bdrv_freeze_backing_chain(commit_top_bs, base, errp) < 0) { > >> + goto fail; > >> + } > > > > Don't error paths need to unfreeze after this? > > Yes, and while debugging this I realized that the error path is wrong: > > if (commit_top_bs) { > bdrv_replace_node(commit_top_bs, top, &error_abort); > } > job_early_fail(&s->common.job); > > Doing bdrv_replace_node() here before job_early_fail() fails with > > Unexpected error in bdrv_check_update_perm() at block.c:1920: > Conflicts with use by commit job 'virtio0' as 'intermediate node', which does not allow 'consistent read' on <node-name> > Aborted Oh, good point. And it seems to have been wrong even since permissions were introduced to the commit job in 8dfba279776. > I'll write a separate patch for this problem. And a test case! :-) (That is, if the errors can even be triggered reliably without modifying the code.) Kevin
diff --git a/block/commit.c b/block/commit.c index 53148e610b..8824d135e0 100644 --- a/block/commit.c +++ b/block/commit.c @@ -73,6 +73,8 @@ static int commit_prepare(Job *job) { CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); + bdrv_unfreeze_backing_chain(s->commit_top_bs, s->base_bs); + /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before * the normal backing chain can be restored. */ blk_unref(s->base); @@ -89,6 +91,8 @@ static void commit_abort(Job *job) CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); BlockDriverState *top_bs = blk_bs(s->top); + bdrv_unfreeze_backing_chain(s->commit_top_bs, s->base_bs); + /* Make sure commit_top_bs and top stay around until bdrv_replace_node() */ bdrv_ref(top_bs); bdrv_ref(s->commit_top_bs); @@ -336,6 +340,10 @@ void commit_start(const char *job_id, BlockDriverState *bs, } } + if (bdrv_freeze_backing_chain(commit_top_bs, base, errp) < 0) { + goto fail; + } + ret = block_job_add_bdrv(&s->common, "base", base, 0, BLK_PERM_ALL, errp); if (ret < 0) { goto fail;
Signed-off-by: Alberto Garcia <berto@igalia.com> --- block/commit.c | 8 ++++++++ 1 file changed, 8 insertions(+)