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);
+
     /* 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;
-- 
2.11.0

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