It will be needed in following commits for persistent bitmaps.
If bitmap is loaded from read-only storage (and we can't mark it
"in use" in this storage) corresponding BdrvDirtyBitmap should be
read-only.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/dirty-bitmap.c         | 16 ++++++++++++++++
 include/block/dirty-bitmap.h |  3 +++
 2 files changed, 19 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 90af37287f..ab6a95cf41 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -44,6 +44,8 @@ struct BdrvDirtyBitmap {
     int64_t size;               /* Size of the bitmap (Number of sectors) */
     bool disabled;              /* Bitmap is read-only */
     int active_iterators;       /* How many iterators are active */
+    bool readonly;              /* Bitmap is read-only and may be changed only
+                                   by deserialize* functions */
     QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
 
@@ -436,6 +438,7 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
                            int64_t cur_sector, int64_t nr_sectors)
 {
     assert(bdrv_dirty_bitmap_enabled(bitmap));
+    assert(!bdrv_dirty_bitmap_readonly(bitmap));
     hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
 }
 
@@ -443,12 +446,14 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
                              int64_t cur_sector, int64_t nr_sectors)
 {
     assert(bdrv_dirty_bitmap_enabled(bitmap));
+    assert(!bdrv_dirty_bitmap_readonly(bitmap));
     hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
 }
 
 void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
 {
     assert(bdrv_dirty_bitmap_enabled(bitmap));
+    assert(!bdrv_dirty_bitmap_readonly(bitmap));
     if (!out) {
         hbitmap_reset_all(bitmap->bitmap);
     } else {
@@ -519,6 +524,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
         if (!bdrv_dirty_bitmap_enabled(bitmap)) {
             continue;
         }
+        assert(!bdrv_dirty_bitmap_readonly(bitmap));
         hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
     }
 }
@@ -540,3 +546,13 @@ int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
 {
     return hbitmap_count(bitmap->meta);
 }
+
+bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap)
+{
+    return bitmap->readonly;
+}
+
+void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap)
+{
+    bitmap->readonly = true;
+}
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 1e17729ac2..0aab5841f5 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -75,4 +75,7 @@ void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap,
                                         bool finish);
 void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
 
+bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
+void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap);
+
 #endif
-- 
2.11.1

On 05/03/2017 08:25 AM, Vladimir Sementsov-Ogievskiy wrote:
> It will be needed in following commits for persistent bitmaps.
> If bitmap is loaded from read-only storage (and we can't mark it
> "in use" in this storage) corresponding BdrvDirtyBitmap should be
> read-only.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/dirty-bitmap.c         | 16 ++++++++++++++++
>  include/block/dirty-bitmap.h |  3 +++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 90af37287f..ab6a95cf41 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -44,6 +44,8 @@ struct BdrvDirtyBitmap {
>      int64_t size;               /* Size of the bitmap (Number of sectors) */
>      bool disabled;              /* Bitmap is read-only */
>      int active_iterators;       /* How many iterators are active */
> +    bool readonly;              /* Bitmap is read-only and may be changed only
> +                                   by deserialize* functions */
>      QLIST_ENTRY(BdrvDirtyBitmap) list;
>  };
>  
> @@ -436,6 +438,7 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>                             int64_t cur_sector, int64_t nr_sectors)
>  {
>      assert(bdrv_dirty_bitmap_enabled(bitmap));
> +    assert(!bdrv_dirty_bitmap_readonly(bitmap));

Doesn't this change the nature of the assertion?

We're going from
    return !(bitmap->disabled || bitmap->successor);
to
	!bitmap->readonly

That makes me a little nervous to ACK this patch, because the
correctness depends on how readonly is updated and manipulated in the
future, which makes it more prone to error.

>      hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
>  }
>  
> @@ -443,12 +446,14 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>                               int64_t cur_sector, int64_t nr_sectors)
>  {
>      assert(bdrv_dirty_bitmap_enabled(bitmap));
> +    assert(!bdrv_dirty_bitmap_readonly(bitmap));
>      hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
>  }
>  
>  void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
>  {
>      assert(bdrv_dirty_bitmap_enabled(bitmap));
> +    assert(!bdrv_dirty_bitmap_readonly(bitmap));
>      if (!out) {
>          hbitmap_reset_all(bitmap->bitmap);
>      } else {
> @@ -519,6 +524,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
>          if (!bdrv_dirty_bitmap_enabled(bitmap)) {
>              continue;
>          }
> +        assert(!bdrv_dirty_bitmap_readonly(bitmap));
>          hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
>      }
>  }
> @@ -540,3 +546,13 @@ int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
>  {
>      return hbitmap_count(bitmap->meta);
>  }
> +
> +bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap)
> +{
> +    return bitmap->readonly;
> +}
> +
> +void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap)
> +{
> +    bitmap->readonly = true;
> +}
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 1e17729ac2..0aab5841f5 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -75,4 +75,7 @@ void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap,
>                                          bool finish);
>  void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
>  
> +bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
> +void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap);
> +
>  #endif
> 

20.05.2017 02:02, John Snow wrote:
>
> On 05/03/2017 08:25 AM, Vladimir Sementsov-Ogievskiy wrote:
>> It will be needed in following commits for persistent bitmaps.
>> If bitmap is loaded from read-only storage (and we can't mark it
>> "in use" in this storage) corresponding BdrvDirtyBitmap should be
>> read-only.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/dirty-bitmap.c         | 16 ++++++++++++++++
>>   include/block/dirty-bitmap.h |  3 +++
>>   2 files changed, 19 insertions(+)
>>
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 90af37287f..ab6a95cf41 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -44,6 +44,8 @@ struct BdrvDirtyBitmap {
>>       int64_t size;               /* Size of the bitmap (Number of sectors) */
>>       bool disabled;              /* Bitmap is read-only */
>>       int active_iterators;       /* How many iterators are active */
>> +    bool readonly;              /* Bitmap is read-only and may be changed only
>> +                                   by deserialize* functions */
>>       QLIST_ENTRY(BdrvDirtyBitmap) list;
>>   };
>>   
>> @@ -436,6 +438,7 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>>                              int64_t cur_sector, int64_t nr_sectors)
>>   {
>>       assert(bdrv_dirty_bitmap_enabled(bitmap));
>> +    assert(!bdrv_dirty_bitmap_readonly(bitmap));
> Doesn't this change the nature of the assertion?
>
> We're going from
>      return !(bitmap->disabled || bitmap->successor);
> to
> 	!bitmap->readonly
>
> That makes me a little nervous to ACK this patch, because the
> correctness depends on how readonly is updated and manipulated in the
> future, which makes it more prone to error.

Sorry for late answer, I was ill.

Hmm, old assert is still here. I just add another one. Readonly means: 
bitmap must not be changed by _get _set and _clear at all - from loading 
up to releasing. It is needed to maintain the case: "bitmap was loaded 
from read-only storage, so that it was not marked as 'in_use' in that 
storge". 'disabled' field is not appropriate because it is managed by user.

>
>>       hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
>>   }
>>   
>> @@ -443,12 +446,14 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>>                                int64_t cur_sector, int64_t nr_sectors)
>>   {
>>       assert(bdrv_dirty_bitmap_enabled(bitmap));
>> +    assert(!bdrv_dirty_bitmap_readonly(bitmap));
>>       hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
>>   }
>>   
>>   void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
>>   {
>>       assert(bdrv_dirty_bitmap_enabled(bitmap));
>> +    assert(!bdrv_dirty_bitmap_readonly(bitmap));
>>       if (!out) {
>>           hbitmap_reset_all(bitmap->bitmap);
>>       } else {
>> @@ -519,6 +524,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
>>           if (!bdrv_dirty_bitmap_enabled(bitmap)) {
>>               continue;
>>           }
>> +        assert(!bdrv_dirty_bitmap_readonly(bitmap));
>>           hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
>>       }
>>   }
>> @@ -540,3 +546,13 @@ int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
>>   {
>>       return hbitmap_count(bitmap->meta);
>>   }
>> +
>> +bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap)
>> +{
>> +    return bitmap->readonly;
>> +}
>> +
>> +void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap)
>> +{
>> +    bitmap->readonly = true;
>> +}
>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>> index 1e17729ac2..0aab5841f5 100644
>> --- a/include/block/dirty-bitmap.h
>> +++ b/include/block/dirty-bitmap.h
>> @@ -75,4 +75,7 @@ void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap,
>>                                           bool finish);
>>   void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
>>   
>> +bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
>> +void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap);
>> +
>>   #endif
>>

