[Qemu-devel] [PATCH 0/5] block/qcow2-bitmap: Enable resize with persistent bitmaps

[Qemu-devel] [PATCH 0/5] block/qcow2-bitmap: Enable resize with persistent bitmaps

[John Snow]

This series aims to enable block resizes when persistent bitmaps are
in use. The basic approach here is to recognize that we now load all
persistent bitmaps in memory, and so we can rely on in-memory resizes
and then flush the changed metadata back to disk.

One part that is potentially now quite strange is that bitmap resizes
may happen twice: once during the qcow2 resize event only if persistent
bitmaps are found, and then again as part of the generic resize callback
event whether or not we have any persistent bitmaps.

The second round is required if we are not using qcow2 or we have only
transient bitmaps. The first round is required as we wish to flush the
bitmaps back to disk atomically with the qcow2 resize to avoid violating
our invariants for the bitmap metadata which is checked in many places.

This is harmless; hbitmap_truncate will recognize the second round as
a no-op.

John Snow (5):
  block/qcow2-bitmap: Skip length check in some cases
  block/qcow2-bitmap: Allow bitmap flushing
  block/qcow2-bitmap: don't remove bitmaps on reopen
  block/qcow2-bitmap: Allow resizes with persistent bitmaps
  tests/qemu-iotests: add bitmap resize test 246

 block/qcow2.h              |   2 +
 block/qcow2-bitmap.c       | 123 +++++++++++----
 block/qcow2.c              |  27 +++-
 tests/qemu-iotests/246     | 114 ++++++++++++++
 tests/qemu-iotests/246.out | 296 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 6 files changed, 528 insertions(+), 35 deletions(-)
 create mode 100755 tests/qemu-iotests/246
 create mode 100644 tests/qemu-iotests/246.out

-- 
2.17.2

[Qemu-devel] [PATCH 1/5] block/qcow2-bitmap: Skip length check in some cases

[John Snow]

If we were to allow resizes, the length check that happens when we load
bitmap headers from disk when we read or store bitmaps would begin to
fail:

Imagine the circumstance where we've resized bitmaps in memory, but they still
have the old values on-disk. The lengths will no longer match bdrv_getlength,
so we must allow this check to be skipped when flushing bitmaps to disk.

Limit this to when we are about to overwrite the headers: we will verify the
outgoing headers, but we will skip verifying the known stale headers.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/qcow2-bitmap.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index c3b210ede1..d02730004a 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -435,7 +435,8 @@ static inline Qcow2BitmapDirEntry *next_dir_entry(Qcow2BitmapDirEntry *entry)
     return (Qcow2BitmapDirEntry *)((uint8_t *)entry + dir_entry_size(entry));
 }
 
-static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry *entry)
+static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry *entry,
+                           bool allow_resize)
 {
     BDRVQcow2State *s = bs->opaque;
     uint64_t phys_bitmap_bytes;
@@ -462,8 +463,14 @@ static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry *entry)
         return len;
     }
 
-    fail = (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) ||
-           (len > ((phys_bitmap_bytes * 8) << entry->granularity_bits));
+    if (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) {
+        return -EINVAL;
+    }
+
+    if (!allow_resize &&
+        (len > ((phys_bitmap_bytes * 8) << entry->granularity_bits))) {
+        return -EINVAL;
+    }
 
     return fail ? -EINVAL : 0;
 }
@@ -534,7 +541,8 @@ static uint32_t bitmap_list_count(Qcow2BitmapList *bm_list)
  * checks it and convert to bitmap list.
  */
 static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
-                                         uint64_t size, Error **errp)
+                                         uint64_t size, bool allow_resize,
+                                         Error **errp)
 {
     int ret;
     BDRVQcow2State *s = bs->opaque;
@@ -593,7 +601,7 @@ static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
             goto fail;
         }
 
-        ret = check_dir_entry(bs, e);
+        ret = check_dir_entry(bs, e, allow_resize);
         if (ret < 0) {
             error_setg(errp, "Bitmap '%.*s' doesn't satisfy the constraints",
                        e->name_size, dir_entry_name_field(e));
@@ -654,7 +662,7 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
     }
 
     bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-                               s->bitmap_directory_size, NULL);
+                               s->bitmap_directory_size, false, NULL);
     if (bm_list == NULL) {
         res->corruptions++;
         return -EINVAL;
@@ -755,7 +763,7 @@ static int bitmap_list_store(BlockDriverState *bs, Qcow2BitmapList *bm_list,
         e->extra_data_size = 0;
         memcpy(e + 1, bm->name, e->name_size);
 
-        if (check_dir_entry(bs, e) < 0) {
+        if (check_dir_entry(bs, e, false) < 0) {
             ret = -EINVAL;
             goto fail;
         }
@@ -957,7 +965,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
     }
 
     bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-                               s->bitmap_directory_size, errp);
+                               s->bitmap_directory_size, false, errp);
     if (bm_list == NULL) {
         return false;
     }
@@ -1066,7 +1074,7 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
     }
 
     bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-                               s->bitmap_directory_size, errp);
+                               s->bitmap_directory_size, false, errp);
     if (bm_list == NULL) {
         return NULL;
     }
@@ -1111,7 +1119,7 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
     }
 
     bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-                               s->bitmap_directory_size, errp);
+                               s->bitmap_directory_size, false, errp);
     if (bm_list == NULL) {
         return -EINVAL;
     }
@@ -1359,7 +1367,7 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
     }
 
     bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-                               s->bitmap_directory_size, errp);
+                               s->bitmap_directory_size, false, errp);
     if (bm_list == NULL) {
         return;
     }
@@ -1412,7 +1420,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
         bm_list = bitmap_list_new();
     } else {
         bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-                                   s->bitmap_directory_size, errp);
+                                   s->bitmap_directory_size, true, errp);
         if (bm_list == NULL) {
             return;
         }
@@ -1593,7 +1601,7 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
     }
 
     bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-                               s->bitmap_directory_size, errp);
+                               s->bitmap_directory_size, false, errp);
     if (bm_list == NULL) {
         goto fail;
     }
-- 
2.17.2

Re: [Qemu-devel] [PATCH 1/5] block/qcow2-bitmap: Skip length check in some cases

[Eric Blake]

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

On 3/5/19 5:43 PM, John Snow wrote:
> If we were to allow resizes, the length check that happens when we load
> bitmap headers from disk when we read or store bitmaps would begin to
> fail:
> 
> Imagine the circumstance where we've resized bitmaps in memory, but they still
> have the old values on-disk. The lengths will no longer match bdrv_getlength,
> so we must allow this check to be skipped when flushing bitmaps to disk.
> 
> Limit this to when we are about to overwrite the headers: we will verify the
> outgoing headers, but we will skip verifying the known stale headers.

No-op until we actually do allow resizes later in the series, but makes
sense.

> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/qcow2-bitmap.c | 34 +++++++++++++++++++++-------------
>  1 file changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index c3b210ede1..d02730004a 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -435,7 +435,8 @@ static inline Qcow2BitmapDirEntry *next_dir_entry(Qcow2BitmapDirEntry *entry)
>      return (Qcow2BitmapDirEntry *)((uint8_t *)entry + dir_entry_size(entry));
>  }
>  
> -static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry *entry)
> +static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry *entry,
> +                           bool allow_resize)
>  {
>      BDRVQcow2State *s = bs->opaque;
>      uint64_t phys_bitmap_bytes;
> @@ -462,8 +463,14 @@ static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry *entry)
>          return len;

Someday, it would be nice to plumb Error* through this function, so that
you can give distinct reasons for failure, rather than lumping all
failures into the nebulous "doesn't meet the constraints" because we
lost context when slamming multiple errors into a single -EINVAL. But
that's a separate patch series.

>      }
>  
> -    fail = (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) ||
> -           (len > ((phys_bitmap_bytes * 8) << entry->granularity_bits));
> +    if (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) {
> +        return -EINVAL;
> +    }
> +
> +    if (!allow_resize &&
> +        (len > ((phys_bitmap_bytes * 8) << entry->granularity_bits))) {
> +        return -EINVAL;
> +    }
>  
>      return fail ? -EINVAL : 0;

Dead conditional - with your refactoring, this line is only reached when
fail == false.

With it changed to 'return 0',
Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [Qemu-devel] [PATCH 1/5] block/qcow2-bitmap: Skip length check in some cases

[Vladimir Sementsov-Ogievskiy]

06.03.2019 2:43, John Snow wrote:
> If we were to allow resizes, the length check that happens when we load
> bitmap headers from disk when we read or store bitmaps would begin to
> fail:
> 
> Imagine the circumstance where we've resized bitmaps in memory, but they still
> have the old values on-disk. The lengths will no longer match bdrv_getlength,
> so we must allow this check to be skipped when flushing bitmaps to disk.
> 
> Limit this to when we are about to overwrite the headers: we will verify the
> outgoing headers, but we will skip verifying the known stale headers.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   block/qcow2-bitmap.c | 34 +++++++++++++++++++++-------------
>   1 file changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index c3b210ede1..d02730004a 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -435,7 +435,8 @@ static inline Qcow2BitmapDirEntry *next_dir_entry(Qcow2BitmapDirEntry *entry)
>       return (Qcow2BitmapDirEntry *)((uint8_t *)entry + dir_entry_size(entry));
>   }
>   
> -static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry *entry)
> +static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry *entry,
> +                           bool allow_resize)
>   {
>       BDRVQcow2State *s = bs->opaque;
>       uint64_t phys_bitmap_bytes;
> @@ -462,8 +463,14 @@ static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry *entry)
>           return len;
>       }
>   
> -    fail = (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) ||
> -           (len > ((phys_bitmap_bytes * 8) << entry->granularity_bits));
> +    if (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) {
> +        return -EINVAL;
> +    }
> +
> +    if (!allow_resize &&
> +        (len > ((phys_bitmap_bytes * 8) << entry->granularity_bits))) {
> +        return -EINVAL;
> +    }
>   
>       return fail ? -EINVAL : 0;
>   }
> @@ -534,7 +541,8 @@ static uint32_t bitmap_list_count(Qcow2BitmapList *bm_list)
>    * checks it and convert to bitmap list.
>    */
>   static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
> -                                         uint64_t size, Error **errp)
> +                                         uint64_t size, bool allow_resize,
> +                                         Error **errp)
>   {
>       int ret;
>       BDRVQcow2State *s = bs->opaque;
> @@ -593,7 +601,7 @@ static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
>               goto fail;
>           }
>   
> -        ret = check_dir_entry(bs, e);
> +        ret = check_dir_entry(bs, e, allow_resize);
>           if (ret < 0) {
>               error_setg(errp, "Bitmap '%.*s' doesn't satisfy the constraints",
>                          e->name_size, dir_entry_name_field(e));
> @@ -654,7 +662,7 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>       }
>   
>       bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> -                               s->bitmap_directory_size, NULL);
> +                               s->bitmap_directory_size, false, NULL);
>       if (bm_list == NULL) {
>           res->corruptions++;
>           return -EINVAL;
> @@ -755,7 +763,7 @@ static int bitmap_list_store(BlockDriverState *bs, Qcow2BitmapList *bm_list,
>           e->extra_data_size = 0;
>           memcpy(e + 1, bm->name, e->name_size);
>   
> -        if (check_dir_entry(bs, e) < 0) {
> +        if (check_dir_entry(bs, e, false) < 0) {
>               ret = -EINVAL;
>               goto fail;
>           }
> @@ -957,7 +965,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>       }
>   
>       bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> -                               s->bitmap_directory_size, errp);
> +                               s->bitmap_directory_size, false, errp);
>       if (bm_list == NULL) {
>           return false;
>       }
> @@ -1066,7 +1074,7 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
>       }
>   
>       bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> -                               s->bitmap_directory_size, errp);
> +                               s->bitmap_directory_size, false, errp);
>       if (bm_list == NULL) {
>           return NULL;
>       }
> @@ -1111,7 +1119,7 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>       }
>   
>       bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> -                               s->bitmap_directory_size, errp);
> +                               s->bitmap_directory_size, false, errp);
>       if (bm_list == NULL) {
>           return -EINVAL;
>       }
> @@ -1359,7 +1367,7 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>       }
>   
>       bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> -                               s->bitmap_directory_size, errp);
> +                               s->bitmap_directory_size, false, errp);
>       if (bm_list == NULL) {
>           return;
>       }
> @@ -1412,7 +1420,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>           bm_list = bitmap_list_new();
>       } else {

Worth comment here, that we assume that image may be resized?

>           bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> -                                   s->bitmap_directory_size, errp);
> +                                   s->bitmap_directory_size, true, errp);
>           if (bm_list == NULL) {
>               return;
>           }
> @@ -1593,7 +1601,7 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>       }
>   
>       bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> -                               s->bitmap_directory_size, errp);
> +                               s->bitmap_directory_size, false, errp);
>       if (bm_list == NULL) {
>           goto fail;
>       }
> 

