qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Emanuele Giuseppe Esposito <eesposit@redhat.com>
To: Stefan Hajnoczi <stefanha@redhat.com>, qemu-devel@nongnu.org
Cc: "Michael S. Tsirkin" <mst@redhat.com>,
	qemu-block@nongnu.org, Kevin Wolf <kwolf@redhat.com>,
	Hanna Reitz <hreitz@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>, Fam Zheng <fam@euphon.net>
Subject: Re: [PATCH 7/8] virtio-blk: don't acquire AioContext in virtio_blk_handle_vq()
Date: Fri, 11 Nov 2022 13:41:03 +0100	[thread overview]
Message-ID: <449f74e7-7108-337e-b8e5-e21cb3ba8dcc@redhat.com> (raw)
In-Reply-To: <20221108211930.876142-8-stefanha@redhat.com>



Am 08/11/2022 um 22:19 schrieb Stefan Hajnoczi:
> There is no need to acquire AioContext in virtio_blk_handle_vq() because
> no APIs used in the function require it and nothing else in the
> virtio-blk code requires mutual exclusion anymore.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/block/virtio-blk.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index faea045178..771d87cfbe 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -784,7 +784,6 @@ void virtio_blk_handle_vq(VirtIOBlock *s, VirtQueue *vq)
>  
>      IO_CODE();
>  
> -    aio_context_acquire(blk_get_aio_context(s->blk));
>      blk_io_plug(s->blk);
>  
>      do {
> @@ -810,7 +809,6 @@ void virtio_blk_handle_vq(VirtIOBlock *s, VirtQueue *vq)
>      }
>  
>      blk_io_unplug(s->blk);
> -    aio_context_release(blk_get_aio_context(s->blk));
>  }
>  
>  static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> 

As already discussed offline, this might be problematic with the work I
am done.

Basically I am trying to replace the AioContext lock with a rwlock for
graph modifications, and in order to use it we must convert all
BlockDriver IO functions in coroutines, because they traverse the graph
and must take the read lock (defined as coroutine_fn).

This implies that for now we need to implement blk_* and bdrv_*
functions in a similar fashion as generated_co_wrapper, therefore
creating a coroutine and polling waiting for it.
And polling uses AIO_WAIT_WHILE, which assumes the AioContext lock to be
taken.

In the future, we will use AIO_WAIT_WHILE_UNLOCKED, as you did in patch
1, but right now it's definitely too early to do it for a g_c_w function.

For this specific case, I see that blk_ioplug/unplug is not called in a
lot of places:
- here, and it's ok
- virtio-scsi, and I think you are going to cover this too soon
- xen-block

So if you manage to make all callers aiocontext-free, then I can just
rebase on top of your series and use AIO_WAIT_WHILE_POLL for
blk_ioplug/unplug when I convert it in coroutine.

Thank you,
Emanuele



  reply	other threads:[~2022-11-11 12:42 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-08 21:19 [PATCH 0/8] virtio-blk: remove AioContext lock Stefan Hajnoczi
2022-11-08 21:19 ` [PATCH 1/8] virtio_queue_aio_attach_host_notifier: " Stefan Hajnoczi
2022-11-11 12:17   ` Emanuele Giuseppe Esposito
2022-11-08 21:19 ` [PATCH 2/8] block-backend: enable_write_cache should be atomic Stefan Hajnoczi
2022-11-11 12:17   ` Emanuele Giuseppe Esposito
2022-11-08 21:19 ` [PATCH 3/8] virtio: categorize callbacks in GS Stefan Hajnoczi
2022-11-11 12:19   ` Emanuele Giuseppe Esposito
2022-11-08 21:19 ` [PATCH 4/8] virtio-blk: mark GLOBAL_STATE_CODE functions Stefan Hajnoczi
2022-11-11 12:19   ` Emanuele Giuseppe Esposito
2022-11-08 21:19 ` [PATCH 5/8] virtio-blk: mark IO_CODE functions Stefan Hajnoczi
2022-11-11 12:20   ` Emanuele Giuseppe Esposito
2022-11-08 21:19 ` [PATCH 6/8] virtio-blk: remove unnecessary AioContext lock from function already safe Stefan Hajnoczi
2022-11-11 12:23   ` Emanuele Giuseppe Esposito
2022-11-08 21:19 ` [PATCH 7/8] virtio-blk: don't acquire AioContext in virtio_blk_handle_vq() Stefan Hajnoczi
2022-11-11 12:41   ` Emanuele Giuseppe Esposito [this message]
2022-11-08 21:19 ` [PATCH 8/8] virtio-blk: minimize virtio_blk_reset() AioContext lock region Stefan Hajnoczi
2022-11-11 12:41   ` Emanuele Giuseppe Esposito

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=449f74e7-7108-337e-b8e5-e21cb3ba8dcc@redhat.com \
    --to=eesposit@redhat.com \
    --cc=fam@euphon.net \
    --cc=hreitz@redhat.com \
    --cc=kwolf@redhat.com \
    --cc=mst@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=stefanha@redhat.com \
    /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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).