Re: [PATCH 15/18] nvmet-tcp: enable TLS handshake upcall

[Sagi Grimberg <sagi@grimberg.me>]

>>> But there might be the odd case where the data_ready
>>> callback is invoked after kTLS has been started but before
>>> we're setting it to nvmet_tcp_data_ready().
>>
>> What does nvmet_tcp_data_ready has to do with it?
>> You are changing nvmet_tcp_listen_data_ready.
>>
> This is not a 'race;
>>> Then we cannot guarantee that sk_user_data really is set
>>> to the 'queue' pointer, so we should skip the function.
>>
>> nvme_tcp_listen_data_ready was invoked, then sk->sk_data_ready
>> is nvme_tcp_listen_data_ready. Are you referring to the case
>> where the callback has changed before the read lock ?
>>
> No. The callback is changed when the userspace daemon starts ktls, as
> that will set sk->sk_data_ready to tls_data_ready().
> 
> So before the userspace upcall we have the chain:
> 
> sk_data_ready -> nvmet_tcp_listen_data_ready()
> 
> and after a (successful) upcall we have the chain
> 
> sk_data_ready -> tls_data_ready
>    -> nvmet_tcp_listen_data_ready()
> 
> once we set our callback we have
> 
> sk_data_ready -> nvmet_tcp_data_ready
>    -> tls_data_ready
>      -> nvmet_tcp_listen_data_ready
> 
> Calling into nvmet_tcp_listen_data_ready() is pointless here,
> but we cannot remove the callback either. So we should to deactivate
> it to avoid any side effects.

Thanks for the explanation.

First, I would like to understand how is this different from
svc.

Second, we don't want nvmet_tcp_listen_data_ready keep being
called from the datapath, especially as it is taking a read
lock, for absolutly no good reason.

Why don't we restore the original socket callback before
we trigger the handshake? then we wait for the handshake
to complete, and then store nvmet_tcp_data_ready once it
is done?