With Eric's suggestion:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 1/5] block/qcow2-bitmap: Skip length check in some cases

[John Snow]

On 3/6/19 10:21 AM, Vladimir Sementsov-Ogievskiy wrote:
> 06.03.2019 2:43, John Snow wrote:
>> If we were to allow resizes, the length check that happens when we load
>> bitmap headers from disk when we read or store bitmaps would begin to
>> fail:
>>
>> Imagine the circumstance where we've resized bitmaps in memory, but they still
>> have the old values on-disk. The lengths will no longer match bdrv_getlength,
>> so we must allow this check to be skipped when flushing bitmaps to disk.
>>
>> Limit this to when we are about to overwrite the headers: we will verify the
>> outgoing headers, but we will skip verifying the known stale headers.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   block/qcow2-bitmap.c | 34 +++++++++++++++++++++-------------
>>   1 file changed, 21 insertions(+), 13 deletions(-)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index c3b210ede1..d02730004a 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -435,7 +435,8 @@ static inline Qcow2BitmapDirEntry *next_dir_entry(Qcow2BitmapDirEntry *entry)
>>       return (Qcow2BitmapDirEntry *)((uint8_t *)entry + dir_entry_size(entry));
>>   }
>>   
>> -static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry *entry)
>> +static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry *entry,
>> +                           bool allow_resize)
>>   {
>>       BDRVQcow2State *s = bs->opaque;
>>       uint64_t phys_bitmap_bytes;
>> @@ -462,8 +463,14 @@ static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry *entry)
>>           return len;
>>       }
>>   
>> -    fail = (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) ||
>> -           (len > ((phys_bitmap_bytes * 8) << entry->granularity_bits));
>> +    if (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) {
>> +        return -EINVAL;
>> +    }
>> +
>> +    if (!allow_resize &&
>> +        (len > ((phys_bitmap_bytes * 8) << entry->granularity_bits))) {
>> +        return -EINVAL;
>> +    }
>>   
>>       return fail ? -EINVAL : 0;
>>   }
>> @@ -534,7 +541,8 @@ static uint32_t bitmap_list_count(Qcow2BitmapList *bm_list)
>>    * checks it and convert to bitmap list.
>>    */
>>   static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
>> -                                         uint64_t size, Error **errp)
>> +                                         uint64_t size, bool allow_resize,
>> +                                         Error **errp)
>>   {
>>       int ret;
>>       BDRVQcow2State *s = bs->opaque;
>> @@ -593,7 +601,7 @@ static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
>>               goto fail;
>>           }
>>   
>> -        ret = check_dir_entry(bs, e);
>> +        ret = check_dir_entry(bs, e, allow_resize);
>>           if (ret < 0) {
>>               error_setg(errp, "Bitmap '%.*s' doesn't satisfy the constraints",
>>                          e->name_size, dir_entry_name_field(e));
>> @@ -654,7 +662,7 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>>       }
>>   
>>       bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>> -                               s->bitmap_directory_size, NULL);
>> +                               s->bitmap_directory_size, false, NULL);
>>       if (bm_list == NULL) {
>>           res->corruptions++;
>>           return -EINVAL;
>> @@ -755,7 +763,7 @@ static int bitmap_list_store(BlockDriverState *bs, Qcow2BitmapList *bm_list,
>>           e->extra_data_size = 0;
>>           memcpy(e + 1, bm->name, e->name_size);
>>   
>> -        if (check_dir_entry(bs, e) < 0) {
>> +        if (check_dir_entry(bs, e, false) < 0) {
>>               ret = -EINVAL;
>>               goto fail;
>>           }
>> @@ -957,7 +965,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>>       }
>>   
>>       bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>> -                               s->bitmap_directory_size, errp);
>> +                               s->bitmap_directory_size, false, errp);
>>       if (bm_list == NULL) {
>>           return false;
>>       }
>> @@ -1066,7 +1074,7 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
>>       }
>>   
>>       bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>> -                               s->bitmap_directory_size, errp);
>> +                               s->bitmap_directory_size, false, errp);
>>       if (bm_list == NULL) {
>>           return NULL;
>>       }
>> @@ -1111,7 +1119,7 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>>       }
>>   
>>       bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>> -                               s->bitmap_directory_size, errp);
>> +                               s->bitmap_directory_size, false, errp);
>>       if (bm_list == NULL) {
>>           return -EINVAL;
>>       }
>> @@ -1359,7 +1367,7 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>>       }
>>   
>>       bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>> -                               s->bitmap_directory_size, errp);
>> +                               s->bitmap_directory_size, false, errp);
>>       if (bm_list == NULL) {
>>           return;
>>       }
>> @@ -1412,7 +1420,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>>           bm_list = bitmap_list_new();
>>       } else {
> 
> Worth comment here, that we assume that image may be resized?
> 

You mean below, to explain the 'true' parameter? Yes, I will do so.

>>           bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>> -                                   s->bitmap_directory_size, errp);
>> +                                   s->bitmap_directory_size, true, errp);
>>           if (bm_list == NULL) {
>>               return;
>>           }
>> @@ -1593,7 +1601,7 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>>       }
>>   
>>       bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>> -                               s->bitmap_directory_size, errp);
>> +                               s->bitmap_directory_size, false, errp);
>>       if (bm_list == NULL) {
>>           goto fail;
>>       }
>>
> 
> With Eric's suggestion:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 

The bad return conditional, I assume? OK, will do.

Re: [Qemu-devel] [PATCH 1/5] block/qcow2-bitmap: Skip length check in some cases

[Vladimir Sementsov-Ogievskiy]

06.03.2019 2:43, John Snow wrote:
> If we were to allow resizes, the length check that happens when we load
> bitmap headers from disk when we read or store bitmaps would begin to
> fail:
> 
> Imagine the circumstance where we've resized bitmaps in memory, but they still
> have the old values on-disk. The lengths will no longer match bdrv_getlength,
> so we must allow this check to be skipped when flushing bitmaps to disk.
> 
> Limit this to when we are about to overwrite the headers: we will verify the
> outgoing headers, but we will skip verifying the known stale headers.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   block/qcow2-bitmap.c | 34 +++++++++++++++++++++-------------
>   1 file changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index c3b210ede1..d02730004a 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -435,7 +435,8 @@ static inline Qcow2BitmapDirEntry *next_dir_entry(Qcow2BitmapDirEntry *entry)
>       return (Qcow2BitmapDirEntry *)((uint8_t *)entry + dir_entry_size(entry));
>   }
>   
> -static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry *entry)
> +static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry *entry,
> +                           bool allow_resize)
>   {
>       BDRVQcow2State *s = bs->opaque;
>       uint64_t phys_bitmap_bytes;
> @@ -462,8 +463,14 @@ static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry *entry)
>           return len;
>       }
>   
> -    fail = (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) ||
> -           (len > ((phys_bitmap_bytes * 8) << entry->granularity_bits));
> +    if (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) {
> +        return -EINVAL;
> +    }
> +
> +    if (!allow_resize &&
> +        (len > ((phys_bitmap_bytes * 8) << entry->granularity_bits))) {
> +        return -EINVAL;
> +    }

now I think, that we don't need additional parameter, but instead do this check only if
! entry->flags & BME_FLAG_IN_USE, which will correspond to:

1. incoming: bitmap loaded from image
2. outgoing: bitmap stored to the image

>   
>       return fail ? -EINVAL : 0;
>   }
> @@ -534,7 +541,8 @@ static uint32_t bitmap_list_count(Qcow2BitmapList *bm_list)
>    * checks it and convert to bitmap list.
>    */
>   static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
> -                                         uint64_t size, Error **errp)
> +                                         uint64_t size, bool allow_resize,
> +                                         Error **errp)
>   {
>       int ret;
>       BDRVQcow2State *s = bs->opaque;
> @@ -593,7 +601,7 @@ static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
>               goto fail;
>           }
>   
> -        ret = check_dir_entry(bs, e);
> +        ret = check_dir_entry(bs, e, allow_resize);
>           if (ret < 0) {
>               error_setg(errp, "Bitmap '%.*s' doesn't satisfy the constraints",
>                          e->name_size, dir_entry_name_field(e));
> @@ -654,7 +662,7 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>       }
>   
>       bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> -                               s->bitmap_directory_size, NULL);
> +                               s->bitmap_directory_size, false, NULL);
>       if (bm_list == NULL) {
>           res->corruptions++;
>           return -EINVAL;
> @@ -755,7 +763,7 @@ static int bitmap_list_store(BlockDriverState *bs, Qcow2BitmapList *bm_list,
>           e->extra_data_size = 0;
>           memcpy(e + 1, bm->name, e->name_size);
>   
> -        if (check_dir_entry(bs, e) < 0) {
> +        if (check_dir_entry(bs, e, false) < 0) {
>               ret = -EINVAL;
>               goto fail;
>           }
> @@ -957,7 +965,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>       }
>   
>       bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> -                               s->bitmap_directory_size, errp);
> +                               s->bitmap_directory_size, false, errp);
>       if (bm_list == NULL) {
>           return false;
>       }
> @@ -1066,7 +1074,7 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
>       }
>   
>       bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> -                               s->bitmap_directory_size, errp);
> +                               s->bitmap_directory_size, false, errp);
>       if (bm_list == NULL) {
>           return NULL;
>       }
> @@ -1111,7 +1119,7 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>       }
>   
>       bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> -                               s->bitmap_directory_size, errp);
> +                               s->bitmap_directory_size, false, errp);
>       if (bm_list == NULL) {
>           return -EINVAL;
>       }
> @@ -1359,7 +1367,7 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>       }
>   
>       bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> -                               s->bitmap_directory_size, errp);
> +                               s->bitmap_directory_size, false, errp);
>       if (bm_list == NULL) {
>           return;
>       }
> @@ -1412,7 +1420,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>           bm_list = bitmap_list_new();
>       } else {
>           bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> -                                   s->bitmap_directory_size, errp);
> +                                   s->bitmap_directory_size, true, errp);
>           if (bm_list == NULL) {
>               return;
>           }
> @@ -1593,7 +1601,7 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>       }
>   
>       bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> -                               s->bitmap_directory_size, errp);
> +                               s->bitmap_directory_size, false, errp);
>       if (bm_list == NULL) {
>           goto fail;
>       }
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 1/5] block/qcow2-bitmap: Skip length check in some cases

[John Snow]

On 3/6/19 11:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> 06.03.2019 2:43, John Snow wrote:
>> If we were to allow resizes, the length check that happens when we load
>> bitmap headers from disk when we read or store bitmaps would begin to
>> fail:
>>
>> Imagine the circumstance where we've resized bitmaps in memory, but they still
>> have the old values on-disk. The lengths will no longer match bdrv_getlength,
>> so we must allow this check to be skipped when flushing bitmaps to disk.
>>
>> Limit this to when we are about to overwrite the headers: we will verify the
>> outgoing headers, but we will skip verifying the known stale headers.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   block/qcow2-bitmap.c | 34 +++++++++++++++++++++-------------
>>   1 file changed, 21 insertions(+), 13 deletions(-)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index c3b210ede1..d02730004a 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -435,7 +435,8 @@ static inline Qcow2BitmapDirEntry *next_dir_entry(Qcow2BitmapDirEntry *entry)
>>       return (Qcow2BitmapDirEntry *)((uint8_t *)entry + dir_entry_size(entry));
>>   }
>>   
>> -static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry *entry)
>> +static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry *entry,
>> +                           bool allow_resize)
>>   {
>>       BDRVQcow2State *s = bs->opaque;
>>       uint64_t phys_bitmap_bytes;
>> @@ -462,8 +463,14 @@ static int check_dir_entry(BlockDriverState *bs, Qcow2BitmapDirEntry *entry)
>>           return len;
>>       }
>>   
>> -    fail = (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) ||
>> -           (len > ((phys_bitmap_bytes * 8) << entry->granularity_bits));
>> +    if (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) {
>> +        return -EINVAL;
>> +    }
>> +
>> +    if (!allow_resize &&
>> +        (len > ((phys_bitmap_bytes * 8) << entry->granularity_bits))) {
>> +        return -EINVAL;
>> +    }
> 
> now I think, that we don't need additional parameter, but instead do this check only if
> ! entry->flags & BME_FLAG_IN_USE, which will correspond to:
> 
> 1. incoming: bitmap loaded from image
> 2. outgoing: bitmap stored to the image
> 

I can't tell if I like this or not, because we'd skip checking the image
on load if it was improperly stored. I guess that's... fine.

Though, it would mean we also skip the consistency check on subsequent
re-reads of the bitmap list, which I think we still want if only as a
sanity check on the code, right?

Further thoughts?

(Maybe this portion of the code is due for a refactor in 4.1 so we can
add better error messages to it, like Eric suggests.)

[Qemu-devel] [PATCH 2/5] block/qcow2-bitmap: Allow bitmap flushing

[John Snow]

