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 <[email protected]> >> >> 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 <[email protected]> >> Signed-off-by: Robert Love <[email protected]> > > 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 _______________________________________________ devel mailing list [email protected] https://lists.open-fcoe.org/mailman/listinfo/devel
