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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3919 bytes --]

On 28.04.20 16:07, Eric Blake wrote:
> On 4/28/20 8:26 AM, Max Reitz wrote:
>> 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) {
> 
> This 'if' is still a bit awkward. Do all filter drivers set this
> function, or will bdrv_make_empty() automatically take care of looking
> through filters?  Should this be a check of a supported_ variable
> instead (similar to how Kevin just added supported_truncate_flags)?
> 
>> -        ret = drv->bdrv_make_empty(bs);
>> +        ret = blk_set_perm(src, BLK_PERM_WRITE, BLK_PERM_ALL, NULL);
>>           if (ret < 0) {
>>               goto ro_cleanup;
>>           }
>> +
>> +        ret = blk_make_empty(src, NULL);
> 
> So, if the driver lacks the callback, you miss calling blk_make_empty()
> even if it would have done something.

We can’t just call blk_make_empty() and then special case -ENOTSUP here,
though, because the BB doesn’t have a WRITE permission beforehand.  So
we have to take that permission before we can call blk_make_empty().
But taking the permission can fail, and then we kind of have to report
the -EPERM, even though the BlockDriver may not support emptying anyway.

I suppose if we just have to take the WRITE_UNCHANGED permission,
though, we can assume that that’s basically always allowed, so it
shouldn’t be that much of a problem there.

Max

>> +        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);
> 
> Old code: if the driver had the callback, use it.
> 
>> -        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);
>> +        if (!old_backing_blk) {
>> +            goto unref_backing;
>> +        }
>> +        ret = blk_make_empty(old_backing_blk, &local_err);
> 
> New code: Call blk_make_empty() whether or not driver has the callback,
> then deal with the fallout.
> 
>> +        blk_unref(old_backing_blk);
>> +        if (ret == -ENOTSUP) {
>> +            error_free(local_err);
>> +            local_err = NULL;
>> +        } else if (ret < 0) {
>>               goto unref_backing;
>>           }
> 
> But this actually looks smarter than the commit case.
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]