Re: [Qemu-devel] [PATCH 2/2] block: Handle multiwrite errors only when all requests have completed

[Stefan Hajnoczi]

On Thu, Jul 1, 2010 at 3:31 PM, Kevin Wolf <address@hidden> wrote:
> Don't try to be clever by freeing all temporary data and calling all callbacks
> when the return value (an error) is certain. Doing so has at least two
> important problems:
>
> * The temporary data that is freed (qiov, possibly zero buffer) is still used
>  by the requests that have not yet completed.
> * Calling the callbacks for all requests in the multiwrite means for the 
> caller
>  that it may free buffers etc. which are still in use.
>
> Just remember the error value and do the cleanup when all requests have
> completed.
>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block.c |    5 +----
>  1 files changed, 1 insertions(+), 4 deletions(-)
>
> diff --git a/block.c b/block.c
> index 9719649..bff7d5a 100644
> --- a/block.c
> +++ b/block.c
> @@ -2056,14 +2056,11 @@ static void multiwrite_cb(void *opaque, int ret)
>
>     if (ret < 0 && !mcb->error) {
>         mcb->error = ret;
> -        multiwrite_user_cb(mcb);
>     }
>
>     mcb->num_requests--;
>     if (mcb->num_requests == 0) {
> -        if (mcb->error == 0) {
> -            multiwrite_user_cb(mcb);
> -        }
> +        multiwrite_user_cb(mcb);

I am a little confused at this point.  Here is the meat of
bdrv_aio_multiwrite() from the kevin/devel branch:

// Run the aio requests
mcb->num_requests = num_reqs;
for (i = 0; i < num_reqs; i++) {
    acb = bdrv_aio_writev(bs, reqs[i].sector, reqs[i].qiov,
        reqs[i].nb_sectors, multiwrite_cb, mcb);

    if (acb == NULL) {
        // We can only fail the whole thing if no request has been
        // submitted yet. Otherwise we'll wait for the submitted AIOs to
        // complete and report the error in the callback.
        if (i == 0) {
            reqs[i].error = -EIO;
            goto fail;
        } else {
            multiwrite_cb(mcb, -EIO);
            break;
        }
    }
}

If bdrv_aio_write() fails on request 2 out of 3, then
mcb->num_requests = 3 but only 2 requests were issued before breaking
the loop.

Now the 2 issued requests complete and call multiwrite_cb().  Since
was mcb->num_requests = 3, it never reaches zero and neither
multiwrite_user_cb(mcb) nor qemu_free(mcb) are ever called.

Is it safe to adjust mcb->num_requests = i before calling
multiwrite_cb() and breaking the loop in the failure case?  That way
the num->num_requests will reach zero.

I hesitate a little because previous requests could have completed,
multiwrite_cb() was called, and mcb->num_requests was decremented?
Then the value of i cannot be used for mcb->num_requests because
previous requests it counts have already completed.

Stefan