[Qemu-devel] Re: [PATCH 06/13] Threadlet: Add dequeue_work threadlet API

[Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>]

On Tue, Jan 04, 2011 at 10:57:39AM +0530, Arun R Bharadwaj wrote:
> @@ -574,33 +574,39 @@ static void paio_remove(struct qemu_paiocb *acb)
>      }
>  }
>  
> -static void paio_cancel(BlockDriverAIOCB *blockacb)
> +/**
> + * dequeue_work: Cancel a task queued on the global queue.
> + * @work: Contains the information of the task that needs to be cancelled.
> + *
> + * Returns: 0 if the task is successfully cancelled.
> + *          1 otherwise.
> + */
> +static int dequeue_work(ThreadletWork *work)
>  {
> -    struct qemu_paiocb *acb = (struct qemu_paiocb *)blockacb;
> -    int active = 0;
> +    int ret = 1;
>  
>      qemu_mutex_lock(&globalqueue.lock);
> -    if (!acb->active) {
> -        QTAILQ_REMOVE(&globalqueue.request_list, &acb->work, node);
> -        acb->ret = -ECANCELED;
> -    } else if (acb->ret == -EINPROGRESS) {
> -        active = 1;
> -    }
> +    QTAILQ_REMOVE(&globalqueue.request_list, work, node);
> +    ret = 0;
>      qemu_mutex_unlock(&globalqueue.lock);
>  
> -    qemu_mutex_lock(&aiocb_mutex);
> -    if (!active) {
> -        acb->ret = -ECANCELED;
> -    } else {
> -        while (acb->ret == -EINPROGRESS) {
> -            /*
> -             * fail safe: if the aio could not be canceled,
> -             * we wait for it
> -             */
> -            qemu_cond_wait(&aiocb_completion, &aiocb_mutex);
> +    return ret;

It always succeeds?  Why bother with the ret local variable?

> +}
> +
> +static void paio_cancel(BlockDriverAIOCB *blockacb)
> +{
> +    struct qemu_paiocb *acb = (struct qemu_paiocb *)blockacb;
> +    if (!acb->active) {
> +        if (dequeue_work(&acb->work) != 0) {
> +            /* Wait for running work item to complete */
> +            qemu_mutex_lock(&aiocb_mutex);
> +            while (acb->ret == -EINPROGRESS) {
> +                qemu_cond_wait(&aiocb_completion, &aiocb_mutex);
> +            }
> +            qemu_mutex_unlock(&aiocb_mutex);
>          }
>      }
> -    qemu_mutex_unlock(&aiocb_mutex);
> +
>      paio_remove(acb);

I'm not convinced this function works.  If the request is active in a
worker thread and paio_cancel() is called then we invoke paio_remove().

Stefan