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