-- 
Best regards,
Vladimir

On 05/19/2017 07:02 PM, John Snow wrote:
> 
> 
> On 05/03/2017 08:25 AM, Vladimir Sementsov-Ogievskiy wrote:
>> It will be needed in following commits for persistent bitmaps.
>> If bitmap is loaded from read-only storage (and we can't mark it
>> "in use" in this storage) corresponding BdrvDirtyBitmap should be
>> read-only.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>  block/dirty-bitmap.c         | 16 ++++++++++++++++
>>  include/block/dirty-bitmap.h |  3 +++
>>  2 files changed, 19 insertions(+)
>>
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 90af37287f..ab6a95cf41 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -44,6 +44,8 @@ struct BdrvDirtyBitmap {
>>      int64_t size;               /* Size of the bitmap (Number of sectors) */
>>      bool disabled;              /* Bitmap is read-only */
>>      int active_iterators;       /* How many iterators are active */
>> +    bool readonly;              /* Bitmap is read-only and may be changed only
>> +                                   by deserialize* functions */
>>      QLIST_ENTRY(BdrvDirtyBitmap) list;
>>  };
>>  
>> @@ -436,6 +438,7 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>>                             int64_t cur_sector, int64_t nr_sectors)
>>  {
>>      assert(bdrv_dirty_bitmap_enabled(bitmap));
>> +    assert(!bdrv_dirty_bitmap_readonly(bitmap));
> 
> Doesn't this change the nature of the assertion?
> 
> We're going from
>     return !(bitmap->disabled || bitmap->successor);
> to
> 	!bitmap->readonly
> 
> That makes me a little nervous to ACK this patch, because the
> correctness depends on how readonly is updated and manipulated in the
> future, which makes it more prone to error.
> 

I must have been *REALLY* tired when I sent this; I had thought you were
replacing an assertion instead of just adding one.

I'm sorry about that.

>>      hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
>>  }
>>  
>> @@ -443,12 +446,14 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>>                               int64_t cur_sector, int64_t nr_sectors)
>>  {
>>      assert(bdrv_dirty_bitmap_enabled(bitmap));
>> +    assert(!bdrv_dirty_bitmap_readonly(bitmap));
>>      hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
>>  }
>>  
>>  void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
>>  {
>>      assert(bdrv_dirty_bitmap_enabled(bitmap));
>> +    assert(!bdrv_dirty_bitmap_readonly(bitmap));
>>      if (!out) {
>>          hbitmap_reset_all(bitmap->bitmap);
>>      } else {
>> @@ -519,6 +524,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
>>          if (!bdrv_dirty_bitmap_enabled(bitmap)) {
>>              continue;
>>          }
>> +        assert(!bdrv_dirty_bitmap_readonly(bitmap));
>>          hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
>>      }
>>  }
>> @@ -540,3 +546,13 @@ int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
>>  {
>>      return hbitmap_count(bitmap->meta);
>>  }
>> +
>> +bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap)
>> +{
>> +    return bitmap->readonly;
>> +}
>> +
>> +void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap)
>> +{
>> +    bitmap->readonly = true;
>> +}
>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>> index 1e17729ac2..0aab5841f5 100644
>> --- a/include/block/dirty-bitmap.h
>> +++ b/include/block/dirty-bitmap.h
>> @@ -75,4 +75,7 @@ void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap,
>>                                          bool finish);
>>  void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
>>  
>> +bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
>> +void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap);
>> +
>>  #endif
>>

On 01.06.2017 01:58, John Snow wrote:
>
> On 05/19/2017 07:02 PM, John Snow wrote:
>>
>> On 05/03/2017 08:25 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> It will be needed in following commits for persistent bitmaps.
>>> If bitmap is loaded from read-only storage (and we can't mark it
>>> "in use" in this storage) corresponding BdrvDirtyBitmap should be
>>> read-only.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block/dirty-bitmap.c         | 16 ++++++++++++++++
>>>   include/block/dirty-bitmap.h |  3 +++
>>>   2 files changed, 19 insertions(+)
>>>
>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>>> index 90af37287f..ab6a95cf41 100644
>>> --- a/block/dirty-bitmap.c
>>> +++ b/block/dirty-bitmap.c
>>> @@ -44,6 +44,8 @@ struct BdrvDirtyBitmap {
>>>       int64_t size;               /* Size of the bitmap (Number of sectors) */
>>>       bool disabled;              /* Bitmap is read-only */
>>>       int active_iterators;       /* How many iterators are active */
>>> +    bool readonly;              /* Bitmap is read-only and may be changed only
>>> +                                   by deserialize* functions */
>>>       QLIST_ENTRY(BdrvDirtyBitmap) list;
>>>   };
>>>   
>>> @@ -436,6 +438,7 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>>>                              int64_t cur_sector, int64_t nr_sectors)
>>>   {
>>>       assert(bdrv_dirty_bitmap_enabled(bitmap));
>>> +    assert(!bdrv_dirty_bitmap_readonly(bitmap));
>> Doesn't this change the nature of the assertion?
>>
>> We're going from
>>      return !(bitmap->disabled || bitmap->successor);
>> to
>> 	!bitmap->readonly
>>
>> That makes me a little nervous to ACK this patch, because the
>> correctness depends on how readonly is updated and manipulated in the
>> future, which makes it more prone to error.
>>
> I must have been *REALLY* tired when I sent this; I had thought you were
> replacing an assertion instead of just adding one.
>
> I'm sorry about that.

I thought so,  don't worry)

>
>>>       hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
>>>   }
>>>   
>>> @@ -443,12 +446,14 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>>>                                int64_t cur_sector, int64_t nr_sectors)
>>>   {
>>>       assert(bdrv_dirty_bitmap_enabled(bitmap));
>>> +    assert(!bdrv_dirty_bitmap_readonly(bitmap));
>>>       hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
>>>   }
>>>   
>>>   void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
>>>   {
>>>       assert(bdrv_dirty_bitmap_enabled(bitmap));
>>> +    assert(!bdrv_dirty_bitmap_readonly(bitmap));
>>>       if (!out) {
>>>           hbitmap_reset_all(bitmap->bitmap);
>>>       } else {
>>> @@ -519,6 +524,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
>>>           if (!bdrv_dirty_bitmap_enabled(bitmap)) {
>>>               continue;
>>>           }
>>> +        assert(!bdrv_dirty_bitmap_readonly(bitmap));
>>>           hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
>>>       }
>>>   }
>>> @@ -540,3 +546,13 @@ int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
>>>   {
>>>       return hbitmap_count(bitmap->meta);
>>>   }
>>> +
>>> +bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap)
>>> +{
>>> +    return bitmap->readonly;
>>> +}
>>> +
>>> +void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap)
>>> +{
>>> +    bitmap->readonly = true;
>>> +}
>>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>>> index 1e17729ac2..0aab5841f5 100644
>>> --- a/include/block/dirty-bitmap.h
>>> +++ b/include/block/dirty-bitmap.h
>>> @@ -75,4 +75,7 @@ void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap,
>>>                                           bool finish);
>>>   void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
>>>   
>>> +bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
>>> +void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap);
>>> +
>>>   #endif
>>>

