[Qemu-devel] [PATCH 02/18] block: BDS deletion during bdrv_drain_recurse
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.
Signed-off-by: Max Reitz
---
block/io.c | 72 +-
1 file changed, 52 insertions(+), 20 deletions(-)
diff --git a/block/io.c b/block/io.c
index 4378ae4c7d..8ec1a564ad 100644
--- a/block/io.c
+++ b/block/io.c
@@ -182,33 +182,57 @@ static void bdrv_drain_invoke(BlockDriverState *bs)
static bool bdrv_drain_recurse(BlockDriverState *bs)
{
-BdrvChild *child, *tmp;
+BdrvChild *child;
bool waited;
+struct BDSToDrain {
+BlockDriverState *bs;
+BdrvDeletedStatus del_stat;
+QLIST_ENTRY(BDSToDrain) next;
+};
+QLIST_HEAD(, BDSToDrain) bs_list = QLIST_HEAD_INITIALIZER(bs_list);
+bool in_main_loop =
+qemu_get_current_aio_context() == qemu_get_aio_context();
waited = BDRV_POLL_WHILE(bs, atomic_read(&bs->in_flight) > 0);
/* Ensure any pending metadata writes are submitted to bs->file. */
bdrv_drain_invoke(bs);
-QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) {
-BlockDriverState *bs = child->bs;
-bool in_main_loop =
-qemu_get_current_aio_context() == qemu_get_aio_context();
-assert(bs->refcnt > 0);
-if (in_main_loop) {
-/* In case the recursive bdrv_drain_recurse processes a
- * block_job_defer_to_main_loop BH and modifies the graph,
- * let's hold a reference to bs until we are done.
- *
- * IOThread doesn't have such a BH, and it is not safe to call
- * bdrv_unref without BQL, so skip doing it there.
- */
-bdrv_ref(bs);
-}
-waited |= bdrv_drain_recurse(bs);
-if (in_main_loop) {
-bdrv_unref(bs);
+/* Draining children may result in other children being removed and maybe
+ * even deleted, so copy the children list first */
+QLIST_FOREACH(child, &bs->children, next) {
+struct BDSToDrain *bs2d = g_new0(struct BDSToDrain, 1);
+
+bs2d->bs = child->bs;
+QLIST_INSERT_HEAD(&bs->deleted_status, &bs2d->del_stat, next);
+
+QLIST_INSERT_HEAD(&bs_list, bs2d, next);
+}
+
+while (!QLIST_EMPTY(&bs_list)) {
+struct BDSToDrain *bs2d = QLIST_FIRST(&bs_list);
+QLIST_REMOVE(bs2d, next);
+
+if (!bs2d->del_stat.deleted) {
+QLIST_REMOVE(&bs2d->del_stat, next);
+
+if (in_main_loop) {
+/* In case the recursive bdrv_drain_recurse processes a
+ * block_job_defer_to_main_loop BH and modifies the graph,
+ * let's hold a reference to the BDS until we are done.
+ *
+ * IOThread doesn't have such a BH, and it is not safe to call
+ * bdrv_unref without BQL, so skip doing it there.
+ */
+bdrv_ref(bs2d->bs);
+}
+waited |= bdrv_drain_recurse(bs2d->bs);
+if (in_main_loop) {
+bdrv_unref(bs2d->bs);
+}
}
+
+g_free(bs2d);
}
return waited;
@@ -252,17 +276,25 @@ static void coroutine_fn
bdrv_co_yield_to_drain(BlockDriverState *bs)
void bdrv_drained_begin(BlockDriverState *bs)
{
+BdrvDeletedStatus del_stat = { .deleted = false };
+
if (qemu_in_coroutine()) {
bdrv_co_yield_to_drain(bs);
return;
}
+QLIST_INSERT_HEAD(&bs->deleted_status, &del_stat, next);
+
if (atomic_fetch_inc(&bs->quiesce_counter) == 0) {
aio_disable_external(bdrv_get_aio_context(bs));
bdrv_parent_drained_begin(bs);
}
-bdrv_drain_recurse(bs);
+if (!del_stat.deleted) {
+QLIST_REMOVE(&del_stat, next);
+
+bdrv_drain_recurse(bs);
+}
}
void bdrv_drained_end(BlockDriverState *bs)
--
2.13.5
Re: [Qemu-devel] [PATCH 02/18] block: BDS deletion during bdrv_drain_recurse
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? 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.
Fam
>
> Signed-off-by: Max Reitz
> ---
> block/io.c | 72
> +-
> 1 file changed, 52 insertions(+), 20 deletions(-)
>
> diff --git a/block/io.c b/block/io.c
> index 4378ae4c7d..8ec1a564ad 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -182,33 +182,57 @@ static void bdrv_drain_invoke(BlockDriverState *bs)
>
> static bool bdrv_drain_recurse(BlockDriverState *bs)
> {
> -BdrvChild *child, *tmp;
> +BdrvChild *child;
> bool waited;
> +struct BDSToDrain {
> +BlockDriverState *bs;
> +BdrvDeletedStatus del_stat;
> +QLIST_ENTRY(BDSToDrain) next;
> +};
> +QLIST_HEAD(, BDSToDrain) bs_list = QLIST_HEAD_INITIALIZER(bs_list);
> +bool in_main_loop =
> +qemu_get_current_aio_context() == qemu_get_aio_context();
>
> waited = BDRV_POLL_WHILE(bs, atomic_read(&bs->in_flight) > 0);
>
> /* Ensure any pending metadata writes are submitted to bs->file. */
> bdrv_drain_invoke(bs);
>
> -QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) {
> -BlockDriverState *bs = child->bs;
> -bool in_main_loop =
> -qemu_get_current_aio_context() == qemu_get_aio_context();
> -assert(bs->refcnt > 0);
> -if (in_main_loop) {
> -/* In case the recursive bdrv_drain_recurse processes a
> - * block_job_defer_to_main_loop BH and modifies the graph,
> - * let's hold a reference to bs until we are done.
> - *
> - * IOThread doesn't have such a BH, and it is not safe to call
> - * bdrv_unref without BQL, so skip doing it there.
> - */
> -bdrv_ref(bs);
> -}
> -waited |= bdrv_drain_recurse(bs);
> -if (in_main_loop) {
> -bdrv_unref(bs);
> +/* Draining children may result in other children being removed and maybe
> + * even deleted, so copy the children list first */
> +QLIST_FOREACH(child, &bs->children, next) {
> +struct BDSToDrain *bs2d = g_new0(struct BDSToDrain, 1);
> +
> +bs2d->bs = child->bs;
> +QLIST_INSERT_HEAD(&bs->deleted_status, &bs2d->del_stat, next);
> +
> +QLIST_INSERT_HEAD(&bs_list, bs2d, next);
> +}
> +
> +while (!QLIST_EMPTY(&bs_list)) {
> +struct BDSToDrain *bs2d = QLIST_FIRST(&bs_list);
> +QLIST_REMOVE(bs2d, next);
> +
> +if (!bs2d->del_stat.deleted) {
> +QLIST_REMOVE(&bs2d->del_stat, next);
> +
> +if (in_main_loop) {
> +/* In case the recursive bdrv_drain_recurse processes a
> + * block_job_defer_to_main_loop BH and modifies the graph,
> + * let's hold a reference to the BDS until we are done.
> + *
> + * IOThread doesn't have such a BH, and it is not safe to
> call
> + * bdrv_unref without BQL, so skip doing it there.
> + */
> +bdrv_ref(bs2d->bs);
> +}
> +waited |= bdrv_drain_recurse(bs2d->bs);
> +if (in_main_loop) {
> +bdrv_unref(bs2d->bs);
> +}
> }
> +
> +g_free(bs2d);
> }
>
> return waited;
> @@ -252,17 +276,25 @@ static void coroutine_fn
> bdrv_co_yield_to_drain(BlockDriverState *bs)
>
> void bdrv_drained_begin(BlockDriverState *bs)
> {
> +BdrvDeletedStatus del_stat = { .deleted = false };
> +
> if (qemu_in_coroutine()) {
> bdrv_co_yield_to_drain(bs);
> return;
> }
>
> +QLIST_INSERT_HEAD(&bs->deleted_status, &del_stat, next);
> +
> if (atomic_fetch_inc(&bs->quiesce_counter) == 0) {
> aio_disable_external(bdrv_get_aio_context(bs));
> bdrv_parent_drained_begin(bs);
> }
>
> -bdrv_drain_recurse(bs);
> +if (!del_stat.deleted) {
> +QLIST_REMOVE(&del_stat, next);
> +
> +bdrv_drain_recurse(bs);
> +}
> }
>
> void bdrv_drained_end(BlockDriverState *bs)
> --
> 2.13.5
>
Re: [Qemu-devel] [PATCH 02/18] block: BDS deletion during bdrv_drain_recurse
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
Re: [Qemu-devel] [PATCH 02/18] block: BDS deletion during bdrv_drain_recurse
On 2017-09-18 18:13, Max Reitz wrote:
> 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.
It turns out it isn't so simple after all... because bdrv_close()
invokes bdrv_drained_begin(). So we may end up with an endless recursion
here.
One way to fix this would be to skip the bdrv_drained_begin() in
bdrv_close() if this would result in such a recursion... But any
solution that comes quickly to my mind would require another BDS field,
too -- just checking the quiesce_counter is probably not enough because
this might just indicate concurrent drainage that stops before
bdrv_close() wants it to stop.
So maybe BdrvDeletedStatus is the simplest solution after all...?
Max
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 02/18] block: BDS deletion during bdrv_drain_recurse
Am 13.09.2017 um 20:18 hat Max Reitz geschrieben:
> 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.
>
> Signed-off-by: Max Reitz
How hard would it be to write a test case for this? qemu-iotests
probably isn't the right tool, but I feel a C unit test would be
possible.
> -QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) {
> -BlockDriverState *bs = child->bs;
> -bool in_main_loop =
> -qemu_get_current_aio_context() == qemu_get_aio_context();
> -assert(bs->refcnt > 0);
Would it make sense to keep this assertion for the !deleted case?
> -if (in_main_loop) {
> -/* In case the recursive bdrv_drain_recurse processes a
> - * block_job_defer_to_main_loop BH and modifies the graph,
> - * let's hold a reference to bs until we are done.
> - *
> - * IOThread doesn't have such a BH, and it is not safe to call
> - * bdrv_unref without BQL, so skip doing it there.
> - */
> -bdrv_ref(bs);
> -}
> -waited |= bdrv_drain_recurse(bs);
> -if (in_main_loop) {
> -bdrv_unref(bs);
> +/* Draining children may result in other children being removed and maybe
> + * even deleted, so copy the children list first */
Maybe it's just me, but I failed to understand this correctly at first.
How about "being removed from their parent" to clarify that it's not the
BDS that is removed, but just the reference?
Kevin
Re: [Qemu-devel] [PATCH 02/18] block: BDS deletion during bdrv_drain_recurse
On 2017-10-10 10:36, Kevin Wolf wrote:
> Am 13.09.2017 um 20:18 hat Max Reitz geschrieben:
>> 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.
>>
>> Signed-off-by: Max Reitz
>
> How hard would it be to write a test case for this? qemu-iotests
> probably isn't the right tool, but I feel a C unit test would be
> possible.
I can look into it, but I can't promise anything.
>> -QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) {
>> -BlockDriverState *bs = child->bs;
>> -bool in_main_loop =
>> -qemu_get_current_aio_context() == qemu_get_aio_context();
>> -assert(bs->refcnt > 0);
>
> Would it make sense to keep this assertion for the !deleted case?
Sure, why not.
>> -if (in_main_loop) {
>> -/* In case the recursive bdrv_drain_recurse processes a
>> - * block_job_defer_to_main_loop BH and modifies the graph,
>> - * let's hold a reference to bs until we are done.
>> - *
>> - * IOThread doesn't have such a BH, and it is not safe to call
>> - * bdrv_unref without BQL, so skip doing it there.
>> - */
>> -bdrv_ref(bs);
>> -}
>> -waited |= bdrv_drain_recurse(bs);
>> -if (in_main_loop) {
>> -bdrv_unref(bs);
>> +/* Draining children may result in other children being removed and
>> maybe
>> + * even deleted, so copy the children list first */
>
> Maybe it's just me, but I failed to understand this correctly at first.
> How about "being removed from their parent" to clarify that it's not the
> BDS that is removed, but just the reference?
Well, it's the BdrvChild that's removed, that's what I meant by
"children". But then the comment speaks of "children list" and means
creation of a list of BDSs, sooo... Yes, some change necessary.
Max
signature.asc
Description: OpenPGP digital signature
