Re: [PATCH 3/5] nvme-fabrics: Add host FC transport support

[James Smart <james.smart@broadcom.com>]

> Are you going to send a FC support patch for nvme-cli as well?

I will, but concentrating on core support for now. I expect a fair 
number of things need to be done in the cli.


>
>> +	assoc_rqst->assoc_cmd.ersp_ratio = cpu_to_be16(ersp_ratio);
>> +	assoc_rqst->assoc_cmd.sqsize = cpu_to_be16(qsize);
>> +	/* TODO:
>> +	 * assoc_rqst->assoc_cmd.cntlid = cpu_to_be16(?);
>> +	 * strncpy(assoc_rqst->assoc_cmd.hostid, ?,
>> +	 *	min(FCNVME_ASSOC_HOSTID_LEN, NVMF_NQN_SIZE));
>> +	 * strncpy(assoc_rqst->assoc_cmd.hostnqn, ?,
>> +	 *	min(FCNVME_ASSOC_HOSTNQN_LEN, NVMF_NQN_SIZE];
>> +	 */
> What is the problem with filling all these out based on the information
> in the nvme_host structure?

Well, I want to get them from the same place that the fabrics routines 
do when they build a capsule. So they should come from the opts struct. 
It's missing the controller id, so I'll need to address that.


>
> This probably needs to be moved, similar to the patch Sagi just
> did for RDMA.  In general it might be a good idea to look at the
> various recent RDMA changes and check if they need to be brought over.

yeah - I typically do a resync every time I repost.


>
>> +	/*
>> +	 * TODO: blk_integrity_rq(rq)  for DIX
>> +	 */
> We'll hopefully be able to just move the DIX handling to common code.
> Any help appreciated..

Yes and no....    and likely a separate discussion.  If DIF is passed, 
you should involve the host port to validate on egress as well has what 
the target controller will do. Although it brings up some headaches if 
the host detects an error (e.g. protocol requires target to be part of 
it).  If DIF is inserted, the question will be whether you want to 
insert DIF at the egress point of the host port or do you pass it over 
the fabric w/o dif and have the tgt controller insert it.  I assume 
we'll need to talk more.

>
> This seems to miss reference counting on the lport and rport structure.
yes - an area of weakness right now.

> It would be good to sort this out before merging. 

I'll make a pass at it.

-- james