-- 
Best regards,
Vladimir.

On 2017-05-03 14:25, Vladimir Sementsov-Ogievskiy wrote:
> It will be needed in following commits for persistent bitmaps.
> If bitmap is loaded from read-only storage (and we can't mark it
> "in use" in this storage) corresponding BdrvDirtyBitmap should be
> read-only.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/dirty-bitmap.c         | 16 ++++++++++++++++
>  include/block/dirty-bitmap.h |  3 +++
>  2 files changed, 19 insertions(+)

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

On 2017-05-03 14:25, Vladimir Sementsov-Ogievskiy wrote:
> It will be needed in following commits for persistent bitmaps.
> If bitmap is loaded from read-only storage (and we can't mark it
> "in use" in this storage) corresponding BdrvDirtyBitmap should be
> read-only.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/dirty-bitmap.c         | 16 ++++++++++++++++
>  include/block/dirty-bitmap.h |  3 +++
>  2 files changed, 19 insertions(+)

Revisiting this again after the whole series: So you never really make
sure that the read-only bitmaps are not written to (except for these
assertions). The idea is that you only set it for read-only BDS and
read-only BDS are never written to. But that assumption is not true,
generally, and can be broken e.g. using a commit job:

$ ./qemu-img create -f qcow2 backing.qcow2 64M
Formatting 'backing.qcow2', fmt=qcow2 size=67108864 encryption=off
    cluster_size=65536 lazy_refcounts=off refcount_bits=16
$ ./qemu-img create -f qcow2 -b backing.qcow2 top.qcow2
Formatting 'top.qcow2', fmt=qcow2 size=67108864
    backing_file=backing.qcow2 encryption=off cluster_size=65536
    lazy_refcounts=off refcount_bits=16
$ x86_64-softmmu/qemu-system-x86_64 -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 9, "major": 2},
"package": " (v2.9.0-632-g4a52d43-dirty)"}, "capabilities": []}}
{'execute': 'qmp_capabilities'}
{"return": {}}
{'execute': 'blockdev-add',
 'arguments': {'node-name': 'backing-node', 'driver': 'qcow2',
               'file': {'driver': 'file', 'filename': 'backing.qcow2'}}}
{"return": {}}
{'execute': 'block-dirty-bitmap-add',
 'arguments': {'node': 'backing-node', 'name': 'foo',
               'persistent': true, 'autoload': true}}
{"return": {}}
{'execute': 'blockdev-del', 'arguments': {'node-name': 'backing-node'}}
{"return": {}}
{'execute': 'blockdev-add',
 'arguments': {'node-name': 'top-node', 'driver': 'qcow2',
               'file': {'driver': 'file', 'filename': 'top.qcow2'}}}
{"return": {}}
{'execute': 'human-monitor-command',
 'arguments': {'command-line': 'qemu-io top-node "write 0 64k"'}}
wrote 65536/65536 bytes at offset 0
64 KiB, 1 ops; 0.0079 sec (7.852 MiB/sec and 125.6281 ops/sec)
{"return": ""}
{'execute': 'block-commit',
 'arguments': {'device': 'top-node', 'job-id': 'commit-job'}}
{"return": {}}
qemu-system-x86_64: block/dirty-bitmap.c:571: bdrv_set_dirty: Assertion
`!bdrv_dirty_bitmap_readonly(bitmap)' failed.
[1]    10872 abort (core dumped)  x86_64-softmmu/qemu-system-x86_64 -qmp
stdio

So there needs to be something else than just assertions to make sure
that read-only bitmaps are never written to.

Max

Thank you for this scenario. Hmm.

So, as I need guarantee that image and bitmap are unchanged, 
bdrv_set_dirty should return error and fail the whole write. Ok?

29.05.2017 21:35, Max Reitz wrote:
> On 2017-05-03 14:25, Vladimir Sementsov-Ogievskiy wrote:
>> It will be needed in following commits for persistent bitmaps.
>> If bitmap is loaded from read-only storage (and we can't mark it
>> "in use" in this storage) corresponding BdrvDirtyBitmap should be
>> read-only.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/dirty-bitmap.c         | 16 ++++++++++++++++
>>   include/block/dirty-bitmap.h |  3 +++
>>   2 files changed, 19 insertions(+)
> Revisiting this again after the whole series: So you never really make
> sure that the read-only bitmaps are not written to (except for these
> assertions). The idea is that you only set it for read-only BDS and
> read-only BDS are never written to. But that assumption is not true,
> generally, and can be broken e.g. using a commit job:
>
> $ ./qemu-img create -f qcow2 backing.qcow2 64M
> Formatting 'backing.qcow2', fmt=qcow2 size=67108864 encryption=off
>      cluster_size=65536 lazy_refcounts=off refcount_bits=16
> $ ./qemu-img create -f qcow2 -b backing.qcow2 top.qcow2
> Formatting 'top.qcow2', fmt=qcow2 size=67108864
>      backing_file=backing.qcow2 encryption=off cluster_size=65536
>      lazy_refcounts=off refcount_bits=16
> $ x86_64-softmmu/qemu-system-x86_64 -qmp stdio
> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 9, "major": 2},
> "package": " (v2.9.0-632-g4a52d43-dirty)"}, "capabilities": []}}
> {'execute': 'qmp_capabilities'}
> {"return": {}}
> {'execute': 'blockdev-add',
>   'arguments': {'node-name': 'backing-node', 'driver': 'qcow2',
>                 'file': {'driver': 'file', 'filename': 'backing.qcow2'}}}
> {"return": {}}
> {'execute': 'block-dirty-bitmap-add',
>   'arguments': {'node': 'backing-node', 'name': 'foo',
>                 'persistent': true, 'autoload': true}}
> {"return": {}}
> {'execute': 'blockdev-del', 'arguments': {'node-name': 'backing-node'}}
> {"return": {}}
> {'execute': 'blockdev-add',
>   'arguments': {'node-name': 'top-node', 'driver': 'qcow2',
>                 'file': {'driver': 'file', 'filename': 'top.qcow2'}}}
> {"return": {}}
> {'execute': 'human-monitor-command',
>   'arguments': {'command-line': 'qemu-io top-node "write 0 64k"'}}
> wrote 65536/65536 bytes at offset 0
> 64 KiB, 1 ops; 0.0079 sec (7.852 MiB/sec and 125.6281 ops/sec)
> {"return": ""}
> {'execute': 'block-commit',
>   'arguments': {'device': 'top-node', 'job-id': 'commit-job'}}
> {"return": {}}
> qemu-system-x86_64: block/dirty-bitmap.c:571: bdrv_set_dirty: Assertion
> `!bdrv_dirty_bitmap_readonly(bitmap)' failed.
> [1]    10872 abort (core dumped)  x86_64-softmmu/qemu-system-x86_64 -qmp
> stdio
>
> So there needs to be something else than just assertions to make sure
> that read-only bitmaps are never written to.
>
> Max
>

-- 
Best regards,
Vladimir

30.05.2017 09:50, Vladimir Sementsov-Ogievskiy wrote:
> Thank you for this scenario. Hmm.
>
> So, as I need guarantee that image and bitmap are unchanged, 
> bdrv_set_dirty should return error and fail the whole write. Ok?

No, bad idea. As bdrv_set_dirty is called after write completed.

