[Qemu-devel] [PATCH v2 0/2] Truncate the tail of the image file in qcow2 shrinking

[Qemu-devel] [PATCH v2 0/2] Truncate the tail of the image file in qcow2 shrinking

[Pavel Butsykin]

Now after shrinking the qcow2 image, at the end of the image file, there might
be a tail that probably will never be used. Although it will not bring any
tangible benefit, we can cut the tail if it is. Yes, it will not free up disk
space, but if the blocks were be allocated sequentially and the image is not
heavily fragmented then the virtual size of the image file will be commensurate
with the real size. It also doesn't look like a great plus.. Well, at least we
can discuss it.

Changes from v1:
- rewrite qcow2_get_last_cluster() function according to Max's comments. (2)

Pavel Butsykin (2):
  qcow2: fix return error code in qcow2_truncate()
  qcow2: truncate the tail of the image file after shrinking the image

 block/qcow2-refcount.c | 22 ++++++++++++++++++++++
 block/qcow2.c          | 27 +++++++++++++++++++++++++--
 block/qcow2.h          |  1 +
 3 files changed, 48 insertions(+), 2 deletions(-)

-- 
2.14.1

[Qemu-devel] [PATCH v2 1/2] qcow2: fix return error code in qcow2_truncate()

[Pavel Butsykin]

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 2174a84d1f..8a4311d338 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3166,7 +3166,7 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset,
         if (old_file_size < 0) {
             error_setg_errno(errp, -old_file_size,
                              "Failed to inquire current file length");
-            return ret;
+            return old_file_size;
         }
 
         nb_new_data_clusters = DIV_ROUND_UP(offset - old_length,
@@ -3195,7 +3195,7 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset,
         if (allocation_start < 0) {
             error_setg_errno(errp, -allocation_start,
                              "Failed to resize refcount structures");
-            return -allocation_start;
+            return allocation_start;
         }
 
         clusters_allocated = qcow2_alloc_clusters_at(bs, allocation_start,
-- 
2.14.1

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

[Pavel Butsykin]

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 <pbutsykin@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;
+        }
+        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,
-- 
2.14.1

Re: [Qemu-devel] [PATCH v2 0/2] Truncate the tail of the image file in qcow2 shrinking

[Daniel P. Berrange]

On Fri, Sep 22, 2017 at 12:39:24PM +0300, Pavel Butsykin wrote:
> Now after shrinking the qcow2 image, at the end of the image file, there might
> be a tail that probably will never be used. Although it will not bring any
> tangible benefit, we can cut the tail if it is. Yes, it will not free up disk
> space, but if the blocks were be allocated sequentially and the image is not
> heavily fragmented then the virtual size of the image file will be commensurate
> with the real size. It also doesn't look like a great plus.. Well, at least we
> can discuss it.

If the block backend has discard support enabled, can't we get the tail
to be discarded rather than merely truncated ?

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 0/2] Truncate the tail of the image file in qcow2 shrinking

[Pavel Butsykin]

On 22.09.2017 12:50, Daniel P. Berrange wrote:
> On Fri, Sep 22, 2017 at 12:39:24PM +0300, Pavel Butsykin wrote:
>> Now after shrinking the qcow2 image, at the end of the image file, there might
>> be a tail that probably will never be used. Although it will not bring any
>> tangible benefit, we can cut the tail if it is. Yes, it will not free up disk
>> space, but if the blocks were be allocated sequentially and the image is not
>> heavily fragmented then the virtual size of the image file will be commensurate
>> with the real size. It also doesn't look like a great plus.. Well, at least we
>> can discuss it.
> 
> If the block backend has discard support enabled, can't we get the tail
> to be discarded rather than merely truncated ?


It has already been implemented. (see 
https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg04581.html)
Sorry, I just forgot to mention that this patch rebased on Max's block
branch (https://github.com/XanClic/qemu/commits/block). Actually the
truncation will always be done on the already discarded area. It can
be useful only if the block backend doesn't support discard or a file
system doesn't support sparse files.

> Regards,
> Daniel
> 

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

[John Snow]

On 09/22/2017 05:39 AM, 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 <pbutsykin@virtuozzo.com>


Reviewed-by: John Snow <jsnow@redhat.com>

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 4372 bytes --]

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 <pbutsykin@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 can live with the current version, though, so:

Reviewed-by: Max Reitz <mreitz@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,
> 



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

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

[Pavel Butsykin]

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

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 <mreitz@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,
>>
> 
> 

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 6413 bytes --]

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 <pbutsykin@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 <mreitz@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,
>>>
>>
>>



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

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

[Pavel Butsykin]

On 27.09.2017 19:36, Max Reitz wrote:
> 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 <pbutsykin@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...

Yes, but what difference will update the size in the header or not, if
the reftable was corrupted. A much more important point here is that the
qcow2_truncate() should return an error and the caller must stop the
work.

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

Yes, I agree here. If the bdrv_truncate() of bs->file failed, we can
print just a warning :) So, I'll send new version of the patch with
this change.

> 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 <mreitz@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,
>>>>
>>>
>>>
> 
>