From: Peter Lieven <pl@kamp.de>
To: Kevin Wolf <kwolf@redhat.com>
Cc: "Daniel P. Berrange" <berrange@redhat.com>,
qemu-block <qemu-block@nongnu.org>,
Christian Theune <ct@flyingcircus.io>,
qemu-devel@nongnu.org, Max Reitz <mreitz@redhat.com>,
Paolo Bonzini <pbonzini@redhat.com>,
Ilya Dryomov <idryomov@gmail.com>
Subject: Re: [PATCH v5 0/6] block/rbd: migrate to coroutines and add write zeroes support
Date: Wed, 7 Jul 2021 20:13:54 +0200 [thread overview]
Message-ID: <AE0414CF-3FC8-44DF-A453-671DDE9CFAD5@kamp.de> (raw)
In-Reply-To: <YOR14GzLqr3EKzcm@redhat.com>
> Am 06.07.2021 um 17:25 schrieb Kevin Wolf <kwolf@redhat.com>:
>
> Am 06.07.2021 um 16:55 hat Peter Lieven geschrieben:
>>> Am 06.07.2021 um 15:19 schrieb Kevin Wolf <kwolf@redhat.com>:
>>>
>>> Am 02.07.2021 um 19:23 hat Ilya Dryomov geschrieben:
>>>> This series migrates the qemu rbd driver from the old aio emulation
>>>> to native coroutines and adds write zeroes support which is important
>>>> for block operations.
>>>>
>>>> To achieve this we first bump the librbd requirement to the already
>>>> outdated luminous release of ceph to get rid of some wrappers and
>>>> ifdef'ry in the code.
>>>
>>> Thanks, applied to the block branch.
>>>
>>> I've only had a very quick look at the patches, but I think there is one
>>> suggestion for a cleanup I can make: The qemu_rbd_finish_bh()
>>> indirection is probably unnecessary now because aio_co_wake() is thread
>>> safe.
>>
>> But this is new, isn’t it?
>
> Not sure in what sense you mean. aio_co_wake() has always been thread
> safe, as far as I know.
>
> Obviously, the old code didn't use aio_co_wake(), but directly called
> some callbacks, so the part that is new is your coroutine conversion
> that enables getting rid of the BH.
>
>> We also have this indirection in iscsi and nfs drivers I think.
>
> Indeed, the resulting codes look the same. In iscsi and nfs it doesn't
> come from an incomplete converstion to coroutines, but they both used
> qemu_coroutine_enter() originally, which resumes the coroutine in the
> current thread...
>
>> Does it matter that the completion callback is called from an librbd
>> thread? Will the coroutine continue to run in the right thread?
>
> ...whereas aio_co_wake() resumes the coroutine in the thread where it
> was running before.
>
> (Before commit 5f50be9b5, this would have been buggy from an librbd
> thread, but now it should work correctly even for threads that are
> neither iothreads nor vcpu threads.)
>
>> I will have a decent look after my vacation.
>
> Sounds good, thanks. Enjoy your vacation!
As I had to fire up my laptop to look into another issue anyway, I have sent two patches for updating MAINTAINERS and to fix the int vs. bool mix for task->complete. As Paolos fix (5f50be9b5) is relatively new and there are maybe other non obvious problems when removing the BH indirection and we are close to soft freeze I would leave the BH removal change for 6.2.
Best,
Peter
next prev parent reply other threads:[~2021-07-07 18:15 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-02 17:23 [PATCH v5 0/6] block/rbd: migrate to coroutines and add write zeroes support Ilya Dryomov
2021-07-02 17:23 ` [PATCH v5 1/6] block/rbd: bump librbd requirement to luminous release Ilya Dryomov
2021-07-02 17:23 ` [PATCH v5 2/6] block/rbd: store object_size in BDRVRBDState Ilya Dryomov
2021-07-02 17:23 ` [PATCH v5 3/6] block/rbd: update s->image_size in qemu_rbd_getlength Ilya Dryomov
2021-07-02 17:23 ` [PATCH v5 4/6] block/rbd: migrate from aio to coroutines Ilya Dryomov
2021-07-02 17:23 ` [PATCH v5 5/6] block/rbd: add write zeroes support Ilya Dryomov
2021-07-02 17:23 ` [PATCH v5 6/6] block/rbd: drop qemu_rbd_refresh_limits Ilya Dryomov
2021-07-02 19:55 ` [PATCH v5 0/6] block/rbd: migrate to coroutines and add write zeroes support Ilya Dryomov
2021-07-06 13:19 ` Kevin Wolf
2021-07-06 14:55 ` Peter Lieven
2021-07-06 15:25 ` Kevin Wolf
2021-07-06 15:48 ` Peter Lieven
2021-07-07 18:13 ` Peter Lieven [this message]
2021-07-08 12:18 ` Kevin Wolf
2021-07-08 18:23 ` Peter Lieven
2021-07-09 10:21 ` Kevin Wolf
2021-09-16 12:34 ` Peter Lieven
2021-10-25 11:39 ` Peter Lieven
2021-10-25 12:58 ` Kevin Wolf
2021-10-26 14:53 ` Peter Lieven
2021-11-15 11:17 ` Peter Lieven
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=AE0414CF-3FC8-44DF-A453-671DDE9CFAD5@kamp.de \
--to=pl@kamp.de \
--cc=berrange@redhat.com \
--cc=ct@flyingcircus.io \
--cc=idryomov@gmail.com \
--cc=kwolf@redhat.com \
--cc=mreitz@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 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).