| Message ID | 1256031192-8292-1-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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.
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
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.
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;
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(-)