[Qemu-devel] [PATCH] raw/linux-aio: Also initialize POSIX AIO

[Qemu-devel] [PATCH] raw/linux-aio: Also initialize POSIX AIO

[Kevin Wolf]

When using Linux AIO raw still falls back to POSIX AIO sometimes, so we should
initialize it.

Not initializing it happens to work if POSIX AIO is used by another drive, or
if the format is not specified (probing the format uses POSIX AIO) or by pure
luck (e.g. it doesn't seem to happen any more with qcow2 since we have re-added
synchronous qcow2 functions).

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/raw-posix.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 20b37a7..5547fb5 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -173,6 +173,10 @@ static int raw_open_common(BlockDriverState *bs, const char *filename,
 #ifdef CONFIG_LINUX_AIO
     if ((bdrv_flags & (BDRV_O_NOCACHE|BDRV_O_NATIVE_AIO)) ==
                       (BDRV_O_NOCACHE|BDRV_O_NATIVE_AIO)) {
+
+        /* We're falling back to POSIX AIO in some cases */
+        paio_init();
+
         s->aio_ctx = laio_init();
         if (!s->aio_ctx) {
             goto out_free_buf;
-- 
1.6.2.5

[Qemu-devel] Re: [PATCH] raw/linux-aio: Also initialize POSIX AIO

[Kevin Wolf]

Am 20.10.2009 11:33, schrieb Kevin Wolf:
> When using Linux AIO raw still falls back to POSIX AIO sometimes, so we should
> initialize it.
> 
> Not initializing it happens to work if POSIX AIO is used by another drive, or
> if the format is not specified (probing the format uses POSIX AIO) or by pure
> luck (e.g. it doesn't seem to happen any more with qcow2 since we have re-added
> synchronous qcow2 functions).

On that note, falling back to POSIX AIO means that paio_submit is called
with a Linux AIO aio_ctx. Which works because this parameter is unused
anyway, but am I the only one to find this ugly?

What is the public interface of paio_submit meant to look like at all?
If aio_ctx is guaranteed to be unused, why not drop it or pass NULL at
least? And if it could be used some time in the future, the raw block
driver needs to be fixed.

That said, I don't even think that the raw block driver is the right
place to distinguish between different AIO variants. Having a generic
aio_submit that calls the right AIO driver depending on the context
would be much cleaner. This would also mean that laio_submit handles the
fallback to paio_submit on its own, which I think is much cleaner than
teaching raw about the capabilities of each driver.

Does this make sense or is there this little detail that is too easy to
miss?

Kevin

[Qemu-devel] Re: [PATCH] raw/linux-aio: Also initialize POSIX AIO

[Christoph Hellwig]

On Tue, Oct 20, 2009 at 12:12:55PM +0200, Kevin Wolf wrote:
> On that note, falling back to POSIX AIO means that paio_submit is called
> with a Linux AIO aio_ctx. Which works because this parameter is unused
> anyway, but am I the only one to find this ugly?
> 
> What is the public interface of paio_submit meant to look like at all?
> If aio_ctx is guaranteed to be unused, why not drop it or pass NULL at
> least? And if it could be used some time in the future, the raw block
> driver needs to be fixed.

Agreed.  Cared to send a patch?

> That said, I don't even think that the raw block driver is the right
> place to distinguish between different AIO variants. Having a generic
> aio_submit that calls the right AIO driver depending on the context
> would be much cleaner. This would also mean that laio_submit handles the
> fallback to paio_submit on its own, which I think is much cleaner than
> teaching raw about the capabilities of each driver.

Seems a bit overkill until we get even more AIO variants at least.  And
yes, that whole area is really ugly.

[Qemu-devel] Re: [PATCH] raw/linux-aio: Also initialize POSIX AIO

[Kevin Wolf]

Am 22.10.2009 10:31, schrieb Christoph Hellwig:
> On Tue, Oct 20, 2009 at 12:12:55PM +0200, Kevin Wolf wrote:
>> On that note, falling back to POSIX AIO means that paio_submit is called
>> with a Linux AIO aio_ctx. Which works because this parameter is unused
>> anyway, but am I the only one to find this ugly?
>>
>> What is the public interface of paio_submit meant to look like at all?
>> If aio_ctx is guaranteed to be unused, why not drop it or pass NULL at
>> least? And if it could be used some time in the future, the raw block
>> driver needs to be fixed.
> 
> Agreed.  Cared to send a patch?

Will do so if we agree to do it this way instead of doing the overkill
suggested below.

>> That said, I don't even think that the raw block driver is the right
>> place to distinguish between different AIO variants. Having a generic
>> aio_submit that calls the right AIO driver depending on the context
>> would be much cleaner. This would also mean that laio_submit handles the
>> fallback to paio_submit on its own, which I think is much cleaner than
>> teaching raw about the capabilities of each driver.
> 
> Seems a bit overkill until we get even more AIO variants at least.  And
> yes, that whole area is really ugly.

Yes, it might look like overkill to introduce a abstraction for exactly
two backends. I felt the same way. But then, the current implementation
just feels totally wrong. It absolutely intransparent when we fall back
to paio, and before debugging the bdrv_read/write emulation I didn't
even know that we're doing it. And, like I said, why should a block
format driver know what AIO method works which way?

I'm not insisting on restructuring though if you think that the effort
would outweigh the benefit of a cleanup.

Kevin

[Qemu-devel] Re: [PATCH] raw/linux-aio: Also initialize POSIX AIO

[Christoph Hellwig]

On Thu, Oct 22, 2009 at 11:05:55AM +0200, Kevin Wolf wrote:
> Yes, it might look like overkill to introduce a abstraction for exactly
> two backends. I felt the same way. But then, the current implementation
> just feels totally wrong. It absolutely intransparent when we fall back
> to paio, and before debugging the bdrv_read/write emulation I didn't
> even know that we're doing it. And, like I said, why should a block
> format driver know what AIO method works which way?

Because the aio method is part of the block driver.  Despite our
code organization linux-aio.c and compat-posix-aio.c aren't generic
abstractions but sub-modules of raw-posix.  They would be better of
beeing renamed to block/raw-posix-aio-linux.c and
block/raw-posix-aio-pthreads.c, but with the latency of getting patches
into qemu that would just make developing any block code a nightmare
while those patches are in the queue.

[Qemu-devel] Re: [PATCH] raw/linux-aio: Also initialize POSIX AIO

[Kevin Wolf]

Am 25.10.2009 08:19, schrieb Christoph Hellwig:
> On Thu, Oct 22, 2009 at 11:05:55AM +0200, Kevin Wolf wrote:
>> Yes, it might look like overkill to introduce a abstraction for exactly
>> two backends. I felt the same way. But then, the current implementation
>> just feels totally wrong. It absolutely intransparent when we fall back
>> to paio, and before debugging the bdrv_read/write emulation I didn't
>> even know that we're doing it. And, like I said, why should a block
>> format driver know what AIO method works which way?
> 
> Because the aio method is part of the block driver.  Despite our
> code organization linux-aio.c and compat-posix-aio.c aren't generic
> abstractions but sub-modules of raw-posix.

Well, my question was not if they are sub-modules of raw-posix - they
clearly are - but rather if they should be.

But ok, I'll just submit a patch drop the context parameter in paio_* then.

Kevin

[Qemu-devel] Re: [PATCH] raw/linux-aio: Also initialize POSIX AIO

[Christoph Hellwig]

On Mon, Oct 26, 2009 at 09:36:55AM +0100, Kevin Wolf wrote:
> Well, my question was not if they are sub-modules of raw-posix - they
> clearly are - but rather if they should be.

linux-aio is just a trvial wrapper for the native Linux AIO abi to be
used by QEMU, I can't see why it wouldn't.  posix-aio-compat is a bit
more involved and I could see the point of splitting out the thread pool
implementation for other uses, but until we actually have another user
it's rather pointless.