[Qemu-devel] [PATCH] Discard old bitmap directories in QCOW2 image

[Qemu-devel] [PATCH] Discard old bitmap directories in QCOW2 image

[Andrey Shinkevich]

Clean QCOW2 image from bitmap obsolete directory when a new one
is allocated and stored. It slows down the image growth a little bit.
The flag QCOW2_DISCARD_ALWAYS allows a call to raw_co_pdiscard()
that does the actual cleaning of the image on disk.
With the flag QCOW2_DISCARD_OTHER, a reference count of the cluster
is updated only.

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 block/qcow2-bitmap.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index b946301..682cb09 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -202,7 +202,7 @@ static void clear_bitmap_table(BlockDriverState *bs, uint64_t *bitmap_table,
             continue;
         }
 
-        qcow2_free_clusters(bs, addr, s->cluster_size, QCOW2_DISCARD_OTHER);
+        qcow2_free_clusters(bs, addr, s->cluster_size, QCOW2_DISCARD_ALWAYS);
         bitmap_table[i] = 0;
     }
 }
@@ -257,7 +257,7 @@ static int free_bitmap_clusters(BlockDriverState *bs, Qcow2BitmapTable *tb)
 
     clear_bitmap_table(bs, bitmap_table, tb->size);
     qcow2_free_clusters(bs, tb->offset, tb->size * sizeof(uint64_t),
-                        QCOW2_DISCARD_OTHER);
+                        QCOW2_DISCARD_ALWAYS);
     g_free(bitmap_table);
 
     tb->offset = 0;
@@ -801,7 +801,7 @@ fail:
     g_free(dir);
 
     if (!in_place && dir_offset > 0) {
-        qcow2_free_clusters(bs, dir_offset, dir_size, QCOW2_DISCARD_OTHER);
+        qcow2_free_clusters(bs, dir_offset, dir_size, QCOW2_DISCARD_ALWAYS);
     }
 
     return ret;
@@ -904,14 +904,14 @@ static int update_ext_header_and_dir(BlockDriverState *bs,
     }
 
     if (old_size > 0) {
-        qcow2_free_clusters(bs, old_offset, old_size, QCOW2_DISCARD_OTHER);
+        qcow2_free_clusters(bs, old_offset, old_size, QCOW2_DISCARD_ALWAYS);
     }
 
     return 0;
 
 fail:
     if (new_offset > 0) {
-        qcow2_free_clusters(bs, new_offset, new_size, QCOW2_DISCARD_OTHER);
+        qcow2_free_clusters(bs, new_offset, new_size, QCOW2_DISCARD_ALWAYS);
     }
 
     s->bitmap_directory_offset = old_offset;
@@ -1242,7 +1242,7 @@ fail:
 
     if (tb_offset > 0) {
         qcow2_free_clusters(bs, tb_offset, tb_size * sizeof(tb[0]),
-                            QCOW2_DISCARD_OTHER);
+                            QCOW2_DISCARD_ALWAYS);
     }
 
     g_free(tb);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] Discard old bitmap directories in QCOW2 image

[Vladimir Sementsov-Ogievskiy]

12.02.2019 15:35, Andrey Shinkevich wrote:
> Clean QCOW2 image from bitmap obsolete directory when a new one
> is allocated and stored. It slows down the image growth a little bit.
> The flag QCOW2_DISCARD_ALWAYS allows a call to raw_co_pdiscard()
> that does the actual cleaning of the image on disk.
> With the flag QCOW2_DISCARD_OTHER, a reference count of the cluster
> is updated only.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

side question: should not we change discard_passthrough[QCOW2_DISCARD_OTHER] to
true or at least flags&BDRV_O_UNMAP by default? What is the reason of not discarding
things in qcow2-cluster?

