[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 09/22] block: introduce persistent dirty bitmaps
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH 09/22] block: introduce persistent dirty bitmaps |
Date: |
Wed, 12 Oct 2016 20:24:06 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 |
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 <address@hidden>
>>> ---
>>> 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
signature.asc
Description: OpenPGP digital signature