iscsi: add support for iSCSI NOPs

PATCH RESEND In review Peter Lieven 2 participants 4 messages Dec 3, 2012 – Dec 5, 2012 Block layer

Share & tools

Peter Lieven

author

2012-12-03 19:34 UTC #0 lore raw

PATCH RESEND 1/1 iscsi: add support for iSCSI NOPs

This patch will send NOP-Out PDUs every 5 seconds to the iSCSI target. If a consecutive number of NOP-In replies fail a reconnect is initiated. iSCSI NOPs help to ensure that the connection to the target is still operational. This should not, but in reality may be the case even if the TCP connection is still alive if there are bugs in either the target or the initiator implementation.

Reported-by Ronnie Sahlberg <[email protected]>

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

---
 block/iscsi.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

block/iscsi.c +43 -0

index d0b1a10..fab4c8b 100644
@@ -47,6 +47,9 @@ typedef struct IscsiLun {
     int block_size;
     uint64_t num_blocks;
     int events;
+
+    QEMUTimer *nop_timer;
+    int nops_in_flight;
 } IscsiLun;
 
 typedef struct IscsiAIOCB {

@@ -72,6 +75,9 @@ struct IscsiTask {
     int complete;
 };
 
+#define NOP_INTERVAL 5000
+#define MAX_NOP_FAILURES 3
+
 static void
 iscsi_bh_cb(void *p)
 {

@@ -925,6 +931,35 @@ static char *parse_initiator_name(const char *target)
     }
 }
 
