| Message ID | 1475232808-4852-10-git-send-email-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
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. > assert(QLIST_EMPTY(&bs->dirty_bitmaps)); > > @@ -3969,3 +3974,28 @@ void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp) > > parent_bs->drv->bdrv_del_child(parent_bs, child, errp); > } > + > +void bdrv_store_persistent_bitmaps(BlockDriverState *bs, Error **errp) > +{ > + BlockDriver *drv = bs->drv; > + > + if (!bdrv_has_persistent_bitmaps(bs)) { > + return; > + } > + > + if (!drv) { > + error_setg_errno(errp, ENOMEDIUM, > + "Can't store persistent bitmaps to %s", > + bdrv_get_device_or_node_name(bs)); > + return; > + } > + > + if (!drv->bdrv_store_persistent_bitmaps) { > + error_setg_errno(errp, ENOTSUP, > + "Can't store persistent bitmaps to %s", > + bdrv_get_device_or_node_name(bs)); > + return; > + } > + > + drv->bdrv_store_persistent_bitmaps(bs, errp); > +} > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index 623e1d1..0314581 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -44,6 +44,7 @@ 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 persistent; /* bitmap must be saved to owner disk image */ > bool autoload; /* bitmap must be autoloaded on image opening */ > QLIST_ENTRY(BdrvDirtyBitmap) list; > }; > @@ -72,6 +73,7 @@ void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap) > g_free(bitmap->name); > bitmap->name = NULL; > > + bitmap->persistent = false; > bitmap->autoload = false; > } > > @@ -241,6 +243,8 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, > bitmap->name = NULL; > successor->name = name; > bitmap->successor = NULL; > + successor->persistent = bitmap->persistent; > + bitmap->persistent = false; > successor->autoload = bitmap->autoload; > bitmap->autoload = false; > bdrv_release_dirty_bitmap(bs, bitmap); > @@ -555,3 +559,26 @@ bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap) > { > return bitmap->autoload; > } > + > +void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, > + bool persistent) The second parameter should be aligned to the opening parenthesis. Max > +{ > + bitmap->persistent = persistent; > +} > + > +bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap) > +{ > + return bitmap->persistent; > +} > + > +bool bdrv_has_persistent_bitmaps(BlockDriverState *bs) > +{ > + BdrvDirtyBitmap *bm; > + QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) { > + if (bm->persistent) { > + return true; > + } > + } > + > + return false; > +}
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/dirty-bitmap.c b/block/dirty-bitmap.c > index 623e1d1..0314581 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c [...] > @@ -555,3 +559,26 @@ bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap) > { > return bitmap->autoload; > } > + > +void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, > + bool persistent) > +{ > + bitmap->persistent = persistent; After some thinking, I think this function should be more complex: It should check whether the node the bitmap is attached to actually can handle persistent bitmaps and whether it would actually support storing *this* bitmap. For instance, a qcow2 node would not support writing overly large bitmaps (limited by BME_MAX_TABLE_SIZE and BME_MAX_PHYS_SIZE) or bitmaps with overly large granularities (BME_MAX_GRANULARITY_BITS) or bitmaps whose name is already occupied by some bitmap that is already stored in the file but has not been loaded. Checking this here will trivially prevent users from creating such bitmaps and will also preempt detection of such failures during bdrv_close() when they cannot be handled gracefully. Max > +}
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 > >> assert(QLIST_EMPTY(&bs->dirty_bitmaps)); >> >> @@ -3969,3 +3974,28 @@ void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp) >> >> parent_bs->drv->bdrv_del_child(parent_bs, child, errp); >> } >> + >> +void bdrv_store_persistent_bitmaps(BlockDriverState *bs, Error **errp) >> +{ >> + BlockDriver *drv = bs->drv; >> + >> + if (!bdrv_has_persistent_bitmaps(bs)) { >> + return; >> + } >> + >> + if (!drv) { >> + error_setg_errno(errp, ENOMEDIUM, >> + "Can't store persistent bitmaps to %s", >> + bdrv_get_device_or_node_name(bs)); >> + return; >> + } >> + >> + if (!drv->bdrv_store_persistent_bitmaps) { >> + error_setg_errno(errp, ENOTSUP, >> + "Can't store persistent bitmaps to %s", >> + bdrv_get_device_or_node_name(bs)); >> + return; >> + } >> + >> + drv->bdrv_store_persistent_bitmaps(bs, errp); >> +} >> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >> index 623e1d1..0314581 100644 >> --- a/block/dirty-bitmap.c >> +++ b/block/dirty-bitmap.c >> @@ -44,6 +44,7 @@ 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 persistent; /* bitmap must be saved to owner disk image */ >> bool autoload; /* bitmap must be autoloaded on image opening */ >> QLIST_ENTRY(BdrvDirtyBitmap) list; >> }; >> @@ -72,6 +73,7 @@ void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap) >> g_free(bitmap->name); >> bitmap->name = NULL; >> >> + bitmap->persistent = false; >> bitmap->autoload = false; >> } >> >> @@ -241,6 +243,8 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, >> bitmap->name = NULL; >> successor->name = name; >> bitmap->successor = NULL; >> + successor->persistent = bitmap->persistent; >> + bitmap->persistent = false; >> successor->autoload = bitmap->autoload; >> bitmap->autoload = false; >> bdrv_release_dirty_bitmap(bs, bitmap); >> @@ -555,3 +559,26 @@ bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap) >> { >> return bitmap->autoload; >> } >> + >> +void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, >> + bool persistent) > The second parameter should be aligned to the opening parenthesis. > > Max > >> +{ >> + bitmap->persistent = persistent; >> +} >> + >> +bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap) >> +{ >> + return bitmap->persistent; >> +} >> + >> +bool bdrv_has_persistent_bitmaps(BlockDriverState *bs) >> +{ >> + BdrvDirtyBitmap *bm; >> + QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) { >> + if (bm->persistent) { >> + return true; >> + } >> + } >> + >> + return false; >> +}
On 07.10.2016 22:28, 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/dirty-bitmap.c b/block/dirty-bitmap.c >> index 623e1d1..0314581 100644 >> --- a/block/dirty-bitmap.c >> +++ b/block/dirty-bitmap.c > [...] > >> @@ -555,3 +559,26 @@ bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap) >> { >> return bitmap->autoload; >> } >> + >> +void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, >> + bool persistent) >> +{ >> + bitmap->persistent = persistent; > After some thinking, I think this function should be more complex: It > should check whether the node the bitmap is attached to actually can > handle persistent bitmaps and whether it would actually support storing > *this* bitmap. > > For instance, a qcow2 node would not support writing overly large > bitmaps (limited by BME_MAX_TABLE_SIZE and BME_MAX_PHYS_SIZE) or bitmaps > with overly large granularities (BME_MAX_GRANULARITY_BITS) or bitmaps > whose name is already occupied by some bitmap that is already stored in > the file but has not been loaded. > > Checking this here will trivially prevent users from creating such > bitmaps and will also preempt detection of such failures during > bdrv_close() when they cannot be handled gracefully. > > Max Good point, but I can't do it exactly as you say, because I call this function from qcow2_read_bitmaps, for just created bitmap and it should not be checked and of course it's name is occupied.. > >> +}
On 12.10.2016 14:38, Vladimir Sementsov-Ogievskiy wrote: > On 07.10.2016 22:28, 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/dirty-bitmap.c b/block/dirty-bitmap.c >>> index 623e1d1..0314581 100644 >>> --- a/block/dirty-bitmap.c >>> +++ b/block/dirty-bitmap.c >> [...] >> >>> @@ -555,3 +559,26 @@ bool bdrv_dirty_bitmap_get_autoload(const >>> BdrvDirtyBitmap *bitmap) >>> { >>> return bitmap->autoload; >>> } >>> + >>> +void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, >>> + bool persistent) >>> +{ >>> + bitmap->persistent = persistent; >> After some thinking, I think this function should be more complex: It >> should check whether the node the bitmap is attached to actually can >> handle persistent bitmaps and whether it would actually support storing >> *this* bitmap. >> >> For instance, a qcow2 node would not support writing overly large >> bitmaps (limited by BME_MAX_TABLE_SIZE and BME_MAX_PHYS_SIZE) or bitmaps >> with overly large granularities (BME_MAX_GRANULARITY_BITS) or bitmaps >> whose name is already occupied by some bitmap that is already stored in >> the file but has not been loaded. >> >> Checking this here will trivially prevent users from creating such >> bitmaps and will also preempt detection of such failures during >> bdrv_close() when they cannot be handled gracefully. >> >> Max > > Good point, but I can't do it exactly as you say, because I call this > function from qcow2_read_bitmaps, for just created bitmap and it > should not be checked and of course it's name is occupied.. So, I'll add an additional checking function, to call it from qmp_block_dirty_bitmap_add, if persistent parameter is set to true. > >> >>> +} > >
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
On 12.10.2016 14:30, Vladimir Sementsov-Ogievskiy wrote: > On 12.10.2016 14:38, Vladimir Sementsov-Ogievskiy wrote: >> On 07.10.2016 22:28, 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/dirty-bitmap.c b/block/dirty-bitmap.c >>>> index 623e1d1..0314581 100644 >>>> --- a/block/dirty-bitmap.c >>>> +++ b/block/dirty-bitmap.c >>> [...] >>> >>>> @@ -555,3 +559,26 @@ bool bdrv_dirty_bitmap_get_autoload(const >>>> BdrvDirtyBitmap *bitmap) >>>> { >>>> return bitmap->autoload; >>>> } >>>> + >>>> +void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, >>>> + bool persistent) >>>> +{ >>>> + bitmap->persistent = persistent; >>> After some thinking, I think this function should be more complex: It >>> should check whether the node the bitmap is attached to actually can >>> handle persistent bitmaps and whether it would actually support storing >>> *this* bitmap. >>> >>> For instance, a qcow2 node would not support writing overly large >>> bitmaps (limited by BME_MAX_TABLE_SIZE and BME_MAX_PHYS_SIZE) or bitmaps >>> with overly large granularities (BME_MAX_GRANULARITY_BITS) or bitmaps >>> whose name is already occupied by some bitmap that is already stored in >>> the file but has not been loaded. >>> >>> Checking this here will trivially prevent users from creating such >>> bitmaps and will also preempt detection of such failures during >>> bdrv_close() when they cannot be handled gracefully. >>> >>> Max >> >> Good point, but I can't do it exactly as you say, because I call this >> function from qcow2_read_bitmaps, for just created bitmap and it >> should not be checked and of course it's name is occupied.. > > So, I'll add an additional checking function, to call it from > qmp_block_dirty_bitmap_add, if persistent parameter is set to true. That would work just as well, yes. Thanks! Max
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); + } bdrv_release_named_dirty_bitmaps(bs); assert(QLIST_EMPTY(&bs->dirty_bitmaps)); @@ -3969,3 +3974,28 @@ void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp) parent_bs->drv->bdrv_del_child(parent_bs, child, errp); } + +void bdrv_store_persistent_bitmaps(BlockDriverState *bs, Error **errp) +{ + BlockDriver *drv = bs->drv; + + if (!bdrv_has_persistent_bitmaps(bs)) { + return; + } + + if (!drv) { + error_setg_errno(errp, ENOMEDIUM, + "Can't store persistent bitmaps to %s", + bdrv_get_device_or_node_name(bs)); + return; + } + + if (!drv->bdrv_store_persistent_bitmaps) { + error_setg_errno(errp, ENOTSUP, + "Can't store persistent bitmaps to %s", + bdrv_get_device_or_node_name(bs)); + return; + } + + drv->bdrv_store_persistent_bitmaps(bs, errp); +} diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 623e1d1..0314581 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -44,6 +44,7 @@ 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 persistent; /* bitmap must be saved to owner disk image */ bool autoload; /* bitmap must be autoloaded on image opening */ QLIST_ENTRY(BdrvDirtyBitmap) list; }; @@ -72,6 +73,7 @@ void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap) g_free(bitmap->name); bitmap->name = NULL; + bitmap->persistent = false; bitmap->autoload = false; } @@ -241,6 +243,8 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, bitmap->name = NULL; successor->name = name; bitmap->successor = NULL; + successor->persistent = bitmap->persistent; + bitmap->persistent = false; successor->autoload = bitmap->autoload; bitmap->autoload = false; bdrv_release_dirty_bitmap(bs, bitmap); @@ -555,3 +559,26 @@ bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap) { return bitmap->autoload; } + +void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, + bool persistent) +{ + bitmap->persistent = persistent; +} + +bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap) +{ + return bitmap->persistent; +} + +bool bdrv_has_persistent_bitmaps(BlockDriverState *bs) +{ + BdrvDirtyBitmap *bm; + QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) { + if (bm->persistent) { + return true; + } + } + + return false; +} diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index c086436..81520cd 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -530,6 +530,7 @@ int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp) goto out; } + bdrv_dirty_bitmap_set_persistance(bitmap, true); bdrv_dirty_bitmap_set_autoload(bitmap, true); } else { /* leave bitmap in the image */ diff --git a/include/block/block.h b/include/block/block.h index 7edce5c..8457b60 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -532,4 +532,6 @@ void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child, Error **errp); void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp); +void bdrv_store_persistent_bitmaps(BlockDriverState *bs, Error **errp); + #endif diff --git a/include/block/block_int.h b/include/block/block_int.h index 0ca6a78..a4a4816 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -320,6 +320,8 @@ struct BlockDriver { void (*bdrv_del_child)(BlockDriverState *parent, BdrvChild *child, Error **errp); + void (*bdrv_store_persistent_bitmaps)(BlockDriverState *bs, Error **errp); + QLIST_ENTRY(BlockDriver) list; }; diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h index 45a389a..8dbd16b 100644 --- a/include/block/dirty-bitmap.h +++ b/include/block/dirty-bitmap.h @@ -77,4 +77,10 @@ void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap); void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload); bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap); +void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, + bool persistent); +bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap); + +bool bdrv_has_persistent_bitmaps(BlockDriverState *bs); + #endif
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(+)