From 55f76cfb0cdba9a52357117495ecccba6c8cffcc Mon Sep 17 00:00:00 2001 From: Peter Lieven Date: Wed, 21 Nov 2012 17:02:59 +0100 Subject: [PATCH 1/2] SCSI add support for iovectors If an application passes buffers to libiscsi for reading they are currently added to a linked list one by one. This leads to a malloc for each buffer object plus O(n) walks trough the list to add the buffer to then end of the list. Additionally the buffer read routine takes up to O(n) iterations to find the right buffer for a request. This patch introduces an scsi_iovector struct to pass buffers to an scsi task. Adding a new buffer is in O(1) and finding the right buffer to also. Malloc requirements are in O(log(n)). Additionally the scsi_iovector struct is itended to be binary compatible to an QEMUIOVector allowing to pass this structure directly to the library. Initial tests have been made booting an Ubuntu LTS 12.04.1 Desktop server up to the login prompt. The following observations have been made with regards to scsi_malloc calls: original implementation: ~11.500 mallocs using iovector instead of list: ~ 7.500 mallocs passing the iovector directly: 0 mallocs To enable this feature in qemu for testing the following patch might be used: diff --git a/block/iscsi.c b/block/iscsi.c index a6a819d..2809c15 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -390,11 +390,16 @@ iscsi_aio_readv(BlockDriverState *bs, int64_t sector_num, return NULL; } +#if defined(LIBISCSI_FEATURE_IOVECTOR) + assert(sizeof(struct QEMUIOVector) == sizeof(struct scsi_iovector)); + scsi_iovector_assign(acb->task, (struct scsi_iovector*) acb->qiov); +#else for (i = 0; i < acb->qiov->niov; i++) { scsi_task_add_data_in_buffer(acb->task, acb->qiov->iov[i].iov_len, acb->qiov->iov[i].iov_base); } +#endif iscsi_set_events(iscsilun); --- Signed-off-by: Peter Lieven --- include/iscsi.h | 3 + include/scsi-lowlevel.h | 20 ++++++- lib/init.c | 2 + lib/libiscsi.def | 1 + lib/libiscsi.syms | 1 + lib/scsi-lowlevel.c | 129 ++++++++++++++++++++++++++-------------- 6 files changed, 112 insertions(+), 44 deletions(-) diff --git a/include/iscsi.h b/include/iscsi.h index 3aadafd..b893ce7 100644 --- a/include/iscsi.h +++ b/include/iscsi.h @@ -33,6 +33,9 @@ extern "C" { struct iscsi_context; struct sockaddr; +/* FEATURES */ +#define LIBISCSI_FEATURE_IOVECTOR (1) + #define MAX_STRING_SIZE (255) /* diff --git a/include/scsi-lowlevel.h b/include/scsi-lowlevel.h index 0e1a78e..7c173c7 100644 --- a/include/scsi-lowlevel.h +++ b/include/scsi-lowlevel.h @@ -197,6 +197,20 @@ enum scsi_residual { SCSI_RESIDUAL_OVERFLOW }; +#if !defined(_SYS_UIO_H) && !defined(CONFIG_IOVEC) +struct iovec { + void *iov_base; + size_t iov_len; +}; +#endif + +struct scsi_iovector { + struct iovec *iov; + int niov; + int nalloc; + size_t size; +}; + struct scsi_task { int status; @@ -217,7 +231,9 @@ struct scsi_task { uint32_t cmdsn; uint32_t lun; - struct scsi_data_buffer *in_buffers; + size_t buffers_offset; + int buffers_consumed; + struct scsi_iovector *buffers; }; /* This function will free a scsi task structure. @@ -707,6 +723,8 @@ EXTERN struct scsi_task *scsi_cdb_report_supported_opcodes(int report_timeouts, void *scsi_malloc(struct scsi_task *task, size_t size); +EXTERN void scsi_iovector_assign(struct scsi_task *task, struct scsi_iovector *iov); + #ifdef __cplusplus } #endif diff --git a/lib/init.c b/lib/init.c index 86ae172..521f4a2 100644 --- a/lib/init.c +++ b/lib/init.c @@ -32,6 +32,8 @@ #include "iscsi.h" #include "iscsi-private.h" #include "slist.h" +#include "scsi-lowlevel.h" + inline void* iscsi_malloc(struct iscsi_context *iscsi, size_t size) { void * ptr = malloc(size); diff --git a/lib/libiscsi.def b/lib/libiscsi.def index cbc374b..d24f487 100644 --- a/lib/libiscsi.def +++ b/lib/libiscsi.def @@ -176,4 +176,5 @@ scsi_sense_ascq_str scsi_sense_key_str scsi_set_task_private_ptr scsi_task_add_data_in_buffer +scsi_iovector_assign scsi_version_to_str diff --git a/lib/libiscsi.syms b/lib/libiscsi.syms index 19408a9..48b53d2 100644 --- a/lib/libiscsi.syms +++ b/lib/libiscsi.syms @@ -174,4 +174,5 @@ scsi_sense_ascq_str scsi_sense_key_str scsi_set_task_private_ptr scsi_task_add_data_in_buffer +scsi_iovector_assign scsi_version_to_str diff --git a/lib/scsi-lowlevel.c b/lib/scsi-lowlevel.c index 7a9c9b3..3cb2eb2 100644 --- a/lib/scsi-lowlevel.c +++ b/lib/scsi-lowlevel.c @@ -2533,58 +2533,101 @@ scsi_get_task_private_ptr(struct scsi_task *task) return task->ptr; } +void +scsi_iovector_assign(struct scsi_task *task, struct scsi_iovector *iov) +{ + task->buffers = iov; +} +#define IOVECTOR_INITAL_ALLOC (16) -struct scsi_data_buffer { - struct scsi_data_buffer *next; - uint32_t len; - unsigned char *data; -}; +int +scsi_iovector_add(struct scsi_task *task, int len, unsigned char *buf) +{ + if (len < 0) { + return -1; + } + + if (task->buffers == NULL) { + task->buffers = scsi_malloc(task, sizeof(struct scsi_iovector) + + IOVECTOR_INITAL_ALLOC*sizeof(struct iovec) + 15); + if (task->buffers == NULL) { + return -1; + } + + memset(task->buffers, 0, sizeof(struct scsi_iovector)); + /* align by 16 bytes */ + task->buffers->iov = (struct iovec *) (((uintptr_t)task->buffers + + sizeof(struct scsi_iovector) + 15)&~0xf); + task->buffers->nalloc = IOVECTOR_INITAL_ALLOC; + } + + /* iovec allocation is too small */ + if (task->buffers->nalloc < task->buffers->niov + 1) { + struct iovec *old_iov = task->buffers->iov; + task->buffers->iov = scsi_malloc(task, 2 * task->buffers->nalloc * sizeof(struct iovec)); + if (task->buffers->iov == NULL) { + return -1; + } + if (old_iov != NULL) { + memcpy(task->buffers->iov, old_iov, task->buffers->niov * sizeof(struct iovec)); + } + task->buffers->nalloc <<= 1; + } + + task->buffers->iov[task->buffers->niov].iov_len = len; + task->buffers->iov[task->buffers->niov].iov_base = buf; + task->buffers->niov++; + task->buffers->size += len; + + return 0; +} + +unsigned char * +scsi_iovector_get_buffer(struct scsi_task *task, uint32_t pos, ssize_t *count) +{ + if (task->buffers == NULL) { + return NULL; + } + + if (pos == 0 && count == NULL) return task->buffers->iov[0].iov_base; + + if (task->buffers->niov <= task->buffers_consumed) { + /* someone issued a read but did not provide enough user buffers for all the data. + * maybe someone tried to read just 512 bytes off a MMC device? + */ + return NULL; + } + + pos-= task->buffers_offset; + + struct iovec *iov = &task->buffers->iov[task->buffers_consumed]; + + while (pos >= iov->iov_len) { + task->buffers_offset += iov->iov_len; + task->buffers_consumed++; + pos -= iov->iov_len; + if (task->buffers->niov <= task->buffers_consumed) { + return NULL; + } + iov = &task->buffers->iov[task->buffers_consumed]; + } + + if (count && *count >= (ssize_t)(iov->iov_len - pos)) { + *count = iov->iov_len - pos; + } + + return (unsigned char *) iov->iov_base + pos; +} int scsi_task_add_data_in_buffer(struct scsi_task *task, int len, unsigned char *buf) { - struct scsi_data_buffer *data_buf; - - if (len < 0) { - return -1; - } - data_buf = scsi_malloc(task, sizeof(struct scsi_data_buffer)); - if (data_buf == NULL) { - return -1; - } - - data_buf->len = len; - data_buf->data = buf; - - SLIST_ADD_END(&task->in_buffers, data_buf); - return 0; + return scsi_iovector_add(task, len, buf); } unsigned char * scsi_task_get_data_in_buffer(struct scsi_task *task, uint32_t pos, ssize_t *count) { - struct scsi_data_buffer *sdb; - - sdb = task->in_buffers; - if (sdb == NULL) { - return NULL; - } - - while (pos >= sdb->len) { - pos -= sdb->len; - sdb = sdb->next; - if (sdb == NULL) { - /* someone issued a read but did not provide enough user buffers for all the data. - * maybe someone tried to read just 512 bytes off a MMC device? - */ - return NULL; - } - } - - if (count && *count > (ssize_t)(sdb->len - pos)) { - *count = sdb->len - pos; - } - - return &sdb->data[pos]; + return scsi_iovector_get_buffer(task, pos, count); } From e7cc6dc1cad5430380cf9c85f81ac0414b3aa30c Mon Sep 17 00:00:00 2001 From: Peter Lieven Date: Fri, 23 Nov 2012 15:43:00 +0100 Subject: [PATCH 2/2] SCSI add support for POSIX compatible iovectos This patch defines an scsi_iovec struct which is guaranteed to be POSIX compatible. It furthermore adds support for in+out iovectors for bi-directional operations Signed-off-by: Peter Lieven --- include/iscsi.h | 5 +++ include/scsi-lowlevel.h | 17 ++++---- lib/init.c | 1 - lib/libiscsi.def | 4 +- lib/libiscsi.syms | 4 +- lib/scsi-lowlevel.c | 92 ++++++++++++++++++++++++----------------- 6 files changed, 74 insertions(+), 49 deletions(-) diff --git a/include/iscsi.h b/include/iscsi.h index b893ce7..cddd298 100644 --- a/include/iscsi.h +++ b/include/iscsi.h @@ -950,6 +950,11 @@ iscsi_report_supported_opcodes_sync(struct iscsi_context *iscsi, int lun, * task->datain.data will be NULL */ EXTERN int scsi_task_add_data_in_buffer(struct scsi_task *task, int len, unsigned char *buf); +EXTERN int scsi_task_add_data_out_buffer(struct scsi_task *task, int len, unsigned char *buf); + +struct scsi_iovec; +EXTERN void scsi_task_set_iov_out(struct scsi_task *task, struct scsi_iovec *iov, int niov); +EXTERN void scsi_task_set_iov_in(struct scsi_task *task, struct scsi_iovec *iov, int niov); /* * This function is used when you want to cancel a scsi task. diff --git a/include/scsi-lowlevel.h b/include/scsi-lowlevel.h index 7c173c7..40707b4 100644 --- a/include/scsi-lowlevel.h +++ b/include/scsi-lowlevel.h @@ -197,18 +197,20 @@ enum scsi_residual { SCSI_RESIDUAL_OVERFLOW }; -#if !defined(_SYS_UIO_H) && !defined(CONFIG_IOVEC) -struct iovec { +/* struct scsi_iovec follows the POSIX struct iovec + definition and *MUST* never change. */ +struct scsi_iovec { void *iov_base; size_t iov_len; }; -#endif struct scsi_iovector { - struct iovec *iov; + struct scsi_iovec *iov; int niov; int nalloc; size_t size; + size_t offset; + int consumed; }; struct scsi_task { @@ -231,9 +233,8 @@ struct scsi_task { uint32_t cmdsn; uint32_t lun; - size_t buffers_offset; - int buffers_consumed; - struct scsi_iovector *buffers; + struct scsi_iovector iovector_in; + struct scsi_iovector iovector_out; }; /* This function will free a scsi task structure. @@ -723,8 +724,6 @@ EXTERN struct scsi_task *scsi_cdb_report_supported_opcodes(int report_timeouts, void *scsi_malloc(struct scsi_task *task, size_t size); -EXTERN void scsi_iovector_assign(struct scsi_task *task, struct scsi_iovector *iov); - #ifdef __cplusplus } #endif diff --git a/lib/init.c b/lib/init.c index 521f4a2..d5c4351 100644 --- a/lib/init.c +++ b/lib/init.c @@ -32,7 +32,6 @@ #include "iscsi.h" #include "iscsi-private.h" #include "slist.h" -#include "scsi-lowlevel.h" inline void* iscsi_malloc(struct iscsi_context *iscsi, size_t size) { diff --git a/lib/libiscsi.def b/lib/libiscsi.def index d24f487..ea01803 100644 --- a/lib/libiscsi.def +++ b/lib/libiscsi.def @@ -176,5 +176,7 @@ scsi_sense_ascq_str scsi_sense_key_str scsi_set_task_private_ptr scsi_task_add_data_in_buffer -scsi_iovector_assign +scsi_task_add_data_out_buffer +scsi_task_set_iov_in +scsi_task_set_iov_out scsi_version_to_str diff --git a/lib/libiscsi.syms b/lib/libiscsi.syms index 48b53d2..3344061 100644 --- a/lib/libiscsi.syms +++ b/lib/libiscsi.syms @@ -174,5 +174,7 @@ scsi_sense_ascq_str scsi_sense_key_str scsi_set_task_private_ptr scsi_task_add_data_in_buffer -scsi_iovector_assign +scsi_task_add_data_out_buffer +scsi_task_set_iov_in +scsi_task_set_iov_out scsi_version_to_str diff --git a/lib/scsi-lowlevel.c b/lib/scsi-lowlevel.c index 3cb2eb2..ab2940f 100644 --- a/lib/scsi-lowlevel.c +++ b/lib/scsi-lowlevel.c @@ -2534,83 +2534,89 @@ scsi_get_task_private_ptr(struct scsi_task *task) } void -scsi_iovector_assign(struct scsi_task *task, struct scsi_iovector *iov) +scsi_task_set_iov_out(struct scsi_task *task, struct scsi_iovec *iov, int niov) { - task->buffers = iov; + task->iovector_out.iov = iov; + task->iovector_out.niov = niov; +} + +void +scsi_task_set_iov_in(struct scsi_task *task, struct scsi_iovec *iov, int niov) +{ + task->iovector_in.iov = iov; + task->iovector_in.niov = niov; } #define IOVECTOR_INITAL_ALLOC (16) int -scsi_iovector_add(struct scsi_task *task, int len, unsigned char *buf) +scsi_iovector_add(struct scsi_task *task, struct scsi_iovector *iovector, int len, unsigned char *buf) { if (len < 0) { return -1; } - if (task->buffers == NULL) { - task->buffers = scsi_malloc(task, sizeof(struct scsi_iovector) - + IOVECTOR_INITAL_ALLOC*sizeof(struct iovec) + 15); - if (task->buffers == NULL) { + if (iovector->iov == NULL) { + iovector->iov = scsi_malloc(task, IOVECTOR_INITAL_ALLOC*sizeof(struct iovec)); + if (iovector->iov == NULL) { return -1; } - - memset(task->buffers, 0, sizeof(struct scsi_iovector)); - /* align by 16 bytes */ - task->buffers->iov = (struct iovec *) (((uintptr_t)task->buffers + - sizeof(struct scsi_iovector) + 15)&~0xf); - task->buffers->nalloc = IOVECTOR_INITAL_ALLOC; + iovector->nalloc = IOVECTOR_INITAL_ALLOC; } /* iovec allocation is too small */ - if (task->buffers->nalloc < task->buffers->niov + 1) { - struct iovec *old_iov = task->buffers->iov; - task->buffers->iov = scsi_malloc(task, 2 * task->buffers->nalloc * sizeof(struct iovec)); - if (task->buffers->iov == NULL) { + if (iovector->nalloc < iovector->niov + 1) { + struct scsi_iovec *old_iov = iovector->iov; + iovector->iov = scsi_malloc(task, 2 * iovector->nalloc * sizeof(struct iovec)); + if (iovector->iov == NULL) { return -1; } - if (old_iov != NULL) { - memcpy(task->buffers->iov, old_iov, task->buffers->niov * sizeof(struct iovec)); - } - task->buffers->nalloc <<= 1; + memcpy(iovector->iov, old_iov, iovector->niov * sizeof(struct iovec)); + iovector->nalloc <<= 1; } - task->buffers->iov[task->buffers->niov].iov_len = len; - task->buffers->iov[task->buffers->niov].iov_base = buf; - task->buffers->niov++; - task->buffers->size += len; + iovector->iov[iovector->niov].iov_len = len; + iovector->iov[iovector->niov].iov_base = buf; + iovector->niov++; + iovector->size += len; return 0; } unsigned char * -scsi_iovector_get_buffer(struct scsi_task *task, uint32_t pos, ssize_t *count) +scsi_iovector_get_buffer(struct scsi_iovector *iovector, uint32_t pos, ssize_t *count) { - if (task->buffers == NULL) { + if (iovector->iov == NULL) { return NULL; } - if (pos == 0 && count == NULL) return task->buffers->iov[0].iov_base; + if (pos == 0 && count == NULL) return iovector->iov[0].iov_base; - if (task->buffers->niov <= task->buffers_consumed) { + if (pos < iovector->offset) { + /* start over in case we are going backwards */ + iovector->offset = 0; + iovector->consumed = 0; + } + + if (iovector->niov <= iovector->consumed) { /* someone issued a read but did not provide enough user buffers for all the data. * maybe someone tried to read just 512 bytes off a MMC device? */ return NULL; } - pos-= task->buffers_offset; + struct scsi_iovec *iov = &iovector->iov[iovector->consumed]; - struct iovec *iov = &task->buffers->iov[task->buffers_consumed]; + pos-= iovector->offset; while (pos >= iov->iov_len) { - task->buffers_offset += iov->iov_len; - task->buffers_consumed++; + iovector->offset += iov->iov_len; + iovector->consumed++; pos -= iov->iov_len; - if (task->buffers->niov <= task->buffers_consumed) { + if (iovector->niov <= iovector->consumed) { return NULL; } - iov = &task->buffers->iov[task->buffers_consumed]; + iov = &iovector->iov[iovector->consumed]; } if (count && *count >= (ssize_t)(iov->iov_len - pos)) { @@ -2623,11 +2629,23 @@ scsi_iovector_get_buffer(struct scsi_task *task, uint32_t pos, ssize_t *count) int scsi_task_add_data_in_buffer(struct scsi_task *task, int len, unsigned char *buf) { - return scsi_iovector_add(task, len, buf); + return scsi_iovector_add(task, &task->iovector_in, len, buf); } unsigned char * scsi_task_get_data_in_buffer(struct scsi_task *task, uint32_t pos, ssize_t *count) { - return scsi_iovector_get_buffer(task, pos, count); + return scsi_iovector_get_buffer(&task->iovector_in, pos, count); +} + +int +scsi_task_add_data_out_buffer(struct scsi_task *task, int len, unsigned char *buf) +{ + return scsi_iovector_add(task, &task->iovector_out, len, buf); +} + +unsigned char * +scsi_task_get_data_out_buffer(struct scsi_task *task, uint32_t pos, ssize_t *count) +{ + return scsi_iovector_get_buffer(&task->iovector_out, pos, count); }