qcow2: discard bitmap when removed

[Qemu-devel] [PATCH v2] qcow2: discard bitmap when removed

Posted by Andrey Shinkevich 6 years, 8 months ago

When a bitmap is removed, we can clean some space on the disk. The size
of a cluster may be larger, so is the size of the bitmap that includes
many clusters. Some bitmaps can be as large as tens of megabytes.
The flag QCOW2_DISCARD_ALWAYS allows a call to the raw_co_pdiscard()
that does the actual cleaning of the image on disk, while with the flag
QCOW2_DISCARD_OTHER, a reference count of the cluster is updated only.

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
v1: Discard old bitmap directories in QCOW2 image

In the first version of the patch, any call to qcow2_free_clusters() in
block/qcow2-bitmap.c was with the flag QCOW2_DISCARD_ALWAYS, even in the
cases when the cleaned space is insignificant, particularly, in case of
bitmap directories.
Discussed in the email thread with the message ID
<1549974951-731285-1-git-send-email-andrey.shinkevich@virtuozzo.com>

 block/qcow2-bitmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 3ee524d..162744e 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;
     }
 }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2] qcow2: discard bitmap when removed

Posted by Vladimir Sementsov-Ogievskiy 6 years, 8 months ago

28.02.2019 12:26, Andrey Shinkevich wrote:
> When a bitmap is removed, we can clean some space on the disk. The size
> of a cluster may be larger, so is the size of the bitmap that includes
> many clusters. Some bitmaps can be as large as tens of megabytes.
> The flag QCOW2_DISCARD_ALWAYS allows a call to the raw_co_pdiscard()
> that does the actual cleaning of the image on disk, while with the flag
> QCOW2_DISCARD_OTHER, a reference count of the cluster is updated only.

not quite right, as it depends on configuration.

So I'd reword the whole message to be simpler, like:

Bitmap data may take a lot of disk space, so it's better to discard it always.

> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>


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

> ---
> v1: Discard old bitmap directories in QCOW2 image
> 
> In the first version of the patch, any call to qcow2_free_clusters() in
> block/qcow2-bitmap.c was with the flag QCOW2_DISCARD_ALWAYS, even in the
> cases when the cleaned space is insignificant, particularly, in case of
> bitmap directories.
> Discussed in the email thread with the message ID
> <1549974951-731285-1-git-send-email-andrey.shinkevich@virtuozzo.com>
> 
>   block/qcow2-bitmap.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 3ee524d..162744e 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;
>       }
>   }
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [Qemu-block] [PATCH v2] qcow2: discard bitmap when removed

Posted by John Snow 6 years, 7 months ago


On 2/28/19 4:26 AM, Andrey Shinkevich wrote:
> When a bitmap is removed, we can clean some space on the disk. The size
> of a cluster may be larger, so is the size of the bitmap that includes
> many clusters. Some bitmaps can be as large as tens of megabytes.
> The flag QCOW2_DISCARD_ALWAYS allows a call to the raw_co_pdiscard()
> that does the actual cleaning of the image on disk, while with the flag
> QCOW2_DISCARD_OTHER, a reference count of the cluster is updated only.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
> v1: Discard old bitmap directories in QCOW2 image
> 
> In the first version of the patch, any call to qcow2_free_clusters() in
> block/qcow2-bitmap.c was with the flag QCOW2_DISCARD_ALWAYS, even in the
> cases when the cleaned space is insignificant, particularly, in case of
> bitmap directories.
> Discussed in the email thread with the message ID
> <1549974951-731285-1-git-send-email-andrey.shinkevich@virtuozzo.com>
> 
>  block/qcow2-bitmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 3ee524d..162744e 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;
>      }
>  }
> 
Looks like 4.1 material now. Kevin may have been expecting me to engage
with this, even though I do not maintain the qcow2-bitmap.c file. (And
I'm very behind on reading mail that was not directly addressed to me,
sorry...!)

Please resend when the 4.1 window opens and we'll address this then.

--js

Re: [Qemu-devel] [Qemu-block] [PATCH v2] qcow2: discard bitmap when removed

Posted by Andrey Shinkevich 6 years, 7 months ago


On 19/03/2019 01:18, John Snow wrote:
> 
> 
> On 2/28/19 4:26 AM, Andrey Shinkevich wrote:
>> When a bitmap is removed, we can clean some space on the disk. The size
>> of a cluster may be larger, so is the size of the bitmap that includes
>> many clusters. Some bitmaps can be as large as tens of megabytes.
>> The flag QCOW2_DISCARD_ALWAYS allows a call to the raw_co_pdiscard()
>> that does the actual cleaning of the image on disk, while with the flag
>> QCOW2_DISCARD_OTHER, a reference count of the cluster is updated only.
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>> v1: Discard old bitmap directories in QCOW2 image
>>
>> In the first version of the patch, any call to qcow2_free_clusters() in
>> block/qcow2-bitmap.c was with the flag QCOW2_DISCARD_ALWAYS, even in the
>> cases when the cleaned space is insignificant, particularly, in case of
>> bitmap directories.
>> Discussed in the email thread with the message ID
>> <1549974951-731285-1-git-send-email-andrey.shinkevich@virtuozzo.com>
>>
>>   block/qcow2-bitmap.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 3ee524d..162744e 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;
>>       }
>>   }
>>
> Looks like 4.1 material now. Kevin may have been expecting me to engage
> with this, even though I do not maintain the qcow2-bitmap.c file. (And
> I'm very behind on reading mail that was not directly addressed to me,
> sorry...!)
> 
> Please resend when the 4.1 window opens and we'll address this then.
> 
> --js
> 
Thank you, John.
Well noted.

