[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 02/18] block: BDS deletion during bdrv_drain_rec
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH 02/18] block: BDS deletion during bdrv_drain_recurse |
Date: |
Mon, 18 Sep 2017 18:13:03 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 |
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
- [Qemu-devel] [PATCH 00/18] block/mirror: Add active-sync mirroring, Max Reitz, 2017/09/13
- [Qemu-devel] [PATCH 01/18] block: Add BdrvDeletedStatus, Max Reitz, 2017/09/13
- [Qemu-devel] [PATCH 02/18] block: BDS deletion during bdrv_drain_recurse, Max Reitz, 2017/09/13
- [Qemu-devel] [PATCH 03/18] blockjob: Make drained_{begin, end} public, Max Reitz, 2017/09/13
- [Qemu-devel] [PATCH 04/18] block/mirror: Pull out mirror_perform(), Max Reitz, 2017/09/13
- [Qemu-devel] [PATCH 05/18] block/mirror: Convert to coroutines, Max Reitz, 2017/09/13
- [Qemu-devel] [PATCH 06/18] block/mirror: Use CoQueue to wait on in-flight ops, Max Reitz, 2017/09/13
- [Qemu-devel] [PATCH 07/18] block/mirror: Wait for in-flight op conflicts, Max Reitz, 2017/09/13