+static void iscsi_nop_cb(struct iscsi_context *iscsi, int status, void *command_data, void *private_data)
+{
+    IscsiLun *iscsilun = private_data;
+
+    if (iscsilun) {
+        iscsilun->nops_in_flight = 0;
+    }
+}
+
+static void iscsi_nop_timed_event(void *opaque)
+{
+    IscsiLun *iscsilun = opaque;
+
+    if (iscsilun->nops_in_flight > MAX_NOP_FAILURES) {
+        error_report("iSCSI: NOP timeout. Reconnecting...");
+        iscsi_reconnect(iscsilun->iscsi);
+        iscsilun->nops_in_flight = 0;
+    }
+
+    if (iscsi_nop_out_async(iscsilun->iscsi, iscsi_nop_cb, NULL, 0, iscsilun) != 0) {
+        error_report("iSCSI: failed to sent NOP-Out. Disabling NOP messages.");
+        return;
+    }
+
+    qemu_mod_timer(iscsilun->nop_timer, qemu_get_clock_ms(rt_clock) + NOP_INTERVAL);
+    iscsi_set_events(iscsilun);
+    iscsilun->nops_in_flight++;
+}
+
 /*
  * We support iscsi url's on the form
  * iscsi://[<username>%<password>@]<host>[:<port>]/<targetname>/<lun>

@@ -1036,6 +1071,10 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags)
 
     ret = 0;
 
+    /* Set up a timer for sending out iSCSI NOPs */
+    iscsilun->nop_timer = qemu_new_timer_ms(rt_clock, iscsi_nop_timed_event, iscsilun);
+    qemu_mod_timer(iscsilun->nop_timer, qemu_get_clock_ms(rt_clock) + NOP_INTERVAL);
+
 out:
     if (initiator_name != NULL) {
         g_free(initiator_name);

@@ -1058,6 +1097,10 @@ static void iscsi_close(BlockDriverState *bs)
     IscsiLun *iscsilun = bs->opaque;
     struct iscsi_context *iscsi = iscsilun->iscsi;
 
+    if (iscsilun->nop_timer) {
+        qemu_del_timer(iscsilun->nop_timer);
+        qemu_free_timer(iscsilun->nop_timer);
+    }
     qemu_aio_set_fd_handler(iscsi_get_fd(iscsi), NULL, NULL, NULL, NULL);
     iscsi_destroy_context(iscsi);
     memset(iscsilun, 0, sizeof(IscsiLun));

Signature · 1 line

1.7.9.5

ronnie sahlberg

Acked-by on v1 1/1

2012-12-04 05:10 UTC #1 lore raw

Acked-by [email protected] (Ronnie Sahlberg)

This verified that the service is actually operational and is much more reliable than TCP-KEEPALIVES.
This is the proper way to monitor that the iscsi target is alive.

We should as a later patch add the ability to configure this via the qemu config file instead of using hardcoded values.

regards
ronnie sahlberg

Quoted 98 lines · Peter Lieven

Reported-by Ronnie Sahlberg <[email protected]>

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

---
 block/iscsi.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

block/iscsi.c +43 -1

index d0b1a10..fab4c8b 100644
@@ -47,6 +47,9 @@ typedef struct IscsiLun {
     int block_size;
     uint64_t num_blocks;
     int events;
+
+    QEMUTimer *nop_timer;
+    int nops_in_flight;
 } IscsiLun;
 
 typedef struct IscsiAIOCB {

@@ -72,6 +75,9 @@ struct IscsiTask {
     int complete;
 };
 
+#define NOP_INTERVAL 5000
+#define MAX_NOP_FAILURES 3
+
 static void
 iscsi_bh_cb(void *p)
 {

@@ -925,6 +931,35 @@ static char *parse_initiator_name(const char *target)
     }
 }
 
+static void iscsi_nop_cb(struct iscsi_context *iscsi, int status, void *command_data, void *private_data)
+{
+    IscsiLun *iscsilun = private_data;
+
+    if (iscsilun) {
+        iscsilun->nops_in_flight = 0;
+    }
+}
+
+static void iscsi_nop_timed_event(void *opaque)
+{
+    IscsiLun *iscsilun = opaque;
+
+    if (iscsilun->nops_in_flight > MAX_NOP_FAILURES) {
+        error_report("iSCSI: NOP timeout. Reconnecting...");
+        iscsi_reconnect(iscsilun->iscsi);
+        iscsilun->nops_in_flight = 0;
+    }
+
+    if (iscsi_nop_out_async(iscsilun->iscsi, iscsi_nop_cb, NULL, 0, iscsilun) != 0) {
+        error_report("iSCSI: failed to sent NOP-Out. Disabling NOP messages.");
+        return;
+    }
+
+    qemu_mod_timer(iscsilun->nop_timer, qemu_get_clock_ms(rt_clock) + NOP_INTERVAL);
+    iscsi_set_events(iscsilun);
+    iscsilun->nops_in_flight++;
+}
+
 /*
  * We support iscsi url's on the form
  * iscsi://[<username>%<password>@]<host>[:<port>]/<targetname>/<lun>

@@ -1036,6 +1071,10 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags)
 
     ret = 0;
 
+    /* Set up a timer for sending out iSCSI NOPs */
+    iscsilun->nop_timer = qemu_new_timer_ms(rt_clock, iscsi_nop_timed_event, iscsilun);
+    qemu_mod_timer(iscsilun->nop_timer, qemu_get_clock_ms(rt_clock) + NOP_INTERVAL);
+
 out:
     if (initiator_name != NULL) {
         g_free(initiator_name);

@@ -1058,6 +1097,10 @@ static void iscsi_close(BlockDriverState *bs)
     IscsiLun *iscsilun = bs->opaque;
     struct iscsi_context *iscsi = iscsilun->iscsi;
 
+    if (iscsilun->nop_timer) {
+        qemu_del_timer(iscsilun->nop_timer);
+        qemu_free_timer(iscsilun->nop_timer);
+    }
     qemu_aio_set_fd_handler(iscsi_get_fd(iscsi), NULL, NULL, NULL, NULL);
     iscsi_destroy_context(iscsi);
     memset(iscsilun, 0, sizeof(IscsiLun));
--

1.7.9.5

Peter Lieven

author

2012-12-04 07:31 UTC #2 lore raw

Quoted 6 lines · ronnie sahlberg

Acked-by [email protected] (Ronnie Sahlberg)
This verified that the service is actually operational and is much more reliable than TCP-KEEPALIVES.
This is the proper way to monitor that the iscsi target is alive.

Yes, especially because (at least under linux) keepalives are only send if the output buffer is empty. I have added tcp user timeout option earlier, but its timeout is not as reliable as the NOPs.

Quoted 3 lines

We should as a later patch add the ability to configure this via the qemu config file instead of using hardcoded values.

Of course, but I can report from several hundred connections that I did not have any false positives during the last weeks with that values. But this might depend on the target that is used.

I have one concern where I would appreciate feedback:
Is the qemu timer fired in the same thread as libiscsi is running? Otherwise the iscsi_nop_timed_event() cound interfere with an ongoing reconnect that has been inititated by libiscsi (if it detects the broken connection earlier).

Peter

Quoted 105 lines

regards
ronnie sahlberg

Quoted 98 lines · Peter Lieven

Reported-by Ronnie Sahlberg <[email protected]>

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

---
  block/iscsi.c |   43 +++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 43 insertions(+)

block/iscsi.c +43 -1

index d0b1a10..fab4c8b 100644
@@ -47,6 +47,9 @@ typedef struct IscsiLun {
      int block_size;
      uint64_t num_blocks;
      int events;
+
+    QEMUTimer *nop_timer;
+    int nops_in_flight;
  } IscsiLun;
 
  typedef struct IscsiAIOCB {

@@ -72,6 +75,9 @@ struct IscsiTask {
      int complete;
  };
 
+#define NOP_INTERVAL 5000
+#define MAX_NOP_FAILURES 3
+
  static void
  iscsi_bh_cb(void *p)
  {

@@ -925,6 +931,35 @@ static char *parse_initiator_name(const char *target)
      }
  }
 
+static void iscsi_nop_cb(struct iscsi_context *iscsi, int status, void *command_data, void *private_data)
+{
+    IscsiLun *iscsilun = private_data;
+
+    if (iscsilun) {
+        iscsilun->nops_in_flight = 0;
+    }
+}
+
+static void iscsi_nop_timed_event(void *opaque)
+{
+    IscsiLun *iscsilun = opaque;
+
+    if (iscsilun->nops_in_flight > MAX_NOP_FAILURES) {
+        error_report("iSCSI: NOP timeout. Reconnecting...");
+        iscsi_reconnect(iscsilun->iscsi);
+        iscsilun->nops_in_flight = 0;
+    }
+
+    if (iscsi_nop_out_async(iscsilun->iscsi, iscsi_nop_cb, NULL, 0, iscsilun) != 0) {
+        error_report("iSCSI: failed to sent NOP-Out. Disabling NOP messages.");
+        return;
+    }
+
+    qemu_mod_timer(iscsilun->nop_timer, qemu_get_clock_ms(rt_clock) + NOP_INTERVAL);
+    iscsi_set_events(iscsilun);
+    iscsilun->nops_in_flight++;
+}
+
  /*
   * We support iscsi url's on the form
   * iscsi://[<username>%<password>@]<host>[:<port>]/<targetname>/<lun>

@@ -1036,6 +1071,10 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags)
 
      ret = 0;
 
+    /* Set up a timer for sending out iSCSI NOPs */
+    iscsilun->nop_timer = qemu_new_timer_ms(rt_clock, iscsi_nop_timed_event, iscsilun);
+    qemu_mod_timer(iscsilun->nop_timer, qemu_get_clock_ms(rt_clock) + NOP_INTERVAL);
+
  out:
      if (initiator_name != NULL) {
          g_free(initiator_name);

@@ -1058,6 +1097,10 @@ static void iscsi_close(BlockDriverState *bs)
      IscsiLun *iscsilun = bs->opaque;
      struct iscsi_context *iscsi = iscsilun->iscsi;
 
+    if (iscsilun->nop_timer) {
+        qemu_del_timer(iscsilun->nop_timer);
+        qemu_free_timer(iscsilun->nop_timer);
+    }
      qemu_aio_set_fd_handler(iscsi_get_fd(iscsi), NULL, NULL, NULL, NULL);
      iscsi_destroy_context(iscsi);
      memset(iscsilun, 0, sizeof(IscsiLun));
--

1.7.9.5

Peter Lieven

author

2012-12-05 08:36 UTC #3 lore raw

In reply to ronnie sahlberg

All, please ignore this patch. During testing we found a problem in live usage. We will resubmit once this is fixed.

Peter

Am 04.12.2012 um 06:03 schrieb ronnie sahlberg:

Quoted 114 lines

Acked-by [email protected] (Ronnie Sahlberg)

This verified that the service is actually operational and is much more reliable than TCP-KEEPALIVES.
This is the proper way to monitor that the iscsi target is alive.

We should as a later patch add the ability to configure this via the qemu config file instead of using hardcoded values.

regards
ronnie sahlberg

Quoted 98 lines · Peter Lieven

Reported-by Ronnie Sahlberg <[email protected]>

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

---
block/iscsi.c | 43 +++++++++++++++++++++++++++++++++++++++++++

1 file changed, 43 insertions(+)

block/iscsi.c +6 -0

index d0b1a10..fab4c8b 100644
@@ -47,6 +47,9 @@ typedef struct IscsiLun {
    int block_size;
    uint64_t num_blocks;
    int events;
+
+    QEMUTimer *nop_timer;
+    int nops_in_flight;
 } IscsiLun;
 
 typedef struct IscsiAIOCB {

@@ -72,6 +75,9 @@ struct IscsiTask {
    int complete;
 };
 
+#define NOP_INTERVAL 5000
+#define MAX_NOP_FAILURES 3
+

static void
iscsi_bh_cb(void *p)
{

block/iscsi.c +33 -0

@@ -925,6 +931,35 @@ static char *parse_initiator_name(const char *target)
    }
 }
 
+static void iscsi_nop_cb(struct iscsi_context *iscsi, int status, void *command_data, void *private_data)
+{
+    IscsiLun *iscsilun = private_data;
+
+    if (iscsilun) {
+        iscsilun->nops_in_flight = 0;
+    }
+}
+
+static void iscsi_nop_timed_event(void *opaque)
+{
+    IscsiLun *iscsilun = opaque;
+
+    if (iscsilun->nops_in_flight > MAX_NOP_FAILURES) {
+        error_report("iSCSI: NOP timeout. Reconnecting...");
+        iscsi_reconnect(iscsilun->iscsi);
+        iscsilun->nops_in_flight = 0;
+    }
+
+    if (iscsi_nop_out_async(iscsilun->iscsi, iscsi_nop_cb, NULL, 0, iscsilun) != 0) {
+        error_report("iSCSI: failed to sent NOP-Out. Disabling NOP messages.");
+        return;
+    }
+
+    qemu_mod_timer(iscsilun->nop_timer, qemu_get_clock_ms(rt_clock) + NOP_INTERVAL);
+    iscsi_set_events(iscsilun);
+    iscsilun->nops_in_flight++;
+}
+
 /*
 * We support iscsi url's on the form
 * iscsi://[<username>%<password>@]<host>[:<port>]/<targetname>/<lun>

@@ -1036,6 +1071,10 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags)
 
    ret = 0;
 
+    /* Set up a timer for sending out iSCSI NOPs */
+    iscsilun->nop_timer = qemu_new_timer_ms(rt_clock, iscsi_nop_timed_event, iscsilun);
+    qemu_mod_timer(iscsilun->nop_timer, qemu_get_clock_ms(rt_clock) + NOP_INTERVAL);
+

out:

    if (initiator_name != NULL) {
        g_free(initiator_name);

block/iscsi.c +4 -1

@@ -1058,6 +1097,10 @@ static void iscsi_close(BlockDriverState *bs)
    IscsiLun *iscsilun = bs->opaque;
    struct iscsi_context *iscsi = iscsilun->iscsi;
 
+    if (iscsilun->nop_timer) {
+        qemu_del_timer(iscsilun->nop_timer);
+        qemu_free_timer(iscsilun->nop_timer);
+    }
    qemu_aio_set_fd_handler(iscsi_get_fd(iscsi), NULL, NULL, NULL, NULL);
    iscsi_destroy_context(iscsi);
    memset(iscsilun, 0, sizeof(IscsiLun));
--

1.7.9.5