Re: [PATCH 4/4] block: Use blk_make_empty() after commits

[Kevin Wolf]

Am 28.04.2020 um 15:26 hat Max Reitz geschrieben:
> bdrv_commit() already has a BlockBackend pointing to the BDS that we
> want to empty, it just has the wrong permissions.
> 
> qemu-img commit has no BlockBackend pointing to the old backing file
> yet, but introducing one is simple.
> 
> After this commit, bdrv_make_empty() is the only remaining caller of
> BlockDriver.bdrv_make_empty().
> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>  block/commit.c |  8 +++++++-
>  qemu-img.c     | 19 ++++++++++++++-----
>  2 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/block/commit.c b/block/commit.c
> index 8e672799af..24720ba67d 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -493,10 +493,16 @@ int bdrv_commit(BlockDriverState *bs)
>      }
>  
>      if (drv->bdrv_make_empty) {
> -        ret = drv->bdrv_make_empty(bs);
> +        ret = blk_set_perm(src, BLK_PERM_WRITE, BLK_PERM_ALL, NULL);

This is very likely to fail because the common case is that the source
node is attached to a guest device that doesn't share writes.
(qemu-iotests 131 and 274 catch this.)

So I think after my theoretical comment in patch 1, this is the
practical reason why we need WRITE_UNCHANGED rather than WRITE.

Also, why don't you take this permission from the start so that we would
error out right away rather than failing after waiting for the all the
data to be copied?

>          if (ret < 0) {
>              goto ro_cleanup;
>          }
> +
> +        ret = blk_make_empty(src, NULL);
> +        if (ret < 0) {
> +            goto ro_cleanup;
> +        }
> +
>          blk_flush(src);
>      }
>  
> diff --git a/qemu-img.c b/qemu-img.c
> index 821cbf610e..a5e8659867 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1065,11 +1065,20 @@ static int img_commit(int argc, char **argv)
>          goto unref_backing;
>      }
>  
> -    if (!drop && bs->drv->bdrv_make_empty) {
> -        ret = bs->drv->bdrv_make_empty(bs);
> -        if (ret) {
> -            error_setg_errno(&local_err, -ret, "Could not empty %s",
> -                             filename);
> +    if (!drop) {
> +        BlockBackend *old_backing_blk;
> +
> +        old_backing_blk = blk_new_with_bs(bs, BLK_PERM_WRITE, BLK_PERM_ALL,
> +                                          &local_err);

Oh, you actually depend on another series that you didn't mention in
the cover letter.

> +        if (!old_backing_blk) {
> +            goto unref_backing;
> +        }
> +        ret = blk_make_empty(old_backing_blk, &local_err);
> +        blk_unref(old_backing_blk);
> +        if (ret == -ENOTSUP) {
> +            error_free(local_err);
> +            local_err = NULL;
> +        } else if (ret < 0) {
>              goto unref_backing;
>          }
>      }

Kevin