[PATCH] block: qcow2-bitmap: fix persistent bitmap corruption on ENOSPC

[PATCH] block: qcow2-bitmap: fix persistent bitmap corruption on ENOSPC

[Christian Barry]

From: Christian Barry <christian.barry@usp.br>

Changed qcow2_co_remove_persistent_dirty_bitmap() to treat free_bitmap_clusters()
failure as fatal and roll back the header update through a new helper
function rollback_ext_header_and_dir_helper(), which was also applied
to update_ext_header_and_dir(), which had internal rollback logic.
Changed bitmap_list_store() to zero the cluster tail after write,
guaranteeing that qemu-img won't parse into stale bytes.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3516

Signed-off-by: Christian Barry <christian.barry@usp.br>
Co-developed by: Eduardo Augusto Cavalcanti <eduardoaugustoabc@ime.usp.br>
Signed-off-by: Eduardo Augusto Cavalcanti <eduardoaugustoabc@ime.usp.br>
---
 block/qcow2-bitmap.c | 37 ++++++++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 256ec99878..5796298157 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -737,10 +737,12 @@ static int GRAPH_RDLOCK
 bitmap_list_store(BlockDriverState *bs, Qcow2BitmapList *bm_list,
                   uint64_t *offset, uint64_t *size, bool in_place)
 {
+    BDRVQcow2State *s = bs->opaque;
     int ret;
     uint8_t *dir;
     int64_t dir_offset = 0;
     uint64_t dir_size = 0;
+    uint64_t tail_size;
     Qcow2Bitmap *bm;
     Qcow2BitmapDirEntry *e;
 
@@ -810,6 +812,14 @@ bitmap_list_store(BlockDriverState *bs, Qcow2BitmapList *bm_list,
         goto fail;
     }
 
+    tail_size = ROUND_UP(dir_size, s->cluster_size) - dir_size;
+    if (tail_size > 0) {
+        ret = bdrv_pwrite_zeroes(bs->file, dir_offset + dir_size, tail_size, 0);
+        if (ret < 0) {
+            goto fail;
+        }
+    }
+
     g_free(dir);
 
     if (!in_place) {
@@ -882,6 +892,19 @@ update_ext_header_and_dir_in_place(BlockDriverState *bs,
      */
 }
 
+static void rollback_ext_header_and_dir_helper(BlockDriverState *bs,
+					       uint64_t old_offset,
+					       uint64_t old_size,
+					       uint32_t old_nb_bitmaps,
+					       uint64_t old_autocl)
+{
+    BDRVQcow2State *s = bs->opaque;
+    s->bitmap_directory_offset = old_offset;
+    s->bitmap_directory_size = old_size;
+    s->nb_bitmaps = old_nb_bitmaps;
+    s->autoclear_features = old_autocl;
+}
+
 static int GRAPH_RDLOCK
 update_ext_header_and_dir(BlockDriverState *bs, Qcow2BitmapList *bm_list)
 {
@@ -937,10 +960,7 @@ fail:
         qcow2_free_clusters(bs, new_offset, new_size, QCOW2_DISCARD_OTHER);
     }
 
-    s->bitmap_directory_offset = old_offset;
-    s->bitmap_directory_size = old_size;
-    s->nb_bitmaps = old_nb_bitmaps;
-    s->autoclear_features = old_autocl;
+    rollback_ext_header_and_dir_helper(bs, old_offset, old_size, old_nb_bitmaps, old_autocl);
 
     return ret;
 }
@@ -1460,6 +1480,10 @@ int coroutine_fn qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
 {
     int ret;
     BDRVQcow2State *s = bs->opaque;
+    uint64_t old_offset = s->bitmap_directory_offset;
+    uint64_t old_size = s->bitmap_directory_size;
+    uint32_t old_nb_bitmaps = s->nb_bitmaps;
+    uint64_t old_autocl = s->autoclear_features;
     Qcow2Bitmap *bm = NULL;
     Qcow2BitmapList *bm_list;
 
@@ -1495,7 +1519,10 @@ int coroutine_fn qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
         goto out;
     }
 
-    free_bitmap_clusters(bs, &bm->table);
+    ret = free_bitmap_clusters(bs, &bm->table);
+    if (ret < 0){
+	rollback_ext_header_and_dir_helper(bs, old_offset, old_size, old_nb_bitmaps, old_autocl);
+    }
 
 out:
     qemu_co_mutex_unlock(&s->lock);
-- 
2.43.0

Re: [PATCH] block: qcow2-bitmap: fix persistent bitmap corruption on ENOSPC

[Vladimir Sementsov-Ogievskiy]

10.06.26 05:33, Christian Barry пишет:
> From: Christian Barry <christian.barry@usp.br>
> 
> Changed qcow2_co_remove_persistent_dirty_bitmap() to treat free_bitmap_clusters()
> failure as fatal and roll back the header update through a new helper
> function rollback_ext_header_and_dir_helper(), which was also applied
> to update_ext_header_and_dir(), which had internal rollback logic.
> Changed bitmap_list_store() to zero the cluster tail after write,
> guaranteeing that qemu-img won't parse into stale bytes.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3516

Hi!

First, seem like two different fixes merged into one commit, please if
possible, make them separate two commits.

Next, please, make commit messages more descriptive, about what the
issue we solve (link is good as additional information, but the commit
message itself should be complete).

For example, I don't understand free_bitmap_clusters makes the image
unreadable. And we we fix it in one place, leaving other calls to
free_bitmap_clusters() unchanged. Could we fix free_bitmap_clusters()
itself? See also my comments below

> 
> Signed-off-by: Christian Barry <christian.barry@usp.br>
> Co-developed by: Eduardo Augusto Cavalcanti <eduardoaugustoabc@ime.usp.br>
> Signed-off-by: Eduardo Augusto Cavalcanti <eduardoaugustoabc@ime.usp.br>
> ---
>   block/qcow2-bitmap.c | 37 ++++++++++++++++++++++++++++++++-----
>   1 file changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 256ec99878..5796298157 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -737,10 +737,12 @@ static int GRAPH_RDLOCK
>   bitmap_list_store(BlockDriverState *bs, Qcow2BitmapList *bm_list,
>                     uint64_t *offset, uint64_t *size, bool in_place)
>   {
> +    BDRVQcow2State *s = bs->opaque;
>       int ret;
>       uint8_t *dir;
>       int64_t dir_offset = 0;
>       uint64_t dir_size = 0;
> +    uint64_t tail_size;
>       Qcow2Bitmap *bm;
>       Qcow2BitmapDirEntry *e;
>   
> @@ -810,6 +812,14 @@ bitmap_list_store(BlockDriverState *bs, Qcow2BitmapList *bm_list,
>           goto fail;
>       }
>   
> +    tail_size = ROUND_UP(dir_size, s->cluster_size) - dir_size;
> +    if (tail_size > 0) {
> +        ret = bdrv_pwrite_zeroes(bs->file, dir_offset + dir_size, tail_size, 0);
> +        if (ret < 0) {
> +            goto fail;
> +        }
> +    }

If zeroing of unused bytes help to fix some issue, it obviously means,
that something is wrong in deeper logic.

> +
>       g_free(dir);
>   
>       if (!in_place) {
> @@ -882,6 +892,19 @@ update_ext_header_and_dir_in_place(BlockDriverState *bs,
>        */
>   }
>   
> +static void rollback_ext_header_and_dir_helper(BlockDriverState *bs,
> +					       uint64_t old_offset,
> +					       uint64_t old_size,
> +					       uint32_t old_nb_bitmaps,
> +					       uint64_t old_autocl)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    s->bitmap_directory_offset = old_offset;
> +    s->bitmap_directory_size = old_size;
> +    s->nb_bitmaps = old_nb_bitmaps;
> +    s->autoclear_features = old_autocl;
> +}

I think, better to inline this code, or refactor deeper. This way it only
makes code more complicated: we can't deduplicate first part of logic,
which is defining these 4 variables, but do that for the second part.

> +
>   static int GRAPH_RDLOCK
>   update_ext_header_and_dir(BlockDriverState *bs, Qcow2BitmapList *bm_list)
>   {
> @@ -937,10 +960,7 @@ fail:
>           qcow2_free_clusters(bs, new_offset, new_size, QCOW2_DISCARD_OTHER);
>       }
>   
> -    s->bitmap_directory_offset = old_offset;
> -    s->bitmap_directory_size = old_size;
> -    s->nb_bitmaps = old_nb_bitmaps;
> -    s->autoclear_features = old_autocl;
> +    rollback_ext_header_and_dir_helper(bs, old_offset, old_size, old_nb_bitmaps, old_autocl);
>   
>       return ret;
>   }
> @@ -1460,6 +1480,10 @@ int coroutine_fn qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>   {
>       int ret;
>       BDRVQcow2State *s = bs->opaque;
> +    uint64_t old_offset = s->bitmap_directory_offset;
> +    uint64_t old_size = s->bitmap_directory_size;
> +    uint32_t old_nb_bitmaps = s->nb_bitmaps;
> +    uint64_t old_autocl = s->autoclear_features;
>       Qcow2Bitmap *bm = NULL;
>       Qcow2BitmapList *bm_list;
>   
> @@ -1495,7 +1519,10 @@ int coroutine_fn qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>           goto out;
>       }
>   
> -    free_bitmap_clusters(bs, &bm->table);
> +    ret = free_bitmap_clusters(bs, &bm->table);
> +    if (ret < 0){
> +	rollback_ext_header_and_dir_helper(bs, old_offset, old_size, old_nb_bitmaps, old_autocl);
> +    }

Again, this looks strange too. Ignoring failure of freeing _unused_ clusters
should not be a problem. If it is, it means that things are broken earlier
and something should be fixed at earlier stage.

>   
>   out:
>       qemu_co_mutex_unlock(&s->lock);


---

Let me try to understand the issue at https://gitlab.com/qemu-project/qemu/-/work_items/3516..


1. We failed to flush bitmaps. So, autoclear bit should be unset in the header and the whole bitmap extension considered inconsistent

2. On next qcow2 open, no bitmaps should be loaded, and we'll see message of

                 error_printf("Some clusters may be leaked, "
                              "run 'qemu-img check -r' on the image "
                              "file to fix.");


What's wrong with that?



-- 
Best regards,
Vladimir

Re: [PATCH] block: qcow2-bitmap: fix persistent bitmap corruption on ENOSPC

[Kevin Wolf]

Am 16.06.2026 um 11:06 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 10.06.26 05:33, Christian Barry пишет:
> > From: Christian Barry <christian.barry@usp.br>
> > 
> > Changed qcow2_co_remove_persistent_dirty_bitmap() to treat free_bitmap_clusters()
> > failure as fatal and roll back the header update through a new helper
> > function rollback_ext_header_and_dir_helper(), which was also applied
> > to update_ext_header_and_dir(), which had internal rollback logic.
> > Changed bitmap_list_store() to zero the cluster tail after write,
> > guaranteeing that qemu-img won't parse into stale bytes.
> > 
> > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3516
> 
> Hi!
> 
> First, seem like two different fixes merged into one commit, please if
> possible, make them separate two commits.
> 
> Next, please, make commit messages more descriptive, about what the
> issue we solve (link is good as additional information, but the commit
> message itself should be complete).
> 
> For example, I don't understand free_bitmap_clusters makes the image
> unreadable. And we we fix it in one place, leaving other calls to
> free_bitmap_clusters() unchanged. Could we fix free_bitmap_clusters()
> itself? See also my comments below

I haven't looked at the patch yet, but this sounds like apart from
splitting the patches into one logical change per patch and improving
the commit message, the patch series should also include a test case
(probably qemu-iotests) that demonstrates the problem and how the patch
changes behaviour.

Kevin