Patchwork iscsi: fix race between task completition and task abortion

login
register
mail settings
Submitter Stefan Priebe - Profihost AG
Date Aug. 14, 2012, 6:44 a.m.
Message ID <1344926686-9197-1-git-send-email-s.priebe@profihost.ag>
Download mbox | patch
Permalink /patch/177173/
State New
Headers show

Comments

Stefan Priebe - Profihost AG - Aug. 14, 2012, 6:44 a.m.
From: spriebe <git@profihost.ag>

---
 block/iscsi.c |   36 ++++++++++++++++++++----------------
 1 files changed, 20 insertions(+), 16 deletions(-)
Stefan Hajnoczi - Aug. 14, 2012, 10:35 a.m.
On Tue, Aug 14, 2012 at 08:44:46AM +0200, Stefan Priebe wrote:
> From: spriebe <git@profihost.ag>

CCing Ronnie, iSCSI block driver author.

> 
> ---
>  block/iscsi.c |   36 ++++++++++++++++++++----------------
>  1 files changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 12ca76d..257f97f 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -76,6 +76,10 @@ static void
>  iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void *command_data,
>                      void *private_data)
>  {
> +           IscsiAIOCB *acb = (IscsiAIOCB *)private_data;
> +
> +           scsi_free_scsi_task(acb->task);
> +           acb->task = NULL;
>  }
>  
>  static void
> @@ -85,14 +89,19 @@ iscsi_aio_cancel(BlockDriverAIOCB *blockacb)
>      IscsiLun *iscsilun = acb->iscsilun;
>  
>      acb->common.cb(acb->common.opaque, -ECANCELED);
> -    acb->canceled = 1;
>  
> -    /* send a task mgmt call to the target to cancel the task on the target */
> -    iscsi_task_mgmt_abort_task_async(iscsilun->iscsi, acb->task,
> -                                     iscsi_abort_task_cb, NULL);
> +    if (acb->canceled != 0)
> +        return;
> +
> +    acb->canceled = 1;
>  
> -    /* then also cancel the task locally in libiscsi */
> -    iscsi_scsi_task_cancel(iscsilun->iscsi, acb->task);
> +    /* send a task mgmt call to the target to cancel the task on the target
> +     * this also cancels the task in libiscsi
> +     */
> +    if (acb->task) {
> +        iscsi_task_mgmt_abort_task_async(iscsilun->iscsi, acb->task,
> +                                         iscsi_abort_task_cb, &acb);
> +    }
>  }
>  
>  static AIOPool iscsi_aio_pool = {
> @@ -184,6 +193,11 @@ iscsi_readv_writev_bh_cb(void *p)
>      }
>  
>      qemu_aio_release(acb);
> +
> +    if (acb->task) {
> +      scsi_free_scsi_task(acb->task);
> +      acb->task = NULL;
> +    }
>  }
>  
>  
> @@ -212,8 +226,6 @@ iscsi_aio_write16_cb(struct iscsi_context *iscsi, int status,
>      }
>  
>      iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
> -    scsi_free_scsi_task(acb->task);
> -    acb->task = NULL;
>  }
>  
>  static int64_t sector_qemu2lun(int64_t sector, IscsiLun *iscsilun)
> @@ -313,8 +325,6 @@ iscsi_aio_read16_cb(struct iscsi_context *iscsi, int status,
>      }
>  
>      iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
> -    scsi_free_scsi_task(acb->task);
> -    acb->task = NULL;
>  }
>  
>  static BlockDriverAIOCB *
> @@ -429,8 +439,6 @@ iscsi_synccache10_cb(struct iscsi_context *iscsi, int status,
>      }
>  
>      iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
> -    scsi_free_scsi_task(acb->task);
> -    acb->task = NULL;
>  }
>  
>  static BlockDriverAIOCB *
> @@ -483,8 +491,6 @@ iscsi_unmap_cb(struct iscsi_context *iscsi, int status,
>      }
>  
>      iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
> -    scsi_free_scsi_task(acb->task);
> -    acb->task = NULL;
>  }
>  
>  static BlockDriverAIOCB *
> @@ -560,8 +566,6 @@ iscsi_aio_ioctl_cb(struct iscsi_context *iscsi, int status,
>      }
>  
>      iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
> -    scsi_free_scsi_task(acb->task);
> -    acb->task = NULL;
>  }
>  
>  static BlockDriverAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
> -- 
> 1.7.2.5
> 
>
ronniesahlberg@gmail.com - Aug. 14, 2012, 12:09 p.m.
Stefan H

How should I do Acked-by properly,


Is a reply with the text

Acked-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>

sufficient ?


regards
ronnie sahlberg


