From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41907) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a9oAY-0002o2-Qm for qemu-devel@nongnu.org; Fri, 18 Dec 2015 01:05:36 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a9oAT-00010m-Pt for qemu-devel@nongnu.org; Fri, 18 Dec 2015 01:05:34 -0500 Received: from mail-wm0-x231.google.com ([2a00:1450:400c:c09::231]:32964) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a9oAT-00010O-Jj for qemu-devel@nongnu.org; Fri, 18 Dec 2015 01:05:29 -0500 Received: by mail-wm0-x231.google.com with SMTP id p187so51144790wmp.0 for ; Thu, 17 Dec 2015 22:05:29 -0800 (PST) Sender: Paolo Bonzini References: <1450374401-31352-1-git-send-email-pbonzini@redhat.com> <1450374401-31352-44-git-send-email-pbonzini@redhat.com> <20151218005716.GD25529@ad.usersys.redhat.com> From: Paolo Bonzini Message-ID: <5673A227.4@redhat.com> Date: Fri, 18 Dec 2015 07:05:27 +0100 MIME-Version: 1.0 In-Reply-To: <20151218005716.GD25529@ad.usersys.redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PULL 43/45] scsi: always call notifier on async cancellation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: qemu-devel@nongnu.org On 18/12/2015 01:57, Fam Zheng wrote: > Oh hang on, in scsi_req_dequeue, if req->enqueued is already false, the > matching scsi_req_unref is never called. The matching unref for scsi_req_cancel_async's ref is in scsi_req_cancel_complete. You're right that there is a leak if we get to the second cancellation with req->aiocb, and we should never get there with !req->aiocb. So the patch is wrong, but we should add some documentation instead of plainly reverting it: diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c index 00bddc9..378bf4d 100644 --- a/hw/scsi/scsi-bus.c +++ b/hw/scsi/scsi-bus.c @@ -1759,6 +1759,17 @@ void scsi_req_cancel_async(SCSIRequest *req, Notifier *notifier) if (notifier) { notifier_list_add(&req->cancel_notifiers, notifier); } + if (req->io_canceled) { + /* Canceling a second time after scsi_req_cancel_complete + * is a programming error, hence a blk_aio_cancel_async is + * pending; when it finishes, scsi_req_cancel_complete + * will be called and will call the notifier we just + * added. Just wait for that. + */ + assert(req->aiocb); + return; + } + /* Dropped in scsi_req_cancel_complete. */ scsi_req_ref(req); scsi_req_dequeue(req); req->io_canceled = true; @@ -1775,6 +1784,8 @@ void scsi_req_cancel(SCSIRequest *req) if (!req->enqueued) { return; } + assert(!req->io_canceled); + /* Dropped in scsi_req_cancel_complete. */ scsi_req_ref(req); scsi_req_dequeue(req); req->io_canceled = true; Does this look sane? Thanks, Paolo