[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 4/4] block: Use blk_make_empty() after commits
From: |
Max Reitz |
Subject: |
Re: [PATCH 4/4] block: Use blk_make_empty() after commits |
Date: |
Wed, 29 Apr 2020 09:58:04 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 |
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 <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) {
>
> 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.
>
signature.asc
Description: OpenPGP digital signature
- [PATCH 2/4] block: Use bdrv_make_empty() where possible, (continued)
- [PATCH 2/4] block: Use bdrv_make_empty() where possible, Max Reitz, 2020/04/28
- [PATCH 3/4] block: Add blk_make_empty(), Max Reitz, 2020/04/28
- [PATCH 4/4] block: Use blk_make_empty() after commits, Max Reitz, 2020/04/28
- Re: [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly, no-reply, 2020/04/28
- Re: [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly, no-reply, 2020/04/28
- Re: [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly, Eric Blake, 2020/04/28
- Re: [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly, no-reply, 2020/04/28
- Re: [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly, no-reply, 2020/04/28
- Re: [PATCH 0/4] block: Do not call BlockDriver.bdrv_make_empty() directly, no-reply, 2020/04/28