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

Nicholas A. Bellinger nab at linux-iscsi.org
Mon Jan 10 23:01:19 UTC 2011


On Mon, 2011-01-10 at 14:22 -0800, Joe Eykholt wrote:
> 
> 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_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.
> 

Ahhh, I had missed this part..  Thanks for the clarification here.

> 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.

Agreed.  So I will plan on coding these changes up as a post merge
enchancement aimed at .39 w/ TCM_FC/OpenFCoE, and try to get these
posted early for the team's review in the next weeks.

Thanks!

--nab





More information about the devel mailing list