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

[Qemu-devel] [PATCH 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.

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 | 21 +++++++++++++++++++++
 block/qcow2.c          | 23 +++++++++++++++++++++--
 block/qcow2.h          |  1 +
 3 files changed, 43 insertions(+), 2 deletions(-)

-- 
2.14.1

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

[Pavel Butsykin]

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.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

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

[Eric Blake]

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

On 09/20/2017 08:58 AM, Pavel Butsykin wrote:
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> ---
>  block/qcow2.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Wow, bdrv_truncate() has had problems!  You found fixes unrelated to my
current struggles to fix the dirty-bitmap failure.

Reviewed-by: Eric Blake <eblake@redhat.com>
CC: qemu-stable@nongnu.org

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

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

[John Snow]

On 09/20/2017 09:58 AM, Pavel Butsykin wrote:
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.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,
> 

Yikes...

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

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

[Max Reitz]

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

On 2017-09-20 15:58, Pavel Butsykin wrote:
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> ---
>  block/qcow2.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[Qemu-devel] [PATCH 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 | 21 +++++++++++++++++++++
 block/qcow2.c          | 19 +++++++++++++++++++
 block/qcow2.h          |  1 +
 3 files changed, 41 insertions(+)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 88d5a3f1ad..5e221a166c 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -3181,3 +3181,24 @@ 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, last_cluster, nb_clusters = size_to_clusters(s, size);
+    uint64_t refcount;
+
+    for (i = 0, last_cluster = 0; i < nb_clusters; i++) {
+        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));
+            continue;
+        }
+
+        if (refcount > 0) {
+            last_cluster = i;
+        }
+    }
+    return last_cluster;
+}
diff --git a/block/qcow2.c b/block/qcow2.c
index 8a4311d338..c3b6dd44c4 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 image_end_offset, old_file_size;
         if (prealloc != PREALLOC_MODE_OFF) {
             error_setg(errp,
                        "Preallocation can't be used for shrinking an image");
@@ -3134,6 +3135,24 @@ 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;
+        }
+        image_end_offset = (qcow2_get_last_cluster(bs, old_file_size) + 1) *
+                           s->cluster_size;
+        if (image_end_offset < old_file_size) {
+            ret = bdrv_truncate(bs->file, image_end_offset,
+                                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] [Qemu-block] [PATCH 2/2] qcow2: truncate the tail of the image file after shrinking the image

[John Snow]

On 09/20/2017 09:58 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>
> ---
>  block/qcow2-refcount.c | 21 +++++++++++++++++++++
>  block/qcow2.c          | 19 +++++++++++++++++++
>  block/qcow2.h          |  1 +
>  3 files changed, 41 insertions(+)
> 
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 88d5a3f1ad..5e221a166c 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -3181,3 +3181,24 @@ 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, last_cluster, nb_clusters = size_to_clusters(s, size);
> +    uint64_t refcount;
> +
> +    for (i = 0, last_cluster = 0; i < nb_clusters; i++) {
> +        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));
> +            continue;
> +        }
> +
> +        if (refcount > 0) {
> +            last_cluster = i;
> +        }
> +    }
> +    return last_cluster;
> +}> diff --git a/block/qcow2.c b/block/qcow2.c
> index 8a4311d338..c3b6dd44c4 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 image_end_offset, old_file_size;
>          if (prealloc != PREALLOC_MODE_OFF) {
>              error_setg(errp,
>                         "Preallocation can't be used for shrinking an image");
> @@ -3134,6 +3135,24 @@ 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;
> +        }
> +        image_end_offset = (qcow2_get_last_cluster(bs, old_file_size) + 1) *
> +                           s->cluster_size;
> +        if (image_end_offset < old_file_size) {
> +            ret = bdrv_truncate(bs->file, image_end_offset,
> +                                PREALLOC_MODE_OFF, NULL);
> +            if (ret < 0) {
> +                error_setg_errno(errp, -ret,
> +                                 "Failed to truncate the tail of the image");

I've recently become skeptical of what partial resize successes look
like, but that's an issue for another day entirely.

> +                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,
> 

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

Looks sane to me, but under which circumstances might we grow such a
tail? I assume the actual truncate call aligns to cluster boundaries as
appropriate, so is this a bit of a "quick fix" to cull unused clusters
that happened to be near the truncate boundary?

It might be worth documenting the circumstances that produces this
unused space that will never get used. My hunch is that such unused
space should likely be getting reclaimed elsewhere and not here, but
perhaps I'm misunderstanding the causal factors.

--js

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

[Pavel Butsykin]

On 21.09.2017 00:38, John Snow wrote:
> 
> 
> On 09/20/2017 09:58 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>
>> ---
>>   block/qcow2-refcount.c | 21 +++++++++++++++++++++
>>   block/qcow2.c          | 19 +++++++++++++++++++
>>   block/qcow2.h          |  1 +
>>   3 files changed, 41 insertions(+)
>>
>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>> index 88d5a3f1ad..5e221a166c 100644
>> --- a/block/qcow2-refcount.c
>> +++ b/block/qcow2-refcount.c
>> @@ -3181,3 +3181,24 @@ 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, last_cluster, nb_clusters = size_to_clusters(s, size);
>> +    uint64_t refcount;
>> +
>> +    for (i = 0, last_cluster = 0; i < nb_clusters; i++) {
>> +        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));
>> +            continue;
>> +        }
>> +
>> +        if (refcount > 0) {
>> +            last_cluster = i;
>> +        }
>> +    }
>> +    return last_cluster;
>> +}> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 8a4311d338..c3b6dd44c4 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 image_end_offset, old_file_size;
>>           if (prealloc != PREALLOC_MODE_OFF) {
>>               error_setg(errp,
>>                          "Preallocation can't be used for shrinking an image");
>> @@ -3134,6 +3135,24 @@ 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;
>> +        }
>> +        image_end_offset = (qcow2_get_last_cluster(bs, old_file_size) + 1) *
>> +                           s->cluster_size;
>> +        if (image_end_offset < old_file_size) {
>> +            ret = bdrv_truncate(bs->file, image_end_offset,
>> +                                PREALLOC_MODE_OFF, NULL);
>> +            if (ret < 0) {
>> +                error_setg_errno(errp, -ret,
>> +                                 "Failed to truncate the tail of the image");
> 
> I've recently become skeptical of what partial resize successes look
> like, but that's an issue for another day entirely.
> 
>> +                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,
>>
> 
> Reviewed-by: John Snow <jsnow@redhat.com>
> 
> Looks sane to me, but under which circumstances might we grow such a
> tail? I assume the actual truncate call aligns to cluster boundaries as
> appropriate, so is this a bit of a "quick fix" to cull unused clusters
> that happened to be near the truncate boundary?
> 
> It might be worth documenting the circumstances that produces this
> unused space that will never get used. My hunch is that such unused
> space should likely be getting reclaimed elsewhere and not here, but
> perhaps I'm misunderstanding the causal factors.
>

This is a consequence of how we implemented shrinking the qcow2 image.
(https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg04580.html)
But on the other hand, if we need to shrink the qcow2 image without
copying the data, this is the only way. The same guest offset can be
converted to almost any host offset in the file i.e. the first guest
cluster may be located somewhere at the end or the middle of the image
file. So we can't just take and truncate the image file on the border of
the truncation, therefore to shrink the image we just discard the
clusters that corresponds to the truncated area. The result is a
sparse image file where the apparent file size differs from actual size.
And the tail in this case is the difference between the actual size and
last used cluster in the image, so in fact the cutting of the tail does
not change the apparent file size.

> --js
> 

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

[John Snow]

On 09/21/2017 05:49 AM, Pavel Butsykin wrote:
> On 21.09.2017 00:38, John Snow wrote:
>>
>>
>> On 09/20/2017 09:58 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>
>>> ---
>>>   block/qcow2-refcount.c | 21 +++++++++++++++++++++
>>>   block/qcow2.c          | 19 +++++++++++++++++++
>>>   block/qcow2.h          |  1 +
>>>   3 files changed, 41 insertions(+)
>>>
>>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>>> index 88d5a3f1ad..5e221a166c 100644
>>> --- a/block/qcow2-refcount.c
>>> +++ b/block/qcow2-refcount.c
>>> @@ -3181,3 +3181,24 @@ 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, last_cluster, nb_clusters = size_to_clusters(s, size);
>>> +    uint64_t refcount;
>>> +
>>> +    for (i = 0, last_cluster = 0; i < nb_clusters; i++) {
>>> +        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));
>>> +            continue;
>>> +        }
>>> +
>>> +        if (refcount > 0) {
>>> +            last_cluster = i;
>>> +        }
>>> +    }
>>> +    return last_cluster;
>>> +}> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index 8a4311d338..c3b6dd44c4 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 image_end_offset, old_file_size;
>>>           if (prealloc != PREALLOC_MODE_OFF) {
>>>               error_setg(errp,
>>>                          "Preallocation can't be used for shrinking
>>> an image");
>>> @@ -3134,6 +3135,24 @@ 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;
>>> +        }
>>> +        image_end_offset = (qcow2_get_last_cluster(bs,
>>> old_file_size) + 1) *
>>> +                           s->cluster_size;
>>> +        if (image_end_offset < old_file_size) {
>>> +            ret = bdrv_truncate(bs->file, image_end_offset,
>>> +                                PREALLOC_MODE_OFF, NULL);
>>> +            if (ret < 0) {
>>> +                error_setg_errno(errp, -ret,
>>> +                                 "Failed to truncate the tail of the
>>> image");
>>
>> I've recently become skeptical of what partial resize successes look
>> like, but that's an issue for another day entirely.
>>
>>> +                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,
>>>
>>
>> Reviewed-by: John Snow <jsnow@redhat.com>
>>
>> Looks sane to me, but under which circumstances might we grow such a
>> tail? I assume the actual truncate call aligns to cluster boundaries as
>> appropriate, so is this a bit of a "quick fix" to cull unused clusters
>> that happened to be near the truncate boundary?
>>
>> It might be worth documenting the circumstances that produces this
>> unused space that will never get used. My hunch is that such unused
>> space should likely be getting reclaimed elsewhere and not here, but
>> perhaps I'm misunderstanding the causal factors.
>>
> 
> This is a consequence of how we implemented shrinking the qcow2 image.
> (https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg04580.html)
> But on the other hand, if we need to shrink the qcow2 image without
> copying the data, this is the only way. The same guest offset can be
> converted to almost any host offset in the file i.e. the first guest
> cluster may be located somewhere at the end or the middle of the image
> file. So we can't just take and truncate the image file on the border of
> the truncation, therefore to shrink the image we just discard the
> clusters that corresponds to the truncated area. The result is a
> sparse image file where the apparent file size differs from actual size.
> And the tail in this case is the difference between the actual size and
> last used cluster in the image, so in fact the cutting of the tail does
> not change the apparent file size.
> 

Oh, duh, I get it. The truncation itself creates a lot of sparseness.

Thanks for the explanation.

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

[Eric Blake]

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

On 09/21/2017 11:48 AM, John Snow wrote:
>>> Looks sane to me, but under which circumstances might we grow such a
>>> tail? I assume the actual truncate call aligns to cluster boundaries as
>>> appropriate, so is this a bit of a "quick fix" to cull unused clusters
>>> that happened to be near the truncate boundary?
>>>
>>> It might be worth documenting the circumstances that produces this
>>> unused space that will never get used. My hunch is that such unused
>>> space should likely be getting reclaimed elsewhere and not here, but
>>> perhaps I'm misunderstanding the causal factors.
>>>
>>
>> This is a consequence of how we implemented shrinking the qcow2 image.
>> (https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg04580.html)
>> But on the other hand, if we need to shrink the qcow2 image without
>> copying the data, this is the only way. The same guest offset can be
>> converted to almost any host offset in the file i.e. the first guest
>> cluster may be located somewhere at the end or the middle of the image
>> file. So we can't just take and truncate the image file on the border of
>> the truncation, therefore to shrink the image we just discard the
>> clusters that corresponds to the truncated area. The result is a
>> sparse image file where the apparent file size differs from actual size.
>> And the tail in this case is the difference between the actual size and
>> last used cluster in the image, so in fact the cutting of the tail does
>> not change the apparent file size.
>>
> 
> Oh, duh, I get it. The truncation itself creates a lot of sparseness.

It is also interesting to think about whether we should someday
implement a qcow2 defragmenting operation (either a simple one: pull all
later clusters into earlier holes, but the end result is not necessarily
contiguous; or a complex one: shuffle things so that all clusters are in
order, even though it may require moving some clusters around twice), so
that you can remove all holes from the file (perhaps useful if the file
is stored on a system that does not support holes).  But not for this
series :)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

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

[Max Reitz]

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

On 2017-09-20 15:58, 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 | 21 +++++++++++++++++++++
>  block/qcow2.c          | 19 +++++++++++++++++++
>  block/qcow2.h          |  1 +
>  3 files changed, 41 insertions(+)
> 
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 88d5a3f1ad..5e221a166c 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -3181,3 +3181,24 @@ 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, last_cluster, nb_clusters = size_to_clusters(s, size);
> +    uint64_t refcount;
> +
> +    for (i = 0, last_cluster = 0; i < nb_clusters; i++) {
> +        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));
> +            continue;
> +        }
> +
> +        if (refcount > 0) {
> +            last_cluster = i;
> +        }
> +    }
> +    return last_cluster;
> +}