On Tue, Aug 14, 2012 at 8:35 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Aug 14, 2012 at 08:44:46AM +0200, Stefan Priebe wrote:
>> From: spriebe <git@profihost.ag>
>
> CCing Ronnie, iSCSI block driver author.
>
>>
>> ---
>>  block/iscsi.c |   36 ++++++++++++++++++++----------------
>>  1 files changed, 20 insertions(+), 16 deletions(-)
>>
>> diff --git a/block/iscsi.c b/block/iscsi.c
>> index 12ca76d..257f97f 100644
>> --- a/block/iscsi.c
>> +++ b/block/iscsi.c
>> @@ -76,6 +76,10 @@ static void
>>  iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void *command_data,
>>                      void *private_data)
>>  {
>> +           IscsiAIOCB *acb = (IscsiAIOCB *)private_data;
>> +
>> +           scsi_free_scsi_task(acb->task);
>> +           acb->task = NULL;
>>  }
>>
>>  static void
>> @@ -85,14 +89,19 @@ iscsi_aio_cancel(BlockDriverAIOCB *blockacb)
>>      IscsiLun *iscsilun = acb->iscsilun;
>>
>>      acb->common.cb(acb->common.opaque, -ECANCELED);
>> -    acb->canceled = 1;
>>
>> -    /* send a task mgmt call to the target to cancel the task on the target */
>> -    iscsi_task_mgmt_abort_task_async(iscsilun->iscsi, acb->task,
>> -                                     iscsi_abort_task_cb, NULL);
>> +    if (acb->canceled != 0)
>> +        return;
>> +
>> +    acb->canceled = 1;
>>
>> -    /* then also cancel the task locally in libiscsi */
>> -    iscsi_scsi_task_cancel(iscsilun->iscsi, acb->task);
>> +    /* send a task mgmt call to the target to cancel the task on the target
>> +     * this also cancels the task in libiscsi
>> +     */
>> +    if (acb->task) {
>> +        iscsi_task_mgmt_abort_task_async(iscsilun->iscsi, acb->task,
>> +                                         iscsi_abort_task_cb, &acb);
>> +    }
>>  }
>>
>>  static AIOPool iscsi_aio_pool = {
>> @@ -184,6 +193,11 @@ iscsi_readv_writev_bh_cb(void *p)
>>      }
>>
>>      qemu_aio_release(acb);
>> +
>> +    if (acb->task) {
>> +      scsi_free_scsi_task(acb->task);
>> +      acb->task = NULL;
>> +    }
>>  }
>>
>>
>> @@ -212,8 +226,6 @@ iscsi_aio_write16_cb(struct iscsi_context *iscsi, int status,
>>      }
>>
>>      iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
>> -    scsi_free_scsi_task(acb->task);
>> -    acb->task = NULL;
>>  }
>>
>>  static int64_t sector_qemu2lun(int64_t sector, IscsiLun *iscsilun)
>> @@ -313,8 +325,6 @@ iscsi_aio_read16_cb(struct iscsi_context *iscsi, int status,
>>      }
>>
>>      iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
>> -    scsi_free_scsi_task(acb->task);
>> -    acb->task = NULL;
>>  }
>>
>>  static BlockDriverAIOCB *
>> @@ -429,8 +439,6 @@ iscsi_synccache10_cb(struct iscsi_context *iscsi, int status,
>>      }
>>
>>      iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
>> -    scsi_free_scsi_task(acb->task);
>> -    acb->task = NULL;
>>  }
>>
>>  static BlockDriverAIOCB *
>> @@ -483,8 +491,6 @@ iscsi_unmap_cb(struct iscsi_context *iscsi, int status,
>>      }
>>
>>      iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
>> -    scsi_free_scsi_task(acb->task);
>> -    acb->task = NULL;
>>  }
>>
>>  static BlockDriverAIOCB *
>> @@ -560,8 +566,6 @@ iscsi_aio_ioctl_cb(struct iscsi_context *iscsi, int status,
>>      }
>>
>>      iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
>> -    scsi_free_scsi_task(acb->task);
>> -    acb->task = NULL;
>>  }
>>
>>  static BlockDriverAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
>> --
>> 1.7.2.5
>>
>>
Stefan Hajnoczi - Aug. 14, 2012, 12:11 p.m.
On Tue, Aug 14, 2012 at 1:09 PM, ronnie sahlberg
<ronniesahlberg@gmail.com> wrote:
> Is a reply with the text
>
> Acked-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>
> sufficient ?

Yes
Kevin Wolf - Aug. 14, 2012, 2:08 p.m.
Am 14.08.2012 14:11, schrieb Stefan Hajnoczi:
> On Tue, Aug 14, 2012 at 1:09 PM, ronnie sahlberg
> <ronniesahlberg@gmail.com> wrote:
>> Is a reply with the text
>>
>> Acked-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>>
>> sufficient ?
> 
> Yes

But is this only meant as a question or a real Acked-by and I should
pick it up? (Still for 1.2, I guess?)

