* [Qemu-devel] [PULL 0/2] Block job patches
@ 2015-08-14 13:57 Jeff Cody
2015-08-14 13:57 ` [Qemu-devel] [PULL 1/2] block/mirror: limit qiov to IOV_MAX elements Jeff Cody
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Jeff Cody @ 2015-08-14 13:57 UTC (permalink / raw
To: qemu-block; +Cc: peter.maydell, jcody, qemu-devel
The following changes since commit be1f13ac9d9fc21908975460652a72f5f0c018c5:
Merge remote-tracking branch 'remotes/lalrae/tags/mips-20150813' into staging (2015-08-13 17:47:44 +0100)
are available in the git repository at:
git@github.com:codyprime/qemu-kvm-jtc.git tags/block-pull-request
for you to fetch changes up to e424aff5f307227b1c2512bbb8ece891bb895cef:
mirror: Fix coroutine reentrance (2015-08-14 09:51:31 -0400)
----------------------------------------------------------------
Block job patches
----------------------------------------------------------------
Kevin Wolf (1):
mirror: Fix coroutine reentrance
Stefan Hajnoczi (1):
block/mirror: limit qiov to IOV_MAX elements
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PULL 1/2] block/mirror: limit qiov to IOV_MAX elements
2015-08-14 13:57 [Qemu-devel] [PULL 0/2] Block job patches Jeff Cody
@ 2015-08-14 13:57 ` Jeff Cody
2015-08-14 13:57 ` [Qemu-devel] [PULL 2/2] mirror: Fix coroutine reentrance Jeff Cody
2015-08-14 14:51 ` [Qemu-devel] [PULL 0/2] Block job patches Peter Maydell
2 siblings, 0 replies; 6+ messages in thread
From: Jeff Cody @ 2015-08-14 13:57 UTC (permalink / raw
To: qemu-block; +Cc: peter.maydell, jcody, qemu-devel
From: Stefan Hajnoczi <stefanha@redhat.com>
If mirror has more free buffers than IOV_MAX, preadv(2)/pwritev(2)
EINVAL failures may be encountered.
It is possible to trigger this by setting granularity to a low value
like 8192.
This patch stops appending chunks once IOV_MAX is reached.
The spurious EINVAL failure can be reproduced with a qcow2 image file
and the following QMP invocation:
qmp.command('drive-mirror', device='virtio0', target='/tmp/r7.s1',
granularity=8192, sync='full', mode='absolute-paths',
format='raw')
While the guest is running dd if=/dev/zero of=/var/tmp/foo oflag=direct
bs=4k.
Cc: Jeff Cody <jcody@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Message-id: 1435761950-26714-1-git-send-email-stefanha@redhat.com
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
block/mirror.c | 4 ++++
trace-events | 1 +
2 files changed, 5 insertions(+)
diff --git a/block/mirror.c b/block/mirror.c
index fc4d8f5..0841964 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -245,6 +245,10 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
trace_mirror_break_buf_busy(s, nb_chunks, s->in_flight);
break;
}
+ if (IOV_MAX < nb_chunks + added_chunks) {
+ trace_mirror_break_iov_max(s, nb_chunks, added_chunks);
+ break;
+ }
/* We have enough free space to copy these sectors. */
bitmap_set(s->in_flight_bitmap, next_chunk, added_chunks);
diff --git a/trace-events b/trace-events
index 94bf3bb..8f9614a 100644
--- a/trace-events
+++ b/trace-events
@@ -94,6 +94,7 @@ mirror_yield(void *s, int64_t cnt, int buf_free_count, int in_flight) "s %p dirt
mirror_yield_in_flight(void *s, int64_t sector_num, int in_flight) "s %p sector_num %"PRId64" in_flight %d"
mirror_yield_buf_busy(void *s, int nb_chunks, int in_flight) "s %p requested chunks %d in_flight %d"
mirror_break_buf_busy(void *s, int nb_chunks, int in_flight) "s %p requested chunks %d in_flight %d"
+mirror_break_iov_max(void *s, int nb_chunks, int added_chunks) "s %p requested chunks %d added_chunks %d"
# block/backup.c
backup_do_cow_enter(void *job, int64_t start, int64_t sector_num, int nb_sectors) "job %p start %"PRId64" sector_num %"PRId64" nb_sectors %d"
--
1.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Qemu-devel] [PULL 2/2] mirror: Fix coroutine reentrance
2015-08-14 13:57 [Qemu-devel] [PULL 0/2] Block job patches Jeff Cody
2015-08-14 13:57 ` [Qemu-devel] [PULL 1/2] block/mirror: limit qiov to IOV_MAX elements Jeff Cody
@ 2015-08-14 13:57 ` Jeff Cody
2015-08-14 14:51 ` [Qemu-devel] [PULL 0/2] Block job patches Peter Maydell
2 siblings, 0 replies; 6+ messages in thread
From: Jeff Cody @ 2015-08-14 13:57 UTC (permalink / raw
To: qemu-block; +Cc: peter.maydell, jcody, qemu-devel
From: Kevin Wolf <kwolf@redhat.com>
This fixes a regression introduced by commit dcfb3beb ("mirror: Do zero
write on target if sectors not allocated"), which was reported to cause
aborts with the message "Co-routine re-entered recursively".
The cause for this bug is the following code in mirror_iteration_done():
if (s->common.busy) {
qemu_coroutine_enter(s->common.co, NULL);
}
This has always been ugly because - unlike most places that reenter - it
doesn't have a specific yield that it pairs with, but is more
uncontrolled. What we really mean here is "reenter the coroutine if
it's in one of the four explicit yields in mirror.c".
This used to be equivalent with s->common.busy because neither
mirror_run() nor mirror_iteration() call any function that could yield.
However since commit dcfb3beb this doesn't hold true any more:
bdrv_get_block_status_above() can yield.
So what happens is that bdrv_get_block_status_above() wants to take a
lock that is already held, so it adds itself to the queue of waiting
coroutines and yields. Instead of being woken up by the unlock function,
however, it gets woken up by mirror_iteration_done(), which is obviously
wrong.
In most cases the code actually happens to cope fairly well with such
cases, but in this specific case, the unlock must already have scheduled
the coroutine for wakeup when mirror_iteration_done() reentered it. And
then the coroutine happened to process the scheduled restarts and tried
to reenter itself recursively.
This patch fixes the problem by pairing the reenter in
mirror_iteration_done() with specific yields instead of abusing
s->common.busy.
Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Message-id: 1439455310-11263-1-git-send-email-kwolf@redhat.com
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
block/mirror.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index 0841964..9474443 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -60,6 +60,7 @@ typedef struct MirrorBlockJob {
int sectors_in_flight;
int ret;
bool unmap;
+ bool waiting_for_io;
} MirrorBlockJob;
typedef struct MirrorOp {
@@ -114,11 +115,7 @@ static void mirror_iteration_done(MirrorOp *op, int ret)
qemu_iovec_destroy(&op->qiov);
g_slice_free(MirrorOp, op);
- /* Enter coroutine when it is not sleeping. The coroutine sleeps to
- * rate-limit itself. The coroutine will eventually resume since there is
- * a sleep timeout so don't wake it early.
- */
- if (s->common.busy) {
+ if (s->waiting_for_io) {
qemu_coroutine_enter(s->common.co, NULL);
}
}
@@ -203,7 +200,9 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
/* Wait for I/O to this cluster (from a previous iteration) to be done. */
while (test_bit(next_chunk, s->in_flight_bitmap)) {
trace_mirror_yield_in_flight(s, sector_num, s->in_flight);
+ s->waiting_for_io = true;
qemu_coroutine_yield();
+ s->waiting_for_io = false;
}
do {
@@ -239,7 +238,9 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
*/
while (nb_chunks == 0 && s->buf_free_count < added_chunks) {
trace_mirror_yield_buf_busy(s, nb_chunks, s->in_flight);
+ s->waiting_for_io = true;
qemu_coroutine_yield();
+ s->waiting_for_io = false;
}
if (s->buf_free_count < nb_chunks + added_chunks) {
trace_mirror_break_buf_busy(s, nb_chunks, s->in_flight);
@@ -337,7 +338,9 @@ static void mirror_free_init(MirrorBlockJob *s)
static void mirror_drain(MirrorBlockJob *s)
{
while (s->in_flight > 0) {
+ s->waiting_for_io = true;
qemu_coroutine_yield();
+ s->waiting_for_io = false;
}
}
@@ -510,7 +513,9 @@ static void coroutine_fn mirror_run(void *opaque)
if (s->in_flight == MAX_IN_FLIGHT || s->buf_free_count == 0 ||
(cnt == 0 && s->in_flight > 0)) {
trace_mirror_yield(s, s->in_flight, s->buf_free_count, cnt);
+ s->waiting_for_io = true;
qemu_coroutine_yield();
+ s->waiting_for_io = false;
continue;
} else if (cnt != 0) {
delay_ns = mirror_iteration(s);
--
1.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PULL 0/2] Block job patches
2015-08-14 13:57 [Qemu-devel] [PULL 0/2] Block job patches Jeff Cody
2015-08-14 13:57 ` [Qemu-devel] [PULL 1/2] block/mirror: limit qiov to IOV_MAX elements Jeff Cody
2015-08-14 13:57 ` [Qemu-devel] [PULL 2/2] mirror: Fix coroutine reentrance Jeff Cody
@ 2015-08-14 14:51 ` Peter Maydell
2015-08-14 14:55 ` Jeff Cody
2 siblings, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2015-08-14 14:51 UTC (permalink / raw
To: Jeff Cody; +Cc: QEMU Developers, Qemu-block
On 14 August 2015 at 14:57, Jeff Cody <jcody@redhat.com> wrote:
> The following changes since commit be1f13ac9d9fc21908975460652a72f5f0c018c5:
>
> Merge remote-tracking branch 'remotes/lalrae/tags/mips-20150813' into staging (2015-08-13 17:47:44 +0100)
>
> are available in the git repository at:
>
>
> git@github.com:codyprime/qemu-kvm-jtc.git tags/block-pull-request
>
> for you to fetch changes up to e424aff5f307227b1c2512bbb8ece891bb895cef:
>
> mirror: Fix coroutine reentrance (2015-08-14 09:51:31 -0400)
>
> ----------------------------------------------------------------
> Block job patches
> ----------------------------------------------------------------
>
> Kevin Wolf (1):
> mirror: Fix coroutine reentrance
>
> Stefan Hajnoczi (1):
> block/mirror: limit qiov to IOV_MAX elements
Your pull req tag has not only these two commits in it,
but also a merge commit ("Merge branch 'block-next' into HEAD).
Why is that?
thanks
-- PMM
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PULL 0/2] Block job patches
2015-08-14 14:51 ` [Qemu-devel] [PULL 0/2] Block job patches Peter Maydell
@ 2015-08-14 14:55 ` Jeff Cody
2015-08-14 17:06 ` Peter Maydell
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Cody @ 2015-08-14 14:55 UTC (permalink / raw
To: Peter Maydell; +Cc: QEMU Developers, Qemu-block
On Fri, Aug 14, 2015 at 03:51:03PM +0100, Peter Maydell wrote:
> On 14 August 2015 at 14:57, Jeff Cody <jcody@redhat.com> wrote:
> > The following changes since commit be1f13ac9d9fc21908975460652a72f5f0c018c5:
> >
> > Merge remote-tracking branch 'remotes/lalrae/tags/mips-20150813' into staging (2015-08-13 17:47:44 +0100)
> >
> > are available in the git repository at:
> >
> >
> > git@github.com:codyprime/qemu-kvm-jtc.git tags/block-pull-request
> >
> > for you to fetch changes up to e424aff5f307227b1c2512bbb8ece891bb895cef:
> >
> > mirror: Fix coroutine reentrance (2015-08-14 09:51:31 -0400)
> >
> > ----------------------------------------------------------------
> > Block job patches
> > ----------------------------------------------------------------
> >
> > Kevin Wolf (1):
> > mirror: Fix coroutine reentrance
> >
> > Stefan Hajnoczi (1):
> > block/mirror: limit qiov to IOV_MAX elements
>
> Your pull req tag has not only these two commits in it,
> but also a merge commit ("Merge branch 'block-next' into HEAD).
> Why is that?
>
> thanks
> -- PMM
Hi,
I was trying to keep a commit id stable (for 'block/mirror: limit qiov
to IOV_MAX elements'), so it could be used for a downstream backport
before this patch actually hit the official upstream repo.
Does this cause you any issues (i.e., should I rebase and submit a new
pull request)?
Thanks,
Jeff
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PULL 0/2] Block job patches
2015-08-14 14:55 ` Jeff Cody
@ 2015-08-14 17:06 ` Peter Maydell
0 siblings, 0 replies; 6+ messages in thread
From: Peter Maydell @ 2015-08-14 17:06 UTC (permalink / raw
To: Jeff Cody; +Cc: QEMU Developers, Qemu-block
On 14 August 2015 at 15:55, Jeff Cody <jcody@redhat.com> wrote:
> On Fri, Aug 14, 2015 at 03:51:03PM +0100, Peter Maydell wrote:
>> Your pull req tag has not only these two commits in it,
>> but also a merge commit ("Merge branch 'block-next' into HEAD).
>> Why is that?
> I was trying to keep a commit id stable (for 'block/mirror: limit qiov
> to IOV_MAX elements'), so it could be used for a downstream backport
> before this patch actually hit the official upstream repo.
>
> Does this cause you any issues (i.e., should I rebase and submit a new
> pull request)?
No particular problems, I just like to flag up things I'm not
expecting in case they're accidental. Following discussion
on IRC, I've merged this and pushed it to master.
thanks
-- PMM
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-08-14 17:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-08-14 13:57 [Qemu-devel] [PULL 0/2] Block job patches Jeff Cody
2015-08-14 13:57 ` [Qemu-devel] [PULL 1/2] block/mirror: limit qiov to IOV_MAX elements Jeff Cody
2015-08-14 13:57 ` [Qemu-devel] [PULL 2/2] mirror: Fix coroutine reentrance Jeff Cody
2015-08-14 14:51 ` [Qemu-devel] [PULL 0/2] Block job patches Peter Maydell
2015-08-14 14:55 ` Jeff Cody
2015-08-14 17:06 ` Peter Maydell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.