* [Qemu-devel] [PATCH v2] block/iscsi: Adding iser support in Libiscsi-QEMU
@ 2016-09-21 18:19 Roy Shterman
2016-09-21 18:23 ` no-reply
2016-09-27 10:28 ` Stefan Hajnoczi
0 siblings, 2 replies; 8+ messages in thread
From: Roy Shterman @ 2016-09-21 18:19 UTC (permalink / raw)
To: qemu-devel; +Cc: roysh, roy.shterman, ronniesahlberg, pl, pbonzini
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 <roysh@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
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));
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,
+};
+
static QemuOptsList qemu_iscsi_opts = {
.name = "iscsi",
.head = QTAILQ_HEAD_INITIALIZER(qemu_iscsi_opts.head),
@@ -2048,6 +2119,7 @@ static QemuOptsList qemu_iscsi_opts = {
static void iscsi_block_init(void)
{
bdrv_register(&bdrv_iscsi);
+ bdrv_register(&bdrv_iser);
qemu_add_opts(&qemu_iscsi_opts);
}
--
1.7.8.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] block/iscsi: Adding iser support in Libiscsi-QEMU
2016-09-21 18:19 [Qemu-devel] [PATCH v2] block/iscsi: Adding iser support in Libiscsi-QEMU Roy Shterman
@ 2016-09-21 18:23 ` no-reply
2016-09-27 10:28 ` Stefan Hajnoczi
1 sibling, 0 replies; 8+ messages in thread
From: no-reply @ 2016-09-21 18:23 UTC (permalink / raw)
To: roysh; +Cc: famz, qemu-devel, pl, roy.shterman, ronniesahlberg, pbonzini
Hi,
Your series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 1474481984-10452-1-git-send-email-roysh@mellanox.com
Subject: [Qemu-devel] [PATCH v2] block/iscsi: Adding iser support in Libiscsi-QEMU
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
* [new tag] patchew/1474481984-10452-1-git-send-email-roysh@mellanox.com -> patchew/1474481984-10452-1-git-send-email-roysh@mellanox.com
Switched to a new branch 'test'
6a9d88e block/iscsi: Adding iser support in Libiscsi-QEMU
=== OUTPUT BEGIN ===
Checking PATCH 1/1: block/iscsi: Adding iser support in Libiscsi-QEMU...
WARNING: line over 80 characters
#48: FILE: block/iscsi.c:601:
+ NULL, num_sectors * iscsilun->block_size,
WARNING: line over 80 characters
#49: FILE: block/iscsi.c:602:
+ iscsilun->block_size, 0, 0, fua, 0, 0,
ERROR: line over 90 characters
#50: FILE: block/iscsi.c:603:
+ iscsi_co_generic_cb, &iTask, (struct scsi_iovec *)iov->iov, iov->niov);
WARNING: line over 80 characters
#53: FILE: block/iscsi.c:606:
+ NULL, num_sectors * iscsilun->block_size,
WARNING: line over 80 characters
#54: FILE: block/iscsi.c:607:
+ iscsilun->block_size, 0, 0, fua, 0, 0,
ERROR: line over 90 characters
#55: FILE: block/iscsi.c:608:
+ iscsi_co_generic_cb, &iTask, (struct scsi_iovec *)iov->iov, iov->niov);
ERROR: line over 90 characters
#84: FILE: block/iscsi.c:815:
+ iscsi_co_generic_cb, &iTask, (struct scsi_iovec *)iov->iov, iov->niov);
ERROR: line over 90 characters
#90: FILE: block/iscsi.c:821:
+ iscsi_co_generic_cb, &iTask, (struct scsi_iovec *)iov->iov, iov->niov);
WARNING: line over 80 characters
#116: FILE: block/iscsi.c:1626:
+ iscsi_url = iscsi_parse_full_url(iscsi, uri_string_unescape(filename, -1, NULL));
ERROR: line over 90 characters
#119: FILE: block/iscsi.c:1628:
+ error_setg(errp, "Failed to parse URL : %s", uri_string_unescape(filename, -1, NULL));
ERROR: architecture specific defines should be avoided
#168: FILE: block/iscsi.c:2076:
+#ifdef __linux__
total: 6 errors, 5 warnings, 144 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] block/iscsi: Adding iser support in Libiscsi-QEMU
2016-09-21 18:19 [Qemu-devel] [PATCH v2] block/iscsi: Adding iser support in Libiscsi-QEMU Roy Shterman
2016-09-21 18:23 ` no-reply
@ 2016-09-27 10:28 ` Stefan Hajnoczi
2016-09-27 11:37 ` Roy Shterman
1 sibling, 1 reply; 8+ messages in thread
From: Stefan Hajnoczi @ 2016-09-27 10:28 UTC (permalink / raw)
To: Roy Shterman; +Cc: qemu-devel, pl, roy.shterman, ronniesahlberg, pbonzini
[-- Attachment #1: Type: text/plain, Size: 8105 bytes --]
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 <roysh@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?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] block/iscsi: Adding iser support in Libiscsi-QEMU
2016-09-27 10:28 ` Stefan Hajnoczi
@ 2016-09-27 11:37 ` Roy Shterman
2016-09-27 11:39 ` Peter Lieven
2016-09-27 11:52 ` Paolo Bonzini
0 siblings, 2 replies; 8+ messages in thread
From: Roy Shterman @ 2016-09-27 11:37 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: qemu-devel, pl, roy.shterman, ronniesahlberg, pbonzini
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 <roysh@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.
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.
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] block/iscsi: Adding iser support in Libiscsi-QEMU
2016-09-27 11:37 ` Roy Shterman
@ 2016-09-27 11:39 ` Peter Lieven
2016-09-27 11:52 ` Paolo Bonzini
1 sibling, 0 replies; 8+ messages in thread
From: Peter Lieven @ 2016-09-27 11:39 UTC (permalink / raw)
To: roysh, Stefan Hajnoczi; +Cc: qemu-devel, roy.shterman, ronniesahlberg, pbonzini
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 <roysh@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.
> 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
pl@kamp.de | http://www.kamp.de
Geschäftsführer: Heiner Lante | Michael Lante
Amtsgericht Duisburg | HRB Nr. 12154
USt-Id-Nr.: DE 120607556
...........................................................
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] block/iscsi: Adding iser support in Libiscsi-QEMU
2016-09-27 11:37 ` Roy Shterman
2016-09-27 11:39 ` Peter Lieven
@ 2016-09-27 11:52 ` Paolo Bonzini
2016-09-27 11:58 ` Roy Shterman
1 sibling, 1 reply; 8+ messages in thread
From: Paolo Bonzini @ 2016-09-27 11:52 UTC (permalink / raw)
To: roysh, Stefan Hajnoczi; +Cc: roy.shterman, pl, qemu-devel, ronniesahlberg
On 27/09/2016 13:37, Roy Shterman wrote:
>>>
>>> + 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.
This looks like a libvirt bug. But if libvirt learns to pass iser://
URIs, the unescape is not necessary, is it?
Thanks,
Paolo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] block/iscsi: Adding iser support in Libiscsi-QEMU
2016-09-27 11:52 ` Paolo Bonzini
@ 2016-09-27 11:58 ` Roy Shterman
2016-09-27 12:06 ` Paolo Bonzini
0 siblings, 1 reply; 8+ messages in thread
From: Roy Shterman @ 2016-09-27 11:58 UTC (permalink / raw)
To: Paolo Bonzini, Stefan Hajnoczi
Cc: roy.shterman, pl, qemu-devel, ronniesahlberg
On 9/27/2016 2:52 PM, Paolo Bonzini wrote:
>
> On 27/09/2016 13:37, Roy Shterman wrote:
>>>> + 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.
> This looks like a libvirt bug. But if libvirt learns to pass iser://
> URIs, the unescape is not necessary, is it?
right, but because libvirt inbox versions doesn't support
protocol_name=iser I decided to leave both options available.
The ?iser option and the iser:// option, to also have compatibility with
inbox Libvirt versions.
> Thanks,
>
> Paolo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] block/iscsi: Adding iser support in Libiscsi-QEMU
2016-09-27 11:58 ` Roy Shterman
@ 2016-09-27 12:06 ` Paolo Bonzini
0 siblings, 0 replies; 8+ messages in thread
From: Paolo Bonzini @ 2016-09-27 12:06 UTC (permalink / raw)
To: roysh; +Cc: Stefan Hajnoczi, roy shterman, pl, qemu-devel, ronniesahlberg
> On 9/27/2016 2:52 PM, Paolo Bonzini wrote:
> > On 27/09/2016 13:37, Roy Shterman wrote:
> > > > > + 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.
> > >
> > > because libvirt is parsing '?' char as %3F, I needed to parse to URI
> > > with unescaping.
> >
> > This looks like a libvirt bug. But if libvirt learns to pass iser://
> > URIs, the unescape is not necessary, is it?
>
> right, but because libvirt inbox versions doesn't support
> protocol_name=iser I decided to leave both options available.
> The ?iser option and the iser:// option, to also have compatibility with
> inbox Libvirt versions.
No, please don't do that, especially because unescaping the URI is wrong:
if somebody puts a %2F in the libvirt XML it should _not_ separate the IQN
from the LUN number for example.
Paolo
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-09-27 12:06 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-09-21 18:19 [Qemu-devel] [PATCH v2] block/iscsi: Adding iser support in Libiscsi-QEMU Roy Shterman
2016-09-21 18:23 ` no-reply
2016-09-27 10:28 ` Stefan Hajnoczi
2016-09-27 11:37 ` Roy Shterman
2016-09-27 11:39 ` Peter Lieven
2016-09-27 11:52 ` Paolo Bonzini
2016-09-27 11:58 ` Roy Shterman
2016-09-27 12:06 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).