Usually, we only write out bitmaps when we're about to close out the file,
so we always remove the bitmaps after to make it easier to determine the
source of truth for migration purposes.

However, there may be times we want to flush bitmap data more aggressively,
like after a truncate event where we need to make the metadata consistent
again.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/qcow2.h        |  1 +
 block/qcow2-bitmap.c | 45 ++++++++++++++++++++++++++++++--------------
 2 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 9dd02df831..4c1fdfcbec 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -690,6 +690,7 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
                                  Error **errp);
 int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
+void qcow2_flush_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
 bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
                                       const char *name,
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index d02730004a..4e11b6b05a 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1392,7 +1392,7 @@ fail:
     bitmap_list_free(bm_list);
 }
 
-void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
+static void do_store_bitmaps(BlockDriverState *bs, bool remove, Error **errp)
 {
     BdrvDirtyBitmap *bitmap;
     BDRVQcow2State *s = bs->opaque;
@@ -1474,6 +1474,9 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
             QSIMPLEQ_INSERT_TAIL(&drop_tables, tb, entry);
         }
         bm->flags = bdrv_dirty_bitmap_enabled(bitmap) ? BME_FLAG_AUTO : 0;
+        if (!remove) {
+            bm->flags |= BME_FLAG_IN_USE;
+        }
         bm->granularity_bits = ctz32(bdrv_dirty_bitmap_granularity(bitmap));
         bm->dirty_bitmap = bitmap;
     }
@@ -1503,20 +1506,14 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
         g_free(tb);
     }
 
-    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
-        /* For safety, we remove bitmap after storing.
-         * We may be here in two cases:
-         * 1. bdrv_close. It's ok to drop bitmap.
-         * 2. inactivation. It means migration without 'dirty-bitmaps'
-         *    capability, so bitmaps are not marked with
-         *    BdrvDirtyBitmap.migration flags. It's not bad to drop them too,
-         *    and reload on invalidation.
-         */
-        if (bm->dirty_bitmap == NULL) {
-            continue;
-        }
+    if (remove) {
+        QSIMPLEQ_FOREACH(bm, bm_list, entry) {
+            if (bm->dirty_bitmap == NULL) {
+                continue;
+            }
 
-        bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
+            bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
+        }
     }
 
     bitmap_list_free(bm_list);
@@ -1538,6 +1535,26 @@ fail:
     bitmap_list_free(bm_list);
 }
 
+void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
+{
+    /* For safety, we remove bitmap after storing.
+     * We may be here in two cases:
+     * 1. bdrv_close. It's ok to drop bitmap.
+     * 2. inactivation. It means migration without 'dirty-bitmaps'
+     *    capability, so bitmaps are not marked with
+     *    BdrvDirtyBitmap.migration flags. It's not bad to drop them too,
+     *    and reload on invalidation.
+     */
+    do_store_bitmaps(bs, true, errp);
+}
+
+void qcow2_flush_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
+{
+    /* We're here because of some event like resize or reopen_ro, and we want
+     * to flush bitmaps to disk without removing them from memory. */
+    do_store_bitmaps(bs, false, errp);
+}
+
 int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
 {
     BdrvDirtyBitmap *bitmap;
-- 
2.17.2

Re: [Qemu-devel] [PATCH 2/5] block/qcow2-bitmap: Allow bitmap flushing

[Eric Blake]

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

On 3/5/19 5:43 PM, John Snow wrote:
> Usually, we only write out bitmaps when we're about to close out the file,
> so we always remove the bitmaps after to make it easier to determine the
> source of truth for migration purposes.
> 
> However, there may be times we want to flush bitmap data more aggressively,
> like after a truncate event where we need to make the metadata consistent
> again.

Or, as I've mentioned in other threads, after every
bitmap-add/bitmap-delete operation, so that 'qemu-img info -U' can see
which persistent bitmaps exist. But that's one step further than this
series, so I won't insist on it today.

> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/qcow2.h        |  1 +
>  block/qcow2-bitmap.c | 45 ++++++++++++++++++++++++++++++--------------
>  2 files changed, 32 insertions(+), 14 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [Qemu-devel] [PATCH 2/5] block/qcow2-bitmap: Allow bitmap flushing

[John Snow]

On 3/6/19 7:58 AM, Eric Blake wrote:
> On 3/5/19 5:43 PM, John Snow wrote:
>> Usually, we only write out bitmaps when we're about to close out the file,
>> so we always remove the bitmaps after to make it easier to determine the
>> source of truth for migration purposes.
>>
>> However, there may be times we want to flush bitmap data more aggressively,
>> like after a truncate event where we need to make the metadata consistent
>> again.
> 
> Or, as I've mentioned in other threads, after every
> bitmap-add/bitmap-delete operation, so that 'qemu-img info -U' can see
> which persistent bitmaps exist. But that's one step further than this
> series, so I won't insist on it today.
> 

Yes, I was keeping that in mind as I wrote this. I think it extends to
those cases trivially, so long as Vladimir is OK with what I'm trying to do.

Re: [Qemu-devel] [PATCH 2/5] block/qcow2-bitmap: Allow bitmap flushing

[Vladimir Sementsov-Ogievskiy]

06.03.2019 18:59, John Snow wrote:
> 
> 
> On 3/6/19 7:58 AM, Eric Blake wrote:
>> On 3/5/19 5:43 PM, John Snow wrote:
>>> Usually, we only write out bitmaps when we're about to close out the file,
>>> so we always remove the bitmaps after to make it easier to determine the
>>> source of truth for migration purposes.
>>>
>>> However, there may be times we want to flush bitmap data more aggressively,
>>> like after a truncate event where we need to make the metadata consistent
>>> again.
>>
>> Or, as I've mentioned in other threads, after every
>> bitmap-add/bitmap-delete operation, so that 'qemu-img info -U' can see
>> which persistent bitmaps exist. But that's one step further than this
>> series, so I won't insist on it today.
>>
> 
> Yes, I was keeping that in mind as I wrote this. I think it extends to
> those cases trivially, so long as Vladimir is OK with what I'm trying to do.
> 

I still don't see the point of flushing bitmaps, the only thing is optimizing
qemu-img info --force-share, which should never used except for debugging.

So, at least, if we'll have it, I'd prefer bitmap flushing to be optional,
and other code should not rely on it.


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 2/5] block/qcow2-bitmap: Allow bitmap flushing

[John Snow]

On 3/6/19 11:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> 06.03.2019 18:59, John Snow wrote:
>>
>>
>> On 3/6/19 7:58 AM, Eric Blake wrote:
>>> On 3/5/19 5:43 PM, John Snow wrote:
>>>> Usually, we only write out bitmaps when we're about to close out the file,
>>>> so we always remove the bitmaps after to make it easier to determine the
>>>> source of truth for migration purposes.
>>>>
>>>> However, there may be times we want to flush bitmap data more aggressively,
>>>> like after a truncate event where we need to make the metadata consistent
>>>> again.
>>>
>>> Or, as I've mentioned in other threads, after every
>>> bitmap-add/bitmap-delete operation, so that 'qemu-img info -U' can see
>>> which persistent bitmaps exist. But that's one step further than this
>>> series, so I won't insist on it today.
>>>
>>
>> Yes, I was keeping that in mind as I wrote this. I think it extends to
>> those cases trivially, so long as Vladimir is OK with what I'm trying to do.
>>
> 
> I still don't see the point of flushing bitmaps, the only thing is optimizing
> qemu-img info --force-share, which should never used except for debugging.
> 
> So, at least, if we'll have it, I'd prefer bitmap flushing to be optional,
> and other code should not rely on it.
> 

I don't have plans to add more uses of it personally, but I do think
that resize represents a genuine case of wanting the feature.

[Qemu-devel] [PATCH 3/5] block/qcow2-bitmap: don't remove bitmaps on reopen

[John Snow]

We tend to remove bitmaps when we flush them to disk, but it's not appropriate
in all cases. let the reopen mechanism use the lighter weight flush instead of
the heavier store.

Signed-off-by: John Snow <jsnow@redhat.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 4e11b6b05a..9373055d3b 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1560,7 +1560,7 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
     BdrvDirtyBitmap *bitmap;
     Error *local_err = NULL;
 
-    qcow2_store_persistent_dirty_bitmaps(bs, &local_err);
+    qcow2_flush_persistent_dirty_bitmaps(bs, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
         return -EINVAL;
-- 
2.17.2

Re: [Qemu-devel] [PATCH 3/5] block/qcow2-bitmap: don't remove bitmaps on reopen

[Vladimir Sementsov-Ogievskiy]

06.03.2019 2:43, John Snow wrote:
> We tend to remove bitmaps when we flush them to disk, but it's not appropriate
> in all cases. let the reopen mechanism use the lighter weight flush instead of
> the heavier store.
> 
> Signed-off-by: John Snow <jsnow@redhat.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 4e11b6b05a..9373055d3b 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1560,7 +1560,7 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
>       BdrvDirtyBitmap *bitmap;
>       Error *local_err = NULL;
>   
> -    qcow2_store_persistent_dirty_bitmaps(bs, &local_err);
> +    qcow2_flush_persistent_dirty_bitmaps(bs, &local_err);
>       if (local_err != NULL) {
>           error_propagate(errp, local_err);
>           return -EINVAL;
> 


hmm will it work? if we call qcow2_open after that, it will fail to load bitmaps, as they
are already exist

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 3/5] block/qcow2-bitmap: don't remove bitmaps on reopen

[John Snow]

On 3/6/19 10:28 AM, Vladimir Sementsov-Ogievskiy wrote:
> 06.03.2019 2:43, John Snow wrote:
>> We tend to remove bitmaps when we flush them to disk, but it's not appropriate
>> in all cases. let the reopen mechanism use the lighter weight flush instead of
>> the heavier store.
>>
>> Signed-off-by: John Snow <jsnow@redhat.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 4e11b6b05a..9373055d3b 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -1560,7 +1560,7 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
>>       BdrvDirtyBitmap *bitmap;
>>       Error *local_err = NULL;
>>   
>> -    qcow2_store_persistent_dirty_bitmaps(bs, &local_err);
>> +    qcow2_flush_persistent_dirty_bitmaps(bs, &local_err);
>>       if (local_err != NULL) {
>>           error_propagate(errp, local_err);
>>           return -EINVAL;
>>
> 
> 
> hmm will it work? if we call qcow2_open after that, it will fail to load bitmaps, as they
> are already exist
> 

Hmm, maybe you're right. I didn't test this very well. It seemed wrong
to me so I "fixed" it... let me write a test that covers this.

[Qemu-devel] [PATCH 4/5] block/qcow2-bitmap: Allow resizes with persistent bitmaps

[John Snow]

Since we now load all bitmaps into memory anyway, we can just truncate them
in-memory and then flush them back to disk. Just in case, we will still check
and enforce that this shortcut is valid -- i.e. that any bitmap described
on-disk is indeed in-memory and can be modified.

If there are any inconsistent bitmaps, we refuse to allow the truncate as
we do not actually load these bitmaps into memory, and it isn't safe or
reasonable to attempt to truncate corrupted data.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/qcow2.h        |  1 +
 block/qcow2-bitmap.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c        | 27 ++++++++++++++++++++-------
 3 files changed, 63 insertions(+), 7 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 4c1fdfcbec..b9227a72cc 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -689,6 +689,7 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
 int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
                                  Error **errp);
 int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
+int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 void qcow2_flush_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 9373055d3b..24f280bccc 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1167,6 +1167,48 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
     return qcow2_reopen_bitmaps_rw_hint(bs, NULL, errp);
 }
 
+/* Checks to see if it's safe to resize bitmaps */
+int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp)
+{
+    BDRVQcow2State *s = bs->opaque;
+    Qcow2BitmapList *bm_list;
+    Qcow2Bitmap *bm;
+    int ret = 0;
+
+    if (s->nb_bitmaps == 0) {
+        return 0;
+    }
+
+    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
+                               s->bitmap_directory_size, false, errp);
+    if (bm_list == NULL) {
+        return -EINVAL;
+    }
+
+    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
+        BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
+        if (bitmap == NULL) {
+            /* We rely on all bitmaps being in-memory to be able to resize them,
+             * Otherwise, we'd need to resize them on disk explicitly */
+            error_setg(errp, "Cannot resize qcow2 with persistent bitmaps that "
+                       "were not loaded into memory");
+            ret = -ENOTSUP;
+            goto out;
+        }
+
+        /* The checks against readonly and busy are redundant, but certainly
+         * do no harm. checks against inconsistent are crucial: */
+        if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) {
+            ret = -ENOTSUP;
+            goto out;
+        }
+    }
+
+out:
+    bitmap_list_free(bm_list);
+    return ret;
+}
+
 /* store_bitmap_data()
  * Store bitmap to image, filling bitmap table accordingly.
  */
