On 2017-09-18 05:44, Fam Zheng wrote: > On Wed, 09/13 20:18, Max Reitz wrote: >> Drainined a BDS child may lead to both the original BDS and/or its other >> children being deleted (e.g. if the original BDS represents a block >> job). We should prepare for this in both bdrv_drain_recurse() and >> bdrv_drained_begin() by monitoring whether the BDS we are about to drain >> still exists at all. > > Can the deletion happen when IOThread calls > bdrv_drain_recurse/bdrv_drained_begin?
I don't think so, because (1) my issue was draining a block job and that
can only be completed in the main loop, and (2) I would like to think
it's always impossible, considering that bdrv_unref() may only be called
with the BQL.
> If not, is it enough to do
>
> ...
> if (in_main_loop) {
> bdrv_ref(bs);
> }
> ...
> if (in_main_loop) {
> bdrv_unref(bs);
> }
>
> to protect the main loop case? So the BdrvDeletedStatus state is not needed.
We already have that in bdrv_drained_recurse(), don't we?
The issue here is, though, that QLIST_FOREACH_SAFE() stores the next
child pointer to @tmp. However, once the current child @child is
drained, @tmp may no longer be valid -- it may have been detached from
@bs, and it may even have been deleted.
We could work around the latter by increasing the next child's reference
somehow (but BdrvChild doesn't really have a refcount, and in order to
do so, we would probably have to emulate being a parent or
something...), but then you'd still have the issue of @tmp being
detached from the children list we're trying to iterate over. So
tmp->next is no longer valid.
Anyway, so the latter is the reason why I decided to introduce the bs_list.
But maybe that actually saves us from having to fiddle with BdrvChild...
Since it's just a list of BDSs now, it may be enough to simply
bdrv_ref() all of the BDSs in that list before draining any of them. So
we'd keep creating the bs_list and then we'd move the existing
bdrv_ref() from the drain loop into the loop filling bs_list.
And adding a bdrv_ref()/bdrv_unref() pair to bdrv_drained_begin() should
hopefully work there, too.
Max
signature.asc
Description: OpenPGP digital signature
