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

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


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()

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