diff --git a/block/qcow2.c b/block/qcow2.c
index 7fb2730f09..6fd9252494 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3472,7 +3472,7 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
 {
     BDRVQcow2State *s = bs->opaque;
     uint64_t old_length;
-    int64_t new_l1_size;
+    int64_t new_l1_size, offset_be;
     int ret;
     QDict *options;
 
@@ -3498,10 +3498,8 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
         goto fail;
     }
 
-    /* cannot proceed if image has bitmaps */
-    if (s->nb_bitmaps) {
-        /* TODO: resize bitmaps in the image */
-        error_setg(errp, "Can't resize an image which has bitmaps");
+    /* Only certain persistent bitmaps can be resized: */
+    if (qcow2_truncate_bitmaps_check(bs, errp)) {
         ret = -ENOTSUP;
         goto fail;
     }
@@ -3702,9 +3700,9 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
     bs->total_sectors = offset / BDRV_SECTOR_SIZE;
 
     /* write updated header.size */
-    offset = cpu_to_be64(offset);
+    offset_be = cpu_to_be64(offset);
     ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size),
-                           &offset, sizeof(uint64_t));
+                           &offset_be, sizeof(uint64_t));
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Failed to update the image size");
         goto fail;
@@ -3719,6 +3717,21 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
     if (ret < 0) {
         goto fail;
     }
+
+    /* Flush bitmaps */
+    if (s->nb_bitmaps) {
+        Error *local_err = NULL;
+
+        /* Usually, bitmaps get resized after this call.
+         * Force it earlier so we can make the metadata consistent. */
+        bdrv_dirty_bitmap_truncate(bs, offset);
+        qcow2_flush_persistent_dirty_bitmaps(bs, &local_err);
+        if (local_err) {
+            ret = -EINVAL;
+            goto fail;
+        }
+    }
+
     ret = 0;
 fail:
     qemu_co_mutex_unlock(&s->lock);
-- 
2.17.2

Re: [Qemu-devel] [PATCH 4/5] block/qcow2-bitmap: Allow resizes with persistent bitmaps

[Vladimir Sementsov-Ogievskiy]

06.03.2019 2:43, John Snow wrote:
> Since we now load all bitmaps into memory anyway, we can just truncate them
> in-memory and then flush them back to disk. Just in case, we will still check
> and enforce that this shortcut is valid -- i.e. that any bitmap described
> on-disk is indeed in-memory and can be modified.
> 
> If there are any inconsistent bitmaps, we refuse to allow the truncate as
> we do not actually load these bitmaps into memory, and it isn't safe or
> reasonable to attempt to truncate corrupted data.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   block/qcow2.h        |  1 +
>   block/qcow2-bitmap.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>   block/qcow2.c        | 27 ++++++++++++++++++++-------
>   3 files changed, 63 insertions(+), 7 deletions(-)
> 
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 4c1fdfcbec..b9227a72cc 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -689,6 +689,7 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
>   int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>                                    Error **errp);
>   int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
> +int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
>   void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
>   void qcow2_flush_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
>   int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 9373055d3b..24f280bccc 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1167,6 +1167,48 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
>       return qcow2_reopen_bitmaps_rw_hint(bs, NULL, errp);
>   }
>   
> +/* Checks to see if it's safe to resize bitmaps */
> +int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    Qcow2BitmapList *bm_list;
> +    Qcow2Bitmap *bm;
> +    int ret = 0;
> +
> +    if (s->nb_bitmaps == 0) {
> +        return 0;
> +    }
> +
> +    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> +                               s->bitmap_directory_size, false, errp);
> +    if (bm_list == NULL) {
> +        return -EINVAL;
> +    }
> +
> +    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> +        BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
> +        if (bitmap == NULL) {
> +            /* We rely on all bitmaps being in-memory to be able to resize them,
> +             * Otherwise, we'd need to resize them on disk explicitly */
> +            error_setg(errp, "Cannot resize qcow2 with persistent bitmaps that "
> +                       "were not loaded into memory");
> +            ret = -ENOTSUP;
> +            goto out;
> +        }
> +
> +        /* The checks against readonly and busy are redundant, but certainly
> +         * do no harm. checks against inconsistent are crucial: */
> +        if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) {
> +            ret = -ENOTSUP;
> +            goto out;
> +        }
> +    }
> +
> +out:
> +    bitmap_list_free(bm_list);
> +    return ret;
> +}
> +
>   /* store_bitmap_data()
>    * Store bitmap to image, filling bitmap table accordingly.
>    */
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 7fb2730f09..6fd9252494 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3472,7 +3472,7 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>   {
>       BDRVQcow2State *s = bs->opaque;
>       uint64_t old_length;
> -    int64_t new_l1_size;
> +    int64_t new_l1_size, offset_be;
>       int ret;
>       QDict *options;
>   
> @@ -3498,10 +3498,8 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>           goto fail;
>       }
>   
> -    /* cannot proceed if image has bitmaps */
> -    if (s->nb_bitmaps) {
> -        /* TODO: resize bitmaps in the image */
> -        error_setg(errp, "Can't resize an image which has bitmaps");
> +    /* Only certain persistent bitmaps can be resized: */
> +    if (qcow2_truncate_bitmaps_check(bs, errp)) {
>           ret = -ENOTSUP;
>           goto fail;
>       }
> @@ -3702,9 +3700,9 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>       bs->total_sectors = offset / BDRV_SECTOR_SIZE;
>   
>       /* write updated header.size */
> -    offset = cpu_to_be64(offset);
> +    offset_be = cpu_to_be64(offset);
>       ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size),
> -                           &offset, sizeof(uint64_t));
> +                           &offset_be, sizeof(uint64_t));
>       if (ret < 0) {
>           error_setg_errno(errp, -ret, "Failed to update the image size");
>           goto fail;
> @@ -3719,6 +3717,21 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>       if (ret < 0) {
>           goto fail;
>       }
> +
> +    /* Flush bitmaps */
> +    if (s->nb_bitmaps) {
> +        Error *local_err = NULL;
> +
> +        /* Usually, bitmaps get resized after this call.
> +         * Force it earlier so we can make the metadata consistent. */
> +        bdrv_dirty_bitmap_truncate(bs, offset);
> +        qcow2_flush_persistent_dirty_bitmaps(bs, &local_err);
> +        if (local_err) {
> +            ret = -EINVAL;
> +            goto fail;
> +        }
> +    }

Why to flush after resize? Bitmaps will be IN_USE in the image anyway...

Could we implement resize without flush first,  it would be one patch + test? And then consider
flushing in separate?

> +
>       ret = 0;
>   fail:
>       qemu_co_mutex_unlock(&s->lock);
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 4/5] block/qcow2-bitmap: Allow resizes with persistent bitmaps

[Eric Blake]

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

On 3/6/19 9:33 AM, Vladimir Sementsov-Ogievskiy wrote:

>> +    /* Flush bitmaps */
>> +    if (s->nb_bitmaps) {
>> +        Error *local_err = NULL;
>> +
>> +        /* Usually, bitmaps get resized after this call.
>> +         * Force it earlier so we can make the metadata consistent. */
>> +        bdrv_dirty_bitmap_truncate(bs, offset);
>> +        qcow2_flush_persistent_dirty_bitmaps(bs, &local_err);
>> +        if (local_err) {
>> +            ret = -EINVAL;
>> +            goto fail;
>> +        }
>> +    }
> 
> Why to flush after resize? Bitmaps will be IN_USE in the image anyway...
> 
> Could we implement resize without flush first,  it would be one patch + test? And then consider
> flushing in separate?

What happens with migration if we don't flush the new size to disk, so
the on-disk format has a different size than the in-memory version?

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


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

Re: [Qemu-devel] [PATCH 4/5] block/qcow2-bitmap: Allow resizes with persistent bitmaps

[Vladimir Sementsov-Ogievskiy]

06.03.2019 18:36, Eric Blake wrote:
> On 3/6/19 9:33 AM, Vladimir Sementsov-Ogievskiy wrote:
> 
>>> +    /* Flush bitmaps */
>>> +    if (s->nb_bitmaps) {
>>> +        Error *local_err = NULL;
>>> +
>>> +        /* Usually, bitmaps get resized after this call.
>>> +         * Force it earlier so we can make the metadata consistent. */
>>> +        bdrv_dirty_bitmap_truncate(bs, offset);
>>> +        qcow2_flush_persistent_dirty_bitmaps(bs, &local_err);
>>> +        if (local_err) {
>>> +            ret = -EINVAL;
>>> +            goto fail;
>>> +        }
>>> +    }
>>
>> Why to flush after resize? Bitmaps will be IN_USE in the image anyway...
>>
>> Could we implement resize without flush first,  it would be one patch + test? And then consider
>> flushing in separate?
> 
> What happens with migration if we don't flush the new size to disk, so
> the on-disk format has a different size than the in-memory version?
> 

migration works fine, as we store bitmap not on close but on inactivate.

Bitmaps may be migrated in two ways:

1. dirty-bitmaps migration capability enabled: they migrate through migration channel, RAM to RAM,
    BdrvDirtyBitmap to BdrvDirtyBitmap, stored bitmaps are unrelated and they are marked IN_USE anyway

2. dirty-bitmaps migration capability disabled: they migrate through storage, so they are stored in
    inactivation on source and then loaded in invalidation on target.

A good chunk of information is also in a huge comment inside qcow2_do_open() function.


Note: we use the following hack for resize, I never tried to send it to the list:

in qcow2_co_truncated, instead of "Can't resize an image which has bitmaps":

     if (s->nb_bitmaps) {
         /* FIXME: not loaded bitmaps will be lost */
         Error *local_err = NULL;
         qcow2_remove_all_persistent_dirty_bitmaps(bs, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
             return -EINVAL;
         }
     }

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 4/5] block/qcow2-bitmap: Allow resizes with persistent bitmaps

[John Snow]

