[PATCH 4/5] nvme-fabrics: Add target FC transport support

[James Smart]

On 8/1/2016 9:37 PM, J Freyensee wrote:
>
>> +static void
>> +nvmet_fc_free_target_queue(struct nvmet_fc_tgt_queue *queue)
>> +{
>> +	struct nvmet_fc_tgtport *tgtport = queue->assoc->tgtport;
>> +	unsigned long flags;
>> +
>> +	/*
>> +	 * beware: nvmet layer hangs waiting for a completion if
>> +	 * connect command failed
>> +	 */
>> +	flush_workqueue(queue->work_q);
>> +	if (queue->connected)
>> +		nvmet_sq_destroy(&queue->nvme_sq);
> I was wondering if there is any way for this fc target layer to fake
> send an NVMe completion to the nvmet layer to prevent a nvmet layer
> hang (because I'm assuming the nvmet layer hangs because it never
> receives a connect completion upon failure here), then send a signal to
> tear down the sq.
>
> Or alternatively call nvmet_ctrl_fatal_error() if connect fails as a
> trial/alternative to having the nvmet layer hang?

I haven't tried to fix the bugs in the other layers - enough to do in FC 
already.  Agree to look into this.



>> +
>> +/**
>> + * nvme_fc_register_targetport - transport entry point called by an
>> + *                              LLDD to register the existence of a
>> local
>> + *                              NVME subystem FC port.
>> + * @pinfo:     pointer to information about the port to be
>> registered
>> + * @template:  LLDD entrypoints and operational parameters for the
>> port
>> + * @dev:       physical hardware device node port corresponds to.
>> Will be
>> + *             used for DMA mappings
>> + * @tgtport_p:   pointer to a local port pointer. Upon success, the
>
> looks like the variable tgtport_p does not exist (or it's now called
> portptr)?

Yeah - comment not in sync with the code's name.


>
>> +/*
>> + * Actual processing routine for received FC-NVME LS Requests from
>> the LLD
>> + */
>> +void
>> +nvmet_fc_handle_ls_rqst(struct nvmet_fc_tgtport *tgtport,
>> +			struct nvmet_fc_ls_iod *iod)
>> +{
>> +	struct fcnvme_ls_rqst_w0 *w0 =
>> +			(struct fcnvme_ls_rqst_w0 *)iod->rqstbuf;
>> +
>> +	iod->lsreq->nvmet_fc_private = iod;
>> +	iod->lsreq->rspbuf = iod->rspbuf;
>> +	iod->lsreq->rspdma = iod->rspdma;
>> +	iod->lsreq->done = nvmet_fc_xmt_ls_rsp_done;
>> +	/* Be preventative. handlers will later set to valid length
>> */
>> +	iod->lsreq->rsplen = 0;
>> +
>> +	iod->assoc = NULL;
>> +
>> +	/*
>> +	 * handlers:
>> +	 *   parse request input, set up nvmet req (cmd, rsp,
>>   execute)
>> +	 *   and format the LS response
>> +	 * if non-zero returned, then no futher action taken on the
>> LS
>> +	 * if zero:
>> +	 *   valid to call nvmet layer if execute routine set
>> +	 *   iod->rspbuf contains ls response
>> +	 */
>> +	switch (w0->ls_cmd) {
>> +	case FCNVME_LS_CREATE_ASSOCIATION:
>> +		/* Creates Association and initial Admin
>> Queue/Connection */
>> +		nvmet_fc_ls_create_association(tgtport, iod);
>> +		break;
>> +	case FCNVME_LS_CREATE_CONNECTION:
>> +		/* Creates an IO Queue/Connection */
>> +		nvmet_fc_ls_create_connection(tgtport, iod);
>> +		break;
>> +	case FCNVME_LS_DISCONNECT:
>> +		/* Terminate a Queue/Connection or the Association
>> */
>> +		nvmet_fc_ls_disconnect(tgtport, iod);
>> +		break;
>> +	default:
>> +		iod->lsreq->rsplen = nvmet_fc_format_rjt(iod
>> ->rspbuf,
>> +				NVME_FC_MAX_LS_BUFFER_SIZE, w0
>> ->ls_cmd,
>> +				LSRJT_REASON_INVALID_ELS_CODE,
>> +				LSRJT_EXPL_NO_EXPLANATION, 0);
>> +	}
>> +
>> +	nvmet_fc_xmt_ls_rsp(tgtport, iod);
>> +}
> All the functions in the case() (nvmet_fc_ls_disconnect(),
> nvmet_fc_ls_create_association(), etc)  have internal return values
> (errors and certain values), I'm curious why you wouldn't want to
> bubble up those values through the function calling chain?  Especially
> since there is a comment in the function above that says "if non-zero
> returned, then no futher action taken on the LS".

just style. they all generate a response payload that is then always 
sent, so returning a code didn't really mean much.  The comment is old. 
They used to return the value, but as you can see, they don't now. Will 
clean it up.


>
> .
>> +
>> +static int __init nvmet_fc_init_module(void)
>> +{
>> +	/* ensure NVMET_NR_QUEUES is a power of 2 - required for our
>> masks */
>> +	if (!is_power_of_2((unsigned long)NVMET_NR_QUEUES)) {
>> +		pr_err("%s: NVMET_NR_QUEUES required to be power of
>> 2\n",
>> +				__func__);
> If this is so important that NVMET_NR_QUEUES always be a power of two
> for this FC driver, I'd have the function print out the value in the
> error message for easier diagnosis.

no problem.


>
> And why mask the sign of NVMET_NR_QUEUES?  Yes, it would have to be a
> really careless programmer error that would be caught very quick if the
> #define became negative, but masking out the sign of a #define value
> that seems to be pretty important for FC transport functionality makes
> me a tad nervous.

I'll look at it. I believe I did the type redef to make the calling 
sequence happy.  I'll see if there's a better way.

> Good stuff James,
> J
>
>

Thanks

-- james