iscsi: fix deadlock during login

PATCH Superseded Peter Lieven 5 participants 13 messages Nov 15, 2012 – Nov 19, 2012 Block layer

Share & tools

Peter Lieven

author

2012-11-15 14:50 UTC #0 lore raw

PATCH 1/1 iscsi: fix deadlock during login

If the connection is interrupted before the first login is successfully completed qemu-kvm is waiting forever in qemu_aio_wait().

This is fixed by performing an sync login to the target. If the connection breaks after the first successful login errors are handled internally by libiscsi.

Signed-off-by Peter Lieven <[email protected]>

---
 block/iscsi.c |   56 
+++++++++++++++++++++-----------------------------------
 1 file changed, 21 insertions(+), 35 deletions(-)

block/iscsi.c +21 ~~-35~~

index b5c3161..f44bb57 100644
@@ -798,30 +798,6 @@ iscsi_inquiry_cb(struct iscsi_context *iscsi, int status, void *command_data,
     }
 }
 
-static void
-iscsi_connect_cb(struct iscsi_context *iscsi, int status, void *command_data,
-                 void *opaque)
-{
-    struct IscsiTask *itask = opaque;
-    struct scsi_task *task;
-
-    if (status != 0) {
-        itask->status   = 1;
-        itask->complete = 1;
-        return;
-    }
-
-    task = iscsi_inquiry_task(iscsi, itask->iscsilun->lun,
-                              0, 0, 36,
-                              iscsi_inquiry_cb, opaque);
-    if (task == NULL) {
-        error_report("iSCSI: failed to send inquiry command.");
-        itask->status   = 1;
-        itask->complete = 1;
-        return;
-    }
-}
-
 static int parse_chap(struct iscsi_context *iscsi, const char *target)
 {
     QemuOptsList *list;

@@ -934,7 +910,8 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags)
     IscsiLun *iscsilun = bs->opaque;
     struct iscsi_context *iscsi = NULL;
     struct iscsi_url *iscsi_url = NULL;
-    struct IscsiTask task;
+    struct IscsiTask itask;
+    struct scsi_task *task;
     char *initiator_name = NULL;
     int ret;

@@ -997,27 +974,36 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags)
     /* check if we got HEADER_DIGEST via the options */
     parse_header_digest(iscsi, iscsi_url->target);
 
-    task.iscsilun = iscsilun;
-    task.status = 0;
-    task.complete = 0;
-    task.bs = bs;
+    if (iscsi_full_connect_sync(iscsi, iscsi_url->portal, iscsi_url->lun) != 0) {
+        error_report("iSCSI: Failed to connect to LUN : %s",
+            iscsi_get_error(iscsi));
+        ret = -EINVAL;
+        goto out;
+    }
+
+    itask.iscsilun = iscsilun;
+    itask.status = 0;
+    itask.complete = 0;
+    itask.bs = bs;
 
     iscsilun->iscsi = iscsi;
     iscsilun->lun   = iscsi_url->lun;
 