On 3/6/19 10:33 AM, Vladimir Sementsov-Ogievskiy wrote:
> 06.03.2019 2:43, John Snow wrote:
>> Since we now load all bitmaps into memory anyway, we can just truncate them
>> in-memory and then flush them back to disk. Just in case, we will still check
>> and enforce that this shortcut is valid -- i.e. that any bitmap described
>> on-disk is indeed in-memory and can be modified.
>>
>> If there are any inconsistent bitmaps, we refuse to allow the truncate as
>> we do not actually load these bitmaps into memory, and it isn't safe or
>> reasonable to attempt to truncate corrupted data.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   block/qcow2.h        |  1 +
>>   block/qcow2-bitmap.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>   block/qcow2.c        | 27 ++++++++++++++++++++-------
>>   3 files changed, 63 insertions(+), 7 deletions(-)
>>
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index 4c1fdfcbec..b9227a72cc 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -689,6 +689,7 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
>>   int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>>                                    Error **errp);
>>   int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
>> +int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
>>   void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
>>   void qcow2_flush_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
>>   int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 9373055d3b..24f280bccc 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -1167,6 +1167,48 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
>>       return qcow2_reopen_bitmaps_rw_hint(bs, NULL, errp);
>>   }
>>   
>> +/* Checks to see if it's safe to resize bitmaps */
>> +int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp)
>> +{
>> +    BDRVQcow2State *s = bs->opaque;
>> +    Qcow2BitmapList *bm_list;
>> +    Qcow2Bitmap *bm;
>> +    int ret = 0;
>> +
>> +    if (s->nb_bitmaps == 0) {
>> +        return 0;
>> +    }
>> +
>> +    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>> +                               s->bitmap_directory_size, false, errp);
>> +    if (bm_list == NULL) {
>> +        return -EINVAL;
>> +    }
>> +
>> +    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>> +        BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
>> +        if (bitmap == NULL) {
>> +            /* We rely on all bitmaps being in-memory to be able to resize them,
>> +             * Otherwise, we'd need to resize them on disk explicitly */
>> +            error_setg(errp, "Cannot resize qcow2 with persistent bitmaps that "
>> +                       "were not loaded into memory");
>> +            ret = -ENOTSUP;
>> +            goto out;
>> +        }
>> +
>> +        /* The checks against readonly and busy are redundant, but certainly
>> +         * do no harm. checks against inconsistent are crucial: */
>> +        if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) {
>> +            ret = -ENOTSUP;
>> +            goto out;
>> +        }
>> +    }
>> +
>> +out:
>> +    bitmap_list_free(bm_list);
>> +    return ret;
>> +}
>> +
>>   /* store_bitmap_data()
>>    * Store bitmap to image, filling bitmap table accordingly.
>>    */
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 7fb2730f09..6fd9252494 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -3472,7 +3472,7 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>>   {
>>       BDRVQcow2State *s = bs->opaque;
>>       uint64_t old_length;
>> -    int64_t new_l1_size;
>> +    int64_t new_l1_size, offset_be;
>>       int ret;
>>       QDict *options;
>>   
>> @@ -3498,10 +3498,8 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>>           goto fail;
>>       }
>>   
>> -    /* cannot proceed if image has bitmaps */
>> -    if (s->nb_bitmaps) {
>> -        /* TODO: resize bitmaps in the image */
>> -        error_setg(errp, "Can't resize an image which has bitmaps");
>> +    /* Only certain persistent bitmaps can be resized: */
>> +    if (qcow2_truncate_bitmaps_check(bs, errp)) {
>>           ret = -ENOTSUP;
>>           goto fail;
>>       }
>> @@ -3702,9 +3700,9 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>>       bs->total_sectors = offset / BDRV_SECTOR_SIZE;
>>   
>>       /* write updated header.size */
>> -    offset = cpu_to_be64(offset);
>> +    offset_be = cpu_to_be64(offset);
>>       ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size),
>> -                           &offset, sizeof(uint64_t));
>> +                           &offset_be, sizeof(uint64_t));
>>       if (ret < 0) {
>>           error_setg_errno(errp, -ret, "Failed to update the image size");
>>           goto fail;
>> @@ -3719,6 +3717,21 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>>       if (ret < 0) {
>>           goto fail;
>>       }
>> +
>> +    /* Flush bitmaps */
>> +    if (s->nb_bitmaps) {
>> +        Error *local_err = NULL;
>> +
>> +        /* Usually, bitmaps get resized after this call.
>> +         * Force it earlier so we can make the metadata consistent. */
>> +        bdrv_dirty_bitmap_truncate(bs, offset);
>> +        qcow2_flush_persistent_dirty_bitmaps(bs, &local_err);
>> +        if (local_err) {
>> +            ret = -EINVAL;
>> +            goto fail;
>> +        }
>> +    }
> 
> Why to flush after resize? Bitmaps will be IN_USE in the image anyway...
> 
> Could we implement resize without flush first,  it would be one patch + test? And then consider
> flushing in separate?
> 

If you don't flush it to disk, all calls to retrieve the bm_list begin
failing because of inconsistent metadata.

Future calls to add bitmaps, remove bitmaps, etc will start failing. It
seemed safest and easiest to just flush the whole suite to disk. I am
trying to avoid premature optimizations at this stage, as I believe
resizes will be very infrequent events.

--js

Re: [Qemu-devel] [PATCH 4/5] block/qcow2-bitmap: Allow resizes with persistent bitmaps

[Vladimir Sementsov-Ogievskiy]

06.03.2019 18:41, John Snow wrote:
> 
> 
> On 3/6/19 10:33 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 06.03.2019 2:43, John Snow wrote:
>>> Since we now load all bitmaps into memory anyway, we can just truncate them
>>> in-memory and then flush them back to disk. Just in case, we will still check
>>> and enforce that this shortcut is valid -- i.e. that any bitmap described
>>> on-disk is indeed in-memory and can be modified.
>>>
>>> If there are any inconsistent bitmaps, we refuse to allow the truncate as
>>> we do not actually load these bitmaps into memory, and it isn't safe or
>>> reasonable to attempt to truncate corrupted data.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>    block/qcow2.h        |  1 +
>>>    block/qcow2-bitmap.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>>    block/qcow2.c        | 27 ++++++++++++++++++++-------
>>>    3 files changed, 63 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/block/qcow2.h b/block/qcow2.h
>>> index 4c1fdfcbec..b9227a72cc 100644
>>> --- a/block/qcow2.h
>>> +++ b/block/qcow2.h
>>> @@ -689,6 +689,7 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
>>>    int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>>>                                     Error **errp);
>>>    int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
>>> +int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
>>>    void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
>>>    void qcow2_flush_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
>>>    int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>> index 9373055d3b..24f280bccc 100644
>>> --- a/block/qcow2-bitmap.c
>>> +++ b/block/qcow2-bitmap.c
>>> @@ -1167,6 +1167,48 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
>>>        return qcow2_reopen_bitmaps_rw_hint(bs, NULL, errp);
>>>    }
>>>    
>>> +/* Checks to see if it's safe to resize bitmaps */
>>> +int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp)
>>> +{
>>> +    BDRVQcow2State *s = bs->opaque;
>>> +    Qcow2BitmapList *bm_list;
>>> +    Qcow2Bitmap *bm;
>>> +    int ret = 0;
>>> +
>>> +    if (s->nb_bitmaps == 0) {
>>> +        return 0;
>>> +    }
>>> +
>>> +    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>>> +                               s->bitmap_directory_size, false, errp);
>>> +    if (bm_list == NULL) {
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>>> +        BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
>>> +        if (bitmap == NULL) {
>>> +            /* We rely on all bitmaps being in-memory to be able to resize them,
>>> +             * Otherwise, we'd need to resize them on disk explicitly */
>>> +            error_setg(errp, "Cannot resize qcow2 with persistent bitmaps that "
>>> +                       "were not loaded into memory");
>>> +            ret = -ENOTSUP;
>>> +            goto out;
>>> +        }
>>> +
>>> +        /* The checks against readonly and busy are redundant, but certainly
>>> +         * do no harm. checks against inconsistent are crucial: */
>>> +        if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) {
>>> +            ret = -ENOTSUP;
>>> +            goto out;
>>> +        }
>>> +    }
>>> +
>>> +out:
>>> +    bitmap_list_free(bm_list);
>>> +    return ret;
>>> +}
>>> +
>>>    /* store_bitmap_data()
>>>     * Store bitmap to image, filling bitmap table accordingly.
>>>     */
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index 7fb2730f09..6fd9252494 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -3472,7 +3472,7 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>>>    {
>>>        BDRVQcow2State *s = bs->opaque;
>>>        uint64_t old_length;
>>> -    int64_t new_l1_size;
>>> +    int64_t new_l1_size, offset_be;
>>>        int ret;
>>>        QDict *options;
>>>    
>>> @@ -3498,10 +3498,8 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>>>            goto fail;
>>>        }
>>>    
>>> -    /* cannot proceed if image has bitmaps */
>>> -    if (s->nb_bitmaps) {
>>> -        /* TODO: resize bitmaps in the image */
>>> -        error_setg(errp, "Can't resize an image which has bitmaps");
>>> +    /* Only certain persistent bitmaps can be resized: */
>>> +    if (qcow2_truncate_bitmaps_check(bs, errp)) {
>>>            ret = -ENOTSUP;
>>>            goto fail;
>>>        }
>>> @@ -3702,9 +3700,9 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>>>        bs->total_sectors = offset / BDRV_SECTOR_SIZE;
>>>    
>>>        /* write updated header.size */
>>> -    offset = cpu_to_be64(offset);
>>> +    offset_be = cpu_to_be64(offset);
>>>        ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size),
>>> -                           &offset, sizeof(uint64_t));
>>> +                           &offset_be, sizeof(uint64_t));
>>>        if (ret < 0) {
>>>            error_setg_errno(errp, -ret, "Failed to update the image size");
>>>            goto fail;
>>> @@ -3719,6 +3717,21 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>>>        if (ret < 0) {
>>>            goto fail;
>>>        }
>>> +
>>> +    /* Flush bitmaps */
>>> +    if (s->nb_bitmaps) {
>>> +        Error *local_err = NULL;
>>> +
>>> +        /* Usually, bitmaps get resized after this call.
>>> +         * Force it earlier so we can make the metadata consistent. */
>>> +        bdrv_dirty_bitmap_truncate(bs, offset);
>>> +        qcow2_flush_persistent_dirty_bitmaps(bs, &local_err);
>>> +        if (local_err) {
>>> +            ret = -EINVAL;
>>> +            goto fail;
>>> +        }
>>> +    }
>>
>> Why to flush after resize? Bitmaps will be IN_USE in the image anyway...
>>
>> Could we implement resize without flush first,  it would be one patch + test? And then consider
>> flushing in separate?
>>
> 
> If you don't flush it to disk, all calls to retrieve the bm_list begin
> failing because of inconsistent metadata.
> 
> Future calls to add bitmaps, remove bitmaps, etc will start failing. It
> seemed safest and easiest to just flush the whole suite to disk. I am
> trying to avoid premature optimizations at this stage, as I believe
> resizes will be very infrequent events.

Hmmm, understand.

But I think, if we are going to allow resizes, it definitely means that IN_USE flag covers not
only bitmap data, but size field too. And we must write it to specification. And it is right
even if you flush here, as resize may fail after your flush, and actual disk size will remain
unchanged when bitmaps are flushed with new size.

And if size is covered by IN_USE, we should ignore it for IN_USE bitmaps (for IN_USE by us too).
And it means, that we need check it only on qcow2_open. And not on other cases.

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 4/5] block/qcow2-bitmap: Allow resizes with persistent bitmaps

[Vladimir Sementsov-Ogievskiy]

06.03.2019 18:52, Vladimir Sementsov-Ogievskiy wrote:
> 06.03.2019 18:41, John Snow wrote:
>>
>>
>> On 3/6/19 10:33 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> 06.03.2019 2:43, John Snow wrote:
>>>> Since we now load all bitmaps into memory anyway, we can just truncate them
>>>> in-memory and then flush them back to disk. Just in case, we will still check
>>>> and enforce that this shortcut is valid -- i.e. that any bitmap described
>>>> on-disk is indeed in-memory and can be modified.
>>>>
>>>> If there are any inconsistent bitmaps, we refuse to allow the truncate as
>>>> we do not actually load these bitmaps into memory, and it isn't safe or
>>>> reasonable to attempt to truncate corrupted data.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>>    block/qcow2.h        |  1 +
>>>>    block/qcow2-bitmap.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>>>    block/qcow2.c        | 27 ++++++++++++++++++++-------
>>>>    3 files changed, 63 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/block/qcow2.h b/block/qcow2.h
>>>> index 4c1fdfcbec..b9227a72cc 100644
>>>> --- a/block/qcow2.h
>>>> +++ b/block/qcow2.h
>>>> @@ -689,6 +689,7 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
>>>>    int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>>>>                                     Error **errp);
>>>>    int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
>>>> +int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
>>>>    void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
>>>>    void qcow2_flush_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
>>>>    int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
>>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>>> index 9373055d3b..24f280bccc 100644
>>>> --- a/block/qcow2-bitmap.c
>>>> +++ b/block/qcow2-bitmap.c
>>>> @@ -1167,6 +1167,48 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
>>>>        return qcow2_reopen_bitmaps_rw_hint(bs, NULL, errp);
>>>>    }
>>>> +/* Checks to see if it's safe to resize bitmaps */
>>>> +int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp)
>>>> +{
>>>> +    BDRVQcow2State *s = bs->opaque;
>>>> +    Qcow2BitmapList *bm_list;
>>>> +    Qcow2Bitmap *bm;
>>>> +    int ret = 0;
>>>> +
>>>> +    if (s->nb_bitmaps == 0) {
>>>> +        return 0;
>>>> +    }
>>>> +
>>>> +    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>>>> +                               s->bitmap_directory_size, false, errp);
>>>> +    if (bm_list == NULL) {
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>>>> +        BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
>>>> +        if (bitmap == NULL) {
>>>> +            /* We rely on all bitmaps being in-memory to be able to resize them,
>>>> +             * Otherwise, we'd need to resize them on disk explicitly */
>>>> +            error_setg(errp, "Cannot resize qcow2 with persistent bitmaps that "
>>>> +                       "were not loaded into memory");
>>>> +            ret = -ENOTSUP;
>>>> +            goto out;
>>>> +        }
>>>> +
>>>> +        /* The checks against readonly and busy are redundant, but certainly
>>>> +         * do no harm. checks against inconsistent are crucial: */
>>>> +        if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) {
>>>> +            ret = -ENOTSUP;
>>>> +            goto out;
>>>> +        }
>>>> +    }
>>>> +
>>>> +out:
>>>> +    bitmap_list_free(bm_list);
>>>> +    return ret;
>>>> +}
>>>> +
>>>>    /* store_bitmap_data()
>>>>     * Store bitmap to image, filling bitmap table accordingly.
>>>>     */
>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>> index 7fb2730f09..6fd9252494 100644
>>>> --- a/block/qcow2.c
>>>> +++ b/block/qcow2.c
>>>> @@ -3472,7 +3472,7 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>>>>    {
>>>>        BDRVQcow2State *s = bs->opaque;
>>>>        uint64_t old_length;
>>>> -    int64_t new_l1_size;
>>>> +    int64_t new_l1_size, offset_be;
>>>>        int ret;
>>>>        QDict *options;
>>>> @@ -3498,10 +3498,8 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>>>>            goto fail;
>>>>        }
>>>> -    /* cannot proceed if image has bitmaps */
>>>> -    if (s->nb_bitmaps) {
>>>> -        /* TODO: resize bitmaps in the image */
>>>> -        error_setg(errp, "Can't resize an image which has bitmaps");
>>>> +    /* Only certain persistent bitmaps can be resized: */
>>>> +    if (qcow2_truncate_bitmaps_check(bs, errp)) {
>>>>            ret = -ENOTSUP;
>>>>            goto fail;
>>>>        }
>>>> @@ -3702,9 +3700,9 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>>>>        bs->total_sectors = offset / BDRV_SECTOR_SIZE;
>>>>        /* write updated header.size */
>>>> -    offset = cpu_to_be64(offset);
>>>> +    offset_be = cpu_to_be64(offset);
>>>>        ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size),
>>>> -                           &offset, sizeof(uint64_t));
>>>> +                           &offset_be, sizeof(uint64_t));
>>>>        if (ret < 0) {
>>>>            error_setg_errno(errp, -ret, "Failed to update the image size");
>>>>            goto fail;
>>>> @@ -3719,6 +3717,21 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>>>>        if (ret < 0) {
>>>>            goto fail;
>>>>        }
>>>> +
>>>> +    /* Flush bitmaps */
>>>> +    if (s->nb_bitmaps) {
>>>> +        Error *local_err = NULL;
>>>> +
>>>> +        /* Usually, bitmaps get resized after this call.
>>>> +         * Force it earlier so we can make the metadata consistent. */
>>>> +        bdrv_dirty_bitmap_truncate(bs, offset);
>>>> +        qcow2_flush_persistent_dirty_bitmaps(bs, &local_err);
>>>> +        if (local_err) {
>>>> +            ret = -EINVAL;
>>>> +            goto fail;
>>>> +        }
>>>> +    }
>>>
>>> Why to flush after resize? Bitmaps will be IN_USE in the image anyway...
>>>
>>> Could we implement resize without flush first,  it would be one patch + test? And then consider
>>> flushing in separate?
>>>
>>
>> If you don't flush it to disk, all calls to retrieve the bm_list begin
>> failing because of inconsistent metadata.
>>
>> Future calls to add bitmaps, remove bitmaps, etc will start failing. It
>> seemed safest and easiest to just flush the whole suite to disk. I am
>> trying to avoid premature optimizations at this stage, as I believe
>> resizes will be very infrequent events.
> 
> Hmmm, understand.
> 
> But I think, if we are going to allow resizes, it definitely means that IN_USE flag covers not
> only bitmap data, but size field too. And we must write it to specification. And it is right
> even if you flush here, as resize may fail after your flush, and actual disk size will remain
> unchanged when bitmaps are flushed with new size.

oops, you flush only on success. But, if flush fails, what would be the state of qcow2 image?
resized or not?

> 
> And if size is covered by IN_USE, we should ignore it for IN_USE bitmaps (for IN_USE by us too).
> And it means, that we need check it only on qcow2_open. And not on other cases.
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 4/5] block/qcow2-bitmap: Allow resizes with persistent bitmaps