Wouldn't it make more sense to start from the end of the image?

Max


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

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

[Pavel Butsykin]

On 21.09.2017 18:28, Max Reitz wrote:
> On 2017-09-20 15:58, 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 | 21 +++++++++++++++++++++
>>   block/qcow2.c          | 19 +++++++++++++++++++
>>   block/qcow2.h          |  1 +
>>   3 files changed, 41 insertions(+)
>>
>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>> index 88d5a3f1ad..5e221a166c 100644
>> --- a/block/qcow2-refcount.c
>> +++ b/block/qcow2-refcount.c
>> @@ -3181,3 +3181,24 @@ 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, last_cluster, nb_clusters = size_to_clusters(s, size);
>> +    uint64_t refcount;
>> +
>> +    for (i = 0, last_cluster = 0; i < nb_clusters; i++) {
>> +        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));
>> +            continue;
>> +        }
>> +
>> +        if (refcount > 0) {
>> +            last_cluster = i;
>> +        }
>> +    }
>> +    return last_cluster;
>> +}
> 
> Wouldn't it make more sense to start from the end of the image?

If this will reduce the iterations, then yes. But it will depend on the
situation. If you truncate the image more than half, it can increase the
number of iterations. But intuitively it seems that to start from the
end would be better :)
> Max
> 

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

