* [Qemu-devel] [PATCH] scsi: always call notifier on async cancellation
@ 2015-12-16 18:33 Paolo Bonzini
2015-12-17 1:15 ` Fam Zheng
0 siblings, 1 reply; 4+ messages in thread
From: Paolo Bonzini @ 2015-12-16 18:33 UTC (permalink / raw)
To: qemu-devel; +Cc: famz
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;
--
2.5.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] scsi: always call notifier on async cancellation
2015-12-16 18:33 [Qemu-devel] [PATCH] scsi: always call notifier on async cancellation Paolo Bonzini
@ 2015-12-17 1:15 ` Fam Zheng
2015-12-17 8:41 ` Paolo Bonzini
0 siblings, 1 reply; 4+ messages in thread
From: Fam Zheng @ 2015-12-17 1:15 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] scsi: always call notifier on async cancellation
2015-12-17 1:15 ` Fam Zheng
@ 2015-12-17 8:41 ` Paolo Bonzini
2015-12-17 12:22 ` Fam Zheng
0 siblings, 1 reply; 4+ messages in thread
From: Paolo Bonzini @ 2015-12-17 8:41 UTC (permalink / raw)
To: Fam Zheng; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] scsi: always call notifier on async cancellation
2015-12-17 8:41 ` Paolo Bonzini
@ 2015-12-17 12:22 ` Fam Zheng
0 siblings, 0 replies; 4+ messages in thread
From: Fam Zheng @ 2015-12-17 12:22 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
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>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-12-17 12:22 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-16 18:33 [Qemu-devel] [PATCH] scsi: always call notifier on async cancellation Paolo Bonzini
2015-12-17 1:15 ` Fam Zheng
2015-12-17 8:41 ` Paolo Bonzini
2015-12-17 12:22 ` Fam Zheng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.