-    if (iscsi_full_connect_async(iscsi, iscsi_url->portal, iscsi_url->lun,
-                                 iscsi_connect_cb, &task)
-        != 0) {
-        error_report("iSCSI: Failed to start async connect.");
+    task = iscsi_inquiry_task(iscsi, iscsilun->lun,
+                              0, 0, 36,
+                              iscsi_inquiry_cb, &itask);
+    if (task == NULL) {
+        error_report("iSCSI: failed to send inquiry command.");
         ret = -EINVAL;
         goto out;
     }
 
-    while (!task.complete) {
+    while (!itask.complete) {
         iscsi_set_events(iscsilun);
         qemu_aio_wait();
     }
-    if (task.status != 0) {
+
+    if (itask.status != 0) {
         error_report("iSCSI: Failed to connect to LUN : %s",
                      iscsi_get_error(iscsi));
         ret = -EINVAL;

Signature · 1 line

1.7.9.5

Paolo Bonzini

2012-11-15 15:54 UTC #3 lore raw

In reply to ronnie sahlberg

Il 15/11/2012 15:57, ronnie sahlberg ha scritto:

Quoted 2 lines

I dont know if we should switch to use synchronous code here. It is much nicer if all code is async.

bdrv_open is generally synchronous, so I think Peter's patch is ok.

Paolo

Quoted 2 lines

Is it possible to add a timeout instead that would break out if the connect/login has not completed within a certain amount of time?

ronnie sahlberg

Acked-by

2012-11-15 16:13 UTC #4 lore raw

Quoted 5 lines · Paolo Bonzini

Il 15/11/2012 15:57, ronnie sahlberg ha scritto:
Quoted 2 lines
I dont know if we should switch to use synchronous code here. It is much nicer if all code is async.
bdrv_open is generally synchronous, so I think Peter's patch is ok.

I was thinking about the case where you disconnect/reconnect a device at runtime. Like swapping the medium in a CDROM.
If bdrv_open() is synchronous and blocks for a long time, would that not impact the rest of QEMU?

Otherwise:

Acked-by [email protected]

Quoted 6 lines

Paolo
Quoted 2 lines
Is it possible to add a timeout instead that would break out if the connect/login has not completed within a certain amount of time?

Paolo Bonzini

2012-11-15 16:38 UTC #5 lore raw

Il 15/11/2012 17:13, ronnie sahlberg ha scritto:

Quoted 11 lines · Paolo Bonzini

Quoted 5 lines
Il 15/11/2012 15:57, ronnie sahlberg ha scritto:
Quoted 2 lines
I dont know if we should switch to use synchronous code here. It is much nicer if all code is async.
bdrv_open is generally synchronous, so I think Peter's patch is ok.
I was thinking about the case where you disconnect/reconnect a device at runtime. Like swapping the medium in a CDROM.
If bdrv_open() is synchronous and blocks for a long time, would that not impact the rest of QEMU?

Yes, it's not optimal, but VCPUs would still run until they request I/O.
But usually iscsi devices should be non-removable, no? That leaves
hotplug as the only problematic case.

Paolo

Quoted 10 lines

Otherwise:
Acked-by [email protected]
Quoted 6 lines
Paolo
Quoted 2 lines
Is it possible to add a timeout instead that would break out if the connect/login has not completed within a certain amount of time?

Kevin Wolf

2012-11-16 10:38 UTC #6 lore raw

Am 15.11.2012 17:37, schrieb Paolo Bonzini:

Quoted 8 lines

Il 15/11/2012 17:13, ronnie sahlberg ha scritto:
Quoted 7 lines · Paolo Bonzini
Quoted 5 lines
Il 15/11/2012 15:57, ronnie sahlberg ha scritto:
Quoted 2 lines
I dont know if we should switch to use synchronous code here. It is much nicer if all code is async.
bdrv_open is generally synchronous, so I think Peter's patch is ok.

Quoted 8 lines

Quoted 4 lines
I was thinking about the case where you disconnect/reconnect a device at runtime. Like swapping the medium in a CDROM.
If bdrv_open() is synchronous and blocks for a long time, would that not impact the rest of QEMU?
Yes, it's not optimal, but VCPUs would still run until they request I/O.
But usually iscsi devices should be non-removable, no? That leaves
hotplug as the only problematic case.

I guess we need a bdrv_co_open() for the long term.

Kevin

Peter Lieven

author

2012-11-16 17:48 UTC #7 lore raw

Am 16.11.2012 11:38, schrieb Kevin Wolf:

Quoted 7 lines

Am 15.11.2012 17:37, schrieb Paolo Bonzini:
Quoted 6 lines
Il 15/11/2012 17:13, ronnie sahlberg ha scritto:
Quoted 5 lines · Paolo Bonzini
Quoted 4 lines
Il 15/11/2012 15:57, ronnie sahlberg ha scritto:
Quoted 2 lines
I dont know if we should switch to use synchronous code here. It is much nicer if all code is async.
bdrv_open is generally synchronous, so I think Peter's patch is ok.

Quoted 8 lines

Quoted 7 lines
Quoted 4 lines
I was thinking about the case where you disconnect/reconnect a device at runtime. Like swapping the medium in a CDROM.
If bdrv_open() is synchronous and blocks for a long time, would that not impact the rest of QEMU?
Yes, it's not optimal, but VCPUs would still run until they request I/O.
But usually iscsi devices should be non-removable, no? That leaves
hotplug as the only problematic case.
I guess we need a bdrv_co_open() for the long term.

but for now its save to implement iscsi_open (and iscsi_create) completely sync?

Peter

Quoted 2 lines

Kevin

Paolo Bonzini

2012-11-16 17:55 UTC #8 lore raw

Il 16/11/2012 18:38, Peter Lieven ha scritto:

Quoted 13 lines

Am 16.11.2012 11:38, schrieb Kevin Wolf:
Quoted 12 lines
Am 15.11.2012 17:37, schrieb Paolo Bonzini:
Quoted 11 lines
Il 15/11/2012 17:13, ronnie sahlberg ha scritto:
Quoted 10 lines
Quoted 4 lines · Paolo Bonzini
Il 15/11/2012 15:57, ronnie sahlberg ha scritto:
Quoted 2 lines
I dont know if we should switch to use synchronous code here. It is much nicer if all code is async.
bdrv_open is generally synchronous, so I think Peter's patch is ok.
I was thinking about the case where you disconnect/reconnect a device at runtime. Like swapping the medium in a CDROM.
If bdrv_open() is synchronous and blocks for a long time, would that not impact the rest of QEMU?

Quoted 6 lines

Quoted 4 lines
Quoted 3 lines
Yes, it's not optimal, but VCPUs would still run until they request I/O.
But usually iscsi devices should be non-removable, no? That leaves
hotplug as the only problematic case.
I guess we need a bdrv_co_open() for the long term.
but for now its save to implement iscsi_open (and iscsi_create) completely sync?

Yes.

Paolo

Peter Lieven

2012-11-15 18:28 UTC #9 lore raw

In reply to Paolo Bonzini

Quoted 5 lines · Paolo Bonzini

Il 15/11/2012 15:57, ronnie sahlberg ha scritto:
Quoted 2 lines
I dont know if we should switch to use synchronous code here. It is much nicer if all code is async.
bdrv_open is generally synchronous, so I think Peter's patch is ok.

if all is sync wouldn't it be best to have all code in iscsi_open sync then and convert the iscsi_inquiry and iscsi_readcapacity commands also to sync?

Peter

Quoted 8 lines

Paolo
Quoted 2 lines
Is it possible to add a timeout instead that would break out if the connect/login has not completed within a certain amount of time?

Paolo Bonzini

2012-11-16 07:44 UTC #10 lore raw

Il 15/11/2012 19:28, Peter Lieven ha scritto:

Quoted 7 lines

Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
I dont know if we should switch to use synchronous code here. It is much nicer if all code is async.
bdrv_open is generally synchronous, so I think Peter's patch is ok.
if all is sync wouldn't it be best to have all code in iscsi_open sync then and convert the iscsi_inquiry and iscsi_readcapacity commands also to sync?

Indeed, there is no real advantage in using qemu_aio_wait(). I didn't know libiscsi also had sync APIs. :)

Paolo

Peter Lieven

author

2012-11-16 09:26 UTC #11 lore raw

Am 16.11.2012 08:44, schrieb Paolo Bonzini:

Quoted 9 lines

Il 15/11/2012 19:28, Peter Lieven ha scritto:
Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
Quoted 2 lines
Quoted 2 lines
I dont know if we should switch to use synchronous code here. It is much nicer if all code is async.
bdrv_open is generally synchronous, so I think Peter's patch is ok.
if all is sync wouldn't it be best to have all code in iscsi_open sync then and convert the iscsi_inquiry and iscsi_readcapacity commands also to sync?
Indeed, there is no real advantage in using qemu_aio_wait(). I didn't know libiscsi also had sync APIs. :)

ok, give me some days to rewrite this patch. i think it should be ready by monday. i will also add the missing iscsi_create which is a few lines only then.

peter

Quoted 2 lines

Paolo

Peter Lieven

author

2012-11-19 07:07 UTC #12 lore raw

In reply to Paolo Bonzini

Am 15.11.2012 um 16:54 schrieb Paolo Bonzini <[email protected]>:

Quoted 5 lines

Il 15/11/2012 15:57, ronnie sahlberg ha scritto:
Quoted 2 lines
I dont know if we should switch to use synchronous code here. It is much nicer if all code is async.
bdrv_open is generally synchronous, so I think Peter's patch is ok.

would`t it be better to do the iscsi_inquiry commands and iscsi_readcapacity also sync?

peter

Quoted 6 lines

Paolo
Quoted 2 lines
Is it possible to add a timeout instead that would break out if the connect/login has not completed within a certain amount of time?