Re: [Qemu-devel] [PATCH V15 05/13] quorum: Add quorum_aio_readv.

[Kevin Wolf]

Am 03.02.2014 um 22:51 hat Benoît Canet geschrieben:
> From: Benoît Canet <address@hidden>
> 
> Signed-off-by: Benoit Canet <address@hidden>
> Reviewed-by: Max Reitz <address@hidden>
> ---
>  block/quorum.c | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/block/quorum.c b/block/quorum.c
> index 81bffdd..699b512 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -86,10 +86,19 @@ static void quorum_aio_finalize(QuorumAIOCB *acb)
>      BDRVQuorumState *s = acb->bqs;
>      int ret = 0;
>  
> +    for (i = 0; i < s->total; i++) {
> +        qemu_vfree(acb->aios[i].buf);
> +        acb->aios[i].buf = NULL;
> +        acb->aios[i].ret = 0;

You free acb->aios anyway, no point in clearing the fields.

> +    }
> +
>      acb->common.cb(acb->common.opaque, ret);
>      if (acb->finished) {
>          *acb->finished = true;
>      }
> +    for (i = 0; acb->is_read && i < s->total; i++) {

As Max noted, pulling acb->is_read into the for loop condition is
clever. Code being unnecessarily clever is usually bad.

> +        qemu_iovec_destroy(&acb->aios[i].qiov);
> +    }

Any reason to have two separate for loops? I can't see one why the qiov
should be any longer valid than the data in it. I also can't see the
reason for the different conditions, other than qemu_vfree() being a
no-op for writes.

Keep it simple:

if (acb->is_read) {
    for (i = 0; i < s->total; i++) {
        qemu_vfree(acb->aios[i].buf);
        qemu_iovec_destroy(&acb->aios[i].qiov);
    }
}

It's shorter, less clever and more readable than your version.

>      g_free(acb->aios);
>      qemu_aio_release(acb);
>  }
> @@ -145,6 +154,34 @@ static void quorum_aio_cb(void *opaque, int ret)
>      quorum_aio_finalize(acb);
>  }
>  
> +static BlockDriverAIOCB *quorum_aio_readv(BlockDriverState *bs,
> +                                         int64_t sector_num,
> +                                         QEMUIOVector *qiov,
> +                                         int nb_sectors,
> +                                         BlockDriverCompletionFunc *cb,
> +                                         void *opaque)
> +{
> +    BDRVQuorumState *s = bs->opaque;
> +    QuorumAIOCB *acb = quorum_aio_get(s, bs, qiov, sector_num,
> +                                      nb_sectors, cb, opaque);
> +    int i;
> +
> +    acb->is_read = true;
> +
> +    for (i = 0; i < s->total; i++) {
> +        acb->aios[i].buf = qemu_blockalign(bs->file, qiov->size);

Shouldn't this be s->bs[i] instead of bs->file?

> +        qemu_iovec_init(&acb->aios[i].qiov, qiov->niov);
> +        qemu_iovec_clone(&acb->aios[i].qiov, qiov, acb->aios[i].buf);
> +    }
> +
> +    for (i = 0; i < s->total; i++) {
> +        bdrv_aio_readv(s->bs[i], sector_num, qiov, nb_sectors,
> +                       quorum_aio_cb, &acb->aios[i]);
> +    }
> +
> +    return &acb->common;
> +}
> +
>  static BlockDriverAIOCB *quorum_aio_writev(BlockDriverState *bs,
>                                            int64_t sector_num,
>                                            QEMUIOVector *qiov,
> @@ -172,6 +209,7 @@ static BlockDriver bdrv_quorum = {
>  
>      .instance_size      = sizeof(BDRVQuorumState),
>  
> +    .bdrv_aio_readv     = quorum_aio_readv,
>      .bdrv_aio_writev    = quorum_aio_writev,
>  };

Kevin