[PATCH] nvme-cli: fabrics: Fix to pass "host_traddr" to the kernel driver.

[PATCH] nvme-cli: fabrics: Fix to pass "host_traddr" to the kernel driver.

[Duane Grigsby]

This fix adds the  "host_traddr" to the contructed string that is
passed to the kernel.

Signed-off-by: Darren Trapp <darren.trapp at cavium.com>
---
 fabrics.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fabrics.c b/fabrics.c
index 362ca6f..3f6be53 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -595,6 +595,11 @@ static int connect_ctrl(struct nvmf_disc_rsp_page_entry *e)
 				return -EINVAL;
 			p += len;
 
+			len = sprintf(p, ",host_traddr=%s", cfg.host_traddr);
+			if (len < 0)
+				return -EINVAL;
+			p+= len;
+
 			len = sprintf(p, ",traddr=%s", e->traddr);
 			if (len < 0)
 				return -EINVAL;
-- 
1.7.10.GIT

[PATCH] nvme-cli: fabrics: Fix to pass "host_traddr" to the kernel driver.

[Max Gurtovoy]

Hi Darren,
Can you send an example for usage ?
I want to try this out.

thanks,
Max.

On 2/8/2017 8:46 PM, Duane Grigsby wrote:
> This fix adds the  "host_traddr" to the contructed string that is
> passed to the kernel.
>
> Signed-off-by: Darren Trapp <darren.trapp at cavium.com>
> ---
>  fabrics.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/fabrics.c b/fabrics.c
> index 362ca6f..3f6be53 100644
> --- a/fabrics.c
> +++ b/fabrics.c
> @@ -595,6 +595,11 @@ static int connect_ctrl(struct nvmf_disc_rsp_page_entry *e)
>  				return -EINVAL;
>  			p += len;
>
> +			len = sprintf(p, ",host_traddr=%s", cfg.host_traddr);
> +			if (len < 0)
> +				return -EINVAL;
> +			p+= len;
> +
>  			len = sprintf(p, ",traddr=%s", e->traddr);
>  			if (len < 0)
>  				return -EINVAL;
>

[PATCH] nvme-cli: fabrics: Fix to pass "host_traddr" to the kernel driver.

[Sagi Grimberg]

> @@ -595,6 +595,11 @@ static int connect_ctrl(struct nvmf_disc_rsp_page_entry *e)
>  				return -EINVAL;
>  			p += len;
>
> +			len = sprintf(p, ",host_traddr=%s", cfg.host_traddr);
> +			if (len < 0)
> +				return -EINVAL;
> +			p+= len;
> +

I think this needs to be conditional on if provided at all.

[PATCH] nvme-cli: fabrics: Fix to pass "host_traddr" to the kernel driver.

[Max Gurtovoy]

On 2/15/2017 11:56 AM, Sagi Grimberg wrote:
>
>> @@ -595,6 +595,11 @@ static int connect_ctrl(struct
>> nvmf_disc_rsp_page_entry *e)
>>                  return -EINVAL;
>>              p += len;
>>
>> +            len = sprintf(p, ",host_traddr=%s", cfg.host_traddr);
>> +            if (len < 0)
>> +                return -EINVAL;
>> +            p+= len;
>> +
>
> I think this needs to be conditional on if provided at all.
>

Also, currently this is implemented for FC but I'll want to add an 
option for rdma too.
so we'll be able to use it like that:

nvme connect -t rdma -n testsubsystem -a <target_ip> -w <host_ip> -s 1023

in case we want to choose the interface for rdma_resolve_addr.
I'll put it on my plate.

Max.

> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH] nvme-cli: fabrics: Fix to pass "host_traddr" to the kernel driver.

[Sagi Grimberg]

> Also, currently this is implemented for FC but I'll want to add an
> option for rdma too.
> so we'll be able to use it like that:
>
> nvme connect -t rdma -n testsubsystem -a <target_ip> -w <host_ip> -s 1023
>
> in case we want to choose the interface for rdma_resolve_addr.
> I'll put it on my plate.

