Re: [Qemu-devel] [PATCH] block/iscsi: cancel libiscsi task when ABORT TASK TMF completes
On Tue, Feb 20, 2018 at 3:12 PM, Peter Lieven wrote: > Am 15.02.2018 um 18:27 schrieb Stefan Hajnoczi: >> >> On Thu, Feb 15, 2018 at 03:24:54PM +0100, Peter Lieven wrote: >>> >>> Am 15.02.2018 um 12:15 schrieb Stefan Hajnoczi: The libiscsi iscsi_task_mgmt_async() API documentation says: abort_task will also cancel the scsi task. The callback for the scsi task will be invoked with SCSI_STATUS_CANCELLED The libiscsi implementation does not fulfil this promise. The task's callback is not invoked and its struct iscsi_pdu remains in the internal list (effectively leaked). >>> >>> If that contract is fixed in libiscsi, will the Qemu iSCSI driver still >>> work? >> >> In >> >> +/* If the command callback hasn't been called yet, drop the task */ >> +if (!acb->bh) { >> >> and >> >> +if (status == SCSI_STATUS_CANCELLED) { >> +if (!acb->bh) { >> >> we're mindful of the fact that the callback may have been invoked by >> libiscsi already. There is no risk of double-completion. > > > Hi Stefan, > > thanks for the clarification. I am fine with this change. I will check with > Ronnie for the > libiscsi fix. Great, then this patch can go via Paolo's SCSI tree. Stefan
Re: [Qemu-devel] [PATCH] block/iscsi: cancel libiscsi task when ABORT TASK TMF completes
Am 15.02.2018 um 18:27 schrieb Stefan Hajnoczi: On Thu, Feb 15, 2018 at 03:24:54PM +0100, Peter Lieven wrote: Am 15.02.2018 um 12:15 schrieb Stefan Hajnoczi: The libiscsi iscsi_task_mgmt_async() API documentation says: abort_task will also cancel the scsi task. The callback for the scsi task will be invoked with SCSI_STATUS_CANCELLED The libiscsi implementation does not fulfil this promise. The task's callback is not invoked and its struct iscsi_pdu remains in the internal list (effectively leaked). If that contract is fixed in libiscsi, will the Qemu iSCSI driver still work? In +/* If the command callback hasn't been called yet, drop the task */ +if (!acb->bh) { and +if (status == SCSI_STATUS_CANCELLED) { +if (!acb->bh) { we're mindful of the fact that the callback may have been invoked by libiscsi already. There is no risk of double-completion. Hi Stefan, thanks for the clarification. I am fine with this change. I will check with Ronnie for the libiscsi fix. Peter
Re: [Qemu-devel] [PATCH] block/iscsi: cancel libiscsi task when ABORT TASK TMF completes
On Thu, Feb 15, 2018 at 03:24:54PM +0100, Peter Lieven wrote: > Am 15.02.2018 um 12:15 schrieb Stefan Hajnoczi: > > The libiscsi iscsi_task_mgmt_async() API documentation says: > > > >abort_task will also cancel the scsi task. The callback for the scsi > >task will be invoked with SCSI_STATUS_CANCELLED > > > > The libiscsi implementation does not fulfil this promise. The task's > > callback is not invoked and its struct iscsi_pdu remains in the internal > > list (effectively leaked). > > If that contract is fixed in libiscsi, will the Qemu iSCSI driver still work? In +/* If the command callback hasn't been called yet, drop the task */ +if (!acb->bh) { and +if (status == SCSI_STATUS_CANCELLED) { +if (!acb->bh) { we're mindful of the fact that the callback may have been invoked by libiscsi already. There is no risk of double-completion. Stefan signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH] block/iscsi: cancel libiscsi task when ABORT TASK TMF completes
Am 15.02.2018 um 12:15 schrieb Stefan Hajnoczi: The libiscsi iscsi_task_mgmt_async() API documentation says: abort_task will also cancel the scsi task. The callback for the scsi task will be invoked with SCSI_STATUS_CANCELLED The libiscsi implementation does not fulfil this promise. The task's callback is not invoked and its struct iscsi_pdu remains in the internal list (effectively leaked). If that contract is fixed in libiscsi, will the Qemu iSCSI driver still work? Peter
[Qemu-devel] [PATCH] block/iscsi: cancel libiscsi task when ABORT TASK TMF completes
The libiscsi iscsi_task_mgmt_async() API documentation says: abort_task will also cancel the scsi task. The callback for the scsi task will be invoked with SCSI_STATUS_CANCELLED The libiscsi implementation does not fulfil this promise. The task's callback is not invoked and its struct iscsi_pdu remains in the internal list (effectively leaked). This patch invokes the libiscsi iscsi_scsi_cancel_task() API to force the task's callback to be invoked with SCSI_STATUS_CANCELLED when the ABORT TASK TMF completes and the task's callback hasn't been invoked yet. Signed-off-by: Stefan Hajnoczi --- block/iscsi.c | 16 ++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/block/iscsi.c b/block/iscsi.c index 41e67cb371..4cb188ac2b 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -292,8 +292,12 @@ iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void *command_data, { IscsiAIOCB *acb = private_data; -acb->status = -ECANCELED; -iscsi_schedule_bh(acb); +/* If the command callback hasn't been called yet, drop the task */ +if (!acb->bh) { +/* Call iscsi_aio_ioctl_cb() with SCSI_STATUS_CANCELLED */ +iscsi_scsi_cancel_task(iscsi, acb->task); +} + qemu_aio_unref(acb); /* acquired in iscsi_aio_cancel() */ } @@ -941,6 +945,14 @@ iscsi_aio_ioctl_cb(struct iscsi_context *iscsi, int status, { IscsiAIOCB *acb = opaque; +if (status == SCSI_STATUS_CANCELLED) { +if (!acb->bh) { +acb->status = -ECANCELED; +iscsi_schedule_bh(acb); +} +return; +} + acb->status = 0; if (status < 0) { error_report("Failed to ioctl(SG_IO) to iSCSI lun. %s", -- 2.14.3