[Max Reitz]

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

On 2017-09-21 18:16, Pavel Butsykin wrote:
> On 21.09.2017 18:28, Max Reitz wrote:
>> On 2017-09-20 15:58, 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 | 21 +++++++++++++++++++++
>>>   block/qcow2.c          | 19 +++++++++++++++++++
>>>   block/qcow2.h          |  1 +
>>>   3 files changed, 41 insertions(+)
>>>
>>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>>> index 88d5a3f1ad..5e221a166c 100644
>>> --- a/block/qcow2-refcount.c
>>> +++ b/block/qcow2-refcount.c
>>> @@ -3181,3 +3181,24 @@ 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, last_cluster, nb_clusters = size_to_clusters(s, size);
>>> +    uint64_t refcount;
>>> +
>>> +    for (i = 0, last_cluster = 0; i < nb_clusters; i++) {
>>> +        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));
>>> +            continue;
>>> +        }
>>> +
>>> +        if (refcount > 0) {
>>> +            last_cluster = i;
>>> +        }
>>> +    }
>>> +    return last_cluster;
>>> +}
>>
>> Wouldn't it make more sense to start from the end of the image?
> 
> If this will reduce the iterations, then yes. But it will depend on the
> situation. If you truncate the image more than half, it can increase the
> number of iterations. But intuitively it seems that to start from the
> end would be better :)

