[Open-FCoE] [PATCH 1/5] libfc: add hook for FC-4 provider registration

Joe Eykholt jeykholt at cisco.com
Mon Jan 10 22:22:56 UTC 2011



On 1/10/11 1:22 PM, Nicholas A. Bellinger wrote:
> On Mon, 2011-01-10 at 11:04 -0800, Joe Eykholt wrote:
>> On 1/9/11 4:43 PM, Nicholas A. Bellinger wrote:
>>> On Fri, 2011-01-07 at 18:24 -0800, Robert Love wrote:
>>>> From: Joe Eykholt <jeykholt at cisco.com>
>>>>
>>>> Allow FC-4 provider modules to hook into libfc, mostly for targets.
>>>> This should allow any FC-4 module to handle PRLI requests and maintain
>>>> process-association states.
>>>>
>>>> Each provider registers its ops with libfc and then will be called for
>>>> any incoming PRLI for that FC-4 type on any instance.   The provider
>>>> can decide whether to handle that particular instance using any method
>>>> it likes, such as ACLs or other configuration information.
>>>>
>>>> A count is kept of the number of successful PRLIs from the remote port.
>>>> Providers are called back with an implicit PRLO when the remote port
>>>> is about to be deleted or has been reset.
>>>>
>>>> fc_lport_recv_req() now sends incoming FC-4 requests to FC-4 providers,
>>>> and there is a built-in provider always registered for handling
>>>> incoming ELS requests.
>>>>
>>>> The call to provider recv() routines uses rcu_read_lock()
>>>> so that providers aren't removed during the call.  That lock is very
>>>> cheap and shouldn't affect any performance on ELS requests.
>>>> Providers can rely on the RCU lock to protect a session lookup as well.
>>
>>>>
>>>> Signed-off-by: Joe Eykholt <jeykholt at cisco.com>
>>>> Signed-off-by: Robert Love <robert.w.love at intel.com>
>>>
>>> Hi Robert, Joe and Co,
>>>
>>> So after having a look at these patches again, the more I think we
>>> should consider an explict per-endpoint (TCM_FC Lport level)
>>> registration caller provided by libfc for tcm_fc instead of the global
>>> lport lookup that this current patch requires from the tcm_fc code side.
>>
>>
>> Other providers like SCST, although not in-tree, should be accomodated
>> as well.  The provider can register either when configured or when
>> an lport is attached.
>>
>>> That said, I am thinking that fc_fc4_*register_provider() really should
>>> be getting called from within TCM_FC target WWN+TPGT endpoint context in
>>> drivers/target/tcm_fc/tfc_conf.c:ft_add_tpg() here:
>>>
>>> http://git.kernel.org/?p=linux/kernel/git/nab/lio-core-2.6.git;a=blob;f=drivers/target/tcm_fc/tfc_conf.c;hb=HEAD#l304
>>>
>>> instead of the current usage globally for future lports during
>>> module_init() -> ft_init() here:
>>>
>>> http://git.kernel.org/?p=linux/kernel/git/nab/lio-core-2.6.git;a=blob;f=drivers/target/tcm_fc/tfc_conf.c;hb=HEAD#l648
>>
>> But that doesn't change this patch, right?
>> I think register provider should still be per-lport.
>> The list lookup is not a concern because it's a short list (typically)
>> and is performed so rarely.
>>
> 
> Well, currently this involves two list lookups per PRIL within the FC4
> provider / fabric module;  find the matching target lport from an list
> of internal provider target lports, and find a matching initiator
> NodeACL from rport for target lport.
> 
> So what I would like to see is moving the explict lport lookup from the
> FC provider module into a libfc proper via an explict target lport
> registration API to simplify this for all fc4 target providers.
> 
>>> The main benefit to gain is that we could drop the global lookup for
>>> lports during the prov->prli() call from libfc into TCM_FC tfc_sess.c
>>> code via ft_prli() -> ft_prli_locked() -> ft_tport_create() ->
>>> ft_lport_find_tpg(), and remove Joe's original struct ft_tport
>>> allocation/glue and associate the provider directly with a pointer to
>>> TCM's struct se_portal_group that can then be accessed via
>>> container_of() from the existing struct ft_tpg from ft_add_tpg().
>>>
>>> More below..
>>>
>>>> ---
>>>>  drivers/scsi/libfc/fc_libfc.c |   60 ++++++++++++++++++
>>>>  drivers/scsi/libfc/fc_libfc.h |   11 +++
>>>>  drivers/scsi/libfc/fc_lport.c |   65 +++++++++++++++++---
>>>>  drivers/scsi/libfc/fc_rport.c |  133 +++++++++++++++++++++++++++++++++--------
>>>>  include/scsi/libfc.h          |   26 ++++++++
>>>>  5 files changed, 259 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/drivers/scsi/libfc/fc_libfc.c b/drivers/scsi/libfc/fc_libfc.c
>>>> index 6a48c28..ae3abef 100644
>>>> --- a/drivers/scsi/libfc/fc_libfc.c
>>>> +++ b/drivers/scsi/libfc/fc_libfc.c
> 
> <SNIP>
> 
>>>> +/**
>>>> + * fc_fc4_register_provider() - register FC-4 upper-level provider.
>>>> + * @type: FC-4 type, such as FC_TYPE_FCP
>>>> + * @prov: structure describing provider including ops vector.
>>>> + *
>>>> + * Returns 0 on success, negative error otherwise.
>>>> + */
>>>> +int fc_fc4_register_provider(enum fc_fh_type type, struct fc4_prov *prov)
>>>> +{
>>>> +	struct fc4_prov **prov_entry;
>>>> +	int ret = 0;
>>>> +
>>>> +	if (type >= FC_FC4_PROV_SIZE)
>>>> +		return -EINVAL;
>>>> +	mutex_lock(&fc_prov_mutex);
>>>> +	prov_entry = (prov->recv ? fc_passive_prov : fc_active_prov) + type;
>>>> +	if (*prov_entry)
>>>> +		ret = -EBUSY;
>>>> +	else
>>>> +		*prov_entry = prov;
>>>> +	mutex_unlock(&fc_prov_mutex);
>>>> +	return ret;
>>>> +}
>>>> +EXPORT_SYMBOL(fc_fc4_register_provider);
>>>> +
>>>> +/**
>>>> + * fc_fc4_deregister_provider() - deregister FC-4 upper-level provider.
>>>> + * @type: FC-4 type, such as FC_TYPE_FCP
>>>> + * @prov: structure describing provider including ops vector.
>>>> + */
>>>> +void fc_fc4_deregister_provider(enum fc_fh_type type, struct fc4_prov *prov)
>>>> +{
>>>> +	BUG_ON(type >= FC_FC4_PROV_SIZE);
>>>> +	mutex_lock(&fc_prov_mutex);
>>>> +	if (prov->recv)
>>>> +		rcu_assign_pointer(fc_passive_prov[type], NULL);
>>>> +	else
>>>> +		rcu_assign_pointer(fc_active_prov[type], NULL);
>>>> +	mutex_unlock(&fc_prov_mutex);
>>>> +	synchronize_rcu();
>>>> +}
>>>> +EXPORT_SYMBOL(fc_fc4_deregister_provider);
>>>
>>> So I think this would mean that fc_fc4_*register_provider() would be
>>> walking a global list of struct fc_lport once during explict target
>>> lport configfs registration via ft_add_tpg(), instead of each PRLI for
>>> all tcm_fc registered lports..
>>
>> You still need to do per-PRLI because thats when you know the rport.
>>
> 
> Yes, the second list lookup to locate a specific NodeACL from rport from
> a target lport endpoint is still expected.. 
> 
> <SNIP>
> 
>>>> +
>>>> +/**
>>>> + * fc_lport_recv_req() - The generic lport request handler
>>>> + * @lport: The lport that received the request
>>>> + * @fp: The frame the request is in
>>>> + *
>>>> + * Locking Note: This function should not be called with the lport
>>>> + *		 lock held becuase it may grab the lock.
>>>> + */
>>>> +static void fc_lport_recv_req(struct fc_lport *lport,
>>>> +			      struct fc_frame *fp)
>>>> +{
>>>> +	struct fc_frame_header *fh = fc_frame_header_get(fp);
>>>> +	struct fc_seq *sp = fr_seq(fp);
>>>> +	struct fc4_prov *prov;
>>>> +
>>>> +	/*
>>>> +	 * Use RCU read lock and module_lock to be sure module doesn't
>>>> +	 * deregister and get unloaded while we're calling it.
>>>> +	 * try_module_get() is inlined and accepts a NULL parameter.
>>>> +	 * Only ELSes and FCP target ops should come through here.
>>>> +	 * The locking is unfortunate, and a better scheme is being sought.
>>>> +	 */
>>>> +
>>>> +	rcu_read_lock();
>>>> +	if (fh->fh_type >= FC_FC4_PROV_SIZE)
>>>> +		goto drop;
>>>> +	prov = rcu_dereference(fc_passive_prov[fh->fh_type]);
>>>> +	if (!prov || !try_module_get(prov->module))
>>>> +		goto drop;
>>>> +	rcu_read_unlock();
>>>> +	prov->recv(lport, fp);
>>>> +	module_put(prov->module);
>>>> +	return;
>>>> +drop:
>>>> +	rcu_read_unlock();
>>>> +	FC_LPORT_DBG(lport, "dropping unexpected frame type %x\n", fh->fh_type);
>>>> +	fc_frame_free(fp);
>>>> +	lport->tt.exch_done(sp);
>>>> +}
>>>> +
>>>
>>> And then we could get rid of the try_module_get() callers here in
>>> fc_lport_recv_req() and move the fabric module reference into the libfc
>>> explict target lport registration call made once by configfs context in
>>> tfc_conf.c:ft_add_tpg() code..
>>
>> Maybe that would work, but I don't think I understand what your proposing.
>> The main thing is to avoid jumping into the module after it has been unloaded,
>> or to have a code path active in the module during its unload.
>> If you can illustrate a bit better how your proposed solution works and covers
>> the cases, that'd help.  BTW, try_module_get() is pretty cheap.
> 
> Recall that all target fabric modules (including TCM_FC) by virtue of
> using configfs for their control plane get proper per module reference
> counting 'for free' from target_core_fabric_configfs.c, where the fabric
> module usage count is incremented/decremented for each struct
> config_group that is created/removed from the generic fabric set
> described here:
> 
> http://linux-iscsi.org/wiki/ConfigFS
> 
> So this means that a fc_fc4_register_provider_by_lport() call from
> tfc_conf.c:ft_add_tpg() is already protected from module removal by
> struct config_groups from tfc_wwn_cit and tfc_tpg_cit listed in the URL
> above, and we can safely drop the unnecessary try_module_get() /
> module_put around prov->recv() in fc_lport_recv_req()
> 
> In terms of the try_module_get() and module_put() usage this would
> obviously be a small optimization for fc4 providers using upstream
> target logic, but for mainline libfc fc4 target provider I believe this
> is the cleanest method.
> 
>>
>>>>  /**
>>>>   * fc_lport_reset() - Reset a local port
>>>>   * @lport: The local port which should be reset
>>>> diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c
>>>> index 325bc42..5a02445 100644
>>>> --- a/drivers/scsi/libfc/fc_rport.c
>>>> +++ b/drivers/scsi/libfc/fc_rport.c
>>>> @@ -257,6 +257,8 @@ static void fc_rport_work(struct work_struct *work)
>>>>  	struct fc_rport_operations *rport_ops;
>>>>  	struct fc_rport_identifiers ids;
>>>>  	struct fc_rport *rport;
>>>> +	struct fc4_prov *prov;
>>>> +	u8 type;
>>>>  
>>>>  	mutex_lock(&rdata->rp_mutex);
>>>>  	event = rdata->event;
>>>> @@ -306,6 +308,15 @@ static void fc_rport_work(struct work_struct *work)
>>>>  	case RPORT_EV_FAILED:
>>>>  	case RPORT_EV_LOGO:
>>>>  	case RPORT_EV_STOP:
>>>> +		if (rdata->prli_count) {
>>>> +			mutex_lock(&fc_prov_mutex);
>>>> +			for (type = 1; type < FC_FC4_PROV_SIZE; type++) {
>>>> +				prov = fc_passive_prov[type];
>>>> +				if (prov && prov->prlo)
>>>> +					prov->prlo(rdata);
>>>> +			}
>>>> +			mutex_unlock(&fc_prov_mutex);
>>>> +		}
>>>>  		port_id = rdata->ids.port_id;
>>>>  		mutex_unlock(&rdata->rp_mutex);
>>>>  
>>>> @@ -1643,9 +1654,9 @@ static void fc_rport_recv_prli_req(struct fc_rport_priv *rdata,
>>>>  	unsigned int len;
>>>>  	unsigned int plen;
>>>>  	enum fc_els_spp_resp resp;
>>>> +	enum fc_els_spp_resp passive;
>>>>  	struct fc_seq_els_data rjt_data;
>>>> -	u32 fcp_parm;
>>>> -	u32 roles = FC_RPORT_ROLE_UNKNOWN;
>>>> +	struct fc4_prov *prov;
>>>>  
>>>>  	FC_RPORT_DBG(rdata, "Received PRLI request while in state %s\n",
>>>>  		     fc_rport_state(rdata));
>>>> @@ -1679,46 +1690,41 @@ static void fc_rport_recv_prli_req(struct fc_rport_priv *rdata,
>>>>  	pp->prli.prli_len = htons(len);
>>>>  	len -= sizeof(struct fc_els_prli);
>>>>  
>>>> -	/* reinitialize remote port roles */
>>>> -	rdata->ids.roles = FC_RPORT_ROLE_UNKNOWN;
>>>> -
>>>>  	/*
>>>>  	 * Go through all the service parameter pages and build
>>>>  	 * response.  If plen indicates longer SPP than standard,
>>>>  	 * use that.  The entire response has been pre-cleared above.
>>>>  	 */
>>>>  	spp = &pp->spp;
>>>> +	mutex_lock(&fc_prov_mutex);
>>>>  	while (len >= plen) {
>>>>  		spp->spp_type = rspp->spp_type;
>>>>  		spp->spp_type_ext = rspp->spp_type_ext;
>>>> -		spp->spp_flags = rspp->spp_flags & FC_SPP_EST_IMG_PAIR;
>>>> -		resp = FC_SPP_RESP_ACK;
>>>> -
>>>> -		switch (rspp->spp_type) {
>>>> -		case 0:	/* common to all FC-4 types */
>>>> -			break;
>>>> -		case FC_TYPE_FCP:
>>>> -			fcp_parm = ntohl(rspp->spp_params);
>>>> -			if (fcp_parm & FCP_SPPF_RETRY)
>>>> -				rdata->flags |= FC_RP_FLAGS_RETRY;
>>>> -			rdata->supported_classes = FC_COS_CLASS3;
>>>> -			if (fcp_parm & FCP_SPPF_INIT_FCN)
>>>> -				roles |= FC_RPORT_ROLE_FCP_INITIATOR;
>>>> -			if (fcp_parm & FCP_SPPF_TARG_FCN)
>>>> -				roles |= FC_RPORT_ROLE_FCP_TARGET;
>>>> -			rdata->ids.roles = roles;
>>>> -
>>>> -			spp->spp_params = htonl(lport->service_params);
>>>> -			break;
>>>> -		default:
>>>> -			resp = FC_SPP_RESP_INVL;
>>>> -			break;
>>>> +		resp = 0;
>>>> +
>>>> +		if (rspp->spp_type < FC_FC4_PROV_SIZE) {
>>>> +			prov = fc_active_prov[rspp->spp_type];
>>>> +			if (prov)
>>>> +				resp = prov->prli(rdata, plen, rspp, spp);
>>>> +			prov = fc_passive_prov[rspp->spp_type];
>>>> +			if (prov) {
>>>> +				passive = prov->prli(rdata, plen, rspp, spp);
>>>> +				if (!resp || passive == FC_SPP_RESP_ACK)
>>>> +					resp = passive;
>>>> +			}
>>>> +		}
>>>
>>> So the call into TCM fabric module prov->prli() would need to pass the
>>> pre-registered struct se_portal_group from struct ft_tpg from
>>> ft_add_tpg() via the struct fc_lport *lport parameter into
>>> fc_rport_recv_els_req() -> fc_rport_recv_prli_req()..
>>
>> I don't see why tcm_fc wouldn't pick up the TPG just like it
>> gets the tport today.
>>
> 
> Well, today once prov->prli() is called, the call down into
> 
> ft_prli_locked() ->
> 	
> 	ft_tport_create() ->
> 	
> 		ft_lport_find_tpg()
> 
> requires the internal ft_lport_list lookup from:
> 
> struct ft_tpg *ft_lport_find_tpg(struct fc_lport *lport)
> {
>         struct ft_lport_acl *lacl;
>         struct ft_tpg *tpg;
> 
>         list_for_each_entry(lacl, &ft_lport_list, list) {
>                 if (lacl->wwpn == lport->wwpn) {
>                        list_for_each_entry(tpg, &lacl->tpg_list, list)
>                                return tpg; /* XXX for now return first entry */
>                        return NULL;
>                 }
>         }
>         return NULL;
> }
> 
> I would really like to be able to drop this specific lookup and simply
> hand off the fabric dependent structure (struct ft_tpg * for TCM_FC) by
> using a pointer passed at explict lport target registration time. (eg:
> fc_fc4_register_provider_by_lport()

But usually ft_tport_create() finds the tport directly from the lport, and doesn't
do a lookup.  If the tport had a pointer to the ft_lport_acl, you wouldn't need the lacl
lookup, just the tpg lookup.  In any case, PRLIs are not frequent at all.

In other words, I think the libfc patches and the interfaces are sufficient,
and tcm_fc could change to use the lacl instead of ft_tport, then you get what you want.

>>>> +		if (!resp) {
>>>> +			if (spp->spp_flags & FC_SPP_EST_IMG_PAIR)
>>>> +				resp |= FC_SPP_RESP_CONF;
>>>> +			else
>>>> +				resp |= FC_SPP_RESP_INVL;
>>>>  		}
>>>>  		spp->spp_flags |= resp;
>>>>  		len -= plen;
>>>>  		rspp = (struct fc_els_spp *)((char *)rspp + plen);
>>>>  		spp = (struct fc_els_spp *)((char *)spp + plen);
>>>>  	}
>>>> +	mutex_unlock(&fc_prov_mutex);
>>>>  
>>>>  	/*
>>>>  	 * Send LS_ACC.	 If this fails, the originator should retry.
>>>> @@ -1888,6 +1894,79 @@ int fc_rport_init(struct fc_lport *lport)
>>>>  EXPORT_SYMBOL(fc_rport_init);
>>>>  
>>>
>>> <SNIP>
>>>
>>>> +/**
>>>> + * struct fc4_prov - FC-4 provider registration
>>>> + * @prli:               Handler for incoming PRLI
>>>> + * @prlo:               Handler for session reset
>>>> + * @recv:		Handler for incoming request
>>>> + * @module:		Pointer to module.  May be NULL.
>>>> + */
>>>> +struct fc4_prov {
>>>> +	int (*prli)(struct fc_rport_priv *, u32 spp_len,
>>>> +		    const struct fc_els_spp *spp_in,
>>>> +		    struct fc_els_spp *spp_out);
>>>> +	void (*prlo)(struct fc_rport_priv *);
>>>> +	void (*recv)(struct fc_lport *, struct fc_frame *);
>>>> +	struct module *module;
>>>> +};
>>>> +
>>>
>>> So fc4_prov would be allocated and setup with an individual tcm_fc
>>> struct se_portal_group pointer from struct ft_tpg and it's parent struct
>>> se_wwn -> struct ft_lport in tfc_conf.c:ft_add_tpg() context..
>>
>> That would mean you would allocate the provider per-lport or per TPG
>> which would work.  If you put the provider inside the TPG you could
>> do container_of() to find the TPG.  Still, you want to handle PRLI, I think.
>>
> 
> Correct.
> 
>>> The question then becomes what else needs to be done in order to handle
>>> fc4_prov->prov_se_tpg reference to explict registered struct fc_lport
>>> via ft_add_tpg() when the libfc lport itself is removed..
>>>
>>> rwlove, Joe and Co, thoughts..?
>>
>> That's a pretty different design you're proposing, so
>> you should try it and see how it works, maybe I need to spend some more
>> time understanding it, but I don't want to spend time on this right now.
>>
> 
> Point taken.  My interests here are to now simplify and optimize the
> libfc target provider logic headed for upstream against the proper v4.0
> fabric inpendent configfs control plane now in linux-next.
> 
> While I don't have a strong objection to getting this code merged
> upstream 'as-is' for .38 given it's long time BETA status in
> lio-core-2.6.git and other trees, I think simplfying the control plane
> with an explict target lport registration does make the most sense long
> term.
> 
>> Everyone would do this differently.  I was trying to be agnostic as to
>> the target module internals.  It works for 3-4 different target modules
>> so far, and should work for non-FCP protocols.
>>
> 
> A very fair point.  I think an explict lport registration API and
> hand-off of a fc4 target provider dependent lport/endpoint pointer into
> prov->prli() into would benefit all fc4 target providers.
> 
> That said, I may still try to produce a patch to make these changes
> happen in the next weeks, which for all intensive purposes will end up
> being a for-39 item along with tcm_fc.  Obviously I will need help from
> rwlove and Co' to make this happen, but any other thoughts from your
> side are apperciated.
> 
> Thanks for your comments Joe!
> 
> --nab
> 



More information about the devel mailing list