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 <pbutsy...@virtuozzo.com>
>>> ---
>>> 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 <mre...@redhat.com>
>>
>> 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
