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

Sagi Grimberg
2015-04-30T15:00:45Z

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:
>>> - srp_change_conn_state(target, false);
>>> + ch->connected = false;
>>
>> Shouldn't this be protected by the channel lock (like the target)?
>
> On all CPU architectures I'm familiar with changes of boolean variables
> are atomic so I think modifying that variable without locking is fine.
>
>>> @@ -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.

> And if it
> is attempted to send a task management function over a channel that has
> just been disconnected that should be fine since any channel disconnect
> causes tl_err_work to be queued. That last action will sooner or later
> result in a reconnect.

Will it be fine to queue commands when the channel is in reconnecting
stage?
--
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