[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: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH 02/18] block: BDS deletion during bdrv_drain_recurse |
Date: |
Tue, 10 Oct 2017 10:36:51 +0200 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
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 <address@hidden>
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