Re: [PATCH 04/12] IB/srp: Fix connection state tracking

Bart Van Assche
2015-05-05T09:59:15Z

On 05/05/15 11:45, Sagi Grimberg wrote:
> On 5/5/2015 12:31 PM, Bart Van Assche wrote:
>> On 04/30/15 17:00, Sagi Grimberg wrote:
>>> On 4/30/2015 2:25 PM, Bart Van Assche wrote:
>>>> On 04/30/15 11:51, Sagi Grimberg wrote:
>>>>> On 4/30/2015 11:58 AM, Bart Van Assche wrote:
>>>>>> @@ -170,6 +170,7 @@ struct srp_rdma_ch {
>>>>>>
>>>>>> struct completion tsk_mgmt_done;
>>>>>> u8 tsk_mgmt_status;
>>>>>> + bool connected;
>>>>>
>>>>> I generally agree with this flag, but I'm a bit confused about the
>>>>> semantics of two connected flags? Also, we check the target connected
>>>>> flag on TMFs, although we are executing it on a channel (should we
>>>>> check both?)
>>>>>
>>>>> I'd say keep only the channel connected flag, the target logic needs to
>>>>> be mandated by the state.
>>>>
>>>> I think we need both flags. The ch->connected flag is used only in
>>>> srp_destroy_qp() to verify whether a channel has been disconnected
>>>> before it is destroyed. The target->connected flag provides an easy way
>>>> to test the connected state in e.g. srp_disconnect_target().
>>>
>>> We can just as easily check rport state. rport state is modified before
>>> target-connected. I still think one is redundant.
>>
>> Sorry but I disagree. If e.g. both the fast_io_fail and dev_loss_tmo
>> timers have been disabled then the rport state is SRP_RPORT_RUNNING all
>> the time. At least in that case it is not possible to derive the
>> target->connected state from the rport state.
>
> Can't you rely on ch->connected for that?
>
> We can keep both, but I just think that the meaning of
> target->connected is confusing now.

Hello Sagi,

I will see whether I can remove the target->connected state variable.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html