Re: [Qemu-block] [PATCH 4/6] raw-posix: Switch to bdrv_co_* interfaces

[Eric Blake]

On 06/08/2016 08:10 AM, Kevin Wolf wrote:
> In order to use the modern byte-based .bdrv_co_preadv/pwritev()
> interface, this patch switches raw-posix to coroutine-based interfaces
> as a first step. In terms of semantics and performance, it doesn't make
> a difference with the existing code whether we go from a coroutine to a
> callback-based interface already in block/io.c or only in linux-aio.c
> 
> As there have been concerns in the past that this change may be a step
> in the wrong direction with respect to a possible AIO fast path, the
> old callback-based interface for linux-aio is left around and can be
> reactivated when a fast path (e.g. directly from virtio-blk dataplane,
> bypassing the whole block layer) is implemented.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block/linux-aio.c | 85 
> +++++++++++++++++++++++++++++++++++++++++--------------
>  block/raw-aio.h   |  2 ++
>  block/raw-posix.c | 59 ++++++++++++++++++--------------------
>  3 files changed, 92 insertions(+), 54 deletions(-)
> 

> +
> +int laio_submit_co(BlockDriverState *bs, LinuxAioState *s, int fd,
> +        int64_t sector_num, QEMUIOVector *qiov, int nb_sectors, int type)
> +{
> +    off_t offset = sector_num * 512;

BDRV_SECTOR_SIZE instead of a magic number?

> +    int ret;
> +
> +    struct qemu_laiocb laiocb = {
> +        .co         = qemu_coroutine_self(),
> +        .nbytes     = nb_sectors * 512,

and again


> +
> +BlockAIOCB *laio_submit(BlockDriverState *bs, LinuxAioState *s, int fd,
> +        int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
> +        BlockCompletionFunc *cb, void *opaque, int type)
> +{
> +    struct qemu_laiocb *laiocb;
> +    off_t offset = sector_num * 512;
> +    int ret;
> +
> +    laiocb = qemu_aio_get(&laio_aiocb_info, bs, cb, opaque);
> +    laiocb->nbytes = nb_sectors * 512;

and again


> @@ -1345,14 +1344,26 @@ static BlockAIOCB *raw_aio_submit(BlockDriverState 
> *bs,
>              type |= QEMU_AIO_MISALIGNED;
>  #ifdef CONFIG_LINUX_AIO
>          } else if (s->use_aio) {
> -            return laio_submit(bs, s->aio_ctx, s->fd, sector_num, qiov,
> -                               nb_sectors, cb, opaque, type);
> +            return laio_submit_co(bs, s->aio_ctx, s->fd, sector_num, qiov,
> +                               nb_sectors, type);

Indentation is now off


> @@ -1957,8 +1952,8 @@ BlockDriver bdrv_file = {
>      .bdrv_co_get_block_status = raw_co_get_block_status,
>      .bdrv_co_pwrite_zeroes = raw_co_pwrite_zeroes,
>  
> -    .bdrv_aio_readv = raw_aio_readv,
> -    .bdrv_aio_writev = raw_aio_writev,
> +    .bdrv_co_readv          = raw_co_readv,
> +    .bdrv_co_writev         = raw_co_writev,
>      .bdrv_aio_flush = raw_aio_flush,

Why bother with indented alignment of '=' when none of the neighbors do?

Findings are minor enough, so up to you whether to fix them or just mark
this:

Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature