Re: [PATCH v3 1/2] nvme: add capability to connect to an administrative controller

[Kamaljit Singh]

Hi Hannes,
 
From: Hannes Reinecke <hare@xxxxxxx>
Date: Tuesday, July 22, 2025 at 00:06
>> Suggested-by: Niklas Cassel <cassel@xxxxxxxxxx>
>>
>> Add capability to connect to an administrative controller by
>> preventing ioq creation for admin-controllers.
>>
>> * Add helper nvme_admin_ctrl() to check for an administrative controller
>> * Add helper nvme_override_prohibited_io_queues() to override queue_count
>> * Call nvme_override_prohibited_io_queues() from nvme_init_ctrl_finish()
>>    so it applies to nvme/tcp and nvme/rdma
>>
>> Signed-off-by: Kamaljit Singh <kamaljit.singh1@xxxxxxx>
>> ---
>>   drivers/nvme/host/core.c | 21 +++++++++++++++++++++
>>   1 file changed, 21 insertions(+)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index e533d791955d..105127638c31 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -3149,6 +3149,21 @@ static inline bool nvme_discovery_ctrl(struct nvme_ctrl *ctrl)
>>       return ctrl->opts && ctrl->opts->discovery_nqn;
>>   }
>>
>> +static inline bool nvme_admin_ctrl(struct nvme_ctrl *ctrl)
>> +{
>> +     return ctrl->cntrltype == NVME_CTRL_ADMIN;
>> +}
>> +
>> +/*
>> + * An admin controller has one admin queue, but no I/O queues.
>> + * Override queue_count so it only creates an admin queue.
>> + */
>> +static inline void nvme_override_prohibited_io_queues(struct nvme_ctrl *ctrl)
>> +{
>> +     if (nvme_admin_ctrl(ctrl))
>> +             ctrl->queue_count = 1;
>> +}
>> +
>>   static bool nvme_validate_cntlid(struct nvme_subsystem *subsys,
>>               struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>>   {
>> @@ -3670,6 +3685,12 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl, bool was_suspended)
>>       if (ret)
>>               return ret;
>>
>> +     if (nvme_admin_ctrl(ctrl))
>> +             dev_dbg(ctrl->device,
>> +                     "Subsystem %s is an administrative controller",
>> +                     ctrl->subsys->subnqn);
>> +     nvme_override_prohibited_io_queues(ctrl);
>> +
>>       ret = nvme_configure_apst(ctrl);
>>       if (ret < 0)
>>               return ret;
>
>One wonders why this is done for admin controllers only; surely
>discovery controllers also don't support I/O queues, and should
>therefore have the same setting?
I agree. That was my patch-3 in v2, which I retracted in v3 based on
your comment below. Is someone taking that action item to discuss at FMDS?
I haven't attended it in a while.

If everyone agrees, I can add patch-3 back in and create v5 or since v4
patch-1 was accepted, I can issue a standalone patch just for this change.
Please let me know.



>From: Hannes Reinecke <hare@xxxxxxx>
>Date: Thursday, July 3, 2025 at 02:42
>> Prevent ioq creation for discovery-controllers as the spec prohibits
>> them, similarly to the administrative controllers.
>>
>> Reference: NVMe Base rev 2.2, sec 3.1.3.4, fig 28.
>>
>While this might be true, we already deal with discovery controllers
>just fine, and never had issues with I/O queues being created.
>Presumably because discovery controllers never exposed I/O queues
>in the first place.
>
>Which also means that admin controllers should work already if they
>would not expose I/O queues.
BTW, the reason admin controllers didn't work without my patch is that
nvme-cli "nvme connect" doesn't allow "--nr-io-queues=0" and the default
=1 forces the kernel to create an I/O queue, which my patch overrides.

>
>We _might_ reduce the number of queues to '1' in nvme_set_queue_count(),
>but then we should issue a warning here as this would be a configuration
>error on the target side.
>
>'Prohibited' cuts both ways; the host shouldn't ask for it and the
>controller shouldn't advertise it...
>
>Or that's at least my reading. Might be worthwhile clarifying this
>at FMDS.

Regards,
Kamaljit