> ---
>   block/qcow2-bitmap.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index b946301..682cb09 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -202,7 +202,7 @@ static void clear_bitmap_table(BlockDriverState *bs, uint64_t *bitmap_table,
>               continue;
>           }
>   
> -        qcow2_free_clusters(bs, addr, s->cluster_size, QCOW2_DISCARD_OTHER);
> +        qcow2_free_clusters(bs, addr, s->cluster_size, QCOW2_DISCARD_ALWAYS);
>           bitmap_table[i] = 0;
>       }
>   }
> @@ -257,7 +257,7 @@ static int free_bitmap_clusters(BlockDriverState *bs, Qcow2BitmapTable *tb)
>   
>       clear_bitmap_table(bs, bitmap_table, tb->size);
>       qcow2_free_clusters(bs, tb->offset, tb->size * sizeof(uint64_t),
> -                        QCOW2_DISCARD_OTHER);
> +                        QCOW2_DISCARD_ALWAYS);
>       g_free(bitmap_table);
>   
>       tb->offset = 0;
> @@ -801,7 +801,7 @@ fail:
>       g_free(dir);
>   
>       if (!in_place && dir_offset > 0) {
> -        qcow2_free_clusters(bs, dir_offset, dir_size, QCOW2_DISCARD_OTHER);
> +        qcow2_free_clusters(bs, dir_offset, dir_size, QCOW2_DISCARD_ALWAYS);
>       }
>   
>       return ret;
> @@ -904,14 +904,14 @@ static int update_ext_header_and_dir(BlockDriverState *bs,
>       }
>   
>       if (old_size > 0) {
> -        qcow2_free_clusters(bs, old_offset, old_size, QCOW2_DISCARD_OTHER);
> +        qcow2_free_clusters(bs, old_offset, old_size, QCOW2_DISCARD_ALWAYS);
>       }
>   
>       return 0;
>   
>   fail:
>       if (new_offset > 0) {
> -        qcow2_free_clusters(bs, new_offset, new_size, QCOW2_DISCARD_OTHER);
> +        qcow2_free_clusters(bs, new_offset, new_size, QCOW2_DISCARD_ALWAYS);
>       }
>   
>       s->bitmap_directory_offset = old_offset;
> @@ -1242,7 +1242,7 @@ fail:
>   
>       if (tb_offset > 0) {
>           qcow2_free_clusters(bs, tb_offset, tb_size * sizeof(tb[0]),
> -                            QCOW2_DISCARD_OTHER);
> +                            QCOW2_DISCARD_ALWAYS);
>       }
>   
>       g_free(tb);
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH] Discard old bitmap directories in QCOW2 image

[Andrey Shinkevich]

PINGing
Please review this simple patch.

On 12/02/2019 15:35, Andrey Shinkevich wrote:
> Clean QCOW2 image from bitmap obsolete directory when a new one
> is allocated and stored. It slows down the image growth a little bit.
> The flag QCOW2_DISCARD_ALWAYS allows a call to raw_co_pdiscard()
> that does the actual cleaning of the image on disk.
> With the flag QCOW2_DISCARD_OTHER, a reference count of the cluster
> is updated only.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   block/qcow2-bitmap.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index b946301..682cb09 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -202,7 +202,7 @@ static void clear_bitmap_table(BlockDriverState *bs, uint64_t *bitmap_table,
>               continue;
>           }
>   
> -        qcow2_free_clusters(bs, addr, s->cluster_size, QCOW2_DISCARD_OTHER);
> +        qcow2_free_clusters(bs, addr, s->cluster_size, QCOW2_DISCARD_ALWAYS);
>           bitmap_table[i] = 0;
>       }
>   }
> @@ -257,7 +257,7 @@ static int free_bitmap_clusters(BlockDriverState *bs, Qcow2BitmapTable *tb)
>   
>       clear_bitmap_table(bs, bitmap_table, tb->size);
>       qcow2_free_clusters(bs, tb->offset, tb->size * sizeof(uint64_t),
> -                        QCOW2_DISCARD_OTHER);
> +                        QCOW2_DISCARD_ALWAYS);
>       g_free(bitmap_table);
>   
>       tb->offset = 0;
> @@ -801,7 +801,7 @@ fail:
>       g_free(dir);
>   
>       if (!in_place && dir_offset > 0) {
> -        qcow2_free_clusters(bs, dir_offset, dir_size, QCOW2_DISCARD_OTHER);
> +        qcow2_free_clusters(bs, dir_offset, dir_size, QCOW2_DISCARD_ALWAYS);
>       }
>   
>       return ret;
> @@ -904,14 +904,14 @@ static int update_ext_header_and_dir(BlockDriverState *bs,
>       }
>   
>       if (old_size > 0) {
> -        qcow2_free_clusters(bs, old_offset, old_size, QCOW2_DISCARD_OTHER);
> +        qcow2_free_clusters(bs, old_offset, old_size, QCOW2_DISCARD_ALWAYS);
>       }
>   
>       return 0;
>   
>   fail:
>       if (new_offset > 0) {
> -        qcow2_free_clusters(bs, new_offset, new_size, QCOW2_DISCARD_OTHER);
> +        qcow2_free_clusters(bs, new_offset, new_size, QCOW2_DISCARD_ALWAYS);
>       }
>   
>       s->bitmap_directory_offset = old_offset;
> @@ -1242,7 +1242,7 @@ fail:
>   
>       if (tb_offset > 0) {
>           qcow2_free_clusters(bs, tb_offset, tb_size * sizeof(tb[0]),
> -                            QCOW2_DISCARD_OTHER);
> +                            QCOW2_DISCARD_ALWAYS);
>       }
>   
>       g_free(tb);
> 