That would be nice.

[PATCH] nvme-cli: fabrics: Fix to pass "host_traddr" to the kernel driver.

[Trapp, Darren]

FC requires the host_traddr to make the connection.  If it wasn?t provided, the fc transport wouldn?t have been able to connect to the discovery controller and request the disc page.  Hence, we wouldn?t be here parsing the response.  

Darren Trapp
Senior Director - Engineering
darren.trapp at cavium.com <mailto:darren.trapp at caviumnetworks.com>
T 949-389-6273
M 949-212-8576
26650 Aliso Viejo Pkwy, Aliso Viejo, CA 92656
www.qlogic.com <http://www.qlogic.com/>
 <http://www.qlogic.com/>

On 2/15/17, 1:56 AM, "Linux-nvme on behalf of Sagi Grimberg" <linux-nvme-bounces@lists.infradead.org on behalf of sagi@grimberg.me> wrote:

    
    > @@ -595,6 +595,11 @@ static int connect_ctrl(struct nvmf_disc_rsp_page_entry *e)
    >  				return -EINVAL;
    >  			p += len;
    >
    > +			len = sprintf(p, ",host_traddr=%s", cfg.host_traddr);
    > +			if (len < 0)
    > +				return -EINVAL;
    > +			p+= len;
    > +
    
    I think this needs to be conditional on if provided at all.
    
    _______________________________________________
    Linux-nvme mailing list
    Linux-nvme at lists.infradead.org
    http://lists.infradead.org/mailman/listinfo/linux-nvme
    

[PATCH] nvme-cli: fabrics: Fix to pass "host_traddr" to the kernel driver.

[Sagi Grimberg]

> FC requires the host_traddr to make the connection.  If it wasn?t provided, the fc transport wouldn?t have been able to connect to the discovery controller and request the disc page.  Hence, we wouldn?t be here parsing the response.

Did you test this patch with nvme-loop?

[PATCH] nvme-cli: fabrics: Fix to pass "host_traddr" to the kernel driver.

[Trapp, Darren]

I have not tried the fc target loopback.  But since the discovery response is processed above the fc transport level it should return the same information for the host to process.  (ie transport type FC etc).  

The connection to the target, from the host perspective, would be the same.  

Darren Trapp
Senior Director - Engineering
darren.trapp at cavium.com <mailto:darren.trapp at caviumnetworks.com>
T 949-389-6273
M 949-212-8576
26650 Aliso Viejo Pkwy, Aliso Viejo, CA 92656
www.qlogic.com <http://www.qlogic.com/>
 <http://www.qlogic.com/>

On 2/16/17, 6:17 AM, "Sagi Grimberg" <sagi@grimberg.me> wrote:

    
    > FC requires the host_traddr to make the connection.  If it wasn?t provided, the fc transport wouldn?t have been able to connect to the discovery controller and request the disc page.  Hence, we wouldn?t be here parsing the response.
    
    Did you test this patch with nvme-loop?
    

[PATCH] nvme-cli: fabrics: Fix to pass "host_traddr" to the kernel driver.

[Sagi Grimberg]

> I have not tried the fc target loopback.


After looking at the code again I now recall that the
connection string construction is not shared by transports. My
concern was that other transports (rdma) won't blow up...

So this looks good,

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH] nvme-cli: fabrics: Fix to pass "host_traddr" to the kernel driver.

[Keith Busch]

On Tue, Feb 21, 2017@11:08:14PM +0200, Sagi Grimberg wrote:
> 
> > I have not tried the fc target loopback.
> 
> 
> After looking at the code again I now recall that the
> connection string construction is not shared by transports. My
> concern was that other transports (rdma) won't blow up...
> 
> So this looks good,
> 
> Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

Thanks, applied the patch with the review.