>
> 29.05.2017 21:35, Max Reitz wrote:
>> On 2017-05-03 14:25, Vladimir Sementsov-Ogievskiy wrote:
>>> It will be needed in following commits for persistent bitmaps.
>>> If bitmap is loaded from read-only storage (and we can't mark it
>>> "in use" in this storage) corresponding BdrvDirtyBitmap should be
>>> read-only.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block/dirty-bitmap.c         | 16 ++++++++++++++++
>>>   include/block/dirty-bitmap.h |  3 +++
>>>   2 files changed, 19 insertions(+)
>> Revisiting this again after the whole series: So you never really make
>> sure that the read-only bitmaps are not written to (except for these
>> assertions). The idea is that you only set it for read-only BDS and
>> read-only BDS are never written to. But that assumption is not true,
>> generally, and can be broken e.g. using a commit job:
>>
>> $ ./qemu-img create -f qcow2 backing.qcow2 64M
>> Formatting 'backing.qcow2', fmt=qcow2 size=67108864 encryption=off
>>      cluster_size=65536 lazy_refcounts=off refcount_bits=16
>> $ ./qemu-img create -f qcow2 -b backing.qcow2 top.qcow2
>> Formatting 'top.qcow2', fmt=qcow2 size=67108864
>>      backing_file=backing.qcow2 encryption=off cluster_size=65536
>>      lazy_refcounts=off refcount_bits=16
>> $ x86_64-softmmu/qemu-system-x86_64 -qmp stdio
>> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 9, "major": 2},
>> "package": " (v2.9.0-632-g4a52d43-dirty)"}, "capabilities": []}}
>> {'execute': 'qmp_capabilities'}
>> {"return": {}}
>> {'execute': 'blockdev-add',
>>   'arguments': {'node-name': 'backing-node', 'driver': 'qcow2',
>>                 'file': {'driver': 'file', 'filename': 
>> 'backing.qcow2'}}}
>> {"return": {}}
>> {'execute': 'block-dirty-bitmap-add',
>>   'arguments': {'node': 'backing-node', 'name': 'foo',
>>                 'persistent': true, 'autoload': true}}
>> {"return": {}}
>> {'execute': 'blockdev-del', 'arguments': {'node-name': 'backing-node'}}
>> {"return": {}}
>> {'execute': 'blockdev-add',
>>   'arguments': {'node-name': 'top-node', 'driver': 'qcow2',
>>                 'file': {'driver': 'file', 'filename': 'top.qcow2'}}}
>> {"return": {}}
>> {'execute': 'human-monitor-command',
>>   'arguments': {'command-line': 'qemu-io top-node "write 0 64k"'}}
>> wrote 65536/65536 bytes at offset 0
>> 64 KiB, 1 ops; 0.0079 sec (7.852 MiB/sec and 125.6281 ops/sec)
>> {"return": ""}
>> {'execute': 'block-commit',
>>   'arguments': {'device': 'top-node', 'job-id': 'commit-job'}}
>> {"return": {}}
>> qemu-system-x86_64: block/dirty-bitmap.c:571: bdrv_set_dirty: Assertion
>> `!bdrv_dirty_bitmap_readonly(bitmap)' failed.
>> [1]    10872 abort (core dumped) x86_64-softmmu/qemu-system-x86_64 -qmp
>> stdio
>>
>> So there needs to be something else than just assertions to make sure
>> that read-only bitmaps are never written to.
>>
>> Max
>>
>

-- 
Best regards,
Vladimir

30.05.2017 10:31, Vladimir Sementsov-Ogievskiy wrote:
> 30.05.2017 09:50, Vladimir Sementsov-Ogievskiy wrote:
>> Thank you for this scenario. Hmm.
>>
>> So, as I need guarantee that image and bitmap are unchanged, 
>> bdrv_set_dirty should return error and fail the whole write. Ok?
>
> No, bad idea. As bdrv_set_dirty is called after write completed.

will just check for readonly bitmaps in all callers of set/reset/clear. 
They are few.

>
>>
>> 29.05.2017 21:35, Max Reitz wrote:
>>> On 2017-05-03 14:25, Vladimir Sementsov-Ogievskiy wrote:
>>>> It will be needed in following commits for persistent bitmaps.
>>>> If bitmap is loaded from read-only storage (and we can't mark it
>>>> "in use" in this storage) corresponding BdrvDirtyBitmap should be
>>>> read-only.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>   block/dirty-bitmap.c         | 16 ++++++++++++++++
>>>>   include/block/dirty-bitmap.h |  3 +++
>>>>   2 files changed, 19 insertions(+)
>>> Revisiting this again after the whole series: So you never really make
>>> sure that the read-only bitmaps are not written to (except for these
>>> assertions). The idea is that you only set it for read-only BDS and
>>> read-only BDS are never written to. But that assumption is not true,
>>> generally, and can be broken e.g. using a commit job:
>>>
>>> $ ./qemu-img create -f qcow2 backing.qcow2 64M
>>> Formatting 'backing.qcow2', fmt=qcow2 size=67108864 encryption=off
>>>      cluster_size=65536 lazy_refcounts=off refcount_bits=16
>>> $ ./qemu-img create -f qcow2 -b backing.qcow2 top.qcow2
>>> Formatting 'top.qcow2', fmt=qcow2 size=67108864
>>>      backing_file=backing.qcow2 encryption=off cluster_size=65536
>>>      lazy_refcounts=off refcount_bits=16
>>> $ x86_64-softmmu/qemu-system-x86_64 -qmp stdio
>>> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 9, "major": 2},
>>> "package": " (v2.9.0-632-g4a52d43-dirty)"}, "capabilities": []}}
>>> {'execute': 'qmp_capabilities'}
>>> {"return": {}}
>>> {'execute': 'blockdev-add',
>>>   'arguments': {'node-name': 'backing-node', 'driver': 'qcow2',
>>>                 'file': {'driver': 'file', 'filename': 
>>> 'backing.qcow2'}}}
>>> {"return": {}}
>>> {'execute': 'block-dirty-bitmap-add',
>>>   'arguments': {'node': 'backing-node', 'name': 'foo',
>>>                 'persistent': true, 'autoload': true}}
>>> {"return": {}}
>>> {'execute': 'blockdev-del', 'arguments': {'node-name': 'backing-node'}}
>>> {"return": {}}
>>> {'execute': 'blockdev-add',
>>>   'arguments': {'node-name': 'top-node', 'driver': 'qcow2',
>>>                 'file': {'driver': 'file', 'filename': 'top.qcow2'}}}
>>> {"return": {}}
>>> {'execute': 'human-monitor-command',
>>>   'arguments': {'command-line': 'qemu-io top-node "write 0 64k"'}}
>>> wrote 65536/65536 bytes at offset 0
>>> 64 KiB, 1 ops; 0.0079 sec (7.852 MiB/sec and 125.6281 ops/sec)
>>> {"return": ""}
>>> {'execute': 'block-commit',
>>>   'arguments': {'device': 'top-node', 'job-id': 'commit-job'}}
>>> {"return": {}}
>>> qemu-system-x86_64: block/dirty-bitmap.c:571: bdrv_set_dirty: Assertion
>>> `!bdrv_dirty_bitmap_readonly(bitmap)' failed.
>>> [1]    10872 abort (core dumped) x86_64-softmmu/qemu-system-x86_64 -qmp
>>> stdio
>>>
>>> So there needs to be something else than just assertions to make sure
>>> that read-only bitmaps are never written to.
>>>
>>> Max
>>>
>>
>

-- 
Best regards,
Vladimir

On 2017-05-30 08:50, Vladimir Sementsov-Ogievskiy wrote:
> Thank you for this scenario. Hmm.
> 
> So, as I need guarantee that image and bitmap are unchanged,
> bdrv_set_dirty should return error and fail the whole write. Ok?

I don't know. That would mean that you couldn't commit to an image that
has a persistent auto-loading bitmap, which doesn't seem very nice to me.

