On 28.04.20 17:03, Kevin Wolf wrote:
> 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 <mreitz@redhat.com>
>> ---
>>  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?

Because we only need to take it when the BlockDriver actually supports
bdrv_make_empty(), so I thought I’d put it here where we have the check
anyway.

However, yes, when we take WRITE_UNCHANGED, we might as well take it
unconditionally from the start.  (And then call blk_make_empty()
unconditionally here, too, and let it figure out -ENOTSUP, like Eric noted.)

>>          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.

Well, yes.  I didn’t really realize because I just based it on my
block-next...

Max