Re: [Qemu-block] [RFC PATCH 6/8] quorum: Avoid bdrv_aio_writev() for rewrites

[Alberto Garcia]

On Thu 10 Nov 2016 06:19:07 PM CET, Kevin Wolf wrote:
> Replacing it with bdrv_co_pwritev() prepares us for byte granularity
> requests and gets us rid of the last bdrv_aio_*() user in quorum.
>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block/quorum.c | 52 +++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 33 insertions(+), 19 deletions(-)
>
> diff --git a/block/quorum.c b/block/quorum.c
> index b2bb3af..1426115 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -134,6 +134,11 @@ struct QuorumAIOCB {
>      int children_read;          /* how many children have been read from */
>  };
>  
> +typedef struct QuorumCo {
> +    QuorumAIOCB *acb;
> +    int i;
> +} QuorumCo;

What Eric said: this could be up here in the first patch already.

> -static void quorum_rewrite_aio_cb(void *opaque, int ret)
> -{
> -    QuorumAIOCB *acb = opaque;
> -
> -    /* one less rewrite to do */
> -    acb->rewrite_count--;
> -    qemu_coroutine_enter_if_inactive(acb->co);
> -}

You're moving this logic to quorum_rewrite_entry(), but the comment in
quorum_rewrite_bad_versions() still refers to this function.

> +static void quorum_rewrite_entry(void *opaque)
> +{
> +    QuorumCo *co = opaque;
> +    QuorumAIOCB *acb = co->acb;
> +    BDRVQuorumState *s = acb->bs->opaque;
> +    int ret;
> +
> +    ret = bdrv_co_pwritev(s->children[co->i],
> +                          acb->sector_num * BDRV_SECTOR_SIZE,
> +                          acb->nb_sectors * BDRV_SECTOR_SIZE,
> +                          acb->qiov, 0);
> +    (void) ret;

Why do you need 'ret' at all? If it's a placeholder to remind us to do
something with this value in the future, you can simply add a FIXME
comment.

> +    /* one less rewrite to do */
> +    acb->rewrite_count--;
> +    qemu_coroutine_enter_if_inactive(acb->co);

I think you should only enter acb->co when acb->rewrite_count reaches
zero.

In all other cases the main coroutine simply iterates inside the while()
loop, verifies that the number is still positive and yields again.

The same applies to all other cases of qemu_coroutine_enter_if_inactive,
by the way (I failed to notice it in patch #5).

Berto