-- 
With the best regards,
Andrey Shinkevich

Re: [Qemu-devel] [PATCH] Discard old bitmap directories in QCOW2 image

[Max Reitz]

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

On 18.02.19 16:36, Vladimir Sementsov-Ogievskiy wrote:
> 12.02.2019 15:35, Andrey Shinkevich wrote:
>> Clean QCOW2 image from bitmap obsolete directory when a new one
>> is allocated and stored. It slows down the image growth a little bit.
>> The flag QCOW2_DISCARD_ALWAYS allows a call to raw_co_pdiscard()
>> that does the actual cleaning of the image on disk.
>> With the flag QCOW2_DISCARD_OTHER, a reference count of the cluster
>> is updated only.
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> side question: should not we change discard_passthrough[QCOW2_DISCARD_OTHER] to
> true or at least flags&BDRV_O_UNMAP by default? What is the reason of not discarding
> things in qcow2-cluster?

As far as I remember the reason is that whenever you clean up something
its cluster is probably going to be reused rather soon.  So cleaning up
takes longer, repopulating that cluster takes longer, and you save only
rather little space.

This is also why I don't know whether this patch makes much sense.

Max


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

Re: [Qemu-devel] [PATCH] Discard old bitmap directories in QCOW2 image

[Denis Lunev]

On 2/27/19 4:00 PM, Max Reitz wrote:
> On 18.02.19 16:36, Vladimir Sementsov-Ogievskiy wrote:
>> 12.02.2019 15:35, Andrey Shinkevich wrote:
>>> Clean QCOW2 image from bitmap obsolete directory when a new one
>>> is allocated and stored. It slows down the image growth a little bit.
>>> The flag QCOW2_DISCARD_ALWAYS allows a call to raw_co_pdiscard()
>>> that does the actual cleaning of the image on disk.
>>> With the flag QCOW2_DISCARD_OTHER, a reference count of the cluster
>>> is updated only.
>>>
>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>> side question: should not we change discard_passthrough[QCOW2_DISCARD_OTHER] to
>> true or at least flags&BDRV_O_UNMAP by default? What is the reason of not discarding
>> things in qcow2-cluster?
> As far as I remember the reason is that whenever you clean up something
> its cluster is probably going to be reused rather soon.  So cleaning up
> takes longer, repopulating that cluster takes longer, and you save only
> rather little space.
>
> This is also why I don't know whether this patch makes much sense.
>
> Max
>
This depands upon the amount of actually free clusters in the image.
If there a lot of them at the moment, this one could be refilled
any time later on.

Den

Re: [Qemu-devel] [PATCH] Discard old bitmap directories in QCOW2 image

[Max Reitz]

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

