On 07/06/2017 08:53 PM, John Snow wrote: > > On 07/06/2017 04:05 AM, Vladimir Sementsov-Ogievskiy wrote: >> 06.07.2017 00:46, John Snow wrote: >>> On 07/05/2017 05:24 AM, Vladimir Sementsov-Ogievskiy wrote: >>>> 16.02.2017 16:04, Fam Zheng wrote: >>>>>> + dbms->node_name = bdrv_get_node_name(bs); >>>>>> + if (!dbms->node_name || dbms->node_name[0] == '\0') { >>>>>> + dbms->node_name = bdrv_get_device_name(bs); >>>>>> + } >>>>>> + dbms->bitmap = bitmap; >>>>> What protects the case that the bitmap is released before migration >>>>> completes? >>>>> >>>> What is the source of such deletion? qmp command? Theoretically >>>> possible. >>>> >>>> I see the following variants: >>>> >>>> 1. additional variable BdrvDirtyBItmap.migration, which forbids bitmap >>>> deletion >>>> >>>> 2. make bitmap anonymous (bdrv_dirty_bitmap_make_anon) - it will not be >>>> available through qmp >>>> >>> Making the bitmap anonymous would forbid us to query the bitmap, which >>> there is no general reason to do, excepting the idea that a third party >>> attempting to use the bitmap during a migration is probably a bad idea. >>> I don't really like the idea of "hiding" information from the user, >>> though, because then we'd have to worry about name collisions when we >>> de-anonymized the bitmap again. That's not so palatable. >>> >>>> what do you think? >>>> >>> The modes for bitmaps are getting messy. >>> >>> As a reminder, the officially exposed "modes" of a bitmap are currently: >>> >>> FROZEN: Cannot be reset/deleted. Implication is that the bitmap is >>> otherwise "ACTIVE." >>> DISABLED: Not recording any writes (by choice.) >>> ACTIVE: Actively recording writes. >>> >>> These are documented in the public API as possibilities for >>> DirtyBitmapStatus in block-core.json. We didn't add a new condition for >>> "readonly" either, which I think is actually required: >>> >>> READONLY: Not recording any writes (by necessity.) >>> >>> >>> 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. >> Bitmaps are selected for migration when source is running, so we should >> protect them (from deletion (or frozing or disabling), not from chaning >> bits) before source stop, so that is not like frozen. Bitmaps may be >> changed in this state. >> It is more like ACTIVE. >> > Right, it's not exactly like frozen's _implementation_ today, but... > >> We can move bitmap selection on the point after precopy migration, after >> source stop, but I'm not sure that it would be good. >> >>> 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. >> ? Frozen means that all writes goes to the successor and frozen bitmap >> itself is unchanged, no? >> > I was thinking from the point of view of the API. Of course internally, > you're correct; a "frozen bitmap" is one that is actually disabled and > has an anonymous successor, and that successor records IO. > > From the point of view of the API, a frozen bitmap is just "one bitmap" > that is still recording reads/writes, but is protected from being edited > from QMP. > > It depends on if you're looking at bitmaps as opaque API objects or if > you're looking at the implementation. > > From an API point of view, protecting an Active bitmap from being > renamed/cleared/deleted is functionally identical to the existing case > of protecting a bitmap-and-successor pair during a backup job.
I think that libvirt properly guards QMP avoid commands changing the device tree etc at the moment. Thus we should be fine here. Den >>> DISABLED: Not recording any writes (by choice.) >>> READONLY: Not able to record any writes (by necessity.) >>> ACTIVE: Normal bitmap status. >>> >>> Sound right? >>