[Qemu-devel] [PATCH] block/iscsi: add support for request timeouts

[Qemu-devel] [PATCH] block/iscsi: add support for request timeouts

[Peter Lieven]

libiscsi starting with 1.15 will properly support timeout of iscsi
commands. The default will remain no timeout, but this can
be changed via cmdline parameters, e.g.:

qemu -iscsi timeout=30 -drive file=iscsi://...

If a timeout occurs a reconnect is scheduled and the timed out command
will be requeued for processing after a successful reconnect.

The required API call iscsi_set_timeout is present since libiscsi
1.10 which was released in October 2013. However, due to some bugs
in the libiscsi code the use is not recommended before version 1.15.

Please note that this patch bumps the libiscsi requirement to 1.10
to have all function and macros defined. The patch fixes also a
off-by-one error in the NOP timeout calculation which was fixed
while touching these code parts.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/iscsi.c   | 87 ++++++++++++++++++++++++++++++++++++++++++---------------
 configure       |  6 ++--
 qemu-options.hx |  4 +++
 3 files changed, 72 insertions(+), 25 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 14e97a6..f19a56a 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -69,6 +69,7 @@ typedef struct IscsiLun {
     bool dpofua;
     bool has_write_same;
     bool force_next_flush;
+    bool request_timed_out;
 } IscsiLun;
 
 typedef struct IscsiTask {
@@ -99,7 +100,8 @@ typedef struct IscsiAIOCB {
 #endif
 } IscsiAIOCB;
 
-#define EVENT_INTERVAL 250
+/* libiscsi uses time_t so its enough to process events every second */
+#define EVENT_INTERVAL 1000
 #define NOP_INTERVAL 5000
 #define MAX_NOP_FAILURES 3
 #define ISCSI_CMD_RETRIES ARRAY_SIZE(iscsi_retry_times)
@@ -186,13 +188,18 @@ iscsi_co_generic_cb(struct iscsi_context *iscsi, int status,
                 iTask->do_retry = 1;
                 goto out;
             }
-            /* status 0x28 is SCSI_TASK_SET_FULL. It was first introduced
-             * in libiscsi 1.10.0. Hardcode this value here to avoid
-             * the need to bump the libiscsi requirement to 1.10.0 */
-            if (status == SCSI_STATUS_BUSY || status == 0x28) {
+            if (status == SCSI_STATUS_BUSY || status == SCSI_STATUS_TIMEOUT ||
+                status == SCSI_STATUS_TASK_SET_FULL) {
                 unsigned retry_time =
                     exp_random(iscsi_retry_times[iTask->retries - 1]);
-                error_report("iSCSI Busy/TaskSetFull (retry #%u in %u ms): %s",
+                if (status == SCSI_STATUS_TIMEOUT) {
+                    /* make sure the request is rescheduled AFTER the
+                     * reconnect is initiated */
+                    retry_time = EVENT_INTERVAL * 2;
+                    iTask->iscsilun->request_timed_out = true;
+                }
+                error_report("iSCSI Busy/TaskSetFull/TimeOut"
+                             " (retry #%u in %u ms): %s",
                              iTask->retries, retry_time,
                              iscsi_get_error(iscsi));
                 aio_timer_init(iTask->iscsilun->aio_context,
@@ -276,20 +283,26 @@ iscsi_set_events(IscsiLun *iscsilun)
                            iscsilun);
         iscsilun->events = ev;
     }
-
-    /* newer versions of libiscsi may return zero events. In this
-     * case start a timer to ensure we are able to return to service
-     * once this situation changes. */
-    if (!ev) {
-        timer_mod(iscsilun->event_timer,
-                  qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + EVENT_INTERVAL);
-    }
 }
 
-static void iscsi_timed_set_events(void *opaque)
+static void iscsi_timed_check_events(void *opaque)
 {
     IscsiLun *iscsilun = opaque;
+
+    /* check for timed out requests */
+    iscsi_service(iscsilun->iscsi, 0);
+
+    if (iscsilun->request_timed_out) {
+        iscsilun->request_timed_out = false;
+        iscsi_reconnect(iscsilun->iscsi);
+    }
+
+    /* newer versions of libiscsi may return zero events. Ensure we are able
+     * to return to service once this situation changes. */
     iscsi_set_events(iscsilun);
+
+    timer_mod(iscsilun->event_timer,
+              qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + EVENT_INTERVAL);
 }
 
 static void
@@ -1096,16 +1109,37 @@ static char *parse_initiator_name(const char *target)
     return iscsi_name;
 }
 
+static int parse_timeout(const char *target)
+{
+    QemuOptsList *list;
+    QemuOpts *opts;
+    const char *timeout;
+
+    list = qemu_find_opts("iscsi");
+    if (list) {
+        opts = qemu_opts_find(list, target);
+        if (!opts) {
+            opts = QTAILQ_FIRST(&list->head);
+        }
+        if (opts) {
+            timeout = qemu_opt_get(opts, "timeout");
+            if (timeout) {
+                return atoi(timeout);
+            }
+        }
+    }
+
+    return 0;
+}
+
 static void iscsi_nop_timed_event(void *opaque)
 {
     IscsiLun *iscsilun = opaque;
 
-    if (iscsi_get_nops_in_flight(iscsilun->iscsi) > MAX_NOP_FAILURES) {
+    if (iscsi_get_nops_in_flight(iscsilun->iscsi) >= MAX_NOP_FAILURES) {
         error_report("iSCSI: NOP timeout. Reconnecting...");
-        iscsi_reconnect(iscsilun->iscsi);
-    }
-
-    if (iscsi_nop_out_async(iscsilun->iscsi, NULL, NULL, 0, NULL) != 0) {
+        iscsilun->request_timed_out = true;
+    } else if (iscsi_nop_out_async(iscsilun->iscsi, NULL, NULL, 0, NULL) != 0) {
         error_report("iSCSI: failed to sent NOP-Out. Disabling NOP messages.");
         return;
     }
@@ -1263,10 +1297,13 @@ static void iscsi_attach_aio_context(BlockDriverState *bs,
     timer_mod(iscsilun->nop_timer,
               qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + NOP_INTERVAL);
 
-    /* Prepare a timer for a delayed call to iscsi_set_events */
+    /* Set up a timer for periodic calls to iscsi_set_events and to
+     * scan for command timeout */
     iscsilun->event_timer = aio_timer_new(iscsilun->aio_context,
                                           QEMU_CLOCK_REALTIME, SCALE_MS,
-                                          iscsi_timed_set_events, iscsilun);
+                                          iscsi_timed_check_events, iscsilun);
+    timer_mod(iscsilun->event_timer,
+              qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + EVENT_INTERVAL);
 }
 
 static void iscsi_modesense_sync(IscsiLun *iscsilun)
@@ -1391,6 +1428,8 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
         goto out;
     }
 
+    iscsi_set_timeout(iscsi, parse_timeout(iscsi_url->target));
+
     if (iscsi_full_connect_sync(iscsi, iscsi_url->portal, iscsi_url->lun) != 0) {
         error_setg(errp, "iSCSI: Failed to connect to LUN : %s",
             iscsi_get_error(iscsi));
@@ -1739,6 +1778,10 @@ static QemuOptsList qemu_iscsi_opts = {
             .name = "initiator-name",
             .type = QEMU_OPT_STRING,
             .help = "Initiator iqn name to use when connecting",
+        },{
+            .name = "timeout",
+            .type = QEMU_OPT_NUMBER,
+            .help = "Request timeout in seconds (default 0 = no timeout)",
         },
         { /* end of list */ }
     },
diff --git a/configure b/configure
index 222694f..2ed9db2 100755
--- a/configure
+++ b/configure
@@ -3660,15 +3660,15 @@ if compile_prog "" "" ; then
 fi
 
 ##########################################
-# Do we have libiscsi >= 1.9.0
+# Do we have libiscsi >= 1.10.0
 if test "$libiscsi" != "no" ; then
-  if $pkg_config --atleast-version=1.9.0 libiscsi; then
+  if $pkg_config --atleast-version=1.10.0 libiscsi; then
     libiscsi="yes"
     libiscsi_cflags=$($pkg_config --cflags libiscsi)
     libiscsi_libs=$($pkg_config --libs libiscsi)
   else
     if test "$libiscsi" = "yes" ; then
-      feature_not_found "libiscsi" "Install libiscsi >= 1.9.0"
+      feature_not_found "libiscsi" "Install libiscsi >= 1.10.0"
     fi
     libiscsi="no"
   fi
diff --git a/qemu-options.hx b/qemu-options.hx
index bf1683e..ca37481 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2291,6 +2291,9 @@ By default qemu will use the iSCSI initiator-name
 'iqn.2008-11.org.linux-kvm[:<name>]' but this can also be set from the command
 line or a configuration file.
 
+Since version Qemu 2.4 it is possible to specify a iSCSI request timeout to detect
+stalled requests and force a reestablishment of the session. The timeout
+is specified in seconds. The default is 0 which means no timeout.
 
 Example (without authentication):
 @example
@@ -2318,6 +2321,7 @@ DEF("iscsi", HAS_ARG, QEMU_OPTION_iscsi,
     "-iscsi [user=user][,password=password]\n"
     "       [,header-digest=CRC32C|CR32C-NONE|NONE-CRC32C|NONE\n"
     "       [,initiator-name=initiator-iqn][,id=target-iqn]\n"
+    "       [,timeout=timeout]\n"
     "                iSCSI session parameters\n", QEMU_ARCH_ALL)
 STEXI
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH] block/iscsi: add support for request timeouts