On 27.02.19 14:16, Denis Lunev wrote:
> On 2/27/19 4:00 PM, Max Reitz wrote:
>> On 18.02.19 16:36, Vladimir Sementsov-Ogievskiy wrote:
>>> 12.02.2019 15:35, Andrey Shinkevich wrote:
>>>> Clean QCOW2 image from bitmap obsolete directory when a new one
>>>> is allocated and stored. It slows down the image growth a little bit.
>>>> The flag QCOW2_DISCARD_ALWAYS allows a call to raw_co_pdiscard()
>>>> that does the actual cleaning of the image on disk.
>>>> With the flag QCOW2_DISCARD_OTHER, a reference count of the cluster
>>>> is updated only.
>>>>
>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>
>>> side question: should not we change discard_passthrough[QCOW2_DISCARD_OTHER] to
>>> true or at least flags&BDRV_O_UNMAP by default? What is the reason of not discarding
>>> things in qcow2-cluster?
>> As far as I remember the reason is that whenever you clean up something
>> its cluster is probably going to be reused rather soon.  So cleaning up
>> takes longer, repopulating that cluster takes longer, and you save only
>> rather little space.
>>
>> This is also why I don't know whether this patch makes much sense.
>>
>> Max
>>
> This depands upon the amount of actually free clusters in the image.
> If there a lot of them at the moment, this one could be refilled
> any time later on.

Well, it's a trade-off.  For small structures like the bitmap directory,
I don't see the point in forcefully unmapping them.  We don't do it for
the old L1 table either when that has to be resized.

Discarding a whole bitmap (like in clear_bitmap_table()) is something
else, though.  So I was too quick to dismiss the whole patch; parts of
it do make sense.

For bitmap tables...  I don't know exactly.  I don't think a bitmap
table is ever going to be larger than the L1 table, so it should
probably be handled the same way -- which is to keep QCOW2_DISCARD_OTHER.

Max


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

Re: [Qemu-devel] [PATCH] Discard old bitmap directories in QCOW2 image

[Andrey Shinkevich]

On 27/02/2019 16:25, Max Reitz wrote:
> On 27.02.19 14:16, Denis Lunev wrote:
>> On 2/27/19 4:00 PM, Max Reitz wrote:
>>> On 18.02.19 16:36, Vladimir Sementsov-Ogievskiy wrote:
>>>> 12.02.2019 15:35, Andrey Shinkevich wrote:
>>>>> Clean QCOW2 image from bitmap obsolete directory when a new one
>>>>> is allocated and stored. It slows down the image growth a little bit.
>>>>> The flag QCOW2_DISCARD_ALWAYS allows a call to raw_co_pdiscard()
>>>>> that does the actual cleaning of the image on disk.
>>>>> With the flag QCOW2_DISCARD_OTHER, a reference count of the cluster
>>>>> is updated only.
>>>>>
>>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>
>>>> side question: should not we change discard_passthrough[QCOW2_DISCARD_OTHER] to
>>>> true or at least flags&BDRV_O_UNMAP by default? What is the reason of not discarding
>>>> things in qcow2-cluster?
>>> As far as I remember the reason is that whenever you clean up something
>>> its cluster is probably going to be reused rather soon.  So cleaning up
>>> takes longer, repopulating that cluster takes longer, and you save only
>>> rather little space.
>>>
>>> This is also why I don't know whether this patch makes much sense.
>>>
>>> Max
>>>
>> This depands upon the amount of actually free clusters in the image.
>> If there a lot of them at the moment, this one could be refilled
>> any time later on.
> 
> Well, it's a trade-off.  For small structures like the bitmap directory,
> I don't see the point in forcefully unmapping them.  We don't do it for
> the old L1 table either when that has to be resized.
> 
> Discarding a whole bitmap (like in clear_bitmap_table()) is something
> else, though.  So I was too quick to dismiss the whole patch; parts of
> it do make sense.
> 
> For bitmap tables...  I don't know exactly.  I don't think a bitmap
> table is ever going to be larger than the L1 table, so it should
> probably be handled the same way -- which is to keep QCOW2_DISCARD_OTHER.
> 
> Max
> 
Yes, Max. All what you wrote about do make the sense.
The reason behind the proposition to discard the obsolete bitmap
directories in the image is that it makes debugging of the iotests
easier for a developer. Otherwise, lots of irrelevant symbolic
information in the image forces to track which one is valid.
Particularly, I locate the byte that designates the bitmap flags to see
if the flag 'in-use' is not set. And I do that for every single bitmap
directory in the image.
Again, cleaning up the obsolete bitmap directories in the image serves
for the convenience of debugging. What do you think about it?
-- 
With the best regards,
Andrey Shinkevich

