Re: [Qemu-devel] [PATCH v2] block/iscsi: Adding iser support in Libiscsi-QEMU

[Stefan Hajnoczi]

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 <address@hidden>
> ---
>  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