On Mon, 12/12 19:23, Markus Armbruster wrote:
> 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?
The placeholder will mainly protect "insertions" and therefore the location
woule be on edge where we insert a new filter.
Removing a node doesn't need placeholder protection.
Updating a node (to a different type) probably doesn't need that either but I'm
not 100% sure.
Fam
