[PATCH 12/17] nvme-fabrics: reset connection for secure concatenation

[Sagi Grimberg]

On 08/04/2024 9:21, Hannes Reinecke wrote:
> On 4/7/24 23:46, Sagi Grimberg wrote:
>>
>>
>> On 18/03/2024 17:03, Hannes Reinecke wrote:
>>> When secure concatenation is requested the connection needs to be
>>> reset to enable TLS encryption on the new cnnection.
>>> That implies that the original connection used for the DH-CHAP
>>> negotiation really shouldn't be used, and we should reset as soon
>>> as the DH-CHAP negotiation has succeeded on the admin queue.
>>> The current implementation does not allow to easily skip
>>> connection attempts on the I/O queues, so we connect I/O
>>> queues, but disable namespace scanning on these queues.
>>> With that no I/O can be issued on these queues, so we
>>> can tear them down quickly without having to wait for
>>> quiescing etc.
>>
>> We shouldn't have to connect io queues here. The scan prevention
>> is just a hack...
>>
> Oh, believe me, I tried. What _should_ be possible is to create a
> controller with just admin queues, and skip the io queue setup part.
> But once you do that it's impossible to create io queues after reset
> (which is what you'd need here).
> I tried to fix this, but that ended up with a massive rewrite/reordering
> of the init path. Which sure has its merits, but I deemed out of scope
> for this patchset.
>
> So I decided for this 'hack', and shelved the init path reorg for a
> later patchset.

Interesting that it requires a full rewrite. Can you share some info
what particularly breaks when you change the nr_io_queues between
resets? nr_io_queues can be changed across resets from the controller side
as well.

>
>>> Once that is done we can reset the controller directly
>>> after the ->create_ctrl() callback.
>>
>> Why not set opts->nr_io_queues = 0 for secure concatenation and
>> setting it to the original value before resetting?
>
> See above. The io tagset is allocated in the init path _only_.

ok. So? What is the issue here? is this because ns scanning is checking
the existence of a ctrl->tagset? I predicted that this check will be 
problematic
to assume the ctrl state in the future.

We can add a controller flag for this.

> And creating of the io tagset is tied with connecting the io queues.

Where exactly?

>
> The reset code requires the tagset to be present; it just reconnects
> the io queues.

it connects io_queues if ctrl->queue_count > 1

> So either you do some trickery here like skipping connecting io queues
> (or, indeed, skipping namespace scanning),
> or you end up with a massive reshuffling of the init code path trying
> to separate tagset creation from io queue connections.

Hmm, I'm not exactly sure what separation exactly is required here.