Kevin
Kevin Wolf - Aug. 14, 2012, 2:57 p.m.
Am 14.08.2012 08:44, schrieb Stefan Priebe:
> From: spriebe <git@profihost.ag>
> 
> ---
>  block/iscsi.c |   36 ++++++++++++++++++++----------------
>  1 files changed, 20 insertions(+), 16 deletions(-)

It would be nice to have your full name and a valid email address in the
From: line (needs an update of your git config) and a more detailed
explanation of the problem that you're fixing.

Having a Signed-off-by line, however, is absolutely required and the
patch can't be merged without it.

> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 12ca76d..257f97f 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -76,6 +76,10 @@ static void
>  iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void *command_data,
>                      void *private_data)
>  {
> +           IscsiAIOCB *acb = (IscsiAIOCB *)private_data;
> +
> +           scsi_free_scsi_task(acb->task);
> +           acb->task = NULL;

Please use scripts/checkpatch.pl. qemu uses an indentation of four
spaces, more coding style violations follow.

Kevin
Stefan Priebe - Profihost AG - Aug. 14, 2012, 8:21 p.m.
Am 14.08.2012 16:08, schrieb Kevin Wolf:
> Am 14.08.2012 14:11, schrieb Stefan Hajnoczi:
>> On Tue, Aug 14, 2012 at 1:09 PM, ronnie sahlberg
>> <ronniesahlberg@gmail.com> wrote:
>>> Is a reply with the text
>>>
>>> Acked-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
>>>
>>> sufficient ?
>>
>> Yes
>
> But is this only meant as a question or a real Acked-by and I should
> pick it up? (Still for 1.2, I guess?)

I'll send a new patch in a few minutes. It fixes the signed-off, style 
and a logic problem as well.

Stefan

Patch

diff --git a/block/iscsi.c b/block/iscsi.c
index 12ca76d..257f97f 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -76,6 +76,10 @@  static void
 iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void *command_data,
                     void *private_data)
 {
+           IscsiAIOCB *acb = (IscsiAIOCB *)private_data;
+
+           scsi_free_scsi_task(acb->task);
+           acb->task = NULL;
 }
 
 static void
@@ -85,14 +89,19 @@  iscsi_aio_cancel(BlockDriverAIOCB *blockacb)
     IscsiLun *iscsilun = acb->iscsilun;
 
     acb->common.cb(acb->common.opaque, -ECANCELED);
-    acb->canceled = 1;
 
-    /* send a task mgmt call to the target to cancel the task on the target */
-    iscsi_task_mgmt_abort_task_async(iscsilun->iscsi, acb->task,
-                                     iscsi_abort_task_cb, NULL);
+    if (acb->canceled != 0)
+        return;
+
+    acb->canceled = 1;
 
-    /* then also cancel the task locally in libiscsi */
-    iscsi_scsi_task_cancel(iscsilun->iscsi, acb->task);
+    /* send a task mgmt call to the target to cancel the task on the target
+     * this also cancels the task in libiscsi
+     */
+    if (acb->task) {
+        iscsi_task_mgmt_abort_task_async(iscsilun->iscsi, acb->task,
+                                         iscsi_abort_task_cb, &acb);
+    }
 }
 
 static AIOPool iscsi_aio_pool = {
@@ -184,6 +193,11 @@  iscsi_readv_writev_bh_cb(void *p)
     }
 
     qemu_aio_release(acb);
+
+    if (acb->task) {
+      scsi_free_scsi_task(acb->task);
+      acb->task = NULL;
+    }
 }
 
 
@@ -212,8 +226,6 @@  iscsi_aio_write16_cb(struct iscsi_context *iscsi, int status,
     }
 
     iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
-    scsi_free_scsi_task(acb->task);
-    acb->task = NULL;
 }
 
 static int64_t sector_qemu2lun(int64_t sector, IscsiLun *iscsilun)
@@ -313,8 +325,6 @@  iscsi_aio_read16_cb(struct iscsi_context *iscsi, int status,
     }
 
     iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
-    scsi_free_scsi_task(acb->task);
-    acb->task = NULL;
 }
 
 static BlockDriverAIOCB *
@@ -429,8 +439,6 @@  iscsi_synccache10_cb(struct iscsi_context *iscsi, int status,
     }
 
     iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
-    scsi_free_scsi_task(acb->task);
-    acb->task = NULL;
 }
 
 static BlockDriverAIOCB *
@@ -483,8 +491,6 @@  iscsi_unmap_cb(struct iscsi_context *iscsi, int status,
     }
 
     iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
-    scsi_free_scsi_task(acb->task);
-    acb->task = NULL;
 }
 
 static BlockDriverAIOCB *
@@ -560,8 +566,6 @@  iscsi_aio_ioctl_cb(struct iscsi_context *iscsi, int status,
     }
 
     iscsi_schedule_bh(iscsi_readv_writev_bh_cb, acb);
-    scsi_free_scsi_task(acb->task);
-    acb->task = NULL;
 }
 
 static BlockDriverAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,