On Wed, Sep 21, 2016 at 09:19:44PM +0300, Roy Shterman wrote:
> iSER is a new transport layer supported in Libiscsi,
> iSER provides a zero-copy RDMA capable interface that can
> improve performance.
>
> New API is introduced in abstracion of the Libiscsi transport layer.
> In order to use the new iSER transport, one need to add the ?iser option
> at the end of Libiscsi URI.
>
> For now iSER memory buffers are pre-allocated and pre-registered,
> hence in order to work with iSER from QEMU, one need to enable MEMLOCK
> attribute in the VM to be large enough for all iSER buffers and RDMA
> resources.
>
> A new functionallity is also introduced in this commit, a new API
> to deploy zero-copy command submission. iSER is differing from TCP in
> data-path, hence IO vectors must be transferred already when queueing
> the PDU.
>
> changes from v1:
> - Adding iser as an additional block driver
>
> Signed-off-by: Roy Shterman <ro...@mellanox.com>
> ---
> block/iscsi.c | 80
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 files changed, 76 insertions(+), 4 deletions(-)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 95ce9e1..ac2ef1c 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -41,6 +41,7 @@
> #include "qapi/qmp/qstring.h"
> #include "crypto/secret.h"
>
> +#include "qemu/uri.h"
> #include <iscsi/iscsi.h>
> #include <iscsi/scsi-lowlevel.h>
>
> @@ -595,6 +596,18 @@ iscsi_co_writev_flags(BlockDriverState *bs, int64_t
> sector_num, int nb_sectors,
> iscsi_co_init_iscsitask(iscsilun, &iTask);
> retry:
> if (iscsilun->use_16_for_rw) {
> +#if LIBISCSI_API_VERSION >= (20160603)
> + iTask.task = iscsi_write16_iov_task(iscsilun->iscsi, iscsilun->lun,
> lba,
> + NULL, num_sectors *
> iscsilun->block_size,
> + iscsilun->block_size, 0, 0, fua,
> 0, 0,
> + iscsi_co_generic_cb, &iTask,
> (struct scsi_iovec *)iov->iov, iov->niov);
> + } else {
> + iTask.task = iscsi_write10_iov_task(iscsilun->iscsi, iscsilun->lun,
> lba,
> + NULL, num_sectors *
> iscsilun->block_size,
> + iscsilun->block_size, 0, 0, fua,
> 0, 0,
> + iscsi_co_generic_cb, &iTask,
> (struct scsi_iovec *)iov->iov, iov->niov);
> + }
> +#else
> iTask.task = iscsi_write16_task(iscsilun->iscsi, iscsilun->lun, lba,
> NULL, num_sectors *
> iscsilun->block_size,
> iscsilun->block_size, 0, 0, fua, 0,
> 0,
> @@ -605,11 +618,14 @@ retry:
> iscsilun->block_size, 0, 0, fua, 0,
> 0,
> iscsi_co_generic_cb, &iTask);
> }
> +#endif
> if (iTask.task == NULL) {
> return -ENOMEM;
> }
> +#if LIBISCSI_API_VERSION < (20160603)
> scsi_task_set_iov_out(iTask.task, (struct scsi_iovec *) iov->iov,
> iov->niov);
> +#endif
> while (!iTask.complete) {
> iscsi_set_events(iscsilun);
> qemu_coroutine_yield();
> @@ -792,6 +808,19 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState
> *bs,
> iscsi_co_init_iscsitask(iscsilun, &iTask);
> retry:
> if (iscsilun->use_16_for_rw) {
> +#if LIBISCSI_API_VERSION >= (20160603)
> + iTask.task = iscsi_read16_iov_task(iscsilun->iscsi, iscsilun->lun,
> lba,
> + num_sectors *
> iscsilun->block_size,
> + iscsilun->block_size, 0, 0, 0, 0,
> 0,
> + iscsi_co_generic_cb, &iTask,
> (struct scsi_iovec *)iov->iov, iov->niov);
> + } else {
> + iTask.task = iscsi_read10_iov_task(iscsilun->iscsi, iscsilun->lun,
> lba,
> + num_sectors *
> iscsilun->block_size,
> + iscsilun->block_size,
> + 0, 0, 0, 0, 0,
> + iscsi_co_generic_cb, &iTask,
> (struct scsi_iovec *)iov->iov, iov->niov);
> + }
> +#else
> iTask.task = iscsi_read16_task(iscsilun->iscsi, iscsilun->lun, lba,
> num_sectors * iscsilun->block_size,
> iscsilun->block_size, 0, 0, 0, 0, 0,
> @@ -803,11 +832,13 @@ retry:
> 0, 0, 0, 0, 0,
> iscsi_co_generic_cb, &iTask);
> }
> +#endif
> if (iTask.task == NULL) {
> return -ENOMEM;
> }
> +#if LIBISCSI_API_VERSION < (20160603)
> scsi_task_set_iov_in(iTask.task, (struct scsi_iovec *) iov->iov,
> iov->niov);
> -
> +#endif
These iov changes appear to be independent of iSER. Please split them
into a separate patch. Keeping logical changes in separate patches
makes it easier to review, bisect, backport, etc.
> while (!iTask.complete) {
> iscsi_set_events(iscsilun);
> qemu_coroutine_yield();
> @@ -1592,9 +1623,9 @@ static int iscsi_open(BlockDriverState *bs, QDict
> *options, int flags,
>
> filename = qemu_opt_get(opts, "filename");
>
> - iscsi_url = iscsi_parse_full_url(iscsi, filename);
> + iscsi_url = iscsi_parse_full_url(iscsi, uri_string_unescape(filename,
> -1, NULL));
> if (iscsi_url == NULL) {
> - error_setg(errp, "Failed to parse URL : %s", filename);
> + error_setg(errp, "Failed to parse URL : %s",
> uri_string_unescape(filename, -1, NULL));
uri_string_unescape() returns a newly allocated string. This is a
memory leak!
Is unescaping a bug fix? Please put it into a separate patch.
> ret = -EINVAL;
> goto out;
> }
> @@ -1609,7 +1640,13 @@ static int iscsi_open(BlockDriverState *bs, QDict
> *options, int flags,
> ret = -ENOMEM;
> goto out;
> }
> -
> +#if LIBISCSI_API_VERSION >= (20160603)
> + if (iscsi_init_transport(iscsi, iscsi_url->transport)) {
> + error_setg(errp, ("Error initializing transport."));
> + ret = -EINVAL;
> + goto out;
> + }
> +#endif
> if (iscsi_set_targetname(iscsi, iscsi_url->target)) {
> error_setg(errp, "iSCSI: Failed to set target name.");
> ret = -EINVAL;
> @@ -2010,6 +2047,40 @@ static BlockDriver bdrv_iscsi = {
> .bdrv_attach_aio_context = iscsi_attach_aio_context,
> };
>
> +static BlockDriver bdrv_iser = {
> + .format_name = "iser",
> + .protocol_name = "iser",
> +
> + .instance_size = sizeof(IscsiLun),
> + .bdrv_needs_filename = true,
> + .bdrv_file_open = iscsi_open,
> + .bdrv_close = iscsi_close,
> + .bdrv_create = iscsi_create,
> + .create_opts = &iscsi_create_opts,
> + .bdrv_reopen_prepare = iscsi_reopen_prepare,
> + .bdrv_reopen_commit = iscsi_reopen_commit,
> + .bdrv_invalidate_cache = iscsi_invalidate_cache,
> +
> + .bdrv_getlength = iscsi_getlength,
> + .bdrv_get_info = iscsi_get_info,
> + .bdrv_truncate = iscsi_truncate,
> + .bdrv_refresh_limits = iscsi_refresh_limits,
> +
> + .bdrv_co_get_block_status = iscsi_co_get_block_status,
> + .bdrv_co_pdiscard = iscsi_co_pdiscard,
> + .bdrv_co_pwrite_zeroes = iscsi_co_pwrite_zeroes,
> + .bdrv_co_readv = iscsi_co_readv,
> + .bdrv_co_writev_flags = iscsi_co_writev_flags,
> + .bdrv_co_flush_to_disk = iscsi_co_flush,
> +
> +#ifdef __linux__
> + .bdrv_aio_ioctl = iscsi_aio_ioctl,
> +#endif
> +
> + .bdrv_detach_aio_context = iscsi_detach_aio_context,
> + .bdrv_attach_aio_context = iscsi_attach_aio_context,
> +};
The commit description says ?iser needs to be added to the URI. Why is
it necessary to define a new "iser" protocol block driver?
signature.asc
Description: PGP signature
