Am 03.02.2014 um 22:51 hat Benoît Canet geschrieben:
> From: Benoît Canet <ben...@irqsave.net>
>
> Signed-off-by: Benoit Canet <ben...@irqsave.net>
> Reviewed-by: Max Reitz <mre...@redhat.com>
> ---
> 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
