Am 24.11.2016 um 11:58 hat Pavel Butsykin geschrieben:
On 23.11.2016 17:28, Kevin Wolf wrote:
Am 15.11.2016 um 07:36 hat Pavel Butsykin geschrieben:
It's just byte-based wrappers over bdrv_co_aio_prw_vector(), which provide
a byte-based interface for AIO read/write.
Signed-off-by: Pavel Butsykin <address@hidden>
I'm in the process to phase out the last users of bdrv_aio_*() so that
this set of interfaces can be removed. I'm doing this because it's an
unnecessary redundancy, we have too many wrapper functions that expose
the same functionality with different syntax. So let's not add new
users.
At first sight, you don't even seem to use bdrv_aio_preadv() for actual
parallelism, but you often have a pattern like this:
void foo_cb(void *opaque)
{
...
qemu_coroutine_enter(acb->co);
}
void caller()
{
...
acb = bdrv_aio_preadv(...);
qemu_coroutine_yield();
}
The code will actually become a lot simpler if you use bdrv_co_preadv()
instead because you don't have to have a callback, but you get pure
sequential code.
The part that actually has some parallelism, pcache_readahead_request(),
already creates its own coroutine, so it runs in the background without
using callback-style interfaces.
I used bdrv_co_preadv(), because it conveniently solves the partial
cache hit. To solve the partial cache hit, we need to split a request
into smaller parts, make asynchronous requests and wait for all
requests in one place.
Do you propose to create a coroutine for each part of request? It
seemed to me that bdrv_co_preadv() is a wrapper that allows us to get
rid of the same code.
It's actually the other way round, bdrv_co_preadv() is the "native"
block layer API, and bdrv_aio_*() are wrappers providing an alternative
interface.
I'm looking at pcache_co_readahead(), for example. It looks like this:
bdrv_aio_preadv(bs->file, node->common.offset, &readahead_acb.qiov,
node->common.bytes, pcache_aio_readahead_cb,
&readahead_acb);
qemu_coroutine_yield();
And then we have pcache_aio_readahead_cb(), which ends in:
qemu_coroutine_enter(acb->co);
So here the callback style doesn't buy you anything, it just rips the
code apart in two function. There is no parallelism here anyway,
pcache_co_readahead() doesn't do anything until the callback reenters
it. This is a very obvious example where bdrv_co_preadv() will simplify
the code.
It's similar with the other bdrv_aio_preadv() calls, which are in
pcache_co_preadv():
if (bytes > s->max_aio_size) {
bdrv_aio_preadv(bs->file, offset, qiov, bytes,
pcache_aio_read_cb, &acb);
goto out;
}
update_req_stats(s->req_stats, offset, bytes);
status = pcache_lookup_data(&acb);
if (status == CACHE_MISS) {
bdrv_aio_preadv(bs->file, offset, qiov, bytes,
pcache_aio_read_cb, &acb);
} else if (status == PARTIAL_CACHE_HIT) {
assert(acb.part.qiov.niov != 0);
bdrv_aio_preadv(bs->file, acb.part.offset, &acb.part.qiov,
acb.part.bytes, pcache_aio_read_cb, &acb);
}
pcache_readahead_request(&acb);
if (status == CACHE_HIT && --acb.ref == 0) {
return 0;
}
out:
qemu_coroutine_yield();
Here you have mainly the pcache_readahead_request() call between
bdrv_aio_preadv() and the yield. It only spawns a new coroutine, which
works in the background, so I think you can move it to before the reads
and then the reads can trivially become bdrv_co_preadv() and the
callback can again be inlined instead of ripping the function in two
parts.
The bdrv_aio_pwritev() call in pcache_co_pwritev() is just the same
thing and using the coroutine version results in obvious code
improvements.
And I think this are all uses of bdrv_aio_*() in the pcache driver, so
converting it to use bdrv_co_*() instead isn't only possible, but will
improve the legibility of your code, too. It's a clear win in all three
places.