I'm not quite sure what to do myself. So first I'd definitely want the
commit operation to succeed. That means we'd have to automatically make
the bitmap non-readonly once we write to it. The "readonly" flag would
then be an "unchanged" flag, rather, to signify that the bitmap has not
been changed since it was loaded, which means that it does not need to
be written back to the image file.

Now the issue remains that if you modify a persistent bitmap that is
stored in an image file that is opened RO when it's closed, you won't be
able to write the modifications back.

So in addition, I guess we'd need to "flush" all persistent bitmaps
(that is, write all modifications back to the file and set the
"unchanged" flag (you could also call it "dirty" and then mean the
opposite) for each bitmap) not only when the image is closed or
invalidated, but also when it is reopened read-only.

(block-commit reopens the backing BDS R/W, then writes to them, thus
modifying the dirty bitmaps, and finally reopens the BDS as read-only;
before that happens, we will have to flush the modified bitmap data.)

Max

> 29.05.2017 21:35, Max Reitz wrote:
>> On 2017-05-03 14:25, Vladimir Sementsov-Ogievskiy wrote:
>>> It will be needed in following commits for persistent bitmaps.
>>> If bitmap is loaded from read-only storage (and we can't mark it
>>> "in use" in this storage) corresponding BdrvDirtyBitmap should be
>>> read-only.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block/dirty-bitmap.c         | 16 ++++++++++++++++
>>>   include/block/dirty-bitmap.h |  3 +++
>>>   2 files changed, 19 insertions(+)
>> Revisiting this again after the whole series: So you never really make
>> sure that the read-only bitmaps are not written to (except for these
>> assertions). The idea is that you only set it for read-only BDS and
>> read-only BDS are never written to. But that assumption is not true,
>> generally, and can be broken e.g. using a commit job:
>>
>> $ ./qemu-img create -f qcow2 backing.qcow2 64M
>> Formatting 'backing.qcow2', fmt=qcow2 size=67108864 encryption=off
>>      cluster_size=65536 lazy_refcounts=off refcount_bits=16
>> $ ./qemu-img create -f qcow2 -b backing.qcow2 top.qcow2
>> Formatting 'top.qcow2', fmt=qcow2 size=67108864
>>      backing_file=backing.qcow2 encryption=off cluster_size=65536
>>      lazy_refcounts=off refcount_bits=16
>> $ x86_64-softmmu/qemu-system-x86_64 -qmp stdio
>> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 9, "major": 2},
>> "package": " (v2.9.0-632-g4a52d43-dirty)"}, "capabilities": []}}
>> {'execute': 'qmp_capabilities'}
>> {"return": {}}
>> {'execute': 'blockdev-add',
>>   'arguments': {'node-name': 'backing-node', 'driver': 'qcow2',
>>                 'file': {'driver': 'file', 'filename': 'backing.qcow2'}}}
>> {"return": {}}
>> {'execute': 'block-dirty-bitmap-add',
>>   'arguments': {'node': 'backing-node', 'name': 'foo',
>>                 'persistent': true, 'autoload': true}}
>> {"return": {}}
>> {'execute': 'blockdev-del', 'arguments': {'node-name': 'backing-node'}}
>> {"return": {}}
>> {'execute': 'blockdev-add',
>>   'arguments': {'node-name': 'top-node', 'driver': 'qcow2',
>>                 'file': {'driver': 'file', 'filename': 'top.qcow2'}}}
>> {"return": {}}
>> {'execute': 'human-monitor-command',
>>   'arguments': {'command-line': 'qemu-io top-node "write 0 64k"'}}
>> wrote 65536/65536 bytes at offset 0
>> 64 KiB, 1 ops; 0.0079 sec (7.852 MiB/sec and 125.6281 ops/sec)
>> {"return": ""}
>> {'execute': 'block-commit',
>>   'arguments': {'device': 'top-node', 'job-id': 'commit-job'}}
>> {"return": {}}
>> qemu-system-x86_64: block/dirty-bitmap.c:571: bdrv_set_dirty: Assertion
>> `!bdrv_dirty_bitmap_readonly(bitmap)' failed.
>> [1]    10872 abort (core dumped)  x86_64-softmmu/qemu-system-x86_64 -qmp
>> stdio
>>
>> So there needs to be something else than just assertions to make sure
>> that read-only bitmaps are never written to.
>>
>> Max
>>
> 

31.05.2017 16:43, Max Reitz wrote:
> On 2017-05-30 08:50, Vladimir Sementsov-Ogievskiy wrote:
>> Thank you for this scenario. Hmm.
>>
>> So, as I need guarantee that image and bitmap are unchanged,
>> bdrv_set_dirty should return error and fail the whole write. Ok?
> I don't know. That would mean that you couldn't commit to an image that
> has a persistent auto-loading bitmap, which doesn't seem very nice to me.
>
> I'm not quite sure what to do myself. So first I'd definitely want the
> commit operation to succeed. That means we'd have to automatically make
> the bitmap non-readonly once we write to it. The "readonly" flag would
> then be an "unchanged" flag, rather, to signify that the bitmap has not
> been changed since it was loaded, which means that it does not need to
> be written back to the image file.
>
> Now the issue remains that if you modify a persistent bitmap that is
> stored in an image file that is opened RO when it's closed, you won't be
> able to write the modifications back.
>
> So in addition, I guess we'd need to "flush" all persistent bitmaps
> (that is, write all modifications back to the file and set the
> "unchanged" flag (you could also call it "dirty" and then mean the
> opposite) for each bitmap) not only when the image is closed or
> invalidated, but also when it is reopened read-only.
>
> (block-commit reopens the backing BDS R/W, then writes to them, thus
> modifying the dirty bitmaps, and finally reopens the BDS as read-only;
> before that happens, we will have to flush the modified bitmap data.)

Ok, understand.

We need to consider also setting in_use flag in the image. We _must not_ 
write to image with dirty bitmap,
if in_use flag of this dirty bitmap is not set, as in case of something 
fail we will have image with wrong bitmap with
unset in_use flag (which looks ok).

I see two ways to handle it:

variant 1:
1. readonly field stays as is (see v19, with normal errors, not only 
asserts)
2. immediately after reopening r/w we do "reopening bitmaps r/w", i.e. 
set in_use in the image and set BdrvDirtyBitmap.readonly = false
3. in reopen_prepare, if reopening r-o do "reopening bitmaps r-o", i.e. 
save them into the image and set BdrvDirtyBitmap.readonly = true

variant 2:
1. instead of 'readonly' add 'dirty' field, set dirty to 0 for all 
bitmaps on create
2. before write/discard check this field in all related bitmaps, and if 
dirty=0 (and persistent=1), write IN_USE flag into the image first, set 
dirty=1, and only then do write. (if writing IN_USE=1 failed, fail the 
whole write)
3. in reopen_prepare, if reopening r-o do "reopening bitmaps r-o", i.e. 
save them into the image and set BdrvDirtyBitmap.dirty = 0


>
> Max
>
>> 29.05.2017 21:35, Max Reitz wrote:
>>> On 2017-05-03 14:25, Vladimir Sementsov-Ogievskiy wrote:
>>>> It will be needed in following commits for persistent bitmaps.
>>>> If bitmap is loaded from read-only storage (and we can't mark it
>>>> "in use" in this storage) corresponding BdrvDirtyBitmap should be
>>>> read-only.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>    block/dirty-bitmap.c         | 16 ++++++++++++++++
>>>>    include/block/dirty-bitmap.h |  3 +++
>>>>    2 files changed, 19 insertions(+)
>>> Revisiting this again after the whole series: So you never really make
>>> sure that the read-only bitmaps are not written to (except for these
>>> assertions). The idea is that you only set it for read-only BDS and
>>> read-only BDS are never written to. But that assumption is not true,
>>> generally, and can be broken e.g. using a commit job:
>>>
>>> $ ./qemu-img create -f qcow2 backing.qcow2 64M
>>> Formatting 'backing.qcow2', fmt=qcow2 size=67108864 encryption=off
>>>       cluster_size=65536 lazy_refcounts=off refcount_bits=16
>>> $ ./qemu-img create -f qcow2 -b backing.qcow2 top.qcow2
>>> Formatting 'top.qcow2', fmt=qcow2 size=67108864
>>>       backing_file=backing.qcow2 encryption=off cluster_size=65536
>>>       lazy_refcounts=off refcount_bits=16
>>> $ x86_64-softmmu/qemu-system-x86_64 -qmp stdio
>>> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 9, "major": 2},
>>> "package": " (v2.9.0-632-g4a52d43-dirty)"}, "capabilities": []}}
>>> {'execute': 'qmp_capabilities'}
>>> {"return": {}}
>>> {'execute': 'blockdev-add',
>>>    'arguments': {'node-name': 'backing-node', 'driver': 'qcow2',
>>>                  'file': {'driver': 'file', 'filename': 'backing.qcow2'}}}
>>> {"return": {}}
>>> {'execute': 'block-dirty-bitmap-add',
>>>    'arguments': {'node': 'backing-node', 'name': 'foo',
>>>                  'persistent': true, 'autoload': true}}
>>> {"return": {}}
>>> {'execute': 'blockdev-del', 'arguments': {'node-name': 'backing-node'}}
>>> {"return": {}}
>>> {'execute': 'blockdev-add',
>>>    'arguments': {'node-name': 'top-node', 'driver': 'qcow2',
>>>                  'file': {'driver': 'file', 'filename': 'top.qcow2'}}}
>>> {"return": {}}
>>> {'execute': 'human-monitor-command',
>>>    'arguments': {'command-line': 'qemu-io top-node "write 0 64k"'}}
>>> wrote 65536/65536 bytes at offset 0
>>> 64 KiB, 1 ops; 0.0079 sec (7.852 MiB/sec and 125.6281 ops/sec)
>>> {"return": ""}
>>> {'execute': 'block-commit',
>>>    'arguments': {'device': 'top-node', 'job-id': 'commit-job'}}
>>> {"return": {}}
>>> qemu-system-x86_64: block/dirty-bitmap.c:571: bdrv_set_dirty: Assertion
>>> `!bdrv_dirty_bitmap_readonly(bitmap)' failed.
>>> [1]    10872 abort (core dumped)  x86_64-softmmu/qemu-system-x86_64 -qmp
>>> stdio
>>>
>>> So there needs to be something else than just assertions to make sure
>>> that read-only bitmaps are never written to.
>>>
>>> Max
>>>
>