That's one thing (also, I think usually the end should coincide with
some allocated cluster, but yes, that's just intuition :-)); but also,
this would simplify things a bit because we would no longer need the
last_cluster variable (the loop would just stop at the first allocated
cluster).

Max


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

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

[Max Reitz]

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

On 2017-09-20 15:58, 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 | 21 +++++++++++++++++++++
>  block/qcow2.c          | 19 +++++++++++++++++++
>  block/qcow2.h          |  1 +
>  3 files changed, 41 insertions(+)
> 
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 88d5a3f1ad..5e221a166c 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -3181,3 +3181,24 @@ 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, last_cluster, nb_clusters = size_to_clusters(s, size);
> +    uint64_t refcount;
> +
> +    for (i = 0, last_cluster = 0; i < nb_clusters; i++) {
> +        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));
> +            continue;

Oh, and another thing: If you decide to ignore errors here, I'd at least
consider the cluster allocated.

Of course it would also be possible not to ignore errors, and instead
return them to the caller which would then just not truncate the file.

Max

> +        }
> +
> +        if (refcount > 0) {
> +            last_cluster = i;
> +        }
> +    }
> +    return last_cluster;
> +}


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

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

[Pavel Butsykin]

On 21.09.2017 18:30, Max Reitz wrote:
> On 2017-09-20 15:58, 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 | 21 +++++++++++++++++++++
>>   block/qcow2.c          | 19 +++++++++++++++++++
>>   block/qcow2.h          |  1 +
>>   3 files changed, 41 insertions(+)
>>
>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>> index 88d5a3f1ad..5e221a166c 100644
>> --- a/block/qcow2-refcount.c
>> +++ b/block/qcow2-refcount.c
>> @@ -3181,3 +3181,24 @@ 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, last_cluster, nb_clusters = size_to_clusters(s, size);
>> +    uint64_t refcount;
>> +
>> +    for (i = 0, last_cluster = 0; i < nb_clusters; i++) {
>> +        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));
>> +            continue;
> 
> Oh, and another thing: If you decide to ignore errors here, I'd at least
> consider the cluster allocated.
> 
> Of course it would also be possible not to ignore errors, and instead
> return them to the caller which would then just not truncate the file.

Yes, it seems so safer.

> Max
> 
>> +        }
>> +
>> +        if (refcount > 0) {
>> +            last_cluster = i;
>> +        }
>> +    }
>> +    return last_cluster;
>> +}
>