Re: [Qemu-devel] [PATCH v2 2/2] qcow2: truncate the tail of the image file after shrinking the image

[Max Reitz]

On 2017-09-27 18:27, Pavel Butsykin wrote:
> On 27.09.2017 19:00, Max Reitz wrote:
>> On 2017-09-22 11:39, Pavel Butsykin wrote:
>>> Now after shrinking the image, at the end of the image file, there
>>> might be a
>>> tail that probably will never be used. So we can find the last used
>>> cluster and
>>> cut the tail.
>>>
>>> Signed-off-by: Pavel Butsykin <address@hidden>
>>> ---
>>>   block/qcow2-refcount.c | 22 ++++++++++++++++++++++
>>>   block/qcow2.c          | 23 +++++++++++++++++++++++
>>>   block/qcow2.h          |  1 +
>>>   3 files changed, 46 insertions(+)
>>>
>>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>>> index 88d5a3f1ad..aa3fd6cf17 100644
>>> --- a/block/qcow2-refcount.c
>>> +++ b/block/qcow2-refcount.c
>>> @@ -3181,3 +3181,25 @@ out:
>>>       g_free(reftable_tmp);
>>>       return ret;
>>>   }
>>> +
>>> +int64_t qcow2_get_last_cluster(BlockDriverState *bs, int64_t size)
>>> +{
>>> +    BDRVQcow2State *s = bs->opaque;
>>> +    int64_t i;
>>> +
>>> +    for (i = size_to_clusters(s, size) - 1; i >= 0; i--) {
>>> +        uint64_t refcount;
>>> +        int ret = qcow2_get_refcount(bs, i, &refcount);
>>> +        if (ret < 0) {
>>> +            fprintf(stderr, "Can't get refcount for cluster %"
>>> PRId64 ": %s\n",
>>> +                    i, strerror(-ret));
>>> +            return ret;
>>> +        }
>>> +        if (refcount > 0) {
>>> +            return i;
>>> +        }
>>> +    }
>>> +    qcow2_signal_corruption(bs, true, -1, -1,
>>> +                            "There are no references in the refcount
>>> table.");
>>> +    return -EIO;
>>> +}
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index 8a4311d338..8dfb5393a7 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -3106,6 +3106,7 @@ static int qcow2_truncate(BlockDriverState *bs,
>>> int64_t offset,
>>>       new_l1_size = size_to_l1(s, offset);
>>>         if (offset < old_length) {
>>> +        int64_t last_cluster, old_file_size;
>>>           if (prealloc != PREALLOC_MODE_OFF) {
>>>               error_setg(errp,
>>>                          "Preallocation can't be used for shrinking
>>> an image");
>>> @@ -3134,6 +3135,28 @@ static int qcow2_truncate(BlockDriverState
>>> *bs, int64_t offset,
>>>                                "Failed to discard unused refblocks");
>>>               return ret;
>>>           }
>>> +
>>> +        old_file_size = bdrv_getlength(bs->file->bs);
>>> +        if (old_file_size < 0) {
>>> +            error_setg_errno(errp, -old_file_size,
>>> +                             "Failed to inquire current file length");
>>> +            return old_file_size;
>>> +        }
>>> +        last_cluster = qcow2_get_last_cluster(bs, old_file_size);
>>> +        if (last_cluster < 0) {
>>> +            error_setg_errno(errp, -last_cluster,
>>> +                             "Failed to find the last cluster");
>>> +            return last_cluster;
>>> +        }
>>
>> My idea was rather that you just wouldn't truncate the image file if
>> something fails here.  So in any of these new cases where you currently
>> just report the whole truncate operation as having failed, you could
>> just emit a warning and not do the truncation of bs->file.
> 
> I'm not sure that's right. If the qcow2_get_last_cluster() returned an
> error, probably with the image was a problem.. can we continue to work
> with the image without risking to damage it even more? if something bad
> happened with the reftable we usually mark the image as corrupted, it's
> the same thing.

Well, the only thing that's left to do is to write the new size into the
image header, so I think that should work just fine...

I won't disagree that bdrv_getlength() or qcow2_get_last_cluster()
failing may be reasons to stop truncation (although I don't think they
necessarily are at this point).

But I could well imagine that the below bdrv_truncate() of bs->file
fails for benign reasons, e.g. because the underlying protocol does not
support shrinking of images or something.  Then we probably should carry on.

Max

> Although if the shrink is almost complete, the truncation of bs->file
> isn't so important thing and we could update qcow2 header.
> 
>> I can live with the current version, though, so:
>>
>> Reviewed-by: Max Reitz <address@hidden>
>>
>> But I'll wait for a response from you before merging this series.
>>
>> Max
>>
>>> +        if ((last_cluster + 1) * s->cluster_size < old_file_size) {
>>> +            ret = bdrv_truncate(bs->file, (last_cluster + 1) *
>>> s->cluster_size,
>>> +                                PREALLOC_MODE_OFF, NULL);
>>> +            if (ret < 0) {
>>> +                error_setg_errno(errp, -ret,
>>> +                                 "Failed to truncate the tail of the
>>> image");
>>> +                return ret;
>>> +            }
>>> +        }
>>>       } else {
>>>           ret = qcow2_grow_l1_table(bs, new_l1_size, true);
>>>           if (ret < 0) {
>>> diff --git a/block/qcow2.h b/block/qcow2.h
>>> index 5a289a81e2..782a206ecb 100644
>>> --- a/block/qcow2.h
>>> +++ b/block/qcow2.h
>>> @@ -597,6 +597,7 @@ int qcow2_change_refcount_order(BlockDriverState
>>> *bs, int refcount_order,
>>>                                   BlockDriverAmendStatusCB *status_cb,
>>>                                   void *cb_opaque, Error **errp);
>>>   int qcow2_shrink_reftable(BlockDriverState *bs);
>>> +int64_t qcow2_get_last_cluster(BlockDriverState *bs, int64_t size);
>>>     /* qcow2-cluster.c functions */
>>>   int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
>>>
>>
>>

signature.asc
Description: OpenPGP digital signature