Re: [Qemu-devel] [PATCH] Discard old bitmap directories in QCOW2 image

[Max Reitz]

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

On 27.02.19 16:15, Andrey Shinkevich wrote:
> 
> 
> On 27/02/2019 16:25, Max Reitz wrote:
>> On 27.02.19 14:16, Denis Lunev wrote:
>>> On 2/27/19 4:00 PM, Max Reitz wrote:
>>>> On 18.02.19 16:36, Vladimir Sementsov-Ogievskiy wrote:
>>>>> 12.02.2019 15:35, Andrey Shinkevich wrote:
>>>>>> Clean QCOW2 image from bitmap obsolete directory when a new one
>>>>>> is allocated and stored. It slows down the image growth a little bit.
>>>>>> The flag QCOW2_DISCARD_ALWAYS allows a call to raw_co_pdiscard()
>>>>>> that does the actual cleaning of the image on disk.
>>>>>> With the flag QCOW2_DISCARD_OTHER, a reference count of the cluster
>>>>>> is updated only.
>>>>>>
>>>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>
>>>>> side question: should not we change discard_passthrough[QCOW2_DISCARD_OTHER] to
>>>>> true or at least flags&BDRV_O_UNMAP by default? What is the reason of not discarding
>>>>> things in qcow2-cluster?
>>>> As far as I remember the reason is that whenever you clean up something
>>>> its cluster is probably going to be reused rather soon.  So cleaning up
>>>> takes longer, repopulating that cluster takes longer, and you save only
>>>> rather little space.
>>>>
>>>> This is also why I don't know whether this patch makes much sense.
>>>>
>>>> Max
>>>>
>>> This depands upon the amount of actually free clusters in the image.
>>> If there a lot of them at the moment, this one could be refilled
>>> any time later on.
>>
>> Well, it's a trade-off.  For small structures like the bitmap directory,
>> I don't see the point in forcefully unmapping them.  We don't do it for
>> the old L1 table either when that has to be resized.
>>
>> Discarding a whole bitmap (like in clear_bitmap_table()) is something
>> else, though.  So I was too quick to dismiss the whole patch; parts of
>> it do make sense.
>>
>> For bitmap tables...  I don't know exactly.  I don't think a bitmap
>> table is ever going to be larger than the L1 table, so it should
>> probably be handled the same way -- which is to keep QCOW2_DISCARD_OTHER.
>>
>> Max
>>
> Yes, Max. All what you wrote about do make the sense.
> The reason behind the proposition to discard the obsolete bitmap
> directories in the image is that it makes debugging of the iotests
> easier for a developer. Otherwise, lots of irrelevant symbolic
> information in the image forces to track which one is valid.
> Particularly, I locate the byte that designates the bitmap flags to see
> if the flag 'in-use' is not set. And I do that for every single bitmap
> directory in the image.
> Again, cleaning up the obsolete bitmap directories in the image serves
> for the convenience of debugging. What do you think about it?

I don't think convenience of debugging is a good reason to implement
something that is probably worse behavior when not debugging.  Of
course, if the changed behavior is neither worse nor better, then we
might as well go for it if it eases debugging.  But I think it is a bit
worse.

Also, if you want to debug images you have created yourself (or at least
are used by VMs you have control over), you can simply set
pass-discard-other=on.

Max


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

Re: [Qemu-devel] [PATCH] Discard old bitmap directories in QCOW2 image

