Fam Zheng <f...@redhat.com> writes:
> On Mon, 12/12 14:58, Markus Armbruster wrote:
>> Fam Zheng <f...@redhat.com> writes:
>>
>> > On Thu, 12/08 13:46, Markus Armbruster wrote:
>> >> Fam Zheng <f...@redhat.com> writes:
>> >>
>> >> > On Wed, 12/07 10:48, Kevin Wolf wrote:
>> >> >> > If so I think there is no race to worry about, mirror-filter should
>> >> >> > go
>> >> >> > away only after a QMP command.
>> >> >>
>> >> >> Currently, a mirror job goes away whenever it is done. This is not
>> >> >> directly tied to a QMP command.
>> >> >
>> >> > Ah right, block-job-complete is only "start to complete" and the job
>> >> > goes away
>> >> > at some later point. I thought this is "the" QMP command but it is not.
>> >> >
>> >> >>
>> >> >> Of course, in the new job API we want an explicit job-delete, so in
>> >> >> that case it wouldn't happen, but we need to keep the old case for
>> >> >> compatibility.
>> >> >
>> >> > Another possibility is to add a placeholder node in the right location
>> >> > first
>> >> > then fill in the actual throttling node once created. QMP owns the
>> >> > placeholder
>> >> > node so it won't suddenly vanish when mirror job goes away.
>> >>
>> >> I'm not sure I understand this idea. Could you explain it in a bit more
>> >> detail, perhaps even with a bit of ASCII art?
>> >
>> > OK, I'll base on your version:
>> >
>> > == Basic dynamic reconfiguration operation ==
>> >
>> > The two primitives are "insert placeholder" and bdrv_replace_child;
>> >
>> > A placeholder BDS is a dummy "pass through everything" filter. Inserting
>> > placeholder is something like:
>> >
>> > void bdrv_insert_placeholder(BdrvChild *child) {
>> > BlockDriverState *ph = bdrv_new_placeholder();
>> > ph->file->bs = child->bs;
>> > child->bs = ph;
>> > }
>> >
>> > Consider:
>> >
>> > BB
>> > |
>> > mirror-filter
>> > |
>> > BDS
>> >
>> > Add a throttle filter under BB while the mirror job is running. First
>> > step,
>> > create the dummy _placeholder_ node and throttle-filter node (the
>> > existence of
>> > placeholder node can be an implementation detail hidden from user):
>> >
>> > placeholder = bdrv_insert_placeholder(BB->root);
>> > throttle_filter = bdrv_create_throttle_filter(placeholder);
>> >
>> > we get:
>> >
>> > BB throttle-filter
>> > | /
>> > placeholder
>> > |
>> > mirror-filter
>> > |
>> > BDS
>> >
>> > Second step, move throttle filter in:
>> >
>> > bdrv_replace_child(BB->root, throttle_filter);
>> > bdrv_replace_child(throttle_filter->file,
>> > throttle_filter->file->bs->file->bs);
>> > bdrv_unref(placeholder);
>> >
>> > we get:
>> >
>> > BB
>> > |
>> > throttle-filter
>> > |
>> > mirror-filter
>> > |
>> > BDS
>> >
>> > Does this make us safe from racing with mirror-filter disappearing?
>>
>> Thanks! I figure your placeholder node does immunize the splicing in of
>> the throttle-filter against the removal of mirror-filter.
>>
>> Given a specific dynamic reconfiguration, where do we have to put
>> placeholder nodes to immunize it against arbitrary other dynamic
>> changes?
>
> Generally, a placeholder node is essentially an insertion to the middle of an
> existing edge, namely to change:
>
> NodeA
> / | \
> / | \
> [...] NodeB [...]
>
> into:
>
> NodeA
> / | \
> / | \
> [...] p-holdr [...]
> |
> |
> NodeB
>
> and the operation is safe because of BQL. Our later bdrv_replace_child()
> operations on it will always work because a dynamic reconfiguration won't
> delete
> the placeholder node in any case.
>
> On the other hand, concurrent dynamic changes could yield usual outcomes from
> user PoV (like the involved node is no longer in the same place or
> disappears),
> but I think it is still in some degree under user's control. E.g. a
> mirror-filter won't go away automatically until user does
> block-job-complete/cancel.
I think I understand how we can safely insert a placeholder. My
question is: *where* do we have to insert placeholders before a certain
dynamic reconfiguration to make that reconfiguration safe?
