| Message ID | 20180215111526.2464-1-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block/iscsi: cancel libiscsi task when ABORT TASK TMF completes | expand |
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
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
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
On Tue, Feb 20, 2018 at 3:12 PM, Peter Lieven <pl@kamp.de> 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
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",
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 <stefanha@redhat.com> --- block/iscsi.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)