Re: [PATCH 2/3] nvme-core: don't check non-mdts for disc ctrl

Chaitanya Kulkarni
2022-04-11T21:00:58Z

On 4/11/22 07:14, Keith Busch wrote:
> On Mon, Apr 11, 2022 at 12:40:22PM +0000, Chaitanya Kulkarni wrote:
>> @@ -3101,9 +3101,12 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl)
>> if (ret)
>> return ret;
>>
>> - ret = nvme_init_non_mdts_limits(ctrl);
>> - if (ret < 0)
>> - return ret;
>> + /* only check non mdts for the I/O controller */
>> + if (ctrl->tagset) {
>> + ret = nvme_init_non_mdts_limits(ctrl);
>> + if (ret < 0)
>> + return ret;
>> + }
>
> The nvme pci driver doesn't initialize the ctrl->tagset until after
> nvme_init_ctrl_finish(), so this won't work.

I think we should move nvme_init_non_mdts_limits() from
nvme_init_ctrl_finish() before ctrl actually allocate the I/O tagset ?

Since tagset allocation happens only once, it is guarded by the
new ctrl check in every transport and before each transport calls the
nvme_scan_work() from nvme_start_ctrl() path then proceed to the ns
allocation where these limits max_discard_{segments|sectors}
and max_write_zeroes_sectors actually used in :-

nvme_scan_work()
...
nvme_validate_or_alloc_ns()
nvme_alloc_ns()
nvme_update_ns_info()
nvme_update_disk_info()

1. FC:-
nvme_fc_create_association()
nvme_fc_create_io_queues()
allocate ctrl->tagset. <-- call nvme_init_mdts_limits() before this.
nvme_start_ctrl()
nvme_scan_queue()
nvme_scan_work()
path to namespace allocation and accessing ctrl non mdts limits.

2. PCIe:-
nvme_reset_work()
nvme_dev_add()
allocate ctrl->tagset. <-- call nvme_init_mdts_limits() before this.
nvme_start_ctrl()
nvme_scan_queue()
nvme_scan_work()
path to namespace allocation and accessing ctrl non mdts limits.

3. RDMA :-
nvme_rdma_setup_ctrl
nvme_rdma_configure_io_queues
allocate ctrl->tagset. <-- call nvme_init_mdts_limits() before this.
nvme_start_ctrl()
nvme_scan_queue()
nvme_scan_work()
path to namespace allocation and accessing ctrl non mdts limits.

4. TCP :-
nvme_tcp_setup_ctrl
nvme_tcp_configure_io_queues
allocate ctrl->tagset. <-- call nvme_init_mdts_limits() before this.
nvme_start_ctrl()
nvme_scan_queue()
nvme_scan_work()
path to namespace allocation and accessing ctrl non mdts limits.

-ck

Based on this something like :-

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index e8d6a1e52083..3af2abd67e60 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2872,7 +2872,7 @@ static inline u32 nvme_mps_to_sectors(struct
nvme_ctrl *ctrl, u32 units)
return val;
}

-static int nvme_init_non_mdts_limits(struct nvme_ctrl *ctrl)
+int nvme_init_non_mdts_limits(struct nvme_ctrl *ctrl)
{
struct nvme_command c = { };
struct nvme_id_ctrl_nvm *id;
@@ -2924,6 +2924,7 @@ static int nvme_init_non_mdts_limits(struct
nvme_ctrl *ctrl)
kfree(id);
return ret;
}
+EXPORT_SYMBOL_GPL(nvme_init_non_mdts_limits);

static int nvme_init_identify(struct nvme_ctrl *ctrl)
{
@@ -3101,10 +3102,6 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl)
if (ret)
return ret;

- ret = nvme_init_non_mdts_limits(ctrl);
- if (ret < 0)
- return ret;
-
ret = nvme_configure_apst(ctrl);
if (ret < 0)
return ret;
diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 080f85f4105f..89a3d1d4f48f 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2882,6 +2882,10 @@ nvme_fc_create_io_queues(struct nvme_fc_ctrl *ctrl)
unsigned int nr_io_queues;
int ret;

+ ret = nvme_init_non_mdts_limits(&ctrl->ctrl);
+ if (ret < 0)
+ return ret;
+
nr_io_queues = min(min(opts->nr_io_queues, num_online_cpus()),
ctrl->lport->ops->max_hw_queues);
ret = nvme_set_queue_count(&ctrl->ctrl, &nr_io_queues);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index a1f8403ffd78..1a661226fbb0 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -777,6 +777,7 @@ long nvme_ns_head_chr_ioctl(struct file *file,
unsigned int cmd,
long nvme_dev_ioctl(struct file *file, unsigned int cmd,
unsigned long arg);
int nvme_getgeo(struct block_device *bdev, struct hd_geometry *geo);
+int nvme_init_non_mdts_limits(struct nvme_ctrl *ctrl);

extern const struct attribute_group *nvme_ns_id_attr_groups[];
extern const struct pr_ops nvme_pr_ops;
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 3554c20e78c3..ce4060c3d62e 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2525,6 +2525,13 @@ static void nvme_dev_add(struct nvme_dev *dev)
int ret;

if (!dev->ctrl.tagset) {
+ ret = nvme_init_non_mdts_limits(&dev->ctrl);
+ if (ret < 0) {
+ dev_warn(dev->ctrl.device,
+ "reading non mdts limit error: %d\n", ret);
+ return;
+ }
+
dev->tagset.ops = &nvme_mq_ops;
dev->tagset.nr_hw_queues = dev->online_queues - 1;
dev->tagset.nr_maps = 2; /* default + read */
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index c49b9c3c46f2..f6cf28a3b4c1 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -972,6 +972,10 @@ static int nvme_rdma_configure_io_queues(struct
nvme_rdma_ctrl *ctrl, bool new)
return ret;

if (new) {
+ ret = nvme_init_non_mdts_limits(&ctrl->ctrl);
+ if (ret < 0)
+ goto out_free_io_queues;
+
ctrl->ctrl.tagset = nvme_rdma_alloc_tagset(&ctrl->ctrl,
false);
if (IS_ERR(ctrl->ctrl.tagset)) {
ret = PTR_ERR(ctrl->ctrl.tagset);
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 10fc45d95b86..9147459580d4 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1858,6 +1858,10 @@ static int nvme_tcp_configure_io_queues(struct
nvme_ctrl *ctrl, bool new)
return ret;

if (new) {
+ ret = nvme_init_non_mdts_limits(ctrl);
+ if (ret < 0)
+ goto out_free_io_queues;
+
ctrl->tagset = nvme_tcp_alloc_tagset(ctrl, false);
if (IS_ERR(ctrl->tagset)) {
ret = PTR_ERR(ctrl->tagset);
diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index 59024af2da2e..2ac9017e966a 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -522,6 +522,10 @@ static int nvme_loop_create_io_queues(struct
nvme_loop_ctrl *ctrl)
{
int ret;

+ ret = nvme_init_non_mdts_limits(&ctrl->ctrl);
+ if (ret < 0)
+ return ret;
+
ret = nvme_loop_init_io_queues(ctrl);
if (ret)
return ret;

-ck