[John Snow]

On 3/6/19 10:52 AM, Vladimir Sementsov-Ogievskiy wrote:
> 06.03.2019 18:41, John Snow wrote:
>>
>>
>> On 3/6/19 10:33 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> 06.03.2019 2:43, John Snow wrote:
>>>> Since we now load all bitmaps into memory anyway, we can just truncate them
>>>> in-memory and then flush them back to disk. Just in case, we will still check
>>>> and enforce that this shortcut is valid -- i.e. that any bitmap described
>>>> on-disk is indeed in-memory and can be modified.
>>>>
>>>> If there are any inconsistent bitmaps, we refuse to allow the truncate as
>>>> we do not actually load these bitmaps into memory, and it isn't safe or
>>>> reasonable to attempt to truncate corrupted data.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>>    block/qcow2.h        |  1 +
>>>>    block/qcow2-bitmap.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>>>    block/qcow2.c        | 27 ++++++++++++++++++++-------
>>>>    3 files changed, 63 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/block/qcow2.h b/block/qcow2.h
>>>> index 4c1fdfcbec..b9227a72cc 100644
>>>> --- a/block/qcow2.h
>>>> +++ b/block/qcow2.h
>>>> @@ -689,6 +689,7 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
>>>>    int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>>>>                                     Error **errp);
>>>>    int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
>>>> +int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
>>>>    void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
>>>>    void qcow2_flush_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
>>>>    int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
>>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>>> index 9373055d3b..24f280bccc 100644
>>>> --- a/block/qcow2-bitmap.c
>>>> +++ b/block/qcow2-bitmap.c
>>>> @@ -1167,6 +1167,48 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
>>>>        return qcow2_reopen_bitmaps_rw_hint(bs, NULL, errp);
>>>>    }
>>>>    
>>>> +/* Checks to see if it's safe to resize bitmaps */
>>>> +int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp)
>>>> +{
>>>> +    BDRVQcow2State *s = bs->opaque;
>>>> +    Qcow2BitmapList *bm_list;
>>>> +    Qcow2Bitmap *bm;
>>>> +    int ret = 0;
>>>> +
>>>> +    if (s->nb_bitmaps == 0) {
>>>> +        return 0;
>>>> +    }
>>>> +
>>>> +    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>>>> +                               s->bitmap_directory_size, false, errp);
>>>> +    if (bm_list == NULL) {
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>>>> +        BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
>>>> +        if (bitmap == NULL) {
>>>> +            /* We rely on all bitmaps being in-memory to be able to resize them,
>>>> +             * Otherwise, we'd need to resize them on disk explicitly */
>>>> +            error_setg(errp, "Cannot resize qcow2 with persistent bitmaps that "
>>>> +                       "were not loaded into memory");
>>>> +            ret = -ENOTSUP;
>>>> +            goto out;
>>>> +        }
>>>> +
>>>> +        /* The checks against readonly and busy are redundant, but certainly
>>>> +         * do no harm. checks against inconsistent are crucial: */
>>>> +        if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) {
>>>> +            ret = -ENOTSUP;
>>>> +            goto out;
>>>> +        }
>>>> +    }
>>>> +
>>>> +out:
>>>> +    bitmap_list_free(bm_list);
>>>> +    return ret;
>>>> +}
>>>> +
>>>>    /* store_bitmap_data()
>>>>     * Store bitmap to image, filling bitmap table accordingly.
>>>>     */
>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>> index 7fb2730f09..6fd9252494 100644
>>>> --- a/block/qcow2.c
>>>> +++ b/block/qcow2.c
>>>> @@ -3472,7 +3472,7 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>>>>    {
>>>>        BDRVQcow2State *s = bs->opaque;
>>>>        uint64_t old_length;
>>>> -    int64_t new_l1_size;
>>>> +    int64_t new_l1_size, offset_be;
>>>>        int ret;
>>>>        QDict *options;
>>>>    
>>>> @@ -3498,10 +3498,8 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>>>>            goto fail;
>>>>        }
>>>>    
>>>> -    /* cannot proceed if image has bitmaps */
>>>> -    if (s->nb_bitmaps) {
>>>> -        /* TODO: resize bitmaps in the image */
>>>> -        error_setg(errp, "Can't resize an image which has bitmaps");
>>>> +    /* Only certain persistent bitmaps can be resized: */
>>>> +    if (qcow2_truncate_bitmaps_check(bs, errp)) {
>>>>            ret = -ENOTSUP;
>>>>            goto fail;
>>>>        }
>>>> @@ -3702,9 +3700,9 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>>>>        bs->total_sectors = offset / BDRV_SECTOR_SIZE;
>>>>    
>>>>        /* write updated header.size */
>>>> -    offset = cpu_to_be64(offset);
>>>> +    offset_be = cpu_to_be64(offset);
>>>>        ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size),
>>>> -                           &offset, sizeof(uint64_t));
>>>> +                           &offset_be, sizeof(uint64_t));
>>>>        if (ret < 0) {
>>>>            error_setg_errno(errp, -ret, "Failed to update the image size");
>>>>            goto fail;
>>>> @@ -3719,6 +3717,21 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>>>>        if (ret < 0) {
>>>>            goto fail;
>>>>        }
>>>> +
>>>> +    /* Flush bitmaps */
>>>> +    if (s->nb_bitmaps) {
>>>> +        Error *local_err = NULL;
>>>> +
>>>> +        /* Usually, bitmaps get resized after this call.
>>>> +         * Force it earlier so we can make the metadata consistent. */
>>>> +        bdrv_dirty_bitmap_truncate(bs, offset);
>>>> +        qcow2_flush_persistent_dirty_bitmaps(bs, &local_err);
>>>> +        if (local_err) {
>>>> +            ret = -EINVAL;
>>>> +            goto fail;
>>>> +        }
>>>> +    }
>>>
>>> Why to flush after resize? Bitmaps will be IN_USE in the image anyway...
>>>
>>> Could we implement resize without flush first,  it would be one patch + test? And then consider
>>> flushing in separate?
>>>
>>
>> If you don't flush it to disk, all calls to retrieve the bm_list begin
>> failing because of inconsistent metadata.
>>
>> Future calls to add bitmaps, remove bitmaps, etc will start failing. It
>> seemed safest and easiest to just flush the whole suite to disk. I am
>> trying to avoid premature optimizations at this stage, as I believe
>> resizes will be very infrequent events.
> 
> Hmmm, understand.
> 
> But I think, if we are going to allow resizes, it definitely means that IN_USE flag covers not
> only bitmap data, but size field too. And we must write it to specification. And it is right
> even if you flush here, as resize may fail after your flush, and actual disk size will remain
> unchanged when bitmaps are flushed with new size.
> 
> And if size is covered by IN_USE, we should ignore it for IN_USE bitmaps (for IN_USE by us too).
> And it means, that we need check it only on qcow2_open. And not on other cases.
> 

Hm, so your proposal here is:

- Check metadata constraints (like size) only when not marked IN_USE,
(i.e. on first open)

- It's fine to skip the size constraint for inconsistent bitmaps which
are not loaded, because we do not load their data anyway.

- subsequent bm_list loads do not need to check these constraints
because the bitmaps are already in memory and we will not be using the
size-related metadata.

I think you are correct, but it feels a little "clever" or "tricky" and
it warrants good documentation. I'll audit the codepaths to make sure
this is a safe approach.

(Though, even given that, I *still* want to flush bitmaps to disk on
resize, just to avoid having out of date metadata on the disk -- even if
we're not clearing the IN_USE bit on flush, it still seems "less wrong"
to have correct headers.)


To the point about resize failures:

I think we do have a problem, because no matter what order we choose, we
have a race condition:

- flush bitmaps before resize: If we crash after the flush but before
the resize, the image may be unopenable
- resize before flush (this patch): If we crash before the flush, the
image may be unopenable

So I'll have to address that: we might not be able to avoid losing the
bitmaps during resize (because of a rare failure event) but we need to
make sure the image remains openable.

We may want to amend bitmap loading so that it never FAILS because of
bad constraints, but merely marks the image as corrupt/inconsistent.
Some size issues may be recoverable but others may be "fatal" in the
sense of needing to lose the bitmap.

I'll respin, but I believe this patch series is absolutely crucial for
4.0... We've got until Tuesday!

--js

[Qemu-devel] [PATCH 5/5] tests/qemu-iotests: add bitmap resize test 246

[John Snow]

Test that we can actually resize qcow2 images with persistent bitmaps
correctly. Throw some other goofy stuff at the test while we're at it,
like adding bitmaps of different granularities and at different times.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/246     | 114 ++++++++++++++
 tests/qemu-iotests/246.out | 296 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 411 insertions(+)
 create mode 100755 tests/qemu-iotests/246
 create mode 100644 tests/qemu-iotests/246.out

diff --git a/tests/qemu-iotests/246 b/tests/qemu-iotests/246
new file mode 100755
index 0000000000..e5bd1ce2d1
--- /dev/null
+++ b/tests/qemu-iotests/246
@@ -0,0 +1,114 @@
+#!/usr/bin/env python
+#
+# Test persistent bitmap resizing.
+#
+# Copyright (c) 2019 John Snow for Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# owner=jsnow@redhat.com
+
+import iotests
+from iotests import log
+
+iotests.verify_image_format(supported_fmts=['qcow2'])
+size = 64 * 1024 * 1024 * 1024
+gran_small = 32 * 1024
+gran_large = 128 * 1024
+
+def query_bitmaps(vm):
+    res = vm.qmp("query-block")
+    return { "bitmaps": { device['device']: device.get('dirty-bitmaps', []) for
+                          device in res['return'] } }
+
+with iotests.FilePath('img') as img_path, \
+     iotests.VM() as vm:
+
+    log('--- Preparing image & VM ---\n')
+    iotests.qemu_img_create('-f', iotests.imgfmt, img_path, str(size))
+    vm.add_drive(img_path)
+
+
+    log('--- 1st Boot (Establish Baseline Image) ---\n')
+    vm.launch()
+
+    log('\n--- Adding bitmaps Small, Medium, Large, and Transient ---\n')
+    vm.qmp_log("block-dirty-bitmap-add", node="drive0",
+               name="Small", granularity=gran_small, persistent=True)
+    vm.qmp_log("block-dirty-bitmap-add", node="drive0",
+               name="Medium", persistent=True)
+    vm.qmp_log("block-dirty-bitmap-add", node="drive0",
+               name="Large", granularity=gran_large, persistent=True)
+    vm.qmp_log("block-dirty-bitmap-add", node="drive0",
+               name="Transient", persistent=False)
+
+    log('--- Forcing flush of bitmaps to disk ---\n')
+    log(query_bitmaps(vm), indent=2)
+    vm.shutdown()
+
+
+    log('--- 2nd Boot (Grow Image) ---\n')
+    vm.launch()
+    log(query_bitmaps(vm), indent=2)
+
+    log('--- Adding new bitmap, growing image, and adding 2nd new bitmap ---')
+    vm.qmp_log("block-dirty-bitmap-add", node="drive0",
+               name="New", persistent=True)
+    vm.qmp_log("human-monitor-command",
+               command_line="block_resize drive0 70G")
+    vm.qmp_log("block-dirty-bitmap-add", node="drive0",
+               name="Newtwo", persistent=True)
+    log(query_bitmaps(vm), indent=2)
+
+    log('--- Forcing flush of bitmaps to disk ---\n')
+    vm.shutdown()
+
+
+    log('--- 3rd Boot (Shrink Image) ---\n')
+    vm.launch()
+    log(query_bitmaps(vm), indent=2)
+
+    log('--- Adding "NewB" bitmap, removing "New" bitmap ---')
+    vm.qmp_log("block-dirty-bitmap-add", node="drive0",
+               name="NewB", persistent=True)
+    vm.qmp_log("block-dirty-bitmap-remove", node="drive0",
+               name="New")
+
+    log('--- Truncating image ---\n')
+    vm.qmp_log("human-monitor-command",
+               command_line="block_resize drive0 50G")
+
+    log('--- Adding "NewC" bitmap, removing "NewTwo" bitmap ---')
+    vm.qmp_log("block-dirty-bitmap-add", node="drive0",
+               name="NewC", persistent=True)
+    vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="Newtwo")
+
+    log('--- Forcing flush of bitmaps to disk ---\n')
+    vm.shutdown()
+
+
+    log('--- 4th Boot (Verification and Cleanup) ---\n')
+    vm.launch()
+    log(query_bitmaps(vm), indent=2)
+
+    log('--- Removing all Bitmaps ---\n')
+    vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="Small")
+    vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="Medium")
+    vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="Large")
+    vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="NewB")
+    vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="NewC")
+    log(query_bitmaps(vm), indent=2)
+
+    log('\n--- Done ---\n')
+    vm.shutdown()
diff --git a/tests/qemu-iotests/246.out b/tests/qemu-iotests/246.out
new file mode 100644
index 0000000000..ea591b0e46
--- /dev/null
+++ b/tests/qemu-iotests/246.out
@@ -0,0 +1,296 @@
+--- Preparing image & VM ---
+
+--- 1st Boot (Establish Baseline Image) ---
+
+
+--- Adding bitmaps Small, Medium, Large, and Transient ---
+
+{"execute": "block-dirty-bitmap-add", "arguments": {"granularity": 32768, "name": "Small", "node": "drive0", "persistent": true}}
+{"return": {}}
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "Medium", "node": "drive0", "persistent": true}}
+{"return": {}}
+{"execute": "block-dirty-bitmap-add", "arguments": {"granularity": 131072, "name": "Large", "node": "drive0", "persistent": true}}
+{"return": {}}
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "Transient", "node": "drive0", "persistent": false}}
+{"return": {}}
+--- Forcing flush of bitmaps to disk ---
+
+{
+  "bitmaps": {
+    "drive0": [
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "Transient",
+        "persistent": false,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 131072,
+        "name": "Large",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "Medium",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 32768,
+        "name": "Small",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+--- 2nd Boot (Grow Image) ---
+
+{
+  "bitmaps": {
+    "drive0": [
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 32768,
+        "name": "Small",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "Medium",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 131072,
+        "name": "Large",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+--- Adding new bitmap, growing image, and adding 2nd new bitmap ---
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "New", "node": "drive0", "persistent": true}}
+{"return": {}}
+{"execute": "human-monitor-command", "arguments": {"command-line": "block_resize drive0 70G"}}
+{"return": ""}
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "Newtwo", "node": "drive0", "persistent": true}}
+{"return": {}}
+{
+  "bitmaps": {
+    "drive0": [
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "Newtwo",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "New",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 32768,
+        "name": "Small",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "Medium",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 131072,
+        "name": "Large",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+--- Forcing flush of bitmaps to disk ---
+
+--- 3rd Boot (Shrink Image) ---
+
+{
+  "bitmaps": {
+    "drive0": [
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "Newtwo",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "New",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 32768,
+        "name": "Small",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "Medium",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 131072,
+        "name": "Large",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+--- Adding "NewB" bitmap, removing "New" bitmap ---
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "NewB", "node": "drive0", "persistent": true}}
+{"return": {}}
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "New", "node": "drive0"}}
+{"return": {}}
+--- Truncating image ---
+
+{"execute": "human-monitor-command", "arguments": {"command-line": "block_resize drive0 50G"}}
+{"return": ""}
+--- Adding "NewC" bitmap, removing "NewTwo" bitmap ---
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "NewC", "node": "drive0", "persistent": true}}
+{"return": {}}
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "Newtwo", "node": "drive0"}}
+{"return": {}}
+--- Forcing flush of bitmaps to disk ---
+
+--- 4th Boot (Verification and Cleanup) ---
+
+{
+  "bitmaps": {
+    "drive0": [
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "NewC",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "NewB",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 32768,
+        "name": "Small",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 65536,
+        "name": "Medium",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      },
+      {
+        "busy": false,
+        "count": 0,
+        "granularity": 131072,
+        "name": "Large",
+        "persistent": true,
+        "recording": true,
+        "status": "active"
+      }
+    ]
+  }
+}
+--- Removing all Bitmaps ---
+
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "Small", "node": "drive0"}}
+{"return": {}}
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "Medium", "node": "drive0"}}
+{"return": {}}
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "Large", "node": "drive0"}}
+{"return": {}}
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "NewB", "node": "drive0"}}
+{"return": {}}
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "NewC", "node": "drive0"}}
+{"return": {}}
+{
+  "bitmaps": {
+    "drive0": []
+  }
+}
+
+--- Done ---
+
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index b5ca63cf72..0683f6e10d 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -241,3 +241,4 @@
 239 rw auto quick
 240 auto quick
 242 rw auto quick