-- 
Best regards,
Vladimir

On 2017-05-31 16:29, Vladimir Sementsov-Ogievskiy wrote:
> 31.05.2017 16:43, Max Reitz wrote:
>> On 2017-05-30 08:50, Vladimir Sementsov-Ogievskiy wrote:
>>> Thank you for this scenario. Hmm.
>>>
>>> So, as I need guarantee that image and bitmap are unchanged,
>>> bdrv_set_dirty should return error and fail the whole write. Ok?
>> I don't know. That would mean that you couldn't commit to an image that
>> has a persistent auto-loading bitmap, which doesn't seem very nice to me.
>>
>> I'm not quite sure what to do myself. So first I'd definitely want the
>> commit operation to succeed. That means we'd have to automatically make
>> the bitmap non-readonly once we write to it. The "readonly" flag would
>> then be an "unchanged" flag, rather, to signify that the bitmap has not
>> been changed since it was loaded, which means that it does not need to
>> be written back to the image file.
>>
>> Now the issue remains that if you modify a persistent bitmap that is
>> stored in an image file that is opened RO when it's closed, you won't be
>> able to write the modifications back.
>>
>> So in addition, I guess we'd need to "flush" all persistent bitmaps
>> (that is, write all modifications back to the file and set the
>> "unchanged" flag (you could also call it "dirty" and then mean the
>> opposite) for each bitmap) not only when the image is closed or
>> invalidated, but also when it is reopened read-only.
>>
>> (block-commit reopens the backing BDS R/W, then writes to them, thus
>> modifying the dirty bitmaps, and finally reopens the BDS as read-only;
>> before that happens, we will have to flush the modified bitmap data.)
> 
> Ok, understand.
> 
> We need to consider also setting in_use flag in the image. We _must not_
> write to image with dirty bitmap,
> if in_use flag of this dirty bitmap is not set, as in case of something
> fail we will have image with wrong bitmap with
> unset in_use flag (which looks ok).

Right.

> I see two ways to handle it:
> 
> variant 1:
> 1. readonly field stays as is (see v19, with normal errors, not only
> asserts)
> 2. immediately after reopening r/w we do "reopening bitmaps r/w", i.e.
> set in_use in the image and set BdrvDirtyBitmap.readonly = false
> 3. in reopen_prepare, if reopening r-o do "reopening bitmaps r-o", i.e.
> save them into the image and set BdrvDirtyBitmap.readonly = true

Sounds good, yes.

> variant 2:
> 1. instead of 'readonly' add 'dirty' field, set dirty to 0 for all
> bitmaps on create
> 2. before write/discard check this field in all related bitmaps, and if
> dirty=0 (and persistent=1), write IN_USE flag into the image first, set
> dirty=1, and only then do write. (if writing IN_USE=1 failed, fail the
> whole write)
> 3. in reopen_prepare, if reopening r-o do "reopening bitmaps r-o", i.e.
> save them into the image and set BdrvDirtyBitmap.dirty = 0

Works, too.

I think the second variant would the more "efficient" way (because you
only have to flush out dirty dirty bitmaps), but the first one would be
simpler and has the great advantage of not requiring a write to the
image file when you just want to set a bit in the in-memory dirty
bitmap. So I'd personally go for the first variant.

Max

