block/mirror.c | 2 ++ 1 file changed, 2 insertions(+)
From: Long YunJian <long.yunjian@zte.com.cn>
when blockcommit from active leaf node, sometimes, we get assertion failed with
"mirror_run: Assertion `QLIST_EMPTY(&bs->tracked_requests)' failed" messages.
According to the core file, we find bs->tracked_requests has IO request,
so assertion failed.
(gdb) bt
#0 0x00007f410df707cf in raise () from /lib64/libc.so.6
#1 0x00007f410df5ac05 in abort () from /lib64/libc.so.6
#2 0x00007f410df5aad9 in __assert_fail_base.cold.0 () from /lib64/libc.so.6
#3 0x00007f410df68db6 in __assert_fail () from /lib64/libc.so.6
#4 0x0000556915635371 in mirror_run (job=0x556916ff8600, errp=<optimized out>) at block/mirror.c:1092
#5 0x00005569155e6c53 in job_co_entry (opaque=0x556916ff8600) at job.c:904
#6 0x00005569156d9483 in coroutine_trampoline (i0=<optimized out>, i1=<optimized out>) at util/coroutine-ucontext.c:115
(gdb) p s->mirror_top_bs->backing->bs->tracked_requests
$12 = {lh_first = 0x7f3f07bfb8b0}
(gdb) p s->mirror_top_bs->backing->bs->tracked_requests->lh_first
$13 = (struct BdrvTrackedRequest *) 0x7f3f07bfb8b0
Actually, before excuting assert(QLIST_EMPTY(&bs->tracked_requests)),
it will excute mirror_flush(s). It may handle new I/O request and maybe
pending I/O during this flush. Just likes in bdrv_close fuction,
bdrv_drain(bs) followed by bdrv_flush(bs), we should add bdrv_drain fuction
to handle pending I/O after mirror_flush.
Signed-off-by: Long YunJian <long.yunjian@zte.com.cn>
Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
---
block/mirror.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/block/mirror.c b/block/mirror.c
index efec2c7674..1eec356310 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1079,6 +1079,8 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
s->in_drain = false;
continue;
}
+ /* in case flush left pending I/O */
+ bdrv_drain(bs);
/* The two disks are in sync. Exit and report successful
* completion.
--
2.18.1
[CC-ing qemu-block, Vladimir, Kevin, and John – when sending patches, please look into the MAINTAINERS file or use the scripts/get_maintainer.pl script to find out who to CC on them. It’s very to overlook patches on qemu-devel :/] On 07.12.21 11:56, Yi Wang wrote: > From: Long YunJian <long.yunjian@zte.com.cn> > > when blockcommit from active leaf node, sometimes, we get assertion failed with > "mirror_run: Assertion `QLIST_EMPTY(&bs->tracked_requests)' failed" messages. > According to the core file, we find bs->tracked_requests has IO request, > so assertion failed. > (gdb) bt > #0 0x00007f410df707cf in raise () from /lib64/libc.so.6 > #1 0x00007f410df5ac05 in abort () from /lib64/libc.so.6 > #2 0x00007f410df5aad9 in __assert_fail_base.cold.0 () from /lib64/libc.so.6 > #3 0x00007f410df68db6 in __assert_fail () from /lib64/libc.so.6 > #4 0x0000556915635371 in mirror_run (job=0x556916ff8600, errp=<optimized out>) at block/mirror.c:1092 > #5 0x00005569155e6c53 in job_co_entry (opaque=0x556916ff8600) at job.c:904 > #6 0x00005569156d9483 in coroutine_trampoline (i0=<optimized out>, i1=<optimized out>) at util/coroutine-ucontext.c:115 > (gdb) p s->mirror_top_bs->backing->bs->tracked_requests > $12 = {lh_first = 0x7f3f07bfb8b0} > (gdb) p s->mirror_top_bs->backing->bs->tracked_requests->lh_first > $13 = (struct BdrvTrackedRequest *) 0x7f3f07bfb8b0 > > Actually, before excuting assert(QLIST_EMPTY(&bs->tracked_requests)), > it will excute mirror_flush(s). It may handle new I/O request and maybe > pending I/O during this flush. Just likes in bdrv_close fuction, > bdrv_drain(bs) followed by bdrv_flush(bs), we should add bdrv_drain fuction > to handle pending I/O after mirror_flush. Oh. How is that happening, though? I would have expected that flushing the target BB (and associated BDS) only flushes requests to the OS and lower layers, but the source node (which is `bs`) should (in the case of commit) always be above the target, so I wouldn’t have expected it to get any new requests due to this flush. Do you have a reproducer for this? > Signed-off-by: Long YunJian <long.yunjian@zte.com.cn> > Signed-off-by: Yi Wang <wang.yi59@zte.com.cn> > --- > block/mirror.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/block/mirror.c b/block/mirror.c > index efec2c7674..1eec356310 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -1079,6 +1079,8 @@ static int coroutine_fn mirror_run(Job *job, Error **errp) > s->in_drain = false; > continue; > } > + /* in case flush left pending I/O */ > + bdrv_drain(bs); I don’t think this works, because if we drain, we would also need to flush the target again. Essentially I believe we’d basically need something like do { bdrv_drained_begin(bs); mirror_flush(s); if (!QLIST_EMPTY(&bs->tracked_requests)) { bdrv_drained_end(bs); } } while (!QLIST_EMPTY(&bs->tracked_requests)); (Which I know is really ugly) Hanna > > /* The two disks are in sync. Exit and report successful > * completion.
© 2016 - 2024 Red Hat, Inc.