+246 rw auto quick
-- 
2.17.2

Re: [Qemu-devel] [PATCH 0/5] block/qcow2-bitmap: Enable resize with persistent bitmaps

[John Snow]

On 3/5/19 6:43 PM, John Snow wrote:

++ This series relies on [Qemu-devel] [PATCH v3 0/7] bitmaps: add
inconsistent bit, you can apply it on top of those patches, or you can
use my github staging branch as a basis:

https://github.com/jnsnow/qemu/tree/bitmaps

> This series aims to enable block resizes when persistent bitmaps are
> in use. The basic approach here is to recognize that we now load all
> persistent bitmaps in memory, and so we can rely on in-memory resizes
> and then flush the changed metadata back to disk.
> 
> One part that is potentially now quite strange is that bitmap resizes
> may happen twice: once during the qcow2 resize event only if persistent
> bitmaps are found, and then again as part of the generic resize callback
> event whether or not we have any persistent bitmaps.
> 
> The second round is required if we are not using qcow2 or we have only
> transient bitmaps. The first round is required as we wish to flush the
> bitmaps back to disk atomically with the qcow2 resize to avoid violating
> our invariants for the bitmap metadata which is checked in many places.
> 
> This is harmless; hbitmap_truncate will recognize the second round as
> a no-op.
> 
> John Snow (5):
>   block/qcow2-bitmap: Skip length check in some cases
>   block/qcow2-bitmap: Allow bitmap flushing
>   block/qcow2-bitmap: don't remove bitmaps on reopen
>   block/qcow2-bitmap: Allow resizes with persistent bitmaps
>   tests/qemu-iotests: add bitmap resize test 246
> 
>  block/qcow2.h              |   2 +
>  block/qcow2-bitmap.c       | 123 +++++++++++----
>  block/qcow2.c              |  27 +++-
>  tests/qemu-iotests/246     | 114 ++++++++++++++
>  tests/qemu-iotests/246.out | 296 +++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/group   |   1 +
>  6 files changed, 528 insertions(+), 35 deletions(-)
>  create mode 100755 tests/qemu-iotests/246
>  create mode 100644 tests/qemu-iotests/246.out
> 

Re: [Qemu-devel] [PATCH 0/5] block/qcow2-bitmap: Enable resize with persistent bitmaps

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190305234337.18353-1-jsnow@redhat.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-mingw@fedora SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      crypto/tlssession.o
  CC      crypto/secret.o
/tmp/qemu-test/src/block/qcow2-bitmap.c: In function 'qcow2_truncate_bitmaps_check':
/tmp/qemu-test/src/block/qcow2-bitmap.c:1198:13: error: implicit declaration of function 'bdrv_dirty_bitmap_check'; did you mean 'bdrv_dirty_bitmap_lock'? [-Werror=implicit-function-declaration]
         if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) {
             ^~~~~~~~~~~~~~~~~~~~~~~
             bdrv_dirty_bitmap_lock
/tmp/qemu-test/src/block/qcow2-bitmap.c:1198:13: error: nested extern declaration of 'bdrv_dirty_bitmap_check' [-Werror=nested-externs]
/tmp/qemu-test/src/block/qcow2-bitmap.c:1198:45: error: 'BDRV_BITMAP_DEFAULT' undeclared (first use in this function); did you mean 'DM_OUT_DEFAULT'?
         if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) {
                                             ^~~~~~~~~~~~~~~~~~~
                                             DM_OUT_DEFAULT


The full log is available at
http://patchew.org/logs/20190305234337.18353-1-jsnow@redhat.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH 0/5] block/qcow2-bitmap: Enable resize with persistent bitmaps

[Eric Blake]

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

On 3/5/19 5:43 PM, John Snow wrote:
> This series aims to enable block resizes when persistent bitmaps are
> in use. The basic approach here is to recognize that we now load all
> persistent bitmaps in memory, and so we can rely on in-memory resizes
> and then flush the changed metadata back to disk.
> 
> One part that is potentially now quite strange is that bitmap resizes
> may happen twice: once during the qcow2 resize event only if persistent
> bitmaps are found, and then again as part of the generic resize callback
> event whether or not we have any persistent bitmaps.
> 
> The second round is required if we are not using qcow2 or we have only
> transient bitmaps. The first round is required as we wish to flush the
> bitmaps back to disk atomically with the qcow2 resize to avoid violating
> our invariants for the bitmap metadata which is checked in many places.
> 
> This is harmless; hbitmap_truncate will recognize the second round as
> a no-op.

FWIW - I have not yet reviewed this series closely, but I think it would
be wise to get this initial cut in before softfreeze (we can make
further tweaks to fix bugs in assumptions during rc1 and rc2, but it's a
lot harder to add the series at all if it misses softfreeze).

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


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

Re: [Qemu-devel] [PATCH 0/5] block/qcow2-bitmap: Enable resize with persistent bitmaps

[Vladimir Sementsov-Ogievskiy]

11.03.2019 19:18, Eric Blake wrote:
> On 3/5/19 5:43 PM, John Snow wrote:
>> This series aims to enable block resizes when persistent bitmaps are
>> in use. The basic approach here is to recognize that we now load all
>> persistent bitmaps in memory, and so we can rely on in-memory resizes
>> and then flush the changed metadata back to disk.
>>
>> One part that is potentially now quite strange is that bitmap resizes
>> may happen twice: once during the qcow2 resize event only if persistent
>> bitmaps are found, and then again as part of the generic resize callback
>> event whether or not we have any persistent bitmaps.
>>
>> The second round is required if we are not using qcow2 or we have only
>> transient bitmaps. The first round is required as we wish to flush the
>> bitmaps back to disk atomically with the qcow2 resize to avoid violating
>> our invariants for the bitmap metadata which is checked in many places.
>>
>> This is harmless; hbitmap_truncate will recognize the second round as
>> a no-op.
> 
> FWIW - I have not yet reviewed this series closely, but I think it would
> be wise to get this initial cut in before softfreeze (we can make
> further tweaks to fix bugs in assumptions during rc1 and rc2, but it's a
> lot harder to add the series at all if it misses softfreeze).
> 

I still not convinced that we need bitmap flushing. I think (and Den supports
me) that it's a bad idea. It makes things more difficult without a reason,
except improving debugging with --force-share which should never be used in
production.

Bitmaps are stored in qcow2_inactivate() which is true place for flushing caches.

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 0/5] block/qcow2-bitmap: Enable resize with persistent bitmaps

[Eric Blake]

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

On 3/11/19 12:36 PM, Vladimir Sementsov-Ogievskiy wrote:

>> FWIW - I have not yet reviewed this series closely, but I think it would
>> be wise to get this initial cut in before softfreeze (we can make
>> further tweaks to fix bugs in assumptions during rc1 and rc2, but it's a
>> lot harder to add the series at all if it misses softfreeze).
>>
> 
> I still not convinced that we need bitmap flushing. I think (and Den supports
> me) that it's a bad idea. It makes things more difficult without a reason,
> except improving debugging with --force-share which should never be used in
> production.