31.05.2017 17:44, Max Reitz wrote:
> On 2017-05-31 16:29, Vladimir Sementsov-Ogievskiy wrote:
>> 31.05.2017 16:43, Max Reitz wrote:
>>> On 2017-05-30 08:50, Vladimir Sementsov-Ogievskiy wrote:
>>>> Thank you for this scenario. Hmm.
>>>>
>>>> So, as I need guarantee that image and bitmap are unchanged,
>>>> bdrv_set_dirty should return error and fail the whole write. Ok?
>>> I don't know. That would mean that you couldn't commit to an image that
>>> has a persistent auto-loading bitmap, which doesn't seem very nice to me.
>>>
>>> I'm not quite sure what to do myself. So first I'd definitely want the
>>> commit operation to succeed. That means we'd have to automatically make
>>> the bitmap non-readonly once we write to it. The "readonly" flag would
>>> then be an "unchanged" flag, rather, to signify that the bitmap has not
>>> been changed since it was loaded, which means that it does not need to
>>> be written back to the image file.
>>>
>>> Now the issue remains that if you modify a persistent bitmap that is
>>> stored in an image file that is opened RO when it's closed, you won't be
>>> able to write the modifications back.
>>>
>>> So in addition, I guess we'd need to "flush" all persistent bitmaps
>>> (that is, write all modifications back to the file and set the
>>> "unchanged" flag (you could also call it "dirty" and then mean the
>>> opposite) for each bitmap) not only when the image is closed or
>>> invalidated, but also when it is reopened read-only.
>>>
>>> (block-commit reopens the backing BDS R/W, then writes to them, thus
>>> modifying the dirty bitmaps, and finally reopens the BDS as read-only;
>>> before that happens, we will have to flush the modified bitmap data.)
>> Ok, understand.
>>
>> We need to consider also setting in_use flag in the image. We _must not_
>> write to image with dirty bitmap,
>> if in_use flag of this dirty bitmap is not set, as in case of something
>> fail we will have image with wrong bitmap with
>> unset in_use flag (which looks ok).
> Right.
>
>> I see two ways to handle it:
>>
>> variant 1:
>> 1. readonly field stays as is (see v19, with normal errors, not only
>> asserts)
>> 2. immediately after reopening r/w we do "reopening bitmaps r/w", i.e.
>> set in_use in the image and set BdrvDirtyBitmap.readonly = false
>> 3. in reopen_prepare, if reopening r-o do "reopening bitmaps r-o", i.e.
>> save them into the image and set BdrvDirtyBitmap.readonly = true
> Sounds good, yes.
>
>> variant 2:
>> 1. instead of 'readonly' add 'dirty' field, set dirty to 0 for all
>> bitmaps on create
>> 2. before write/discard check this field in all related bitmaps, and if
>> dirty=0 (and persistent=1), write IN_USE flag into the image first, set
>> dirty=1, and only then do write. (if writing IN_USE=1 failed, fail the
>> whole write)
>> 3. in reopen_prepare, if reopening r-o do "reopening bitmaps r-o", i.e.
>> save them into the image and set BdrvDirtyBitmap.dirty = 0
> Works, too.
>
> I think the second variant would the more "efficient" way (because you
> only have to flush out dirty dirty bitmaps), but the first one would be
> simpler and has the great advantage of not requiring a write to the
> image file when you just want to set a bit in the in-memory dirty
> bitmap. So I'd personally go for the first variant.

Hmm, why not requiring? Both 1 and 2 do write in_use=1, but (1) do this 
on open/reopen, and (2) before the first write to the image.

"set a bit in the in-memory..." - are you saying about not-persistent 
dirty bitmaps? In this case, of course, nothing should be written into 
the image, just set dirty=1.


>
> Max
>

-- 
Best regards,
Vladimir

On 2017-05-31 17:05, Vladimir Sementsov-Ogievskiy wrote:
> 31.05.2017 17:44, Max Reitz wrote:
>> On 2017-05-31 16:29, Vladimir Sementsov-Ogievskiy wrote:
>>> 31.05.2017 16:43, Max Reitz wrote:
>>>> On 2017-05-30 08:50, Vladimir Sementsov-Ogievskiy wrote:
>>>>> Thank you for this scenario. Hmm.
>>>>>
>>>>> So, as I need guarantee that image and bitmap are unchanged,
>>>>> bdrv_set_dirty should return error and fail the whole write. Ok?
>>>> I don't know. That would mean that you couldn't commit to an image that
>>>> has a persistent auto-loading bitmap, which doesn't seem very nice
>>>> to me.
>>>>
>>>> I'm not quite sure what to do myself. So first I'd definitely want the
>>>> commit operation to succeed. That means we'd have to automatically make
>>>> the bitmap non-readonly once we write to it. The "readonly" flag would
>>>> then be an "unchanged" flag, rather, to signify that the bitmap has not
>>>> been changed since it was loaded, which means that it does not need to
>>>> be written back to the image file.
>>>>
>>>> Now the issue remains that if you modify a persistent bitmap that is
>>>> stored in an image file that is opened RO when it's closed, you
>>>> won't be
>>>> able to write the modifications back.
>>>>
>>>> So in addition, I guess we'd need to "flush" all persistent bitmaps
>>>> (that is, write all modifications back to the file and set the
>>>> "unchanged" flag (you could also call it "dirty" and then mean the
>>>> opposite) for each bitmap) not only when the image is closed or
>>>> invalidated, but also when it is reopened read-only.
>>>>
>>>> (block-commit reopens the backing BDS R/W, then writes to them, thus
>>>> modifying the dirty bitmaps, and finally reopens the BDS as read-only;
>>>> before that happens, we will have to flush the modified bitmap data.)
>>> Ok, understand.
>>>
>>> We need to consider also setting in_use flag in the image. We _must not_
>>> write to image with dirty bitmap,
>>> if in_use flag of this dirty bitmap is not set, as in case of something
>>> fail we will have image with wrong bitmap with
>>> unset in_use flag (which looks ok).
>> Right.
>>
>>> I see two ways to handle it:
>>>
>>> variant 1:
>>> 1. readonly field stays as is (see v19, with normal errors, not only
>>> asserts)
>>> 2. immediately after reopening r/w we do "reopening bitmaps r/w", i.e.
>>> set in_use in the image and set BdrvDirtyBitmap.readonly = false
>>> 3. in reopen_prepare, if reopening r-o do "reopening bitmaps r-o", i.e.
>>> save them into the image and set BdrvDirtyBitmap.readonly = true
>> Sounds good, yes.
>>
>>> variant 2:
>>> 1. instead of 'readonly' add 'dirty' field, set dirty to 0 for all
>>> bitmaps on create
>>> 2. before write/discard check this field in all related bitmaps, and if
>>> dirty=0 (and persistent=1), write IN_USE flag into the image first, set
>>> dirty=1, and only then do write. (if writing IN_USE=1 failed, fail the
>>> whole write)
>>> 3. in reopen_prepare, if reopening r-o do "reopening bitmaps r-o", i.e.
>>> save them into the image and set BdrvDirtyBitmap.dirty = 0
>> Works, too.
>>
>> I think the second variant would the more "efficient" way (because you
>> only have to flush out dirty dirty bitmaps), but the first one would be
>> simpler and has the great advantage of not requiring a write to the
>> image file when you just want to set a bit in the in-memory dirty
>> bitmap. So I'd personally go for the first variant.
> 
> Hmm, why not requiring? Both 1 and 2 do write in_use=1, but (1) do this
> on open/reopen, and (2) before the first write to the image.

Oh, I didn't read the "before write/discard". Yes, if you check it
before writing, then you won't have to set the flag through
bdrv_set_dirty().

> "set a bit in the in-memory..." - are you saying about not-persistent
> dirty bitmaps? In this case, of course, nothing should be written into
> the image, just set dirty=1.

No, I did mean persistent bitmaps, but bdrv_set_dirty() just sets the
bit in main memory, of course. It only gets written to the image later
(on reopen/close/invalidate).

Well, your choice. I think both will work. :-)

Max

