qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Peter Lieven
Subject: Re: [Qemu-devel] [PATCH v2] block/iscsi: Adding iser support in Libiscsi-QEMU
Date: Tue, 27 Sep 2016 13:39:22 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0

Am 27.09.2016 um 13:37 schrieb Roy Shterman:


On 9/27/2016 1:28 PM, Stefan Hajnoczi wrote:
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.
I will split the patch into two different patches in v3 and resend it.
      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!
will be fixed in v3

Is unescaping a bug fix?  Please put it into a separate patch.
because libvirt is parsing '?' char as %3F, I needed to parse to URI with 
unescaping.

          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?
PaoloB asked to add this option also, in Libiscsi the URI parser checks for 
'iser://' as a different protocol or '?iser' as URI option.
I will add it into the commit message.




When you send a v3 please check your patches with scripts/checkpatch.pl for 
style errors.

Thanks,
Peter


--

Mit freundlichen Grüßen

Peter Lieven

...........................................................

  KAMP Netzwerkdienste GmbH
  Vestische Str. 89-91 | 46117 Oberhausen
  Tel: +49 (0) 208.89 402-50 | Fax: +49 (0) 208.89 402-40
  address@hidden | http://www.kamp.de

  Geschäftsführer: Heiner Lante | Michael Lante
  Amtsgericht Duisburg | HRB Nr. 12154
  USt-Id-Nr.: DE 120607556

...........................................................




reply via email to

[Prev in Thread] Current Thread [Next in Thread]