And I'm still not convinced that adding bitmap flushing will add a
benchmarkable delay to operation. Although debugging may not be needed
in a production environment, having it after a failure can be a lifesaver.

Looking at this from another point of view: if this series goes in now
(with its use of limited bitmap flushing on resize in order to make
coding bitmap resize easier, rather than global bitmap flushing after
every bitmap add/delete), then the only time that flushing to disk
happens is during resize (which is currently flat-out forbidden), so it
will not penalize existing use cases. And we still have rc1/rc2 to fix
any bugs if we can come up with some other way to get resize to work
without flushing (or even revert things if it proves to be too
invasive). But as a feature addition, if this series does not go in now,
then bitmap resize is stuck waiting for 4.2, regardless of what we can
figure out during rc1/rc2.

> 
> Bitmaps are stored in qcow2_inactivate() which is true place for flushing caches.
> 

There doesn't have to be just one place for flushing caches.  In terms
of IOPs, how much overhead does a flush cost in relation to everything
else?  And how infrequent are bitmap resize, add, and delete events?
Optimizing to the bare minimum of flushes just because it adds minimal
overhead may be premature optimization if that overhead is in the noise
anyway.

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


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

Re: [Qemu-devel] [PATCH 0/5] block/qcow2-bitmap: Enable resize with persistent bitmaps

[Vladimir Sementsov-Ogievskiy]

11.03.2019 20:48, Eric Blake wrote:
> On 3/11/19 12:36 PM, Vladimir Sementsov-Ogievskiy wrote:
> 
>>> FWIW - I have not yet reviewed this series closely, but I think it would
>>> be wise to get this initial cut in before softfreeze (we can make
>>> further tweaks to fix bugs in assumptions during rc1 and rc2, but it's a
>>> lot harder to add the series at all if it misses softfreeze).
>>>
>>
>> I still not convinced that we need bitmap flushing. I think (and Den supports
>> me) that it's a bad idea. It makes things more difficult without a reason,
>> except improving debugging with --force-share which should never be used in
>> production.
> 
> And I'm still not convinced that adding bitmap flushing will add a
> benchmarkable delay to operation. Although debugging may not be needed
> in a production environment, having it after a failure can be a lifesaver.

What is the real debugging case, when we need this information? Keeping in mind,
that it may still be incorrect due to different error paths? (resize failed on
some point before flush, or resize successed but flush failed)

> 
> Looking at this from another point of view: if this series goes in now
> (with its use of limited bitmap flushing on resize in order to make
> coding bitmap resize easier, rather than global bitmap flushing after
> every bitmap add/delete), then the only time that flushing to disk
> happens is during resize (which is currently flat-out forbidden), so it
> will not penalize existing use cases. And we still have rc1/rc2 to fix
> any bugs if we can come up with some other way to get resize to work
> without flushing (or even revert things if it proves to be too
> invasive). But as a feature addition, if this series does not go in now,
> then bitmap resize is stuck waiting for 4.2, regardless of what we can
> figure out during rc1/rc2.
> 
>>
>> Bitmaps are stored in qcow2_inactivate() which is true place for flushing caches.
>>
> 
> There doesn't have to be just one place for flushing caches.  In terms
> of IOPs, how much overhead does a flush cost in relation to everything
> else?

Assume we have 16tb disk. Bitmap with default granularity will take 30mb.
Assume that we have several such bitmaps (for example, several disabled and
one active, remember differential backups and related staff). So, it will
be several hundreds of megabytes.

   And how infrequent are bitmap resize, add, and delete events?
> Optimizing to the bare minimum of flushes just because it adds minimal
> overhead may be premature optimization if that overhead is in the noise
> anyway.
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 0/5] block/qcow2-bitmap: Enable resize with persistent bitmaps

[John Snow]

On 3/11/19 1:36 PM, Vladimir Sementsov-Ogievskiy wrote:
> 11.03.2019 19:18, Eric Blake wrote:
>> On 3/5/19 5:43 PM, John Snow wrote:
>>> This series aims to enable block resizes when persistent bitmaps are
>>> in use. The basic approach here is to recognize that we now load all
>>> persistent bitmaps in memory, and so we can rely on in-memory resizes
>>> and then flush the changed metadata back to disk.
>>>
>>> One part that is potentially now quite strange is that bitmap resizes
>>> may happen twice: once during the qcow2 resize event only if persistent
>>> bitmaps are found, and then again as part of the generic resize callback
>>> event whether or not we have any persistent bitmaps.
>>>
>>> The second round is required if we are not using qcow2 or we have only
>>> transient bitmaps. The first round is required as we wish to flush the
>>> bitmaps back to disk atomically with the qcow2 resize to avoid violating
>>> our invariants for the bitmap metadata which is checked in many places.
>>>
>>> This is harmless; hbitmap_truncate will recognize the second round as
>>> a no-op.
>>
>> FWIW - I have not yet reviewed this series closely, but I think it would
>> be wise to get this initial cut in before softfreeze (we can make
>> further tweaks to fix bugs in assumptions during rc1 and rc2, but it's a
>> lot harder to add the series at all if it misses softfreeze).
>>
> 
> I still not convinced that we need bitmap flushing. I think (and Den supports
> me) that it's a bad idea. It makes things more difficult without a reason,
> except improving debugging with --force-share which should never be used in
> production.
> 

I know you don't want it in the general case, but I think resizing
represents a genuine case for exactly when we want it.

Set aside your concerns about general case flushing for now -- this is a
distraction -- and let us consider ONLY resize events.

If QEMU crashes after a resize event, we will be unable to even open
qcow2 images if we don't resize the metadata information and write it
back out to disk.

It is far simpler -- logically and technically -- to just write this
data out to disk during a resize event. It won't happen often, shouldn't
incur much of an IO penalty, and (I think) is algorithmically
straightforward.

If it's really that much of a problem, we can revise it during the RC
period to eliminate the flush.

> Bitmaps are stored in qcow2_inactivate() which is true place for flushing caches.
> 

Re: [Qemu-devel] [PATCH 0/5] block/qcow2-bitmap: Enable resize with persistent bitmaps

[Vladimir Sementsov-Ogievskiy]

11.03.2019 21:05, John Snow wrote:
> 
> 
> On 3/11/19 1:36 PM, Vladimir Sementsov-Ogievskiy wrote:
>> 11.03.2019 19:18, Eric Blake wrote:
>>> On 3/5/19 5:43 PM, John Snow wrote:
>>>> This series aims to enable block resizes when persistent bitmaps are
>>>> in use. The basic approach here is to recognize that we now load all
>>>> persistent bitmaps in memory, and so we can rely on in-memory resizes
>>>> and then flush the changed metadata back to disk.
>>>>
>>>> One part that is potentially now quite strange is that bitmap resizes
>>>> may happen twice: once during the qcow2 resize event only if persistent
>>>> bitmaps are found, and then again as part of the generic resize callback
>>>> event whether or not we have any persistent bitmaps.
>>>>
>>>> The second round is required if we are not using qcow2 or we have only
>>>> transient bitmaps. The first round is required as we wish to flush the
>>>> bitmaps back to disk atomically with the qcow2 resize to avoid violating
>>>> our invariants for the bitmap metadata which is checked in many places.
>>>>
>>>> This is harmless; hbitmap_truncate will recognize the second round as
>>>> a no-op.
>>>
>>> FWIW - I have not yet reviewed this series closely, but I think it would
>>> be wise to get this initial cut in before softfreeze (we can make
>>> further tweaks to fix bugs in assumptions during rc1 and rc2, but it's a
>>> lot harder to add the series at all if it misses softfreeze).
>>>
>>
>> I still not convinced that we need bitmap flushing. I think (and Den supports
>> me) that it's a bad idea. It makes things more difficult without a reason,
>> except improving debugging with --force-share which should never be used in
>> production.
>>
> 
> I know you don't want it in the general case, but I think resizing
> represents a genuine case for exactly when we want it.
> 
> Set aside your concerns about general case flushing for now -- this is a
> distraction -- and let us consider ONLY resize events.
> 
> If QEMU crashes after a resize event, we will be unable to even open
> qcow2 images if we don't resize the metadata information and write it
> back out to disk.
> 
> It is far simpler -- logically and technically -- to just write this
> data out to disk during a resize event. It won't happen often, shouldn't
> incur much of an IO penalty, and (I think) is algorithmically
> straightforward.
> 
> If it's really that much of a problem, we can revise it during the RC
> period to eliminate the flush.
> 
>> Bitmaps are stored in qcow2_inactivate() which is true place for flushing caches.
>>


Hmm, you are so sure, that I can't continue arguing)
What about patch 3, did you test it?
It's just strange for me that we are going to push unreviewed series just to have
the feature in 4.0.. Why is it so important?
Also, we have no comments from Max and Kevin who are maintainers of the only file,
changed in the series. And why this don't go through their trees?
This all just breaks all patterns I'm used to..

And I hope, I'll send a counter-propasal without flushing in a few minutes...


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 0/5] block/qcow2-bitmap: Enable resize with persistent bitmaps

[John Snow]

On 3/11/19 2:26 PM, Vladimir Sementsov-Ogievskiy wrote:
> 11.03.2019 21:05, John Snow wrote:
>>
>>
>> On 3/11/19 1:36 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> 11.03.2019 19:18, Eric Blake wrote:
>>>> On 3/5/19 5:43 PM, John Snow wrote:
>>>>> This series aims to enable block resizes when persistent bitmaps are
>>>>> in use. The basic approach here is to recognize that we now load all
>>>>> persistent bitmaps in memory, and so we can rely on in-memory resizes
>>>>> and then flush the changed metadata back to disk.
>>>>>
>>>>> One part that is potentially now quite strange is that bitmap resizes
>>>>> may happen twice: once during the qcow2 resize event only if persistent
>>>>> bitmaps are found, and then again as part of the generic resize callback
>>>>> event whether or not we have any persistent bitmaps.
>>>>>
>>>>> The second round is required if we are not using qcow2 or we have only
>>>>> transient bitmaps. The first round is required as we wish to flush the
>>>>> bitmaps back to disk atomically with the qcow2 resize to avoid violating
>>>>> our invariants for the bitmap metadata which is checked in many places.
>>>>>
>>>>> This is harmless; hbitmap_truncate will recognize the second round as
>>>>> a no-op.
>>>>
>>>> FWIW - I have not yet reviewed this series closely, but I think it would
>>>> be wise to get this initial cut in before softfreeze (we can make
>>>> further tweaks to fix bugs in assumptions during rc1 and rc2, but it's a
>>>> lot harder to add the series at all if it misses softfreeze).
>>>>
>>>
>>> I still not convinced that we need bitmap flushing. I think (and Den supports
>>> me) that it's a bad idea. It makes things more difficult without a reason,
>>> except improving debugging with --force-share which should never be used in
>>> production.
>>>
>>
>> I know you don't want it in the general case, but I think resizing
>> represents a genuine case for exactly when we want it.
>>
>> Set aside your concerns about general case flushing for now -- this is a
>> distraction -- and let us consider ONLY resize events.
>>
>> If QEMU crashes after a resize event, we will be unable to even open
>> qcow2 images if we don't resize the metadata information and write it
>> back out to disk.
>>
>> It is far simpler -- logically and technically -- to just write this
>> data out to disk during a resize event. It won't happen often, shouldn't
>> incur much of an IO penalty, and (I think) is algorithmically
>> straightforward.
>>
>> If it's really that much of a problem, we can revise it during the RC
>> period to eliminate the flush.
>>
>>> Bitmaps are stored in qcow2_inactivate() which is true place for flushing caches.
>>>
> 
> 
> Hmm, you are so sure, that I can't continue arguing)
> What about patch 3, did you test it?

I'm going to scrap that part. I'm not staging this series as-is.

> It's just strange for me that we are going to push unreviewed series just to have
> the feature in 4.0.. Why is it so important?

Libvirt is finalizing its API to use the bitmaps feature and consumers
above libvirt, oVirt et al, want to be able to resize their disks. I
consider it part of the minimum necessary package to deliver the libvirt
API, and QEMU 4.0 will be a minimum version to use that API.

> Also, we have no comments from Max and Kevin who are maintainers of the only file,
> changed in the series. And why this don't go through their trees?

I've discussed it with Kevin in the IRC channel; I don't think he has
strong feelings on flushing bitmap metadata versus not as much as he
cares that we make sure we leave openable images on crash. In general I
think bitmaps are "our problem".

> This all just breaks all patterns I'm used to..
> 

Come now, this is melodramatic. This changes nothing about how bitmaps
work except during resize operations which used to be prohibited.

> And I hope, I'll send a counter-propasal without flushing in a few minutes...
> 
Go ahead.

I'm going to have to make a decision tonight, though.

--js