On 05/31/2017 11:53 AM, Max Reitz wrote:
> On 2017-05-31 17:05, Vladimir Sementsov-Ogievskiy wrote:
>> 31.05.2017 17:44, Max Reitz wrote:
>>> On 2017-05-31 16:29, Vladimir Sementsov-Ogievskiy wrote:
>>>> 31.05.2017 16:43, Max Reitz wrote:
>>>>> On 2017-05-30 08:50, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> Thank you for this scenario. Hmm.
>>>>>>
>>>>>> So, as I need guarantee that image and bitmap are unchanged,
>>>>>> bdrv_set_dirty should return error and fail the whole write. Ok?
>>>>> I don't know. That would mean that you couldn't commit to an image that
>>>>> has a persistent auto-loading bitmap, which doesn't seem very nice
>>>>> to me.
>>>>>
>>>>> I'm not quite sure what to do myself. So first I'd definitely want the
>>>>> commit operation to succeed. That means we'd have to automatically make
>>>>> the bitmap non-readonly once we write to it. The "readonly" flag would
>>>>> then be an "unchanged" flag, rather, to signify that the bitmap has not
>>>>> been changed since it was loaded, which means that it does not need to
>>>>> be written back to the image file.
>>>>>
>>>>> Now the issue remains that if you modify a persistent bitmap that is
>>>>> stored in an image file that is opened RO when it's closed, you
>>>>> won't be
>>>>> able to write the modifications back.
>>>>>
>>>>> So in addition, I guess we'd need to "flush" all persistent bitmaps
>>>>> (that is, write all modifications back to the file and set the
>>>>> "unchanged" flag (you could also call it "dirty" and then mean the
>>>>> opposite) for each bitmap) not only when the image is closed or
>>>>> invalidated, but also when it is reopened read-only.
>>>>>
>>>>> (block-commit reopens the backing BDS R/W, then writes to them, thus
>>>>> modifying the dirty bitmaps, and finally reopens the BDS as read-only;
>>>>> before that happens, we will have to flush the modified bitmap data.)
>>>> Ok, understand.
>>>>
>>>> We need to consider also setting in_use flag in the image. We _must not_
>>>> write to image with dirty bitmap,
>>>> if in_use flag of this dirty bitmap is not set, as in case of something
>>>> fail we will have image with wrong bitmap with
>>>> unset in_use flag (which looks ok).
>>> Right.
>>>
>>>> I see two ways to handle it:
>>>>
>>>> variant 1:
>>>> 1. readonly field stays as is (see v19, with normal errors, not only
>>>> asserts)
>>>> 2. immediately after reopening r/w we do "reopening bitmaps r/w", i.e.
>>>> set in_use in the image and set BdrvDirtyBitmap.readonly = false
>>>> 3. in reopen_prepare, if reopening r-o do "reopening bitmaps r-o", i.e.
>>>> save them into the image and set BdrvDirtyBitmap.readonly = true
>>> Sounds good, yes.
>>>
>>>> variant 2:
>>>> 1. instead of 'readonly' add 'dirty' field, set dirty to 0 for all
>>>> bitmaps on create
>>>> 2. before write/discard check this field in all related bitmaps, and if
>>>> dirty=0 (and persistent=1), write IN_USE flag into the image first, set
>>>> dirty=1, and only then do write. (if writing IN_USE=1 failed, fail the
>>>> whole write)
>>>> 3. in reopen_prepare, if reopening r-o do "reopening bitmaps r-o", i.e.
>>>> save them into the image and set BdrvDirtyBitmap.dirty = 0
>>> Works, too.
>>>
>>> I think the second variant would the more "efficient" way (because you
>>> only have to flush out dirty dirty bitmaps), but the first one would be
>>> simpler and has the great advantage of not requiring a write to the
>>> image file when you just want to set a bit in the in-memory dirty
>>> bitmap. So I'd personally go for the first variant.
>>
>> Hmm, why not requiring? Both 1 and 2 do write in_use=1, but (1) do this
>> on open/reopen, and (2) before the first write to the image.
> 
> Oh, I didn't read the "before write/discard". Yes, if you check it
> before writing, then you won't have to set the flag through
> bdrv_set_dirty().
> 
>> "set a bit in the in-memory..." - are you saying about not-persistent
>> dirty bitmaps? In this case, of course, nothing should be written into
>> the image, just set dirty=1.
> 
> No, I did mean persistent bitmaps, but bdrv_set_dirty() just sets the
> bit in main memory, of course. It only gets written to the image later
> (on reopen/close/invalidate).
> 

There may be some benefit to setting in_use immediately as soon as we
admit that we are willing to tolerate writes to the bitmap. It's a
performance hit, but it may help on-disk consistency.

Or maybe that's a fool's errand? This is a design question we've largely
ignored so far, but it's something that will need investigating sooner
or later.

> Well, your choice. I think both will work. :-)
> 
> Max
> 

On 05/31/2017 09:43 AM, Max Reitz wrote:
> On 2017-05-30 08:50, Vladimir Sementsov-Ogievskiy wrote:
>> Thank you for this scenario. Hmm.
>>
>> So, as I need guarantee that image and bitmap are unchanged,
>> bdrv_set_dirty should return error and fail the whole write. Ok?
> 
> I don't know. That would mean that you couldn't commit to an image that
> has a persistent auto-loading bitmap, which doesn't seem very nice to me.
> 
> I'm not quite sure what to do myself. So first I'd definitely want the
> commit operation to succeed. That means we'd have to automatically make
> the bitmap non-readonly once we write to it. The "readonly" flag would
> then be an "unchanged" flag, rather, to signify that the bitmap has not
> been changed since it was loaded, which means that it does not need to
> be written back to the image file.
> 
> Now the issue remains that if you modify a persistent bitmap that is
> stored in an image file that is opened RO when it's closed, you won't be
> able to write the modifications back.
> > So in addition, I guess we'd need to "flush" all persistent bitmaps
> (that is, write all modifications back to the file and set the
> "unchanged" flag (you could also call it "dirty" and then mean the
> opposite) for each bitmap) not only when the image is closed or
> invalidated, but also when it is reopened read-only.
>

Makes sense.

> (block-commit reopens the backing BDS R/W, then writes to them, thus
> modifying the dirty bitmaps, and finally reopens the BDS as read-only;
> before that happens, we will have to flush the modified bitmap data.)
> 

OK, so it would perhaps be enough to toggle the RO flag on/off when
nodes get reopened. When they get reopened RO, we'd need to flush at
that point.

(Right?)

Of course, a changed flag makes this a little moot as it is probably
more flexible; but there is something slightly attractive about the more
rigid form.

(Hmm, for the purposes of periodic flushing, we may want a changed flag
anyway...)

> Max
> 
Thanks for the scenario and the explainer.

On 2017-06-02 00:29, John Snow wrote:
> 
> 
> On 05/31/2017 09:43 AM, Max Reitz wrote:
>> On 2017-05-30 08:50, Vladimir Sementsov-Ogievskiy wrote:
>>> Thank you for this scenario. Hmm.
>>>
>>> So, as I need guarantee that image and bitmap are unchanged,
>>> bdrv_set_dirty should return error and fail the whole write. Ok?
>>
>> I don't know. That would mean that you couldn't commit to an image that
>> has a persistent auto-loading bitmap, which doesn't seem very nice to me.
>>
>> I'm not quite sure what to do myself. So first I'd definitely want the
>> commit operation to succeed. That means we'd have to automatically make
>> the bitmap non-readonly once we write to it. The "readonly" flag would
>> then be an "unchanged" flag, rather, to signify that the bitmap has not
>> been changed since it was loaded, which means that it does not need to
>> be written back to the image file.
>>
>> Now the issue remains that if you modify a persistent bitmap that is
>> stored in an image file that is opened RO when it's closed, you won't be
>> able to write the modifications back.
>>> So in addition, I guess we'd need to "flush" all persistent bitmaps
>> (that is, write all modifications back to the file and set the
>> "unchanged" flag (you could also call it "dirty" and then mean the
>> opposite) for each bitmap) not only when the image is closed or
>> invalidated, but also when it is reopened read-only.
>>
> 
> Makes sense.
> 
>> (block-commit reopens the backing BDS R/W, then writes to them, thus
>> modifying the dirty bitmaps, and finally reopens the BDS as read-only;
>> before that happens, we will have to flush the modified bitmap data.)
>>
> 
> OK, so it would perhaps be enough to toggle the RO flag on/off when
> nodes get reopened. When they get reopened RO, we'd need to flush at
> that point.
> 
> (Right?)

Right.

> Of course, a changed flag makes this a little moot as it is probably
> more flexible; but there is something slightly attractive about the more
> rigid form.
> 
> (Hmm, for the purposes of periodic flushing, we may want a changed flag
> anyway...)

I don't mind either way. I like a dirty flag because this is a common
concept (we basically cache the persistent bitmaps in RAM, so it's
natural for them to have a dirty/clean state); but I also like keeping
the read-only flag because it's probably simpler to implement (and makes
just as much sense).

Max