On 11.10.2016 15:11, Vladimir Sementsov-Ogievskiy wrote:
> On 07.10.2016 20:54, Max Reitz wrote:
>> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
>>> New field BdrvDirtyBitmap.persistent means, that bitmap should be saved
>>> on bdrv_close, using format driver. Format driver should maintain bitmap
>>> storing.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block.c                      | 30 ++++++++++++++++++++++++++++++
>>>   block/dirty-bitmap.c         | 27 +++++++++++++++++++++++++++
>>>   block/qcow2-bitmap.c         |  1 +
>>>   include/block/block.h        |  2 ++
>>>   include/block/block_int.h    |  2 ++
>>>   include/block/dirty-bitmap.h |  6 ++++++
>>>   6 files changed, 68 insertions(+)
>>>
>>> diff --git a/block.c b/block.c
>>> index 804e3d4..1cde03a 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -2196,6 +2196,7 @@ void bdrv_reopen_abort(BDRVReopenState
>>> *reopen_state)
>>>   static void bdrv_close(BlockDriverState *bs)
>>>   {
>>>       BdrvAioNotifier *ban, *ban_next;
>>> +    Error *local_err = NULL;
>>>         assert(!bs->job);
>>>       assert(!bs->refcnt);
>>> @@ -2204,6 +2205,10 @@ static void bdrv_close(BlockDriverState *bs)
>>>       bdrv_flush(bs);
>>>       bdrv_drain(bs); /* in case flush left pending I/O */
>>>   +    bdrv_store_persistent_bitmaps(bs, &local_err);
>>> +    if (local_err != NULL) {
>>> +        error_report_err(local_err);
>>> +    }
>> That seems pretty wrong to me. If the persistent bitmaps cannot be
>> stored, the node should not be closed to avoid loss of data.
>>
>>>       bdrv_release_named_dirty_bitmaps(bs);
>> Especially since the next function will just drop all the dirty bitmaps.
>>
>> I see the issue that bdrv_close() is only called by bdrv_delete() which
>> in turn is only called by bdrv_unref(); and how are you supposed to
>> react to bdrv_unref() failing?
>>
>> So I'm not sure how this issue should be addressed, but this is most
>> certainly not ideal. You should not just drop supposedly persistent
>> dirty bitmaps if they cannot be saved.
>>
>> We really should to have some way to keep the bitmap around if it cannot
>> be saved, but I don't know how to do that either.
>>
>> In any case, we should make sure that the node supports saving
>> persistent dirty bitmaps, because having persistent dirty bitmaps at a
>> node that does not support them is something we can and must prevent
>> beforehand.
>>
>> But I don't know how to handle failure if writing the dirty bitmap
>> fails. I guess one could argue that it's the same as bdrv_flush()
>> failing and thus can be handled in the same way, i.e. ignore it. I'm not
>> happy with that, but I'd accept it if there's no other way.
> 
> For now, the only usage of these bitmaps is incremental backup and
> bitmaps are not critical data. If we lost them we will just do full
> backup. If there will be some critical persistent bdrv dirty bitmaps in
> future, we can introduce a callback BdrvDirtyBitmap.store_failed for
> them, which will somehow handle that case.. Detach bitmap from bs and
> save it in memory, add qmp commands to raw-dump them, etc.. I

Yes, fine with me. Still, we should make an effort to detect the case
that some block driver will not be able to store a certain persistent
bitmap attached to one of its nodes as early as possible, ideally
already when the bitmap is created.

Max