Robert Love wrote:
> 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.
Now instead of changing the state in the rpriv, you're changing
the state in the rdata, which should have already been set
or be set elsewhere.
>>> 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.
It looks like you got all the fcp cases.
I'm not sure whether the fnic cases are covered.
>> 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" ? :)
Right! Oops.
> 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.
We could use the state or lport pointer for that. Putting a
wmb() between setting the rest of the rpriv and the state or
lport pointer would guarantee ordering.
> Thanks for the comments, //Rob
You're always welcome.
Joe
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
