On Fri, 2010-04-09 at 11:05 -0700, Joe Eykholt wrote:
> Robert Love wrote:
<snip>
> > @@ -328,8 +333,7 @@ static void fc_rport_work(struct work_struct *work)
> > lport->tt.exch_mgr_reset(lport, port_id, 0);
> >
> > if (rport) {
> > - rpriv = rport->dd_data;
> > - rpriv->rp_state = RPORT_ST_DELETE;
> > + rdata->rp_state = RPORT_ST_DELETE;
>
> This should not change the rdata state, which will already
> be either DELETE or RESTART.
I'm not changing the state, I'm just changing the variable name from
rpriv to rdata. If there's a bug because of the state assignment I think
it's different from what I'm trying to accomplish with this patch.
>
> > mutex_lock(&rdata->rp_mutex);
> > rdata->rport = NULL;
> > mutex_unlock(&rdata->rp_mutex);
> > @@ -1687,15 +1691,21 @@ void fc_destroy_rport()
> > }
> >
> > /**
> > - * fc_rport_terminate_io() - Stop all outstanding I/O on a remote port
> > - * @rport: The remote port whose I/O should be terminated
> > + * fc_rport_dev_loss_tmo_callbk() - Final cleanup of fc_rport_priv
> > + * @rport: The remote port that is going to be freed
> > + *
> > + * The rport is about to be freed by the FC Transport. The transport
> > + * has already terminated all rport I/O and now it's about to do
> > + * the final cleanup. To libfc this just means decrementing the
> > + * reference count so the fc_rport_priv can be freed.
> > */
> > -void fc_rport_terminate_io(struct fc_rport *rport)
> > +void fc_rport_dev_loss_tmo_callbk(struct fc_rport *rport)
> > {
> > - struct fc_rport_libfc_priv *rpriv = rport->dd_data;
> > - struct fc_lport *lport = rpriv->local_port;
> > + struct fc_rport_priv *rdata = rport_to_rport_priv(rport);
> > + struct fc_lport *lport = rdata->local_port;
> >
> > lport->tt.exch_mgr_reset(lport, 0, rport->port_id);
> > lport->tt.exch_mgr_reset(lport, rport->port_id, 0);
> > + kref_put(&rdata->kref, rdata->local_port->tt.rport_destroy);
> > }
> > -EXPORT_SYMBOL(fc_rport_terminate_io);
> > +EXPORT_SYMBOL(fc_rport_dev_loss_tmo_callbk);
> > diff --git a/include/scsi/libfc.h b/include/scsi/libfc.h
> > index a26bb50..021be0c 100644
> > --- a/include/scsi/libfc.h
> > +++ b/include/scsi/libfc.h
> > @@ -160,24 +160,6 @@ struct fc_rport_operations {
> > };
> >
> > /**
> > - * struct fc_rport_libfc_priv - libfc internal information about a remote
> > port
> > - * @local_port: The associated local port
> > - * @rp_state: Indicates READY for I/O or DELETE when blocked
> > - * @flags: REC and RETRY supported flags
> > - * @e_d_tov: Error detect timeout value (in msec)
> > - * @r_a_tov: Resource allocation timeout value (in msec)
> > - */
> > -struct fc_rport_libfc_priv {
> > - struct fc_lport *local_port;
> > - enum fc_rport_state rp_state;
> > - u16 flags;
> > - #define FC_RP_FLAGS_REC_SUPPORTED (1 << 0)
> > - #define FC_RP_FLAGS_RETRY (1 << 1)
> > - unsigned int e_d_tov;
> > - unsigned int r_a_tov;
> > -};
>
> Instead of removing this we could make it a bit smaller, perhaps.
> It's already tiny. Flags could be u8. The rp_state could
> be packed into a u8.
>
The size doesn't bother me although I think your suggestions are good.
I'm trying to eliminate the initialization race between fc_rport_work
and fc_queuecommand since there's nothing to guarantee that
fc_queuecommand won't get called before fc_rport_libfc_priv is
initialized and we need it initialized before proceeding to get the
flags.
> > -
> > -/**
> > * struct fc_rport_priv - libfc remote port and discovery info
> > * @local_port: The associated local port
> > * @rport: The FC transport remote port
> > @@ -202,6 +184,8 @@ struct fc_rport_priv {
> > struct kref kref;
> > enum fc_rport_state rp_state;
> > struct fc_rport_identifiers ids;
> > + #define FC_RP_FLAGS_REC_SUPPORTED (1 << 0)
> > + #define FC_RP_FLAGS_RETRY (1 << 1)
>
> Let's put the definitions after the flags if we do this.
>
Will do.
> > u16 flags;
> > u16 max_seq;
> > u16 disc_id;
> > @@ -218,6 +202,9 @@ struct fc_rport_priv {
> > u32 supported_classes;
> > };
> >
> > +#define rport_to_rport_priv(r) \
> > + (*((struct fc_rport_priv**)r->dd_data))
>
> r should be in parentheses. Space before **.
>
Thanks, will do.
> > +
> > /**
> > * struct fcoe_dev_stats - fcoe stats structure
> > * @SecondsSinceLastReset: Seconds since the last reset
> > @@ -989,7 +976,7 @@ int fc_lport_bsg_request(struct fc_bsg_job *);
> > * REMOTE PORT LAYER
> > *****************************/
> > int fc_rport_init(struct fc_lport *);
> > -void fc_rport_terminate_io(struct fc_rport *);
> > +void fc_rport_dev_loss_tmo_callbk(struct fc_rport *);
> >
> > /*
> > * DISCOVERY LAYER
>
> I remember struggling with this when I did the rport
> rework a year or two ago. I had initially gotten rid
> of fc_rport_libfc_priv and then brought it back.
> A couple of issues I can remember:
>
> First, the dd_data area still will not be filled in immediately after
> fc_rport is created, so rport_to_rport_priv() could return NULL.
There are only a few times when we want to do a rport_to_rport_priv.
1) dev loss callback
- rport would have been allocated
2) fc_rport_work- when assigning the rport_priv to dd_data
- rport was just created with dd_data and returned
3) fcp_cmd_send
- fc_queuecommand checking for rport_to_rport_priv ensures this call is
never made without a valid rport_priv
4) fc_fcp_timeout
- called from fcp_cmd_send, same reasoning as #3
5) fc_fcp_rec
- REC is after an I/O has passed fc_queuecommand, same as #3
6) fc_fcp_rec_resp
- response to command that passed queuecommand, same as #3
7) fc_fcp_srr
- same as #6
8) fc_queuecommand
- checks for NULL to ensure no I/O passes
I think this shows that the only time rport_to_rport_priv could be
called and return NULL is in fc_queuecommand and it's checking for NULL.
> We need to check for that. Maybe creating with no roles and
> changing them after setting the pointer would fix that.
>
> BTW, there's an existing problem where if the roles change, we don't
> report the role change to scsi_transport_fc. I have a fix for
> that in my next set of patches. It doesn't come up unless you
> have a node that changes from initiator to target.
>
> Two: the remote port could restart, and we could end up with
> two rdatas for the same rport.
>
> After fc_rport_work() takes rdata out of the list, any
> newly-discovered rport or received PLOGI
> will cause a new rdata to be created and used. Then when
> the transport create is called it will return with an rport
> with dd_data already set. We have to either drop the old one
> (which may be in use by drivers somehow) or the new one,
> which may be used by new incoming requests. This seems like
Yeah, this is a problem. Thanks for pointing it out. I'll need to think
more about this.
> the main problem. It simplifies the drivers and fc_fcp if
> they don't need to hold/release rdata or worry about it
> going away. Delaying the free until devloss timeout helps
> with that, but since the rport may restart and get re-created,
> there's the above problem.
>
> We can't create the transport rport as soon as we like
> because we don't know the port_name and node_name and roles
> right away, and those might be used in binding. So, we have
> to keep the rdata independent from the rport, but there's
> got to be the separate dd_data that travels with the
> transport rport and gives just enough info (timeouts and
> flags) that is needed for I/O. It's always allocated
> by the transport, but may not be filled in.
>
> Re-reading some of that old mail might reveal some better
> arguments why this is needed.
>
> I think the existing rport stuff works fairly well.
> I trust it. Let's change it too much.
^ "not" ? :)
The only other solution I have right now is to simply set an
'initialized' flag in fc_rport_libfc_priv and then set it to 1 once
fc_rport_work has set up its members after the remote_port_add. In
fc_queuecommand we'd just check if it was initialized or not.
Thanks for the comments, //Rob
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
