Hi, I checked my patch and found only warnings about long lines (over 80 characters), Splitting those lines will make the code much less verbose in my opinion. If you still think I should split those lines I will do it gladly.
Other than that I couldn't find any coding style issue in my patch. Thanks, Roy -----Original Message----- From: Peter Lieven [mailto:p...@kamp.de] Sent: Thursday, July 28, 2016 11:46 AM To: Roy Shterman <ro...@mellanox.com>; qemu-devel@nongnu.org Cc: ronniesahlb...@gmail.com; pbonz...@redhat.com Subject: Re: [PATCH] block/iscsi: Adding iser support in Libiscsi-QEMU Am 27.07.2016 um 12:02 schrieb Roy Shterman: > 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. > > Signed-off-by: Roy Shterman <ro...@mellanox.com> > --- > block/iscsi.c | 45 +++++++++++++++++++++++++++++++++++++++++---- > 1 files changed, 41 insertions(+), 4 deletions(-) > > diff --git a/block/iscsi.c b/block/iscsi.c index 7e78ade..6b95636 > 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> > > @@ -484,6 +485,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, @@ -494,11 +507,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(); > @@ -677,6 +693,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, @@ -688,11 +717,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(); > @@ -1477,9 +1508,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; > } > @@ -1494,7 +1525,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; Hi Roy, your patch has style problems. Please use scripts/checkpatch.pl to check for errors. Furthermore I would suggest using LIBISCS_FEATURE_ISER and not the API version in the preprocessor commands. Peter
