[PATCH v1 2/5] qemu: Support iser transport in iscsi
Han Han
hhan at redhat.com
Wed May 13 08:02:01 UTC 2020
On Mon, May 11, 2020 at 8:02 PM Peter Krempa <pkrempa at redhat.com> wrote:
> On Mon, Apr 27, 2020 at 10:01:07 +0800, Han Han wrote:
> > The iscsi iser transport is introdcued since QEMU 2.9. For iscsi blockdev
> > json, it will be shown at 'transport' field:
> > 'json:{...,{"driver": "iscsi","transport":"iser",...}}'
> >
> > For legacy drive filename as iscsi uri, it will start with 'iser'
> > scheme:
> > iser://[[username][%<password>]@]<host>[:<port>]/<target-iqn-name>/<lun>
> >
> > By default, the iscsi transport is still tcp.
> >
> > Signed-off-by: Han Han <hhan at redhat.com>
> > ---
> > src/qemu/qemu_backup.c | 1 +
> > src/qemu/qemu_block.c | 16 +++++++++++++---
> > src/qemu/qemu_command.c | 17 +++++++++++++++++
> > src/qemu/qemu_monitor_json.c | 1 +
> > src/storage/storage_file_gluster.c | 7 +++++++
> > src/util/virstoragefile.c | 26 ++++++++++++++++++--------
> > src/util/virstoragefile.h | 1 +
> > 7 files changed, 58 insertions(+), 11 deletions(-)
>
> [...]
>
> > diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> > index d32277d7..003c18f1 100644
> > --- a/src/qemu/qemu_block.c
> > +++ b/src/qemu/qemu_block.c
> > @@ -418,10 +418,16 @@ qemuBlockStorageSourceGetURI(virStorageSourcePtr
> src)
> > if (VIR_ALLOC(uri) < 0)
> > return NULL;
> >
> > - if (src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP) {
> > + if (src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP ||
> > + src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_RDMA ||
> > + src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_ISER)
> > uri->port = src->hosts->port;
>
> RDMA is added here along with ISER. Please add it separately with proper
> justification and a test.
>
> Additionally I'm unsure that the design proposed in this patchset is
> entirely correct. According to the iSER protocol RFC 5046 iSER is meant
>
I just simply adapted it to the qemu iser transport of iscsi:
https://github.com/qemu/qemu/blob/d5c75ec500d96f1d93447f990cd5a4ef5ba27fae/qapi/block-core.json#L3510
Well, iSER is iSCSI specific, it will not be used in multiple protocols
like tcp or unix transport. It is better to design that as
an iscsi only attrib:
<source protocol='iscsi' name='iqn.2013-07.com.example:iscsi-nopool/2'
iser='yes'>
...
> as an extension to the iSCSI protocol. Specifically it's layered on top
> of any transport (called RCaP - "RDMA-Capable Protocol" in the RFC).
>
> https://tools.ietf.org/html/rfc5046#section-1.7
>
> Trying to handle it as a transport is thus wrong ...
>
>
> > + if (src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP) {
> > uri->scheme =
> g_strdup(virStorageNetProtocolTypeToString(src->protocol));
> > + } else if (src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI &&
> > + src->hosts->transport ==
> VIR_STORAGE_NET_HOST_TRANS_ISER) {
>
> ... and weird. E.g. this code could end-up formatting nbd+iser if the
> protocol is not iSCSI in the else branch below.
>
> I'm not even sure whether our use of 'rdma' makes sense, but iSER
> appropriated as a transport seems to be even worse.
>
'rdma' network storage transport doesn't make sense for the qemu driver. It
is only used in gluster.
However, the struct BlockdevOptionsGluster has no attribs associated with
rdma. For the legacy gluster uri
scheme gluster+rdma, in fact it is the same as tcp transport:
https://github.com/qemu/qemu/blob/762fa6d79aa30e1a713444da0399739423f8d00e/block/gluster.c#L375
For the storage driver, I am not sure if gluster+rdma works.
>
> Unfortunately I don't have any other idea besides changing this to
> another network storage protocol rather than transport.
>
> > + uri->scheme = g_strdup("iser");
> > } else {
> > uri->scheme = g_strdup_printf("%s+%s",
> >
> virStorageNetProtocolTypeToString(src->protocol),
> > @@ -497,6 +503,7 @@
> qemuBlockStorageSourceBuildJSONSocketAddress(virStorageNetHostDefPtr host,
> > return NULL;
> > break;
> >
> > + case VIR_STORAGE_NET_HOST_TRANS_ISER:
> > case VIR_STORAGE_NET_HOST_TRANS_RDMA:
> > case VIR_STORAGE_NET_HOST_TRANS_LAST:
> > virReportError(VIR_ERR_INTERNAL_ERROR,
> > @@ -743,6 +750,7 @@
> qemuBlockStorageSourceGetISCSIProps(virStorageSourcePtr src,
> > {
> > qemuDomainStorageSourcePrivatePtr srcPriv =
> QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
> > g_autofree char *target = NULL;
> > + const char *transport = NULL;
> > char *lunStr = NULL;
> > char *username = NULL;
> > char *objalias = NULL;
> > @@ -762,6 +770,7 @@
> qemuBlockStorageSourceGetISCSIProps(virStorageSourcePtr src,
> > */
> >
> > target = g_strdup(src->path);
> > + transport =
> virStorageNetHostTransportTypeToString(src->hosts->transport);
> >
> > /* Separate the target and lun */
> > if ((lunStr = strchr(target, '/'))) {
> > @@ -791,7 +800,7 @@
> qemuBlockStorageSourceGetISCSIProps(virStorageSourcePtr src,
> > "s:portal", portal,
> > "s:target", target,
> > "u:lun", lun,
> > - "s:transport", "tcp",
> > + "s:transport", transport,
> > "S:user", username,
> > "S:password-secret", objalias,
> > "S:initiator-name",
> src->initiator.iqn,
> > @@ -2063,7 +2072,8 @@ qemuBlockGetBackingStoreString(virStorageSourcePtr
> src,
> > /* generate simplified URIs for the easy cases */
> > if (actualType == VIR_STORAGE_TYPE_NETWORK &&
> > src->nhosts == 1 &&
> > - src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP &&
> > + (src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP ||
> > + src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_ISER)
> &&
> > src->timeout == 0 &&
> > src->ncookies == 0 &&
> > src->sslverify == VIR_TRISTATE_BOOL_ABSENT &&
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 45dd8307..c5cf9401 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -1443,6 +1443,16 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk,
> > return -1;
> > }
> > }
> > +
> > + if (disk->src &&
> > + disk->src->hosts &&
> > + disk->src->hosts->transport ==
> VIR_STORAGE_NET_HOST_TRANS_ISER &&
> > + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_ISCSI_TRANSPORT_ISER)) {
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > + _("iSCSI iser transport is not supported by this "
> > + "QEMU binary"));
> > + return -1;
> > + }
> > }
>
> If the design were okay, this would still require interlocking with
> other protocols.
>
> >
> > if (disk->serial &&
> > @@ -4888,6 +4898,13 @@
> qemuBuildSCSIiSCSIHostdevDrvStr(virDomainHostdevDefPtr dev,
> > qemuDomainStorageSourcePrivatePtr srcPriv =
> > QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(iscsisrc->src);
> >
> > + if (iscsisrc->src->hosts->transport ==
> VIR_STORAGE_NET_HOST_TRANS_ISER &&
> > + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_ISCSI_TRANSPORT_ISER)) {
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > + _("This QEMU doesn't support iSCSI iser
> transport"));
> > + return NULL;
> > + }
> > +
> > if (qemuDiskSourceNeedsProps(iscsisrc->src, qemuCaps)) {
> > if (!(srcprops = qemuDiskSourceGetProps(iscsisrc->src)))
> > return NULL;
>
> [...]
>
> > diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> > index ffc8bdb3..4f162f10 100644
> > --- a/src/util/virstoragefile.c
> > +++ b/src/util/virstoragefile.c
> > @@ -97,6 +97,7 @@ VIR_ENUM_IMPL(virStorageNetHostTransport,
> > "tcp",
> > "unix",
> > "rdma",
> > + "iser",
> > );
> >
> > VIR_ENUM_IMPL(virStorageSourcePoolMode,
> > @@ -2839,10 +2840,15 @@
> virStorageSourceParseBackingURI(virStorageSourcePtr src,
> >
> > if (!scheme[0] ||
> > (src->protocol =
> virStorageNetProtocolTypeFromString(scheme[0])) < 0) {
> > - virReportError(VIR_ERR_INTERNAL_ERROR,
> > - _("invalid backing protocol '%s'"),
> > - NULLSTR(scheme[0]));
> > - return -1;
> > + if (STRNEQ(scheme[0], "iser")) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("invalid backing protocol '%s'"),
> > + NULLSTR(scheme[0]));
> > + return -1;
> > + }
> > +
> > + src->protocol = VIR_STORAGE_NET_PROTOCOL_ISCSI;
> > + src->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_ISER;
> > }
> >
> > if (scheme[1] &&
> > @@ -3523,14 +3529,17 @@
> virStorageSourceParseBackingJSONiSCSI(virStorageSourcePtr src,
> > return -1;
> >
> > src->nhosts = 1;
> > + src->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_TCP;
> >
> > - if (STRNEQ_NULLABLE(transport, "tcp")) {
> > + if (STRNEQ(transport, "tcp") && STRNEQ(transport, "iser") &&
> transport) {
>
> If 'transport' is NULL, this will crash on the first strneq. You'd have
> to check it first.
>
>
> > virReportError(VIR_ERR_INVALID_ARG, "%s",
> > - _("only TCP transport is supported for iSCSI
> volumes"));
> > + _("only TCP or iSER transport is supported for
> iSCSI "
> > + "volumes"));
> > return -1;
> > }
> >
> > - src->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_TCP;
> > + if (transport)
> > + src->hosts->transport =
> virStorageNetHostTransportTypeFromString(transport);
> >
> > if (!portal) {
> > virReportError(VIR_ERR_INVALID_ARG, "%s",
> > @@ -4710,7 +4719,8 @@
> virStorageSourceNetworkAssignDefaultPorts(virStorageSourcePtr src)
> > size_t i;
> >
> > for (i = 0; i < src->nhosts; i++) {
> > - if (src->hosts[i].transport == VIR_STORAGE_NET_HOST_TRANS_TCP &&
> > + if ((src->hosts[i].transport == VIR_STORAGE_NET_HOST_TRANS_TCP
> ||
> > + src->hosts[i].transport ==
> VIR_STORAGE_NET_HOST_TRANS_ISER) &&
>
> This would assume that iSER has addressing equal to TCP, but if layered
> on top of something else that might not be the case.
>
> > src->hosts[i].port == 0)
> > src->hosts[i].port =
> virStorageSourceNetworkDefaultPort(src->protocol);
> > }
>
>
--
Best regards,
-----------------------------------
Han Han
Quality Engineer
Redhat.
Email: hhan at redhat.com
Phone: +861065339333
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200513/cb92b579/attachment-0001.htm>
More information about the libvir-list
mailing list