| Message ID | 1495830130-30611-2-git-send-email-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/26/2017 03:21 PM, Kevin Wolf wrote: > The qed driver serialises allocating write requests. When the active > allocation is finished, the AIO callback is called, but after this, the > next allocating request is immediately processed instead of leaving the > coroutine. Resuming another allocation request in the same request > coroutine means that the request now runs in the wrong coroutine. > > The following is one of the possible effects of this: The completed > request will generally reenter its request coroutine in a bottom half, > expecting that it completes the request in bdrv_driver_pwritev(). > However, if the second request actually yielded before leaving the > coroutine, the reused request coroutine is in an entirely different > place and is reentered prematurely. Not a good idea. > > Let's make sure that we exit the coroutine after completing the first > request by resuming the next allocating request only with a bottom > half. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block/qed.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com>
On Fri, May 26, 2017 at 10:21:42PM +0200, Kevin Wolf wrote: > The qed driver serialises allocating write requests. When the active > allocation is finished, the AIO callback is called, but after this, the > next allocating request is immediately processed instead of leaving the > coroutine. Resuming another allocation request in the same request > coroutine means that the request now runs in the wrong coroutine. > > The following is one of the possible effects of this: The completed > request will generally reenter its request coroutine in a bottom half, > expecting that it completes the request in bdrv_driver_pwritev(). > However, if the second request actually yielded before leaving the > coroutine, the reused request coroutine is in an entirely different > place and is reentered prematurely. Not a good idea. > > Let's make sure that we exit the coroutine after completing the first > request by resuming the next allocating request only with a bottom > half. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block/qed.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
diff --git a/block/qed.c b/block/qed.c index 8d899fd..a837a28 100644 --- a/block/qed.c +++ b/block/qed.c @@ -967,6 +967,11 @@ static void qed_aio_complete_bh(void *opaque) qed_release(s); } +static void qed_resume_alloc_bh(void *opaque) +{ + qed_aio_start_io(opaque); +} + static void qed_aio_complete(QEDAIOCB *acb, int ret) { BDRVQEDState *s = acb_to_s(acb); @@ -995,10 +1000,12 @@ static void qed_aio_complete(QEDAIOCB *acb, int ret) * requests multiple times but rather finish one at a time completely. */ if (acb == QSIMPLEQ_FIRST(&s->allocating_write_reqs)) { + QEDAIOCB *next_acb; QSIMPLEQ_REMOVE_HEAD(&s->allocating_write_reqs, next); - acb = QSIMPLEQ_FIRST(&s->allocating_write_reqs); - if (acb) { - qed_aio_start_io(acb); + next_acb = QSIMPLEQ_FIRST(&s->allocating_write_reqs); + if (next_acb) { + aio_bh_schedule_oneshot(bdrv_get_aio_context(acb->common.bs), + qed_resume_alloc_bh, next_acb); } else if (s->header.features & QED_F_NEED_CHECK) { qed_start_need_check_timer(s); }
The qed driver serialises allocating write requests. When the active allocation is finished, the AIO callback is called, but after this, the next allocating request is immediately processed instead of leaving the coroutine. Resuming another allocation request in the same request coroutine means that the request now runs in the wrong coroutine. The following is one of the possible effects of this: The completed request will generally reenter its request coroutine in a bottom half, expecting that it completes the request in bdrv_driver_pwritev(). However, if the second request actually yielded before leaving the coroutine, the reused request coroutine is in an entirely different place and is reentered prematurely. Not a good idea. Let's make sure that we exit the coroutine after completing the first request by resuming the next allocating request only with a bottom half. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/qed.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)