|
| 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); - +#endifThese 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 v3Is 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 ...........................................................
| [Prev in Thread] | Current Thread | [Next in Thread] |