scsi: always call notifier on async cancellation

This was found by code inspection.  If the request is cancelled twice,
the notifier is never called on the second cancellation request,
and hence for example a TMF might never finish.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-bus.c | 3 ---
 1 file changed, 3 deletions(-)

On Wed, 12/16 19:33, Paolo Bonzini wrote:
> This was found by code inspection.  If the request is cancelled twice,
> the notifier is never called on the second cancellation request,
> and hence for example a TMF might never finish.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/scsi/scsi-bus.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index 524a998..4c121fe 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -1759,9 +1759,6 @@ void scsi_req_cancel_async(SCSIRequest *req, Notifier *notifier)
>      if (notifier) {
>          notifier_list_add(&req->cancel_notifiers, notifier);
>      }
> -    if (req->io_canceled) {
> -        return;
> -    }
>      scsi_req_ref(req);
>      scsi_req_dequeue(req);
>      req->io_canceled = true;
       if (req->aiocb) {
           blk_aio_cancel_async(req->aiocb);
       } else {
           scsi_req_cancel_complete(req);
       }

A second TMF must be blk_aio_cancel_async case, otherwise the first one would
have already completed the request synchronously in scsi_req_cancel_complete.
With that in mind, I think returning early is not a problem. But I suppose
these are also idempotent so this change is not breaking anything, either.

Fam

On 17/12/2015 02:15, Fam Zheng wrote:
>> >      if (notifier) {
>> >          notifier_list_add(&req->cancel_notifiers, notifier);
>> >      }
>> > -    if (req->io_canceled) {
>> > -        return;
>> > -    }
>> >      scsi_req_ref(req);
>> >      scsi_req_dequeue(req);
>> >      req->io_canceled = true;
>        if (req->aiocb) {
>            blk_aio_cancel_async(req->aiocb);
>        } else {
>            scsi_req_cancel_complete(req);
>        }
> 
> A second TMF must be blk_aio_cancel_async case, otherwise the first one would
> have already completed the request synchronously in scsi_req_cancel_complete.

Good point.

> With that in mind, I think returning early is not a problem. But I suppose
> these are also idempotent so this change is not breaking anything, either.

Right, the issue is that all these calls are idempotent, but the
notifier may not; that is why I prefer to be safe and ensure that all
notifier additions are matched by a notify.  But you explained well why
this should be safe, I'll add a note to the commit message.

Paolo

On Thu, 12/17 09:41, Paolo Bonzini wrote:
> 
> 
> On 17/12/2015 02:15, Fam Zheng wrote:
> >> >      if (notifier) {
> >> >          notifier_list_add(&req->cancel_notifiers, notifier);
> >> >      }
> >> > -    if (req->io_canceled) {
> >> > -        return;
> >> > -    }
> >> >      scsi_req_ref(req);
> >> >      scsi_req_dequeue(req);
> >> >      req->io_canceled = true;
> >        if (req->aiocb) {
> >            blk_aio_cancel_async(req->aiocb);
> >        } else {
> >            scsi_req_cancel_complete(req);
> >        }
> > 
> > A second TMF must be blk_aio_cancel_async case, otherwise the first one would
> > have already completed the request synchronously in scsi_req_cancel_complete.
> 
> Good point.
> 
> > With that in mind, I think returning early is not a problem. But I suppose
> > these are also idempotent so this change is not breaking anything, either.
> 
> Right, the issue is that all these calls are idempotent, but the
> notifier may not; that is why I prefer to be safe and ensure that all
> notifier additions are matched by a notify.  But you explained well why
> this should be safe, I'll add a note to the commit message.
> 

Thanks, please add my

Reviewed-by: Fam Zheng <famz@redhat.com>