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

Joe Eykholt jeykholt at cisco.com
Mon Jan 10 19:04:46 UTC 2011


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.

> 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
>> @@ -35,6 +35,23 @@ unsigned int fc_debug_logging;
>>  module_param_named(debug_logging, fc_debug_logging, int, S_IRUGO|S_IWUSR);
>>  MODULE_PARM_DESC(debug_logging, "a bit mask of logging levels");
>>  
>> +DEFINE_MUTEX(fc_prov_mutex);
>> +
>> +/*
>> + * Providers which primarily send requests and PRLIs.
>> + */
>> +struct fc4_prov *fc_active_prov[FC_FC4_PROV_SIZE] = {
>> +	[0] = &fc_rport_t0_prov,
>> +	[FC_TYPE_FCP] = &fc_rport_fcp_init,
>> +};
>> +
>> +/*
>> + * Providers which receive requests.
>> + */
>> +struct fc4_prov *fc_passive_prov[FC_FC4_PROV_SIZE] = {
>> +	[FC_TYPE_ELS] = &fc_lport_els_prov,
>> +};
>> +
>>  /**
>>   * libfc_init() - Initialize libfc.ko
>>   */
>> @@ -210,3 +227,46 @@ void fc_fill_reply_hdr(struct fc_frame *fp, const struct fc_frame *in_fp,
>>  	fc_fill_hdr(fp, in_fp, r_ctl, FC_FCTL_RESP, 0, parm_offset);
>>  }
>>  EXPORT_SYMBOL(fc_fill_reply_hdr);
>> +
>> +/**
>> + * 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.

>> diff --git a/drivers/scsi/libfc/fc_libfc.h b/drivers/scsi/libfc/fc_libfc.h
>> index eea0c35..205de28 100644
>> --- a/drivers/scsi/libfc/fc_libfc.h
>> +++ b/drivers/scsi/libfc/fc_libfc.h
>> @@ -94,6 +94,17 @@ extern unsigned int fc_debug_logging;
>>  				(lport)->host->host_no,	##args))
>>  
>>  /*
>> + * FC-4 Providers.
>> + */
>> +extern struct fc4_prov *fc_active_prov[];	/* providers without recv */
>> +extern struct fc4_prov *fc_passive_prov[];	/* providers with recv */
>> +extern struct mutex fc_prov_mutex;		/* lock over table changes */
>> +
>> +extern struct fc4_prov fc_rport_t0_prov;	/* type 0 provider */
>> +extern struct fc4_prov fc_lport_els_prov;	/* ELS provider */
>> +extern struct fc4_prov fc_rport_fcp_init;	/* FCP initiator provider */
>> +
>> +/*
>>   * Set up direct-data placement for this I/O request
>>   */
>>  void fc_fcp_ddp_setup(struct fc_fcp_pkt *fsp, u16 xid);
>> diff --git a/drivers/scsi/libfc/fc_lport.c b/drivers/scsi/libfc/fc_lport.c
>> index c5a10f9..e2cd087 100644
>> --- a/drivers/scsi/libfc/fc_lport.c
>> +++ b/drivers/scsi/libfc/fc_lport.c
>> @@ -849,7 +849,7 @@ out:
>>  }
>>  
>>  /**
>> - * fc_lport_recv_req() - The generic lport request handler
>> + * fc_lport_recv_els_req() - The generic lport ELS request handler
>>   * @lport: The local port that received the request
>>   * @fp:	   The request frame
>>   *
>> @@ -859,9 +859,9 @@ out:
>>   * Locking Note: This function should not be called with the lport
>>   *		 lock held becuase it will grab the lock.
>>   */
>> -static void fc_lport_recv_req(struct fc_lport *lport, struct fc_frame *fp)
>> +static void fc_lport_recv_els_req(struct fc_lport *lport,
>> +				  struct fc_frame *fp)
>>  {
>> -	struct fc_frame_header *fh = fc_frame_header_get(fp);
>>  	void (*recv)(struct fc_lport *, struct fc_frame *);
>>  
>>  	mutex_lock(&lport->lp_mutex);
>> @@ -873,8 +873,7 @@ static void fc_lport_recv_req(struct fc_lport *lport, struct fc_frame *fp)
>>  	 */
>>  	if (!lport->link_up)
>>  		fc_frame_free(fp);
>> -	else if (fh->fh_type == FC_TYPE_ELS &&
>> -		 fh->fh_r_ctl == FC_RCTL_ELS_REQ) {
>> +	else {
>>  		/*
>>  		 * Check opcode.
>>  		 */
>> @@ -903,14 +902,62 @@ static void fc_lport_recv_req(struct fc_lport *lport, struct fc_frame *fp)
>>  		}
>>  
>>  		recv(lport, fp);
>> -	} else {
>> -		FC_LPORT_DBG(lport, "dropping invalid frame (eof %x)\n",
>> -			     fr_eof(fp));
>> -		fc_frame_free(fp);
>>  	}
>>  	mutex_unlock(&lport->lp_mutex);
>>  }
>>  
>> +static int fc_lport_els_prli(struct fc_rport_priv *rdata, u32 spp_len,
>> +			     const struct fc_els_spp *spp_in,
>> +			     struct fc_els_spp *spp_out)
>> +{
>> +	return FC_SPP_RESP_INVL;
>> +}
>> +
>> +struct fc4_prov fc_lport_els_prov = {
>> +	.prli = fc_lport_els_prli,
>> +	.recv = fc_lport_recv_els_req,
>> +};
>> +
>> +/**
>> + * 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.

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

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

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

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.

	Joe




More information about the devel mailing list