Andrey

Re: [Qemu-devel] [Qemu-block] [PATCH v2] qcow2: discard bitmap when removed

Posted by Andrey Shinkevich 6 years, 7 months ago

PINGING

I kindly remind you about this small patch for QEMU 4.1.
I am going to be away for vacation from April, 10th to May, 15th.
-- 
With the best regards,
Andrey Shinkevich


On 19/03/2019 01:18, John Snow wrote:
> 
> 
> On 2/28/19 4:26 AM, Andrey Shinkevich wrote:
>> When a bitmap is removed, we can clean some space on the disk. The size
>> of a cluster may be larger, so is the size of the bitmap that includes
>> many clusters. Some bitmaps can be as large as tens of megabytes.
>> The flag QCOW2_DISCARD_ALWAYS allows a call to the raw_co_pdiscard()
>> that does the actual cleaning of the image on disk, while with the flag
>> QCOW2_DISCARD_OTHER, a reference count of the cluster is updated only.
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>> v1: Discard old bitmap directories in QCOW2 image
>>
>> In the first version of the patch, any call to qcow2_free_clusters() in
>> block/qcow2-bitmap.c was with the flag QCOW2_DISCARD_ALWAYS, even in the
>> cases when the cleaned space is insignificant, particularly, in case of
>> bitmap directories.
>> Discussed in the email thread with the message ID
>> <1549974951-731285-1-git-send-email-andrey.shinkevich@virtuozzo.com>
>>
>>   block/qcow2-bitmap.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 3ee524d..162744e 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;
>>       }
>>   }
>>
> Looks like 4.1 material now. Kevin may have been expecting me to engage
> with this, even though I do not maintain the qcow2-bitmap.c file. (And
> I'm very behind on reading mail that was not directly addressed to me,
> sorry...!)
> 
> Please resend when the 4.1 window opens and we'll address this then.
> 
> --js
>

Re: [Qemu-devel] [PATCH v2] qcow2: discard bitmap when removed

Posted by Max Reitz 6 years, 7 months ago

On 28.02.19 10:26, Andrey Shinkevich wrote:
> When a bitmap is removed, we can clean some space on the disk. The size
> of a cluster may be larger, so is the size of the bitmap that includes
> many clusters. Some bitmaps can be as large as tens of megabytes.
> The flag QCOW2_DISCARD_ALWAYS allows a call to the raw_co_pdiscard()
> that does the actual cleaning of the image on disk, while with the flag
> QCOW2_DISCARD_OTHER, a reference count of the cluster is updated only.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
> v1: Discard old bitmap directories in QCOW2 image
> 
> In the first version of the patch, any call to qcow2_free_clusters() in
> block/qcow2-bitmap.c was with the flag QCOW2_DISCARD_ALWAYS, even in the
> cases when the cleaned space is insignificant, particularly, in case of
> bitmap directories.
> Discussed in the email thread with the message ID
> <1549974951-731285-1-git-send-email-andrey.shinkevich@virtuozzo.com>
> 
>  block/qcow2-bitmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thanks, changed the commit message to Vladimir’s proposal and applied to
my block-next branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block-next

Max

Re: [Qemu-devel] [PATCH v2] qcow2: discard bitmap when removed

Posted by Andrey Shinkevich 6 years, 7 months ago


On 08/04/2019 22:03, Max Reitz wrote:
> On 28.02.19 10:26, Andrey Shinkevich wrote:
>> When a bitmap is removed, we can clean some space on the disk. The size
>> of a cluster may be larger, so is the size of the bitmap that includes
>> many clusters. Some bitmaps can be as large as tens of megabytes.
>> The flag QCOW2_DISCARD_ALWAYS allows a call to the raw_co_pdiscard()
>> that does the actual cleaning of the image on disk, while with the flag
>> QCOW2_DISCARD_OTHER, a reference count of the cluster is updated only.
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>> v1: Discard old bitmap directories in QCOW2 image
>>
>> In the first version of the patch, any call to qcow2_free_clusters() in
>> block/qcow2-bitmap.c was with the flag QCOW2_DISCARD_ALWAYS, even in the
>> cases when the cleaned space is insignificant, particularly, in case of
>> bitmap directories.
>> Discussed in the email thread with the message ID
>> <1549974951-731285-1-git-send-email-andrey.shinkevich@virtuozzo.com>
>>
>>   block/qcow2-bitmap.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Thanks, changed the commit message to Vladimir’s proposal and applied to
> my block-next branch:
> 
> https://git.xanclic.moe/XanClic/qemu/commits/branch/block-next
> 
> Max
> 

Thank you, Max!
-- 
With the best regards,
Andrey Shinkevich