[ronnie sahlberg]

[-- Attachment #1: Type: text/plain, Size: 9363 bytes --]

LGTM

It is good to finally have timeouts that work in libiscsi,  and a consumer
that can use and benefit from it.

On Tue, Jun 16, 2015 at 4:45 AM, Peter Lieven <pl@kamp.de> wrote:

> libiscsi starting with 1.15 will properly support timeout of iscsi
> commands. The default will remain no timeout, but this can
> be changed via cmdline parameters, e.g.:
>
> qemu -iscsi timeout=30 -drive file=iscsi://...
>
> If a timeout occurs a reconnect is scheduled and the timed out command
> will be requeued for processing after a successful reconnect.
>
> The required API call iscsi_set_timeout is present since libiscsi
> 1.10 which was released in October 2013. However, due to some bugs
> in the libiscsi code the use is not recommended before version 1.15.
>
> Please note that this patch bumps the libiscsi requirement to 1.10
> to have all function and macros defined. The patch fixes also a
> off-by-one error in the NOP timeout calculation which was fixed
> while touching these code parts.
>
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/iscsi.c   | 87
> ++++++++++++++++++++++++++++++++++++++++++---------------
>  configure       |  6 ++--
>  qemu-options.hx |  4 +++
>  3 files changed, 72 insertions(+), 25 deletions(-)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 14e97a6..f19a56a 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -69,6 +69,7 @@ typedef struct IscsiLun {
>      bool dpofua;
>      bool has_write_same;
>      bool force_next_flush;
> +    bool request_timed_out;
>  } IscsiLun;
>
>  typedef struct IscsiTask {
> @@ -99,7 +100,8 @@ typedef struct IscsiAIOCB {
>  #endif
>  } IscsiAIOCB;
>
> -#define EVENT_INTERVAL 250
> +/* libiscsi uses time_t so its enough to process events every second */
> +#define EVENT_INTERVAL 1000
>  #define NOP_INTERVAL 5000
>  #define MAX_NOP_FAILURES 3
>  #define ISCSI_CMD_RETRIES ARRAY_SIZE(iscsi_retry_times)
> @@ -186,13 +188,18 @@ iscsi_co_generic_cb(struct iscsi_context *iscsi, int
> status,
>                  iTask->do_retry = 1;
>                  goto out;
>              }
> -            /* status 0x28 is SCSI_TASK_SET_FULL. It was first introduced
> -             * in libiscsi 1.10.0. Hardcode this value here to avoid
> -             * the need to bump the libiscsi requirement to 1.10.0 */
> -            if (status == SCSI_STATUS_BUSY || status == 0x28) {
> +            if (status == SCSI_STATUS_BUSY || status ==
> SCSI_STATUS_TIMEOUT ||
> +                status == SCSI_STATUS_TASK_SET_FULL) {
>                  unsigned retry_time =
>                      exp_random(iscsi_retry_times[iTask->retries - 1]);
> -                error_report("iSCSI Busy/TaskSetFull (retry #%u in %u
> ms): %s",
> +                if (status == SCSI_STATUS_TIMEOUT) {
> +                    /* make sure the request is rescheduled AFTER the
> +                     * reconnect is initiated */
> +                    retry_time = EVENT_INTERVAL * 2;
> +                    iTask->iscsilun->request_timed_out = true;
> +                }
> +                error_report("iSCSI Busy/TaskSetFull/TimeOut"
> +                             " (retry #%u in %u ms): %s",
>                               iTask->retries, retry_time,
>                               iscsi_get_error(iscsi));
>                  aio_timer_init(iTask->iscsilun->aio_context,
> @@ -276,20 +283,26 @@ iscsi_set_events(IscsiLun *iscsilun)
>                             iscsilun);
>          iscsilun->events = ev;
>      }
> -
> -    /* newer versions of libiscsi may return zero events. In this
> -     * case start a timer to ensure we are able to return to service
> -     * once this situation changes. */
> -    if (!ev) {
> -        timer_mod(iscsilun->event_timer,
> -                  qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
> EVENT_INTERVAL);
> -    }
>  }
>
> -static void iscsi_timed_set_events(void *opaque)
> +static void iscsi_timed_check_events(void *opaque)
>  {
>      IscsiLun *iscsilun = opaque;
> +
> +    /* check for timed out requests */
> +    iscsi_service(iscsilun->iscsi, 0);
> +
> +    if (iscsilun->request_timed_out) {
> +        iscsilun->request_timed_out = false;
> +        iscsi_reconnect(iscsilun->iscsi);
> +    }
> +
> +    /* newer versions of libiscsi may return zero events. Ensure we are
> able
> +     * to return to service once this situation changes. */
>      iscsi_set_events(iscsilun);
> +
> +    timer_mod(iscsilun->event_timer,
> +              qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + EVENT_INTERVAL);
>  }
>
>  static void
> @@ -1096,16 +1109,37 @@ static char *parse_initiator_name(const char
> *target)
>      return iscsi_name;
>  }
>
> +static int parse_timeout(const char *target)
> +{
> +    QemuOptsList *list;
> +    QemuOpts *opts;
> +    const char *timeout;
> +
> +    list = qemu_find_opts("iscsi");
> +    if (list) {
> +        opts = qemu_opts_find(list, target);
> +        if (!opts) {
> +            opts = QTAILQ_FIRST(&list->head);
> +        }
> +        if (opts) {
> +            timeout = qemu_opt_get(opts, "timeout");
> +            if (timeout) {
> +                return atoi(timeout);
> +            }
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  static void iscsi_nop_timed_event(void *opaque)
>  {
>      IscsiLun *iscsilun = opaque;
>
> -    if (iscsi_get_nops_in_flight(iscsilun->iscsi) > MAX_NOP_FAILURES) {
> +    if (iscsi_get_nops_in_flight(iscsilun->iscsi) >= MAX_NOP_FAILURES) {
>          error_report("iSCSI: NOP timeout. Reconnecting...");
> -        iscsi_reconnect(iscsilun->iscsi);
> -    }
> -
> -    if (iscsi_nop_out_async(iscsilun->iscsi, NULL, NULL, 0, NULL) != 0) {
> +        iscsilun->request_timed_out = true;
> +    } else if (iscsi_nop_out_async(iscsilun->iscsi, NULL, NULL, 0, NULL)
> != 0) {
>          error_report("iSCSI: failed to sent NOP-Out. Disabling NOP
> messages.");
>          return;
>      }
> @@ -1263,10 +1297,13 @@ static void
> iscsi_attach_aio_context(BlockDriverState *bs,
>      timer_mod(iscsilun->nop_timer,
>                qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + NOP_INTERVAL);
>
> -    /* Prepare a timer for a delayed call to iscsi_set_events */
> +    /* Set up a timer for periodic calls to iscsi_set_events and to
> +     * scan for command timeout */
>      iscsilun->event_timer = aio_timer_new(iscsilun->aio_context,
>                                            QEMU_CLOCK_REALTIME, SCALE_MS,
> -                                          iscsi_timed_set_events,
> iscsilun);
> +                                          iscsi_timed_check_events,
> iscsilun);
> +    timer_mod(iscsilun->event_timer,
> +              qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + EVENT_INTERVAL);
>  }
>
>  static void iscsi_modesense_sync(IscsiLun *iscsilun)
> @@ -1391,6 +1428,8 @@ static int iscsi_open(BlockDriverState *bs, QDict
> *options, int flags,
>          goto out;
>      }
>
> +    iscsi_set_timeout(iscsi, parse_timeout(iscsi_url->target));
> +
>      if (iscsi_full_connect_sync(iscsi, iscsi_url->portal, iscsi_url->lun)
> != 0) {
>          error_setg(errp, "iSCSI: Failed to connect to LUN : %s",
>              iscsi_get_error(iscsi));
> @@ -1739,6 +1778,10 @@ static QemuOptsList qemu_iscsi_opts = {
>              .name = "initiator-name",
>              .type = QEMU_OPT_STRING,
>              .help = "Initiator iqn name to use when connecting",
> +        },{
> +            .name = "timeout",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "Request timeout in seconds (default 0 = no timeout)",
>          },
>          { /* end of list */ }
>      },
> diff --git a/configure b/configure
> index 222694f..2ed9db2 100755
> --- a/configure
> +++ b/configure
> @@ -3660,15 +3660,15 @@ if compile_prog "" "" ; then
>  fi
>
>  ##########################################
> -# Do we have libiscsi >= 1.9.0
> +# Do we have libiscsi >= 1.10.0
>  if test "$libiscsi" != "no" ; then
> -  if $pkg_config --atleast-version=1.9.0 libiscsi; then
> +  if $pkg_config --atleast-version=1.10.0 libiscsi; then
>      libiscsi="yes"
>      libiscsi_cflags=$($pkg_config --cflags libiscsi)
>      libiscsi_libs=$($pkg_config --libs libiscsi)
>    else
>      if test "$libiscsi" = "yes" ; then
> -      feature_not_found "libiscsi" "Install libiscsi >= 1.9.0"
> +      feature_not_found "libiscsi" "Install libiscsi >= 1.10.0"
>      fi
>      libiscsi="no"
>    fi
> diff --git a/qemu-options.hx b/qemu-options.hx
> index bf1683e..ca37481 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2291,6 +2291,9 @@ By default qemu will use the iSCSI initiator-name
>  'iqn.2008-11.org.linux-kvm[:<name>]' but this can also be set from the
> command
>  line or a configuration file.
>
> +Since version Qemu 2.4 it is possible to specify a iSCSI request timeout
> to detect
> +stalled requests and force a reestablishment of the session. The timeout
> +is specified in seconds. The default is 0 which means no timeout.
>
>  Example (without authentication):
>  @example
> @@ -2318,6 +2321,7 @@ DEF("iscsi", HAS_ARG, QEMU_OPTION_iscsi,
>      "-iscsi [user=user][,password=password]\n"
>      "       [,header-digest=CRC32C|CR32C-NONE|NONE-CRC32C|NONE\n"
>      "       [,initiator-name=initiator-iqn][,id=target-iqn]\n"
> +    "       [,timeout=timeout]\n"
>      "                iSCSI session parameters\n", QEMU_ARCH_ALL)
>  STEXI
>
> --
> 1.9.1
>
>

[-- Attachment #2: Type: text/html, Size: 11565 bytes --]

Re: [Qemu-devel] [PATCH] block/iscsi: add support for request timeouts

[Peter Lieven]

Am 23.06.2015 um 01:03 schrieb ronnie sahlberg:
> LGTM
>
> It is good to finally have timeouts that work in libiscsi,  and a consumer that can use and benefit from it.

Paolo, Kevin, Stefan, do you think this is sth for 2.4?

Peter

Re: [Qemu-devel] [Qemu-block] [PATCH] block/iscsi: add support for request timeouts

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1242 bytes --]

On Tue, Jun 16, 2015 at 01:45:07PM +0200, Peter Lieven wrote:
> libiscsi starting with 1.15 will properly support timeout of iscsi
> commands. The default will remain no timeout, but this can
> be changed via cmdline parameters, e.g.:
> 
> qemu -iscsi timeout=30 -drive file=iscsi://...
> 
> If a timeout occurs a reconnect is scheduled and the timed out command
> will be requeued for processing after a successful reconnect.
> 
> The required API call iscsi_set_timeout is present since libiscsi
> 1.10 which was released in October 2013. However, due to some bugs
> in the libiscsi code the use is not recommended before version 1.15.
> 
> Please note that this patch bumps the libiscsi requirement to 1.10
> to have all function and macros defined. The patch fixes also a
> off-by-one error in the NOP timeout calculation which was fixed
> while touching these code parts.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/iscsi.c   | 87 ++++++++++++++++++++++++++++++++++++++++++---------------
>  configure       |  6 ++--
>  qemu-options.hx |  4 +++
>  3 files changed, 72 insertions(+), 25 deletions(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] block/iscsi: add support for request timeouts

[Paolo Bonzini]

On 16/06/2015 13:45, Peter Lieven wrote:
> libiscsi starting with 1.15 will properly support timeout of iscsi
> commands. The default will remain no timeout, but this can
> be changed via cmdline parameters, e.g.:
> 
> qemu -iscsi timeout=30 -drive file=iscsi://...
> 
> If a timeout occurs a reconnect is scheduled and the timed out command
> will be requeued for processing after a successful reconnect.
> 
> The required API call iscsi_set_timeout is present since libiscsi
> 1.10 which was released in October 2013. However, due to some bugs
> in the libiscsi code the use is not recommended before version 1.15.

If so, QEMU should not allow it if libiscsi is older than 1.15.

> Please note that this patch bumps the libiscsi requirement to 1.10
> to have all function and macros defined.

This is not acceptable, unfortunately.  I explained this two months ago
(https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg01847.html)
and it is still true.

libiscsi keeps breaking ABI compatibility and for a while did not even
bump the soname when they do.  This makes it completely impossible for
distros to upgrade to a newer libiscsi, and RHEL7 is thus stuck with 1.9.

Yes, it is 2 years old.  It doesn't matter.  If libiscsi upstream only
_tried_ to preserve ABI compatibility, they wouldn't be in this
situation.  And I know that it is not even trying, because it broke
again sometime between 1.11 and 1.14 for a totally trivial reason:

--- a/iscsi/iscsi.h
+++ b/iscsi/iscsi.h
@@ -91,6 +136,8 @@ struct iscsi_url {
        char target[MAX_STRING_SIZE + 1];
        char user[MAX_STRING_SIZE + 1];
        char passwd[MAX_STRING_SIZE + 1];
+       char target_user[MAX_STRING_SIZE + 1];
+       char target_passwd[MAX_STRING_SIZE + 1];
        int lun;
        struct iscsi_context *iscsi;
 };


This is the only change between these releases that breaks the ABI, but
it is already one too much. :(

(Also, the parsing of URLs into iscsi_url doesn't even try to obey the
RFCs...).

> The patch fixes also a
> off-by-one error in the NOP timeout calculation which was fixed
> while touching these code parts.

Can you please separate this part anyway?

Paolo

Re: [Qemu-devel] [PATCH] block/iscsi: add support for request timeouts

[Peter Lieven]

Am 25.06.2015 um 23:08 schrieb Paolo Bonzini:
>
> On 16/06/2015 13:45, Peter Lieven wrote:
>> libiscsi starting with 1.15 will properly support timeout of iscsi
>> commands. The default will remain no timeout, but this can
>> be changed via cmdline parameters, e.g.:
>>
>> qemu -iscsi timeout=30 -drive file=iscsi://...
>>
>> If a timeout occurs a reconnect is scheduled and the timed out command
>> will be requeued for processing after a successful reconnect.
>>
>> The required API call iscsi_set_timeout is present since libiscsi
>> 1.10 which was released in October 2013. However, due to some bugs
>> in the libiscsi code the use is not recommended before version 1.15.
> If so, QEMU should not allow it if libiscsi is older than 1.15.

Not accept a timeout parameter or ignore it and print a warning?

>
>> Please note that this patch bumps the libiscsi requirement to 1.10
>> to have all function and macros defined.
> This is not acceptable, unfortunately.  I explained this two months ago
> (https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg01847.html)
> and it is still true.

Sorry, i missed that. Can you verify if 1.15.0 has a soname that
makes it possible to jump to 1.15.0 at some point in the future?

>
> libiscsi keeps breaking ABI compatibility and for a while did not even
> bump the soname when they do.  This makes it completely impossible for
> distros to upgrade to a newer libiscsi, and RHEL7 is thus stuck with 1.9.
>
> Yes, it is 2 years old.  It doesn't matter.  If libiscsi upstream only
> _tried_ to preserve ABI compatibility, they wouldn't be in this
> situation.  And I know that it is not even trying, because it broke
> again sometime between 1.11 and 1.14 for a totally trivial reason:
>
> --- a/iscsi/iscsi.h
> +++ b/iscsi/iscsi.h
> @@ -91,6 +136,8 @@ struct iscsi_url {
>         char target[MAX_STRING_SIZE + 1];
>         char user[MAX_STRING_SIZE + 1];
>         char passwd[MAX_STRING_SIZE + 1];
> +       char target_user[MAX_STRING_SIZE + 1];
> +       char target_passwd[MAX_STRING_SIZE + 1];
>         int lun;
>         struct iscsi_context *iscsi;
>  };
>
>
> This is the only change between these releases that breaks the ABI, but
> it is already one too much. :(
>
> (Also, the parsing of URLs into iscsi_url doesn't even try to obey the
> RFCs...).
>
>> The patch fixes also a
>> off-by-one error in the NOP timeout calculation which was fixed
>> while touching these code parts.
> Can you please separate this part anyway?

Sure.

I will send a v2 that is compatible with 1.9.0 and enables the timeout
stuff only for libiscsi >= 1.15.0

Peter

Re: [Qemu-devel] [PATCH] block/iscsi: add support for request timeouts

[Paolo Bonzini]

On 25/06/2015 23:21, Peter Lieven wrote:
> I will send a v2 that is compatible with 1.9.0 and enables the timeout
> stuff only for libiscsi >= 1.15.0

Since Stefan has already applied the patch, restoring 1.9.0
compatibility (and disabling timeouts between 1.9.0 and 1.14.x) on top
of this patch would be enough.

Thanks for your understanding!

Paolo