From b81e9a28a6db88b2b166b95036dc1d0c5df9392e Mon Sep 17 00:00:00 2001 From: Peter Lieven Date: Mon, 2 Jan 2017 15:52:19 +0100 Subject: [PATCH 1/3] Batch pdu read in function iscsi_read_from_socket() iscsi_read_from_socket can currently only read one PDU in each iscsi_service invocation even if there is more data available on the socket. This patch reads all PDUs until the socket would block. It enqueues all complete read PDUs and then processes them in order of arrival. Signed-off-by: Peter Lieven --- include/iscsi-private.h | 1 + lib/socket.c | 170 ++++++++++++++++++++-------------------- 2 files changed, 87 insertions(+), 84 deletions(-) diff --git a/include/iscsi-private.h b/include/iscsi-private.h index b91af60..5d26847 100644 --- a/include/iscsi-private.h +++ b/include/iscsi-private.h @@ -119,6 +119,7 @@ struct iscsi_context { int tcp_keepintvl; int tcp_keepidle; int tcp_syncnt; + int tcp_nonblocking; int current_phase; int next_phase; diff --git a/lib/socket.c b/lib/socket.c index 036b4f4..b225eaf 100644 --- a/lib/socket.c +++ b/lib/socket.c @@ -126,15 +126,15 @@ void iscsi_decrement_iface_rr() { iface_rr--; } -static void set_nonblocking(int fd) +static int set_nonblocking(int fd) { #if defined(WIN32) unsigned long opt = 1; - ioctlsocket(fd, FIONBIO, &opt); + return ioctlsocket(fd, FIONBIO, &opt); #else unsigned v; v = fcntl(fd, F_GETFL, 0); - fcntl(fd, F_SETFL, v | O_NONBLOCK); + return fcntl(fd, F_SETFL, v | O_NONBLOCK); #endif } @@ -203,7 +203,7 @@ static int iscsi_tcp_connect(struct iscsi_context *iscsi, union socket_address * iscsi->fd = iscsi->old_iscsi->fd; } - set_nonblocking(iscsi->fd); + iscsi->tcp_nonblocking = !set_nonblocking(iscsi->fd); iscsi_set_tcp_keepalive(iscsi, iscsi->tcp_keepidle, iscsi->tcp_keepcnt, iscsi->tcp_keepintvl); @@ -568,98 +568,101 @@ iscsi_read_from_socket(struct iscsi_context *iscsi) struct iscsi_in_pdu *in; ssize_t data_size, count, padding_size; - if (iscsi->incoming == NULL) { - iscsi->incoming = iscsi_szmalloc(iscsi, sizeof(struct iscsi_in_pdu)); - iscsi->incoming->hdr = iscsi_szmalloc(iscsi, ISCSI_RAW_HEADER_SIZE + ISCSI_DIGEST_SIZE); + do { if (iscsi->incoming == NULL) { - iscsi_set_error(iscsi, "Out-of-memory: failed to malloc iscsi_in_pdu"); - return -1; - } - } - in = iscsi->incoming; - - /* first we must read the header, including any digests */ - if (in->hdr_pos < ISCSI_HEADER_SIZE) { - /* try to only read the header, the socket is nonblocking, so - * no need to limit the read to what is available in the socket - */ - count = ISCSI_HEADER_SIZE - in->hdr_pos; - count = recv(iscsi->fd, &in->hdr[in->hdr_pos], count, 0); - if (count == 0) { - return -1; - } - if (count < 0) { - if (errno == EINTR || errno == EAGAIN) { - return 0; + iscsi->incoming = iscsi_szmalloc(iscsi, sizeof(struct iscsi_in_pdu)); + iscsi->incoming->hdr = iscsi_szmalloc(iscsi, ISCSI_RAW_HEADER_SIZE + ISCSI_DIGEST_SIZE); + if (iscsi->incoming == NULL) { + iscsi_set_error(iscsi, "Out-of-memory: failed to malloc iscsi_in_pdu"); + return -1; } - iscsi_set_error(iscsi, "read from socket failed, " - "errno:%d", errno); - return -1; } - in->hdr_pos += count; - } + in = iscsi->incoming; - if (in->hdr_pos < ISCSI_HEADER_SIZE) { - /* we don't have the full header yet, so return */ - return 0; - } - - padding_size = iscsi_get_pdu_padding_size(&in->hdr[0]); - data_size = iscsi_get_pdu_data_size(&in->hdr[0]) + padding_size; - - if (data_size < 0 || data_size > (ssize_t)iscsi->initiator_max_recv_data_segment_length) { - iscsi_set_error(iscsi, "Invalid data size received from target (%d)", (int)data_size); - return -1; - } - if (data_size != 0) { - unsigned char padding_buf[3]; - unsigned char *buf = padding_buf; - struct scsi_iovector * iovector_in; - - count = data_size - in->data_pos; - - /* first try to see if we already have a user buffer */ - iovector_in = iscsi_get_scsi_task_iovector_in(iscsi, in); - if (iovector_in != NULL && count > padding_size) { - uint32_t offset = scsi_get_uint32(&in->hdr[40]); - count = iscsi_iovector_readv_writev(iscsi, iovector_in, in->data_pos + offset, count - padding_size, 0); - } else { - if (iovector_in == NULL) { - if (in->data == NULL) { - in->data = iscsi_malloc(iscsi, data_size); - if (in->data == NULL) { - iscsi_set_error(iscsi, "Out-of-memory: failed to malloc iscsi_in_pdu->data(%d)", (int)data_size); - return -1; - } + /* first we must read the header, including any digests */ + if (in->hdr_pos < ISCSI_HEADER_SIZE) { + /* try to only read the header, the socket is nonblocking, so + * no need to limit the read to what is available in the socket + */ + count = ISCSI_HEADER_SIZE - in->hdr_pos; + count = recv(iscsi->fd, &in->hdr[in->hdr_pos], count, 0); + if (count == 0) { + /* remote side has closed the socket. */ + return -1; + } + if (count < 0) { + if (errno == EINTR || errno == EAGAIN) { + break; } - buf = &in->data[in->data_pos]; + iscsi_set_error(iscsi, "read from socket failed, " + "errno:%d", errno); + return -1; } - count = recv(iscsi->fd, buf, count, 0); + in->hdr_pos += count; } - - if (count == 0) { + + if (in->hdr_pos < ISCSI_HEADER_SIZE) { + /* we don't have the full header yet, so return */ + break; + } + + padding_size = iscsi_get_pdu_padding_size(&in->hdr[0]); + data_size = iscsi_get_pdu_data_size(&in->hdr[0]) + padding_size; + + if (data_size < 0 || data_size > (ssize_t)iscsi->initiator_max_recv_data_segment_length) { + iscsi_set_error(iscsi, "Invalid data size received from target (%d)", (int)data_size); return -1; } - if (count < 0) { - if (errno == EINTR || errno == EAGAIN) { - return 0; + if (data_size != 0) { + unsigned char padding_buf[3]; + unsigned char *buf = padding_buf; + struct scsi_iovector * iovector_in; + + count = data_size - in->data_pos; + + /* first try to see if we already have a user buffer */ + iovector_in = iscsi_get_scsi_task_iovector_in(iscsi, in); + if (iovector_in != NULL && count > padding_size) { + uint32_t offset = scsi_get_uint32(&in->hdr[40]); + count = iscsi_iovector_readv_writev(iscsi, iovector_in, in->data_pos + offset, count - padding_size, 0); + } else { + if (iovector_in == NULL) { + if (in->data == NULL) { + in->data = iscsi_malloc(iscsi, data_size); + if (in->data == NULL) { + iscsi_set_error(iscsi, "Out-of-memory: failed to malloc iscsi_in_pdu->data(%d)", (int)data_size); + return -1; + } + } + buf = &in->data[in->data_pos]; + } + count = recv(iscsi->fd, buf, count, 0); } - iscsi_set_error(iscsi, "read from socket failed, " - "errno:%d %s", errno, - iscsi_get_error(iscsi)); - return -1; + + if (count == 0) { + /* remote side has closed the socket. */ + return -1; + } + if (count < 0) { + if (errno == EINTR || errno == EAGAIN) { + break; + } + iscsi_set_error(iscsi, "read from socket failed, " + "errno:%d %s", errno, + iscsi_get_error(iscsi)); + return -1; + } + + in->data_pos += count; } - in->data_pos += count; - } - - if (in->data_pos < data_size) { - return 0; - } - - ISCSI_LIST_ADD_END(&iscsi->inqueue, in); - iscsi->incoming = NULL; + if (in->data_pos < data_size) { + break; + } + ISCSI_LIST_ADD_END(&iscsi->inqueue, in); + iscsi->incoming = NULL; + } while (iscsi->is_loggedin && iscsi->tcp_nonblocking); while (iscsi->inqueue != NULL) { struct iscsi_in_pdu *current = iscsi->inqueue; @@ -671,7 +674,6 @@ iscsi_read_from_socket(struct iscsi_context *iscsi) iscsi_free_iscsi_in_pdu(iscsi, current); } - return 0; } From e058e825bff228b64f564696be2085e7a1fe9154 Mon Sep 17 00:00:00 2001 From: Peter Lieven Date: Tue, 3 Jan 2017 11:13:06 +0100 Subject: [PATCH 2/3] socket: break receive loop if there are no more outstanding PDUs If the length of iscsi->waitpdu and iscsi->inqueue are the same then except for any target initiated NOPs or async messages we should have received any and all possible pdus from this socket and can abort early. This avoids running the loop one more time just to fail with EAGAIN at the recs/readv. Just avoiding that recv/readv syscall will shave at least 10us off this function and thus the latency. Suggested-by: Ronnie Sahlberg Signed-off-by: Peter Lieven --- lib/socket.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/socket.c b/lib/socket.c index b225eaf..9ee86a4 100644 --- a/lib/socket.c +++ b/lib/socket.c @@ -567,6 +567,9 @@ iscsi_read_from_socket(struct iscsi_context *iscsi) { struct iscsi_in_pdu *in; ssize_t data_size, count, padding_size; + int waitpdu_len, inqueue_len = 0; + + ISCSI_LIST_LENGTH(&iscsi->waitpdu, waitpdu_len); do { if (iscsi->incoming == NULL) { @@ -638,7 +641,6 @@ iscsi_read_from_socket(struct iscsi_context *iscsi) } count = recv(iscsi->fd, buf, count, 0); } - if (count == 0) { /* remote side has closed the socket. */ return -1; @@ -652,7 +654,6 @@ iscsi_read_from_socket(struct iscsi_context *iscsi) iscsi_get_error(iscsi)); return -1; } - in->data_pos += count; } @@ -661,8 +662,9 @@ iscsi_read_from_socket(struct iscsi_context *iscsi) } ISCSI_LIST_ADD_END(&iscsi->inqueue, in); + inqueue_len++; iscsi->incoming = NULL; - } while (iscsi->is_loggedin && iscsi->tcp_nonblocking); + } while (iscsi->tcp_nonblocking && inqueue_len < waitpdu_len && iscsi->is_loggedin); while (iscsi->inqueue != NULL) { struct iscsi_in_pdu *current = iscsi->inqueue; From eb7c1d9b0c163036fca796096a61f0df05c3ea5f Mon Sep 17 00:00:00 2001 From: Peter Lieven Date: Fri, 6 Jan 2017 11:48:17 +0100 Subject: [PATCH 3/3] socket: process PDUs directly after receiving them this eliminated the need for an inqueue Signed-off-by: Peter Lieven --- include/iscsi-private.h | 2 -- lib/connect.c | 3 --- lib/init.c | 3 --- lib/socket.c | 28 ++++------------------------ 4 files changed, 4 insertions(+), 32 deletions(-) diff --git a/include/iscsi-private.h b/include/iscsi-private.h index 5d26847..ad97601 100644 --- a/include/iscsi-private.h +++ b/include/iscsi-private.h @@ -61,7 +61,6 @@ struct iscsi_in_pdu { unsigned char *data; }; void iscsi_free_iscsi_in_pdu(struct iscsi_context *iscsi, struct iscsi_in_pdu *in); -void iscsi_free_iscsi_inqueue(struct iscsi_context *iscsi, struct iscsi_in_pdu *inqueue); /* size of chap response field */ #define CHAP_R_SIZE 16 @@ -143,7 +142,6 @@ struct iscsi_context { struct iscsi_pdu *waitpdu; struct iscsi_in_pdu *incoming; - struct iscsi_in_pdu *inqueue; uint32_t max_burst_length; uint32_t first_burst_length; diff --git a/lib/connect.c b/lib/connect.c index 7815973..7faeed5 100644 --- a/lib/connect.c +++ b/lib/connect.c @@ -370,9 +370,6 @@ void iscsi_reconnect_cb(struct iscsi_context *iscsi _U_, int status, if (old_iscsi->incoming != NULL) { iscsi_free_iscsi_in_pdu(old_iscsi, old_iscsi->incoming); } - if (old_iscsi->inqueue != NULL) { - iscsi_free_iscsi_inqueue(old_iscsi, old_iscsi->inqueue); - } if (old_iscsi->outqueue_current != NULL && old_iscsi->outqueue_current->flags & ISCSI_PDU_DELETE_WHEN_SENT) { iscsi->drv->free_pdu(old_iscsi, old_iscsi->outqueue_current); diff --git a/lib/init.c b/lib/init.c index 9941ceb..6aa9ea4 100644 --- a/lib/init.c +++ b/lib/init.c @@ -371,9 +371,6 @@ iscsi_destroy_context(struct iscsi_context *iscsi) if (iscsi->incoming != NULL) { iscsi_free_iscsi_in_pdu(iscsi, iscsi->incoming); } - if (iscsi->inqueue != NULL) { - iscsi_free_iscsi_inqueue(iscsi, iscsi->inqueue); - } iscsi->connect_data = NULL; diff --git a/lib/socket.c b/lib/socket.c index 9ee86a4..19c1148 100644 --- a/lib/socket.c +++ b/lib/socket.c @@ -567,9 +567,6 @@ iscsi_read_from_socket(struct iscsi_context *iscsi) { struct iscsi_in_pdu *in; ssize_t data_size, count, padding_size; - int waitpdu_len, inqueue_len = 0; - - ISCSI_LIST_LENGTH(&iscsi->waitpdu, waitpdu_len); do { if (iscsi->incoming == NULL) { @@ -661,20 +658,13 @@ iscsi_read_from_socket(struct iscsi_context *iscsi) break; } - ISCSI_LIST_ADD_END(&iscsi->inqueue, in); - inqueue_len++; iscsi->incoming = NULL; - } while (iscsi->tcp_nonblocking && inqueue_len < waitpdu_len && iscsi->is_loggedin); - - while (iscsi->inqueue != NULL) { - struct iscsi_in_pdu *current = iscsi->inqueue; - - if (iscsi_process_pdu(iscsi, current) != 0) { + if (iscsi_process_pdu(iscsi, in) != 0) { + iscsi_free_iscsi_in_pdu(iscsi, in); return -1; } - ISCSI_LIST_REMOVE(&iscsi->inqueue, current); - iscsi_free_iscsi_in_pdu(iscsi, current); - } + iscsi_free_iscsi_in_pdu(iscsi, in); + } while (iscsi->tcp_nonblocking && iscsi->waitpdu && iscsi->is_loggedin); return 0; } @@ -990,16 +980,6 @@ iscsi_free_iscsi_in_pdu(struct iscsi_context *iscsi, struct iscsi_in_pdu *in) in=NULL; } -void -iscsi_free_iscsi_inqueue(struct iscsi_context *iscsi, struct iscsi_in_pdu *inqueue) -{ - while (inqueue != NULL) { - struct iscsi_in_pdu *next = inqueue->next; - iscsi_free_iscsi_in_pdu(iscsi, inqueue); - inqueue = next; - } -} - void iscsi_set_tcp_syncnt(struct iscsi_context *iscsi, int value) { iscsi->tcp_syncnt=value;