From efcf6024765b2ac2027152e50c5c252aea1ea978 Mon Sep 17 00:00:00 2001 From: Peter Lieven Date: Mon, 4 May 2015 16:41:40 +0200 Subject: [PATCH 1/5] nop: print a warning if the oldest element in iscsi->waitpdu queue is stuck Signed-off-by: Peter Lieven --- include/iscsi-private.h | 1 + lib/login.c | 2 +- lib/nop.c | 6 ++++++ 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/include/iscsi-private.h b/include/iscsi-private.h index d1d8d5e..f5efd84 100644 --- a/include/iscsi-private.h +++ b/include/iscsi-private.h @@ -87,6 +87,7 @@ struct iscsi_context { unsigned char isid[6]; uint32_t itt; uint32_t cmdsn; + uint32_t min_cmdsn_waiting; uint32_t expcmdsn; uint32_t maxcmdsn; uint32_t statsn; diff --git a/lib/login.c b/lib/login.c index 9b813d7..81d1d2b 100644 --- a/lib/login.c +++ b/lib/login.c @@ -794,7 +794,7 @@ iscsi_login_async(struct iscsi_context *iscsi, iscsi_command_cb cb, if (!iscsi->current_phase && !iscsi->secneg_phase) { iscsi->itt = (u_int32_t) rand(); iscsi->cmdsn = (u_int32_t) rand(); - iscsi->expcmdsn = iscsi->maxcmdsn = iscsi->cmdsn; + iscsi->expcmdsn = iscsi->maxcmdsn = iscsi->min_cmdsn_waiting = iscsi->cmdsn; } pdu = iscsi_allocate_pdu(iscsi, diff --git a/lib/nop.c b/lib/nop.c index 548e73e..f828822 100644 --- a/lib/nop.c +++ b/lib/nop.c @@ -143,6 +143,12 @@ iscsi_process_nop_out_reply(struct iscsi_context *iscsi, struct iscsi_pdu *pdu, "NOP-In received (pdu->itt %08x, pdu->ttt %08x, iscsi->maxcmdsn %08x, iscsi->expcmdsn %08x, iscsi->statsn %08x)", pdu->itt, 0xffffffff, iscsi->maxcmdsn, iscsi->expcmdsn, iscsi->statsn); + if (iscsi->waitpdu->cmdsn == iscsi->min_cmdsn_waiting) { + ISCSI_LOG(iscsi, 2, "Oldest element in waitqueue is unchanged since last NOP-In (iscsi->min_cmdsn_waiting %08x)", + iscsi->min_cmdsn_waiting); + } + iscsi->min_cmdsn_waiting = iscsi->waitpdu->cmdsn; + iscsi->nops_in_flight = 0; if (pdu->callback == NULL) { From e9d67540e16ed45ec71cd17b9745203cf52e9c98 Mon Sep 17 00:00:00 2001 From: Peter Lieven Date: Mon, 4 May 2015 17:00:08 +0200 Subject: [PATCH 2/5] add LIBISCSI_IGNORE_NOP_OUT_ON_STUCK_WAITPDU_QUEUE environment variable this variable is a helper to tell libiscsi to not reset the nops_in_flight on receival of a NOP-Out if the oldest element in the waitpdu queue is not changed since the last NOP-Out. The idea is that we can detect a command that the target silently dropped by this mechanism and run into a NOP timeout forcing a reconnect. This environment variable is not suitable if a command is send that is taking more than the allowed timeout for a NOP-Out as a reply to a initiator generated NOP-In (or several NOP-Outs if more than 1 in flight is allowed). Signed-off-by: Peter Lieven --- lib/nop.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/nop.c b/lib/nop.c index f828822..f4fec90 100644 --- a/lib/nop.c +++ b/lib/nop.c @@ -21,6 +21,7 @@ #endif #include +#include #include "iscsi.h" #include "iscsi-private.h" @@ -146,11 +147,14 @@ iscsi_process_nop_out_reply(struct iscsi_context *iscsi, struct iscsi_pdu *pdu, if (iscsi->waitpdu->cmdsn == iscsi->min_cmdsn_waiting) { ISCSI_LOG(iscsi, 2, "Oldest element in waitqueue is unchanged since last NOP-In (iscsi->min_cmdsn_waiting %08x)", iscsi->min_cmdsn_waiting); + if (getenv("LIBISCSI_IGNORE_NOP_OUT_ON_STUCK_WAITPDU_QUEUE") == NULL) { + iscsi->nops_in_flight = 0; + } + } else { + iscsi->nops_in_flight = 0; } iscsi->min_cmdsn_waiting = iscsi->waitpdu->cmdsn; - iscsi->nops_in_flight = 0; - if (pdu->callback == NULL) { return 0; } From 724f44c31bb79d75bb6d6a49463ea79c11691297 Mon Sep 17 00:00:00 2001 From: Peter Lieven Date: Tue, 5 May 2015 13:57:10 +0200 Subject: [PATCH 3/5] socket: change the order of POLLIN and POLLOUT in iscsi_service the original idea of writing to the socket before reading was to put data on the wire as early as possible and avoid potential latency increase due to long taking callbacks invoked in iscsi_read_from_socket. Benchmarking with libiscsi userland tools have shown that changing the order increases throughput by about 5%. The reason is that during POLLIN we might receive an updated MaxCmdSN which might us allow to send new data out during POLLOUT. For Qemu the change doesn't matter since POLLIN and POLLOUT are processed independently. Signed-off-by: Peter Lieven --- lib/socket.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/lib/socket.c b/lib/socket.c index 15095e3..da89c5a 100644 --- a/lib/socket.c +++ b/lib/socket.c @@ -879,17 +879,16 @@ iscsi_service(struct iscsi_context *iscsi, int revents) return 0; } - if (revents & POLLOUT && (iscsi->outqueue != NULL || iscsi->outqueue_current != NULL)) { - if (iscsi_write_to_socket(iscsi) != 0) { - return iscsi_service_reconnect_if_loggedin(iscsi); - } - } if (revents & POLLIN) { if (iscsi_read_from_socket(iscsi) != 0) { return iscsi_service_reconnect_if_loggedin(iscsi); } } - + if (revents & POLLOUT) { + if (iscsi_write_to_socket(iscsi) != 0) { + return iscsi_service_reconnect_if_loggedin(iscsi); + } + } return 0; } From b4098c535ebfe904be4c6a966a23b0f7b31d7f97 Mon Sep 17 00:00:00 2001 From: Peter Lieven Date: Tue, 5 May 2015 14:10:44 +0200 Subject: [PATCH 4/5] init: make LIBISCSI_CACHE_ALLOCATIONS a public environment variable this variable was introduced for iscsi-test-cu only. This patch makes it a generic environment variable that can be set per context. Signed-off-by: Peter Lieven --- include/iscsi-private.h | 1 + include/iscsi.h | 2 +- lib/connect.c | 1 + lib/init.c | 14 +++++++++----- test-tool/iscsi-test-cu.c | 5 ----- 5 files changed, 12 insertions(+), 11 deletions(-) diff --git a/include/iscsi-private.h b/include/iscsi-private.h index f5efd84..00f215c 100644 --- a/include/iscsi-private.h +++ b/include/iscsi-private.h @@ -153,6 +153,7 @@ struct iscsi_context { void* smalloc_ptrs[SMALL_ALLOC_MAX_FREE]; int smalloc_free; size_t smalloc_size; + int cache_allocations; time_t next_reconnect; int scsi_timeout; diff --git a/include/iscsi.h b/include/iscsi.h index c8f41bb..7aa2a44 100644 --- a/include/iscsi.h +++ b/include/iscsi.h @@ -51,7 +51,7 @@ struct sockaddr; "[:]\"" -EXTERN void iscsi_set_cache_allocations(int ca); +EXTERN void iscsi_set_cache_allocations(struct iscsi_context *iscsi, int ca); /* * The following three functions are used to integrate libiscsi in an event diff --git a/lib/connect.c b/lib/connect.c index dec2a4b..2fecb6c 100644 --- a/lib/connect.c +++ b/lib/connect.c @@ -411,6 +411,7 @@ int iscsi_reconnect(struct iscsi_context *old_iscsi) iscsi->tcp_keepcnt = old_iscsi->tcp_keepcnt; iscsi->tcp_keepintvl = old_iscsi->tcp_keepintvl; iscsi->tcp_syncnt = old_iscsi->tcp_syncnt; + iscsi->cache_allocations = old_iscsi->cache_allocations; iscsi->reconnect_max_retries = old_iscsi->reconnect_max_retries; diff --git a/lib/init.c b/lib/init.c index 54dd1b1..0f40468 100644 --- a/lib/init.c +++ b/lib/init.c @@ -33,15 +33,13 @@ #include "iscsi-private.h" #include "slist.h" -int cache_allocations = 1; - /** * Whether or not the internal memory allocator caches allocations. Disable * memory allocation caching to improve the accuracy of Valgrind reports. */ -void iscsi_set_cache_allocations(int ca) +void iscsi_set_cache_allocations(struct iscsi_context *iscsi, int ca) { - cache_allocations = ca; + iscsi->cache_allocations = ca; } void* iscsi_malloc(struct iscsi_context *iscsi, size_t size) { @@ -96,7 +94,7 @@ void iscsi_sfree(struct iscsi_context *iscsi, void* ptr) { if (ptr == NULL) { return; } - if (cache_allocations) { + if (iscsi->cache_allocations) { if (iscsi->smalloc_free == SMALL_ALLOC_MAX_FREE) { int i; /* SMALL_ALLOC_MAX_FREE should be adjusted that this */ @@ -122,6 +120,7 @@ iscsi_create_context(const char *initiator_name) { struct iscsi_context *iscsi; size_t required = ISCSI_RAW_HEADER_SIZE + ISCSI_DIGEST_SIZE; + char *ca; if (!initiator_name[0]) { return NULL; @@ -204,6 +203,11 @@ iscsi_create_context(const char *initiator_name) } ISCSI_LOG(iscsi,5,"small allocation size is %d byte", iscsi->smalloc_size); + ca = getenv("LIBISCSI_CACHE_ALLOCATIONS"); + if (!ca || atoi(ca) != 0) { + iscsi->cache_allocations = 1; + } + return iscsi; } diff --git a/test-tool/iscsi-test-cu.c b/test-tool/iscsi-test-cu.c index 7aa90f1..ae236b2 100644 --- a/test-tool/iscsi-test-cu.c +++ b/test-tool/iscsi-test-cu.c @@ -971,11 +971,6 @@ main(int argc, char *argv[]) }; int i, c; int opt_idx = 0; - char *ca; - - ca = getenv("LIBISCSI_CACHE_ALLOCATIONS"); - if (ca && atoi(ca) == 0) - iscsi_set_cache_allocations(0); sd = malloc(sizeof(struct scsi_device)); memset(sd, '\0', sizeof(struct scsi_device)); From a027931a2c1da83c838cccfeaa1114aaa4ec364b Mon Sep 17 00:00:00 2001 From: Peter Lieven Date: Tue, 5 May 2015 14:14:26 +0200 Subject: [PATCH 5/5] init: simplify the case that the freelist is full in iscsi_sfree if the freelist gets full we are likely in a low load situation so we do not need to make all the work copying the pointers. Signed-off-by: Peter Lieven --- lib/init.c | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/lib/init.c b/lib/init.c index 0f40468..5a867e1 100644 --- a/lib/init.c +++ b/lib/init.c @@ -94,24 +94,15 @@ void iscsi_sfree(struct iscsi_context *iscsi, void* ptr) { if (ptr == NULL) { return; } - if (iscsi->cache_allocations) { - if (iscsi->smalloc_free == SMALL_ALLOC_MAX_FREE) { - int i; - /* SMALL_ALLOC_MAX_FREE should be adjusted that this */ - /* happens rarely */ - ISCSI_LOG(iscsi, 6, "smalloc free == SMALLOC_MAX_FREE"); - /* remove oldest half of free pointers and copy - * upper half to lower half */ - iscsi->smalloc_free >>= 1; - for (i = 0; i < iscsi->smalloc_free; i++) { - iscsi_free(iscsi, iscsi->smalloc_ptrs[i]); - iscsi->smalloc_ptrs[i] = - iscsi->smalloc_ptrs[i + iscsi->smalloc_free]; - } - } - iscsi->smalloc_ptrs[iscsi->smalloc_free++] = ptr; - } else { + if (!iscsi->cache_allocations) { iscsi_free(iscsi, ptr); + } else if (iscsi->smalloc_free == SMALL_ALLOC_MAX_FREE) { + /* SMALL_ALLOC_MAX_FREE should be adjusted that this */ + /* happens rarely */ + ISCSI_LOG(iscsi, 6, "smalloc free == SMALLOC_MAX_FREE"); + iscsi_free(iscsi, ptr); + } else { + iscsi->smalloc_ptrs[iscsi->smalloc_free++] = ptr; } }