On 11/17/2017 12:30 PM, Vladimir Sementsov-Ogievskiy wrote: > 17.11.2017 20:20, John Snow wrote: >> >> On 11/17/2017 09:46 AM, Vladimir Sementsov-Ogievskiy wrote: >>> 14.11.2017 02:32, John Snow wrote: >>>> On 10/30/2017 12:32 PM, Vladimir Sementsov-Ogievskiy wrote: >>>>> Make it possible to set bitmap 'frozen' without a successor. >>>>> This is needed to protect the bitmap during outgoing bitmap postcopy >>>>> migration. >>>>> >>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> >>>>> --- >>>>> include/block/dirty-bitmap.h | 1 + >>>>> block/dirty-bitmap.c | 22 ++++++++++++++++++++-- >>>>> 2 files changed, 21 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/include/block/dirty-bitmap.h >>>>> b/include/block/dirty-bitmap.h >>>>> index a9e2a92e4f..ae6d697850 100644 >>>>> --- a/include/block/dirty-bitmap.h >>>>> +++ b/include/block/dirty-bitmap.h >>>>> @@ -39,6 +39,7 @@ uint32_t >>>>> bdrv_get_default_bitmap_granularity(BlockDriverState *bs); >>>>> uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap >>>>> *bitmap); >>>>> bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap); >>>>> bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap); >>>>> +void bdrv_dirty_bitmap_set_frozen(BdrvDirtyBitmap *bitmap, bool >>>>> frozen); >>>>> const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap); >>>>> int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap); >>>>> DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap >>>>> *bitmap); >>>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >>>>> index 7578863aa1..67fc6bd6e0 100644 >>>>> --- a/block/dirty-bitmap.c >>>>> +++ b/block/dirty-bitmap.c >>>>> @@ -40,6 +40,8 @@ struct BdrvDirtyBitmap { >>>>> QemuMutex *mutex; >>>>> HBitmap *bitmap; /* Dirty bitmap implementation */ >>>>> HBitmap *meta; /* Meta dirty bitmap */ >>>>> + bool frozen; /* Bitmap is frozen, it can't be >>>>> modified >>>>> + through QMP */ >>>> I hesitate, because this now means that we have two independent bits of >>>> state we need to update and maintain consistency with. >>> it was your proposal))) >>> >>> https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg01172.html >>> >>> " >>> Your new use case here sounds like Frozen to me, but it simply does not >>> have an anonymous successor to force it to be recognized as "frozen." We >>> can add a `bool protected` or `bool frozen` field to force recognition >>> of this status and adjust the json documentation accordingly. >>> >>> I think then we'd have four recognized states: >>> >>> FROZEN: Cannot be reset/deleted. Bitmap is in-use by a block job or >>> other internal process. Bitmap is otherwise ACTIVE. >>> DISABLED: Not recording any writes (by choice.) >>> READONLY: Not able to record any writes (by necessity.) >>> ACTIVE: Normal bitmap status. >>> >>> Sound right? >>> " >>> >>> >> I was afraid you'd say that :( >> >> It's okay, anyway. I shouldn't let myself go so long between reviews >> like this, because you catch me changing my mind. Anyway, please go >> ahead with it. I don't want to delay you on something that works because >> I can't make up *my* mind. > > Hm, if you remember, reusing "frozen" state was strange for me too. And > it's not > late to move to > 1. make a new state: MIGRATION, which disallows qmp operations on bitmap > 2. or just make them unnamed, so they can't be touched by qmp
"Migrating" is fine as a state name. You could probably announce this by having it be "frozen" in the usual way (it has a successor) and a new bool that lets you do whatever special handling you need to do for it. > > anything is ok for me as well as leaving it as is. It's all little > things, the core is patch 10. > > "frozen" sounds like unchanged, but user will see dirty-count changing > in query-block. I guess it's a strange misnomer now... or maybe just always was a bad name, since it's not really "frozen" but rather "locked" in a way that the QMP user cannot interfere with it -- but it's still a live, functioning object. > I'm remembering what I was talking about, but I think my preference is illustrably worse. I was trying to avoid boolean bloat by advocating re-use, but the re-use is kind of confusing... I think I was hoping that a bitmap on the receiving end could simply be "frozen" in the usual way, in that it has a successor recording writes. I think the way you want to handle it though is with different semantics for cleanup and recovery which makes it not quite the same state, which needs either a new bool or some such. Go with what you think is cleanest at this point, including if you want to leave it alone. I don't want to cause you to respin it over bikeshedding. --js