[Andrey Shinkevich]

On 27/02/2019 18:22, Max Reitz wrote:
> On 27.02.19 16:15, Andrey Shinkevich wrote:
>>
>>
>> On 27/02/2019 16:25, Max Reitz wrote:
>>> On 27.02.19 14:16, Denis Lunev wrote:
>>>> On 2/27/19 4:00 PM, Max Reitz wrote:
>>>>> On 18.02.19 16:36, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> 12.02.2019 15:35, Andrey Shinkevich wrote:
>>>>>>> Clean QCOW2 image from bitmap obsolete directory when a new one
>>>>>>> is allocated and stored. It slows down the image growth a little bit.
>>>>>>> The flag QCOW2_DISCARD_ALWAYS allows a call to raw_co_pdiscard()
>>>>>>> that does the actual cleaning of the image on disk.
>>>>>>> With the flag QCOW2_DISCARD_OTHER, a reference count of the cluster
>>>>>>> is updated only.
>>>>>>>
>>>>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>>
>>>>>> side question: should not we change discard_passthrough[QCOW2_DISCARD_OTHER] to
>>>>>> true or at least flags&BDRV_O_UNMAP by default? What is the reason of not discarding
>>>>>> things in qcow2-cluster?
>>>>> As far as I remember the reason is that whenever you clean up something
>>>>> its cluster is probably going to be reused rather soon.  So cleaning up
>>>>> takes longer, repopulating that cluster takes longer, and you save only
>>>>> rather little space.
>>>>>
>>>>> This is also why I don't know whether this patch makes much sense.
>>>>>
>>>>> Max
>>>>>
>>>> This depands upon the amount of actually free clusters in the image.
>>>> If there a lot of them at the moment, this one could be refilled
>>>> any time later on.
>>>
>>> Well, it's a trade-off.  For small structures like the bitmap directory,
>>> I don't see the point in forcefully unmapping them.  We don't do it for
>>> the old L1 table either when that has to be resized.
>>>
>>> Discarding a whole bitmap (like in clear_bitmap_table()) is something
>>> else, though.  So I was too quick to dismiss the whole patch; parts of
>>> it do make sense.
>>>
>>> For bitmap tables...  I don't know exactly.  I don't think a bitmap
>>> table is ever going to be larger than the L1 table, so it should
>>> probably be handled the same way -- which is to keep QCOW2_DISCARD_OTHER.
>>>
>>> Max
>>>
>> Yes, Max. All what you wrote about do make the sense.
>> The reason behind the proposition to discard the obsolete bitmap
>> directories in the image is that it makes debugging of the iotests
>> easier for a developer. Otherwise, lots of irrelevant symbolic
>> information in the image forces to track which one is valid.
>> Particularly, I locate the byte that designates the bitmap flags to see
>> if the flag 'in-use' is not set. And I do that for every single bitmap
>> directory in the image.
>> Again, cleaning up the obsolete bitmap directories in the image serves
>> for the convenience of debugging. What do you think about it?
> 
> I don't think convenience of debugging is a good reason to implement
> something that is probably worse behavior when not debugging.  Of
> course, if the changed behavior is neither worse nor better, then we
> might as well go for it if it eases debugging.  But I think it is a bit
> worse.
> 
> Also, if you want to debug images you have created yourself (or at least
> are used by VMs you have control over), you can simply set
> pass-discard-other=on.
> 
> Max
> 
Thank you for your answer. So, I will leave the QCOW2_DISCARD_ALWAYS
in the clear_bitmap_table() only and will prepare the next version of
the patch, shall I?
-- 
With the best regards,
Andrey Shinkevich

Re: [Qemu-devel] [PATCH] Discard old bitmap directories in QCOW2 image

[Max Reitz]

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

