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 <p...@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 <p...@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
>
>
