All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kevin Wolf <kwolf@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, eesposit@redhat.com
Subject: Re: [PATCH 07/15] block-backend: enter aio coroutine only after drain
Date: Mon, 16 Jan 2023 16:57:10 +0100	[thread overview]
Message-ID: <Y8Vz1pgvIHt769c4@redhat.com> (raw)
In-Reply-To: <20221212125920.248567-8-pbonzini@redhat.com>

Am 12.12.2022 um 13:59 hat Paolo Bonzini geschrieben:
> When called from within (another) coroutine, aio_co_enter will not
> enter a coroutine immediately; instead the new coroutine is scheduled
> to run after qemu_coroutine_yield().  This however might cause the
> currently-running coroutine to yield without having raised blk->in_flight.

I assume you're talking about the blk_aio_prwv() path here. However,
calling blk_inc_in_flight() is the very first thing it does (before even
calling bdrv_coroutine_enter -> aio_co_enter), so I don't understand how
it could happen that it yields before increasing the counter.

> If it was a ->drained_begin() callback who scheduled the coroutine,

Which one? The one that executes blk_aio_prwv()?

> bdrv_drained_begin() might exit without waiting for the I/O operation
> to finish.  Right now, this is masked by unnecessary polling done by
> bdrv_drained_begin() after the callbacks return, but it is wrong and
> a latent bug.
> 
> So, ensure that blk_inc_in_flight() and blk_wait_while_drained()
> are called before aio_co_enter().  To do so, pull the call to
> blk_wait_while_drained() out of the blk_co_do_* functions, which are
> called from the AIO coroutines, and place them separately in the public
> blk_co_* functions and in blk_aio_prwv.

You can't call blk_wait_while_drained() in blk_aio_prwv() because the
latter isn't a coroutine_fn.

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/block-backend.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)

Kevin



  reply	other threads:[~2023-01-16 15:58 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-12-12 12:59 [PATCH 00/12] More cleanups and fixes for drain Paolo Bonzini
2022-12-12 12:59 ` [PATCH 01/15] Revert "block: Remove poll parameter from bdrv_parent_drained_begin_single()" Paolo Bonzini
2022-12-12 12:59 ` [PATCH 02/15] Revert "block: Don't poll in bdrv_replace_child_noperm()" Paolo Bonzini
2022-12-12 12:59 ` [PATCH 03/15] block: Pull polling out of bdrv_parent_drained_begin_single() Paolo Bonzini
2022-12-12 12:59 ` [PATCH 04/15] test-bdrv-drain.c: remove test_detach_by_parent_cb() Paolo Bonzini
2022-12-12 12:59 ` [PATCH 05/15] tests/unit/test-bdrv-drain.c: graph setup functions can't run in coroutines Paolo Bonzini
2022-12-12 12:59 ` [PATCH 06/15] tests/qemu-iotests/030: test_stream_parallel should use auto_finalize=False Paolo Bonzini
2022-12-12 12:59 ` [PATCH 07/15] block-backend: enter aio coroutine only after drain Paolo Bonzini
2023-01-16 15:57   ` Kevin Wolf [this message]
2022-12-12 12:59 ` [PATCH 08/15] nbd: a BlockExport always has a BlockBackend Paolo Bonzini
2022-12-12 12:59 ` [PATCH 09/15] block-backend: make global properties write-once Paolo Bonzini
2022-12-12 12:59 ` [PATCH 10/15] block-backend: always wait for drain before starting operation Paolo Bonzini
2023-01-16 16:48   ` Kevin Wolf
2022-12-12 12:59 ` [PATCH 11/15] block-backend: make queued_requests thread-safe Paolo Bonzini
2023-01-11 20:44   ` Stefan Hajnoczi
2023-01-16 16:55   ` Kevin Wolf
2022-12-12 12:59 ` [PATCH 12/15] block: limit bdrv_co_yield_to_drain to drain_begin Paolo Bonzini
2022-12-12 12:59 ` [PATCH 13/15] block: second argument of bdrv_do_drained_end is always NULL Paolo Bonzini
2022-12-12 12:59 ` [PATCH 14/15] block: second argument of bdrv_do_drained_begin and bdrv_drain_poll " Paolo Bonzini
2022-12-12 12:59 ` [PATCH 15/15] block: only get out of coroutine context for polling Paolo Bonzini
2023-01-16 15:51 ` [PATCH 00/12] More cleanups and fixes for drain Kevin Wolf
2023-01-16 17:25 ` Kevin Wolf

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Y8Vz1pgvIHt769c4@redhat.com \
    --to=kwolf@redhat.com \
    --cc=eesposit@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.