On 27.02.19 16:48, Andrey Shinkevich wrote:
> 
> 
> On 27/02/2019 18:22, Max Reitz wrote:
>> On 27.02.19 16:15, Andrey Shinkevich wrote:
>>>
>>>
>>> On 27/02/2019 16:25, Max Reitz wrote:
>>>> On 27.02.19 14:16, Denis Lunev wrote:
>>>>> On 2/27/19 4:00 PM, Max Reitz wrote:
>>>>>> On 18.02.19 16:36, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>> 12.02.2019 15:35, Andrey Shinkevich wrote:
>>>>>>>> Clean QCOW2 image from bitmap obsolete directory when a new one
>>>>>>>> is allocated and stored. It slows down the image growth a little bit.
>>>>>>>> The flag QCOW2_DISCARD_ALWAYS allows a call to raw_co_pdiscard()
>>>>>>>> that does the actual cleaning of the image on disk.
>>>>>>>> With the flag QCOW2_DISCARD_OTHER, a reference count of the cluster
>>>>>>>> is updated only.
>>>>>>>>
>>>>>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>>>
>>>>>>> side question: should not we change discard_passthrough[QCOW2_DISCARD_OTHER] to
>>>>>>> true or at least flags&BDRV_O_UNMAP by default? What is the reason of not discarding
>>>>>>> things in qcow2-cluster?
>>>>>> As far as I remember the reason is that whenever you clean up something
>>>>>> its cluster is probably going to be reused rather soon.  So cleaning up
>>>>>> takes longer, repopulating that cluster takes longer, and you save only
>>>>>> rather little space.
>>>>>>
>>>>>> This is also why I don't know whether this patch makes much sense.
>>>>>>
>>>>>> Max
>>>>>>
>>>>> This depands upon the amount of actually free clusters in the image.
>>>>> If there a lot of them at the moment, this one could be refilled
>>>>> any time later on.
>>>>
>>>> Well, it's a trade-off.  For small structures like the bitmap directory,
>>>> I don't see the point in forcefully unmapping them.  We don't do it for
>>>> the old L1 table either when that has to be resized.
>>>>
>>>> Discarding a whole bitmap (like in clear_bitmap_table()) is something
>>>> else, though.  So I was too quick to dismiss the whole patch; parts of
>>>> it do make sense.
>>>>
>>>> For bitmap tables...  I don't know exactly.  I don't think a bitmap
>>>> table is ever going to be larger than the L1 table, so it should
>>>> probably be handled the same way -- which is to keep QCOW2_DISCARD_OTHER.
>>>>
>>>> Max
>>>>
>>> Yes, Max. All what you wrote about do make the sense.
>>> The reason behind the proposition to discard the obsolete bitmap
>>> directories in the image is that it makes debugging of the iotests
>>> easier for a developer. Otherwise, lots of irrelevant symbolic
>>> information in the image forces to track which one is valid.
>>> Particularly, I locate the byte that designates the bitmap flags to see
>>> if the flag 'in-use' is not set. And I do that for every single bitmap
>>> directory in the image.
>>> Again, cleaning up the obsolete bitmap directories in the image serves
>>> for the convenience of debugging. What do you think about it?
>>
>> I don't think convenience of debugging is a good reason to implement
>> something that is probably worse behavior when not debugging.  Of
>> course, if the changed behavior is neither worse nor better, then we
>> might as well go for it if it eases debugging.  But I think it is a bit
>> worse.
>>
>> Also, if you want to debug images you have created yourself (or at least
>> are used by VMs you have control over), you can simply set
>> pass-discard-other=on.
>>
>> Max
>>
> Thank you for your answer. So, I will leave the QCOW2_DISCARD_ALWAYS
> in the clear_bitmap_table() only and will prepare the next version of
> the patch, shall I?

(Sorry for the late reply, I was on vacation :-/)

Sounds good to me, yes.

Max


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

Re: [Qemu-devel] [PATCH] Discard old bitmap directories in QCOW2 image

[Andrey Shinkevich]

