Message ID | 1474481984-10452-1-git-send-email-roysh@mellanox.com |
---|---|
State | New |
Headers | show |
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
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?
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. >
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
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
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
> 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
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); }
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(-)