On 01/04/2019 21:00, Max Reitz wrote:
> On 27.02.19 16:48, Andrey Shinkevich wrote:
>>
>>
>> On 27/02/2019 18:22, Max Reitz wrote:
>>> On 27.02.19 16:15, Andrey Shinkevich wrote:
>>>>
>>>>
>>>> On 27/02/2019 16:25, Max Reitz wrote:
>>>>> On 27.02.19 14:16, Denis Lunev wrote:
>>>>>> On 2/27/19 4:00 PM, Max Reitz wrote:
>>>>>>> On 18.02.19 16:36, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>>> 12.02.2019 15:35, Andrey Shinkevich wrote:
>>>>>>>>> Clean QCOW2 image from bitmap obsolete directory when a new one
>>>>>>>>> is allocated and stored. It slows down the image growth a little bit.
>>>>>>>>> The flag QCOW2_DISCARD_ALWAYS allows a call to raw_co_pdiscard()
>>>>>>>>> that does the actual cleaning of the image on disk.
>>>>>>>>> With the flag QCOW2_DISCARD_OTHER, a reference count of the cluster
>>>>>>>>> is updated only.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>>>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>>>>
>>>>>>>> side question: should not we change discard_passthrough[QCOW2_DISCARD_OTHER] to
>>>>>>>> true or at least flags&BDRV_O_UNMAP by default? What is the reason of not discarding
>>>>>>>> things in qcow2-cluster?
>>>>>>> As far as I remember the reason is that whenever you clean up something
>>>>>>> its cluster is probably going to be reused rather soon.  So cleaning up
>>>>>>> takes longer, repopulating that cluster takes longer, and you save only
>>>>>>> rather little space.
>>>>>>>
>>>>>>> This is also why I don't know whether this patch makes much sense.
>>>>>>>
>>>>>>> Max
>>>>>>>
>>>>>> This depands upon the amount of actually free clusters in the image.
>>>>>> If there a lot of them at the moment, this one could be refilled
>>>>>> any time later on.
>>>>>
>>>>> Well, it's a trade-off.  For small structures like the bitmap directory,
>>>>> I don't see the point in forcefully unmapping them.  We don't do it for
>>>>> the old L1 table either when that has to be resized.
>>>>>
>>>>> Discarding a whole bitmap (like in clear_bitmap_table()) is something
>>>>> else, though.  So I was too quick to dismiss the whole patch; parts of
>>>>> it do make sense.
>>>>>
>>>>> For bitmap tables...  I don't know exactly.  I don't think a bitmap
>>>>> table is ever going to be larger than the L1 table, so it should
>>>>> probably be handled the same way -- which is to keep QCOW2_DISCARD_OTHER.
>>>>>
>>>>> Max
>>>>>
>>>> Yes, Max. All what you wrote about do make the sense.
>>>> The reason behind the proposition to discard the obsolete bitmap
>>>> directories in the image is that it makes debugging of the iotests
>>>> easier for a developer. Otherwise, lots of irrelevant symbolic
>>>> information in the image forces to track which one is valid.
>>>> Particularly, I locate the byte that designates the bitmap flags to see
>>>> if the flag 'in-use' is not set. And I do that for every single bitmap
>>>> directory in the image.
>>>> Again, cleaning up the obsolete bitmap directories in the image serves
>>>> for the convenience of debugging. What do you think about it?
>>>
>>> I don't think convenience of debugging is a good reason to implement
>>> something that is probably worse behavior when not debugging.  Of
>>> course, if the changed behavior is neither worse nor better, then we
>>> might as well go for it if it eases debugging.  But I think it is a bit
>>> worse.
>>>
>>> Also, if you want to debug images you have created yourself (or at least
>>> are used by VMs you have control over), you can simply set
>>> pass-discard-other=on.
>>>
>>> Max
>>>
>> Thank you for your answer. So, I will leave the QCOW2_DISCARD_ALWAYS
>> in the clear_bitmap_table() only and will prepare the next version of
>> the patch, shall I?
> 
> (Sorry for the late reply, I was on vacation :-/)
> 
> Sounds good to me, yes.
> 
> Max
> 

Hello, Max!
Welcome back.
I sent the updated patch on 28/02/2019, the message ID is
<1551346019-293202-1-git-send-email-andrey.shinkevich@virtuozzo.com>
-- 
With the best regards,
Andrey Shinkevich