[PATCH V3 00/10] nvme: use xarray for ns tracking

[PATCH V3 00/10] nvme: use xarray for ns tracking

[Chaitanya Kulkarni]

Hi,

This patch-series uses ctrl->namespaces with an xarray for host-core and
target-core. We can see following performance improvement when running
fio with 32 parallel jobs where first 16 namespaces and last 16
namespaces are used for I/O using NVMeOF (nvme-loop) backed by nulli blk
devices mapped 1:1 on target namespaces.

For host even though nvme_find_get_ns() doesn't fall into the fast path
yet it does for NVMeOF passthru. This prepares us to improve performance
for future NVMeOF passthru backend which is under review which uses the
similar data structure as target.

Following are the performance numbers with NVMeOF (nvme-loop) backed by
null_blk devices mapped 1:1 on NVMeOF target backend :-

Bandwidth ~16.4198% increase with XArray (higher the better) :-
-----------------------------------------------------------------------

default-1.fio.log:  read:  IOPS=820k,  BW=3204MiB/s  (3360MB/s)(188GiB/60002msec)
default-2.fio.log:  read:  IOPS=835k,  BW=3260MiB/s  (3418MB/s)(191GiB/60002msec)
default-3.fio.log:  read:  IOPS=834k,  BW=3257MiB/s  (3415MB/s)(191GiB/60001msec)
xarray-1.fio.log:   read:  IOPS=966k,  BW=3772MiB/s  (3955MB/s)(221GiB/60003msec)
xarray-2.fio.log:   read:  IOPS=966k,  BW=3775MiB/s  (3958MB/s)(221GiB/60002msec)
xarray-3.fio.log:   read:  IOPS=965k,  BW=3769MiB/s  (3952MB/s)(221GiB/60002msec)

Latency (submission) ~25% decrease with XArray (lower the better) :-
------------------------------------------------------------------------

default-1.fio.log:  slat  (usec):  min=8,  max=26066,  avg=25.18,  stdev=47.72
default-2.fio.log:  slat  (usec):  min=8,  max=907,    avg=20.24,  stdev=7.36
default-3.fio.log:  slat  (usec):  min=8,  max=723,    avg=20.21,  stdev=7.16
xarray-1.fio.log:   slat  (usec):  min=8,  max=639,    avg=14.84,  stdev=1.50
xarray-2.fio.log:   slat  (usec):  min=8,  max=840,    avg=14.84,  stdev=1.51
xarray-3.fio.log:   slat  (usec):  min=8,  max=2161,   avg=15.08,  stdev=9.56

CPU usage (system) ~12.2807% decrease with XArray (lower the better) :-
-----------------------------------------------------------------------

default-1.fio.log:  cpu  :  usr=3.92%,  sys=57.25%,  ctx=2159595,  majf=0,  minf=2807
default-2.fio.log:  cpu  :  usr=3.98%,  sys=57.99%,  ctx=1565139,  majf=0,  minf=2425
default-3.fio.log:  cpu  :  usr=3.99%,  sys=57.85%,  ctx=1563792,  majf=0,  minf=2977
xarray-1.fio.log:   cpu  :  usr=4.47%,  sys=50.88%,  ctx=1810927,  majf=0,  minf=2478
xarray-2.fio.log:   cpu  :  usr=4.47%,  sys=50.88%,  ctx=1812184,  majf=0,  minf=2176
xarray-3.fio.log:   cpu  :  usr=4.49%,  sys=50.86%,  ctx=1816963,  majf=0,  minf=2736

We did not hear anything on XArray bits, it will be great to have some feedback
from Matthew Willcox so we can atleast converge XArray part of this series.

I'm still doing more deeper testing but basic things seems to work with
performance numbers being consistent with couple of outstanding issues.
(using xa for ns remove instead of list_head and overall xa comments from
Matthew).

Regards,
Chaitanya

* Changes from V2:-
-------------------

1.  Add Xarray __xa_load() API as a preparation patch.
2.  Remove the id_ctrl call in nvme_dev_user_cmd().
3.  Remove the switch error check for xa_insert().
4.  Don't change the ns->kref code. when calling xa_erase().
5.  Keep XArray for deletion in the nvme_remove_invalid_namespaces()
    see [1].
6.  Keep XArray for deletion in the nvme_remove_namespaces() see [1].
7.  Remove randomly changed the lines to alingn the coding style in
    nvmet patch.
8.  Remove remaining #include nvme.h from the nvmet patch.
9.  Remove the xa_empty() from nvmet_max_nsid().
10. Centralize the blk-mq queue wrapper. The blk-mq queue related
    wrapper functions nvme_kill_queues(), nvme_unfreeze(),
    nvme_wait_freeze(), nvme_start_freeze(), nvme_stop_queues(),
    nvme_start_queues(), nvme_start_queues(), and nvme_sync_queues()
    differ in only one line i.e. blk_mq_queue_xxx() call. For the one
    line we have 7 functions and 7 exported symbols. Using a 
    centralize ctrl-queue action function and well defined enums
    represnting names of the helpers we can minimize the code and
    exported symbol and still maintain the redability.

* Change from V1:-
------------------

1. Use xarray instead of rcu locks.

[1] Keeping the Xarray for ns deletion in nvme_remove_namespaces() and
    nvme_remove_invalid_namepaces() :-

1.1. No impact on the fast patch :-

    The list_head is only needed for the deletion purpose which and has no
    impact on the fast path.

1.2 Effects on cacheline size :-

    Size of the struct nvme_ns without list head is 112 [1.3] which still
    has some room for future members so that we can fit members in 2
    cachelines before 3rd one is needed. With addition it becomes 128 i.e.
    for any new member we will need 3rd cacheline the test machine has 
    cacheline size == 64.

    # getconf LEVEL1_DCACHE_LINESIZE
    64

    Even though nvme_fine_get_ns() is not in the fast path yet with
    addtion of passthru it will be, also with any new field we will have
    to fetch one extra cache line.


[1.3] Without addition of the list_head size 112 :-

# dmesg  -c
[  737.953355] nvme_core_init 4601 sizeof(nvme_ns) 112 <---
# git diff
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index a8c39254dae1..89c10cb7f6ef 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4598,6 +4598,7 @@ static int __init nvme_core_init(void)
 {
        int result = -ENOMEM;
 
+       pr_info("%s %d sizeof(nvme_ns) %lu\n", __func__, __LINE__, sizeof(struct nvme_ns));
        _nvme_check_size();
 
        nvme_wq = alloc_workqueue("nvme-wq",

[1.4] With addition of the list_head size becomes 128 :-

# dmesg  -c
[  772.513266] nvme_core_init 4601 sizeof(nvme_ns) 128 < ----
# git diff
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index a8c39254dae1..89c10cb7f6ef 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4598,6 +4598,7 @@ static int __init nvme_core_init(void)
 {
        int result = -ENOMEM;
 
+       pr_info("%s %d sizeof(nvme_ns) %lu\n", __func__, __LINE__, sizeof(struct nvme_ns));
        _nvme_check_size();
 
        nvme_wq = alloc_workqueue("nvme-wq",
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 1e8dedee74df..2907f6fe0400 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -394,6 +394,7 @@ enum nvme_ns_features {
 };
 
 struct nvme_ns {
+       struct list_head list;
        struct nvme_ctrl *ctrl;
        struct request_queue *queue;
        struct gendisk *disk;

Chaitanya Kulkarni (10):
  xarray: add __xa_load() version
  nvme-core: use xarray for ctrl ns tracking
  nvme: centralize queue action nvme_kill_queues()
  nvme: centralize queue action nvme_unfreeze()
  nvme: centralize queue action nvme_wait_freeze()
  nvme: centralize queue action nvme_start_freeze()
  nvme: centralize queue action nvme_stop_queues()
  nvme: centralize queue action nvme_start_queues()
  nvme: centralize queue action nvme_sync_queues()
  nvmet: use xarray for ctrl ns storing

 drivers/nvme/host/core.c        | 291 ++++++++++++++------------------
 drivers/nvme/host/fc.c          |   4 +-
 drivers/nvme/host/multipath.c   |  15 +-
 drivers/nvme/host/nvme.h        |  23 +--
 drivers/nvme/host/pci.c         |  28 +--
 drivers/nvme/host/rdma.c        |   6 +-
 drivers/nvme/host/tcp.c         |   6 +-
 drivers/nvme/target/admin-cmd.c |  17 +-
 drivers/nvme/target/core.c      |  58 ++-----
 drivers/nvme/target/loop.c      |   2 +-
 drivers/nvme/target/nvmet.h     |   3 +-
 include/linux/xarray.h          |   1 +
 lib/xarray.c                    |  26 ++-
 13 files changed, 212 insertions(+), 268 deletions(-)

-- 
2.26.0


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH V3 01/10] xarray: add __xa_load() version

[Chaitanya Kulkarni]

This patch adds a new xarray API __xa_load() that moves xa_load() core
to its low level function __xa_load(). Caller is responsible for handling
RCU locking. This API is needed for NVMe subsystem so that it can take
an advantage of the RCU locking provided by the XAarray for reference
counting.

This is a preparation patch for replacing linked list with XArray for
NVMe host and core subsystem.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 include/linux/xarray.h |  1 +
 lib/xarray.c           | 26 ++++++++++++++++++++++----
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/include/linux/xarray.h b/include/linux/xarray.h
index b4d70e7568b2..ea63005779ab 100644
--- a/include/linux/xarray.h
+++ b/include/linux/xarray.h
@@ -344,6 +344,7 @@ struct xarray {
  */
 #define DEFINE_XARRAY_ALLOC1(name) DEFINE_XARRAY_FLAGS(name, XA_FLAGS_ALLOC1)
 
+void *__xa_load(struct xarray *xa, unsigned long index);
 void *xa_load(struct xarray *, unsigned long index);
 void *xa_store(struct xarray *, unsigned long index, void *entry, gfp_t);
 void *xa_erase(struct xarray *, unsigned long index);
diff --git a/lib/xarray.c b/lib/xarray.c
index e9e641d3c0c3..cb9188e74505 100644
--- a/lib/xarray.c
+++ b/lib/xarray.c
@@ -1288,24 +1288,42 @@ void *xas_find_conflict(struct xa_state *xas)
 EXPORT_SYMBOL_GPL(xas_find_conflict);
 
 /**
- * xa_load() - Load an entry from an XArray.
+ * __xa_load() - core load API without rcu read locking.
  * @xa: XArray.
  * @index: index into array.
  *
- * Context: Any context.  Takes and releases the RCU lock.
+ * Context: Any context. Caller is responsible for the RCU lock/unlock.
  * Return: The entry at @index in @xa.
  */
-void *xa_load(struct xarray *xa, unsigned long index)
+void *__xa_load(struct xarray *xa, unsigned long index)
 {
 	XA_STATE(xas, xa, index);
 	void *entry;
 
-	rcu_read_lock();
 	do {
 		entry = xas_load(&xas);
 		if (xa_is_zero(entry))
 			entry = NULL;
 	} while (xas_retry(&xas, entry));
+
+	return entry;
+}
+EXPORT_SYMBOL_GPL(__xa_load);
+
+/**
+ * xa_load() - Load an entry from an XArray.
+ * @xa: XArray.
+ * @index: index into array.
+ *
+ * Context: Any context.  Takes and releases the RCU lock.
+ * Return: The entry at @index in @xa.
+ */
+void *xa_load(struct xarray *xa, unsigned long index)
+{
+	void *entry;
+
+	rcu_read_lock();
+	entry = __xa_load(xa, index);
 	rcu_read_unlock();
 
 	return entry;
-- 
2.26.0


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH V3 02/10] nvme-core: use xarray for ctrl ns tracking

[Chaitanya Kulkarni]

This patch replaces the ctrl->namespaces tracking from linked list to
xarray and improves the performance. For host even though
nvme_find_get_ns() doesn't fall into the fast path yet it does for
NVMeOF passthru patch-series which is currently under review. This
prepares us to improve performance for future NVMeOF passthru backend
since nvme_find_get_ns() uses same data structure as it does in target
to find namespace in I/O patch from nsid specified in the nvme_rw_cmd.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/host/core.c      | 188 ++++++++++++++++------------------
 drivers/nvme/host/multipath.c |  15 ++-
 drivers/nvme/host/nvme.h      |   5 +-
 3 files changed, 98 insertions(+), 110 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index b9b4aa7b53ce..297835bda996 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1426,9 +1426,9 @@ static u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 static void nvme_update_formats(struct nvme_ctrl *ctrl, u32 *effects)
 {
 	struct nvme_ns *ns;
+	unsigned long idx;
 
-	down_read(&ctrl->namespaces_rwsem);
-	list_for_each_entry(ns, &ctrl->namespaces, list)
+	xa_for_each(&ctrl->namespaces, idx, ns)
 		if (_nvme_revalidate_disk(ns->disk))
 			nvme_set_queue_dying(ns);
 		else if (blk_queue_is_zoned(ns->disk->queue)) {
@@ -1440,7 +1440,6 @@ static void nvme_update_formats(struct nvme_ctrl *ctrl, u32 *effects)
 			 */
 			*effects |= NVME_CMD_EFFECTS_NCC;
 		}
-	up_read(&ctrl->namespaces_rwsem);
 }
 
 static void nvme_passthru_end(struct nvme_ctrl *ctrl, u32 effects)
@@ -3188,34 +3187,33 @@ static int nvme_dev_open(struct inode *inode, struct file *file)
 static int nvme_dev_user_cmd(struct nvme_ctrl *ctrl, void __user *argp)
 {
 	struct nvme_ns *ns;
-	int ret;
+	unsigned long idx;
+	int ret = -EINVAL;
+	int count = 0;
 
-	down_read(&ctrl->namespaces_rwsem);
-	if (list_empty(&ctrl->namespaces)) {
+	if (xa_empty(&ctrl->namespaces)) {
 		ret = -ENOTTY;
-		goto out_unlock;
+		goto out;
 	}
-
-	ns = list_first_entry(&ctrl->namespaces, struct nvme_ns, list);
-	if (ns != list_last_entry(&ctrl->namespaces, struct nvme_ns, list)) {
-		dev_warn(ctrl->device,
-			"NVME_IOCTL_IO_CMD not supported when multiple namespaces present!\n");
-		ret = -EINVAL;
-		goto out_unlock;
+	xa_for_each(&ctrl->namespaces, idx, ns) {
+		if (count > 0)
+			goto err;
+		count++;
 	}
 
 	dev_warn(ctrl->device,
 		"using deprecated NVME_IOCTL_IO_CMD ioctl on the char device!\n");
 	kref_get(&ns->kref);
-	up_read(&ctrl->namespaces_rwsem);
 
 	ret = nvme_user_cmd(ctrl, ns, argp);
 	nvme_put_ns(ns);
-	return ret;
 
-out_unlock:
-	up_read(&ctrl->namespaces_rwsem);
+out:
 	return ret;
+err:
+	dev_warn(ctrl->device,
+			"NVME_IOCTL_IO_CMD not supported when multiple namespaces present!\n");
+	goto out;
 }
 
 static long nvme_dev_ioctl(struct file *file, unsigned int cmd,
@@ -3783,31 +3781,16 @@ static int nvme_init_ns_head(struct nvme_ns *ns, unsigned nsid,
 	return ret;
 }
 
-static int ns_cmp(void *priv, struct list_head *a, struct list_head *b)
-{
-	struct nvme_ns *nsa = container_of(a, struct nvme_ns, list);
-	struct nvme_ns *nsb = container_of(b, struct nvme_ns, list);
-
-	return nsa->head->ns_id - nsb->head->ns_id;
-}
-
 static struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 {
-	struct nvme_ns *ns, *ret = NULL;
+	struct nvme_ns *ns;
 
-	down_read(&ctrl->namespaces_rwsem);
-	list_for_each_entry(ns, &ctrl->namespaces, list) {
-		if (ns->head->ns_id == nsid) {
-			if (!kref_get_unless_zero(&ns->kref))
-				continue;
-			ret = ns;
-			break;
-		}
-		if (ns->head->ns_id > nsid)
-			break;
-	}
-	up_read(&ctrl->namespaces_rwsem);
-	return ret;
+	rcu_read_lock();
+	ns = __xa_load(&ctrl->namespaces, nsid);
+	ns = ns && kref_get_unless_zero(&ns->kref) ? ns : NULL;
+	rcu_read_unlock();
+
+	return ns;
 }
 
 static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
@@ -3877,9 +3860,9 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 		}
 	}
 
-	down_write(&ctrl->namespaces_rwsem);
-	list_add_tail(&ns->list, &ctrl->namespaces);
-	up_write(&ctrl->namespaces_rwsem);
+	ret = xa_insert(&ctrl->namespaces, nsid, ns, GFP_KERNEL);
+	if (ret)
+		goto out_put_disk;
 
 	nvme_get_ctrl(ctrl);
 
@@ -3911,6 +3894,8 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 
 static void nvme_ns_remove(struct nvme_ns *ns)
 {
+	struct xarray *xa = &ns->ctrl->namespaces;
+
 	if (test_and_set_bit(NVME_NS_REMOVING, &ns->flags))
 		return;
 
@@ -3933,9 +3918,7 @@ static void nvme_ns_remove(struct nvme_ns *ns)
 			blk_integrity_unregister(ns->disk);
 	}
 
-	down_write(&ns->ctrl->namespaces_rwsem);
-	list_del_init(&ns->list);
-	up_write(&ns->ctrl->namespaces_rwsem);
+	xa_erase(xa, ns->head->ns_id);
 
 	nvme_mpath_check_last_path(ns);
 	nvme_put_ns(ns);
@@ -3967,19 +3950,32 @@ static void nvme_validate_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 static void nvme_remove_invalid_namespaces(struct nvme_ctrl *ctrl,
 					unsigned nsid)
 {
-	struct nvme_ns *ns, *next;
-	LIST_HEAD(rm_list);
+	struct xarray *namespaces = &ctrl->namespaces;
+	struct xarray rm_array;
+	unsigned long tnsid;
+	struct nvme_ns *ns;
+	unsigned long idx;
+	int ret;
 
-	down_write(&ctrl->namespaces_rwsem);
-	list_for_each_entry_safe(ns, next, &ctrl->namespaces, list) {
-		if (ns->head->ns_id > nsid || test_bit(NVME_NS_DEAD, &ns->flags))
-			list_move_tail(&ns->list, &rm_list);
+	xa_init(&rm_array);
+
+	xa_lock(namespaces);
+	xa_for_each(namespaces, idx, ns) {
+		tnsid = ns->head->ns_id;
+		if (tnsid > nsid || test_bit(NVME_NS_DEAD, &ns->flags)) {
+			xa_unlock(namespaces);
+			xa_erase(namespaces, tnsid);
+			/* Even if insert fails keep going */
+			ret = xa_insert(&rm_array, nsid, ns, GFP_KERNEL);
+			if (ret)
+				pr_err("xa_insert %d\n", ret);
+			xa_lock(namespaces);
+		}
 	}
-	up_write(&ctrl->namespaces_rwsem);
+	xa_unlock(namespaces);
 
-	list_for_each_entry_safe(ns, next, &rm_list, list)
+	xa_for_each(&rm_array, idx, ns)
 		nvme_ns_remove(ns);
-
 }
 
 static int nvme_scan_ns_list(struct nvme_ctrl *ctrl)
@@ -4077,10 +4073,6 @@ static void nvme_scan_work(struct work_struct *work)
 	if (nvme_scan_ns_list(ctrl) != 0)
 		nvme_scan_ns_sequential(ctrl);
 	mutex_unlock(&ctrl->scan_lock);
-
-	down_write(&ctrl->namespaces_rwsem);
-	list_sort(NULL, &ctrl->namespaces, ns_cmp);
-	up_write(&ctrl->namespaces_rwsem);
 }
 
 /*
@@ -4090,8 +4082,13 @@ static void nvme_scan_work(struct work_struct *work)
  */
 void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
 {
-	struct nvme_ns *ns, *next;
-	LIST_HEAD(ns_list);
+	struct xarray rm_array;
+	unsigned long tnsid;
+	struct nvme_ns *ns;
+	unsigned long idx;
+	int ret;
+
+	xa_init(&rm_array);
 
 	/*
 	 * make sure to requeue I/O to all namespaces as these
@@ -4112,11 +4109,19 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
 	if (ctrl->state == NVME_CTRL_DEAD)
 		nvme_kill_queues(ctrl);
 
-	down_write(&ctrl->namespaces_rwsem);
-	list_splice_init(&ctrl->namespaces, &ns_list);
-	up_write(&ctrl->namespaces_rwsem);
+	xa_lock(&ctrl->namespaces);
+	xa_for_each(&ctrl->namespaces, idx, ns) {
+		tnsid = ns->head->ns_id;
+		xa_unlock(&ctrl->namespaces);
+		xa_erase(&ctrl->namespaces, tnsid);
+		ret = xa_insert(&rm_array, tnsid, ns, GFP_KERNEL);
+		if (ret)
+			pr_err("xa_insert %d\n", ret);
+		xa_lock(&ctrl->namespaces);
+	}
+	xa_unlock(&ctrl->namespaces);
 
-	list_for_each_entry_safe(ns, next, &ns_list, list)
+	xa_for_each(&rm_array, idx, ns)
 		nvme_ns_remove(ns);
 }
 EXPORT_SYMBOL_GPL(nvme_remove_namespaces);
@@ -4338,6 +4343,10 @@ static void nvme_free_ctrl(struct device *dev)
 	if (subsys && ctrl->instance != subsys->instance)
 		ida_simple_remove(&nvme_instance_ida, ctrl->instance);
 
+	WARN_ON_ONCE(!xa_empty(&ctrl->namespaces));
+
+	xa_destroy(&ctrl->namespaces);
+
 	list_for_each_entry_safe(cel, next, &ctrl->cels, entry) {
 		list_del(&cel->entry);
 		kfree(cel);
@@ -4372,9 +4381,8 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 	ctrl->state = NVME_CTRL_NEW;
 	spin_lock_init(&ctrl->lock);
 	mutex_init(&ctrl->scan_lock);
-	INIT_LIST_HEAD(&ctrl->namespaces);
 	INIT_LIST_HEAD(&ctrl->cels);
-	init_rwsem(&ctrl->namespaces_rwsem);
+	xa_init(&ctrl->namespaces);
 	ctrl->dev = dev;
 	ctrl->ops = ops;
 	ctrl->quirks = quirks;
@@ -4454,101 +4462,87 @@ EXPORT_SYMBOL_GPL(nvme_init_ctrl);
 void nvme_kill_queues(struct nvme_ctrl *ctrl)
 {
 	struct nvme_ns *ns;
-
-	down_read(&ctrl->namespaces_rwsem);
+	unsigned long idx;
 
 	/* Forcibly unquiesce queues to avoid blocking dispatch */
 	if (ctrl->admin_q && !blk_queue_dying(ctrl->admin_q))
 		blk_mq_unquiesce_queue(ctrl->admin_q);
 
-	list_for_each_entry(ns, &ctrl->namespaces, list)
+	xa_for_each(&ctrl->namespaces, idx, ns)
 		nvme_set_queue_dying(ns);
-
-	up_read(&ctrl->namespaces_rwsem);
 }
 EXPORT_SYMBOL_GPL(nvme_kill_queues);
 
 void nvme_unfreeze(struct nvme_ctrl *ctrl)
 {
 	struct nvme_ns *ns;
+	unsigned long idx;
 
-	down_read(&ctrl->namespaces_rwsem);
-	list_for_each_entry(ns, &ctrl->namespaces, list)
+	xa_for_each(&ctrl->namespaces, idx, ns)
 		blk_mq_unfreeze_queue(ns->queue);
-	up_read(&ctrl->namespaces_rwsem);
 }
 EXPORT_SYMBOL_GPL(nvme_unfreeze);
 
 void nvme_wait_freeze_timeout(struct nvme_ctrl *ctrl, long timeout)
 {
 	struct nvme_ns *ns;
+	unsigned long idx;
 
-	down_read(&ctrl->namespaces_rwsem);
-	list_for_each_entry(ns, &ctrl->namespaces, list) {
+	xa_for_each(&ctrl->namespaces, idx, ns) {
 		timeout = blk_mq_freeze_queue_wait_timeout(ns->queue, timeout);
 		if (timeout <= 0)
 			break;
 	}
-	up_read(&ctrl->namespaces_rwsem);
 }
 EXPORT_SYMBOL_GPL(nvme_wait_freeze_timeout);
 
 void nvme_wait_freeze(struct nvme_ctrl *ctrl)
 {
 	struct nvme_ns *ns;
+	unsigned long idx;
 
-	down_read(&ctrl->namespaces_rwsem);
-	list_for_each_entry(ns, &ctrl->namespaces, list)
+	xa_for_each(&ctrl->namespaces, idx, ns)
 		blk_mq_freeze_queue_wait(ns->queue);
-	up_read(&ctrl->namespaces_rwsem);
 }
 EXPORT_SYMBOL_GPL(nvme_wait_freeze);
 
 void nvme_start_freeze(struct nvme_ctrl *ctrl)
 {
 	struct nvme_ns *ns;
+	unsigned long idx;
 
-	down_read(&ctrl->namespaces_rwsem);
-	list_for_each_entry(ns, &ctrl->namespaces, list)
+	xa_for_each(&ctrl->namespaces, idx, ns)
 		blk_freeze_queue_start(ns->queue);
-	up_read(&ctrl->namespaces_rwsem);
 }
 EXPORT_SYMBOL_GPL(nvme_start_freeze);
 
 void nvme_stop_queues(struct nvme_ctrl *ctrl)
 {
 	struct nvme_ns *ns;
+	unsigned long idx;
 
-	down_read(&ctrl->namespaces_rwsem);
-	list_for_each_entry(ns, &ctrl->namespaces, list)
+	xa_for_each(&ctrl->namespaces, idx, ns)
 		blk_mq_quiesce_queue(ns->queue);
-	up_read(&ctrl->namespaces_rwsem);
 }
 EXPORT_SYMBOL_GPL(nvme_stop_queues);
 
 void nvme_start_queues(struct nvme_ctrl *ctrl)
 {
 	struct nvme_ns *ns;
+	unsigned long idx;
 
-	down_read(&ctrl->namespaces_rwsem);
-	list_for_each_entry(ns, &ctrl->namespaces, list)
+	xa_for_each(&ctrl->namespaces, idx, ns)
 		blk_mq_unquiesce_queue(ns->queue);
-	up_read(&ctrl->namespaces_rwsem);
 }
 EXPORT_SYMBOL_GPL(nvme_start_queues);
 
-
 void nvme_sync_queues(struct nvme_ctrl *ctrl)
 {
 	struct nvme_ns *ns;
+	unsigned long idx;
 
-	down_read(&ctrl->namespaces_rwsem);
-	list_for_each_entry(ns, &ctrl->namespaces, list)
+	xa_for_each(&ctrl->namespaces, idx, ns)
 		blk_sync_queue(ns->queue);
-	up_read(&ctrl->namespaces_rwsem);
-
-	if (ctrl->admin_q)
-		blk_sync_queue(ctrl->admin_q);
 }
 EXPORT_SYMBOL_GPL(nvme_sync_queues);
 
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 74bad4e3d377..af486864a1dc 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -115,13 +115,12 @@ bool nvme_failover_req(struct request *req)
 void nvme_kick_requeue_lists(struct nvme_ctrl *ctrl)
 {
 	struct nvme_ns *ns;
+	unsigned long idx;
 
-	down_read(&ctrl->namespaces_rwsem);
-	list_for_each_entry(ns, &ctrl->namespaces, list) {
+	xa_for_each(&ctrl->namespaces, idx, ns) {
 		if (ns->head->disk)
 			kblockd_schedule_work(&ns->head->requeue_work);
 	}
-	up_read(&ctrl->namespaces_rwsem);
 }
 
 static const char *nvme_ana_state_names[] = {
@@ -155,13 +154,12 @@ bool nvme_mpath_clear_current_path(struct nvme_ns *ns)
 void nvme_mpath_clear_ctrl_paths(struct nvme_ctrl *ctrl)
 {
 	struct nvme_ns *ns;
+	unsigned long idx;
 
 	mutex_lock(&ctrl->scan_lock);
-	down_read(&ctrl->namespaces_rwsem);
-	list_for_each_entry(ns, &ctrl->namespaces, list)
+	xa_for_each(&ctrl->namespaces, idx, ns)
 		if (nvme_mpath_clear_current_path(ns))
 			kblockd_schedule_work(&ns->head->requeue_work);
-	up_read(&ctrl->namespaces_rwsem);
 	mutex_unlock(&ctrl->scan_lock);
 }
 
@@ -495,6 +493,7 @@ static int nvme_update_ana_state(struct nvme_ctrl *ctrl,
 	u32 nr_nsids = le32_to_cpu(desc->nnsids), n = 0;
 	unsigned *nr_change_groups = data;
 	struct nvme_ns *ns;
+	unsigned long idx;
 
 	dev_dbg(ctrl->device, "ANA group %d: %s.\n",
 			le32_to_cpu(desc->grpid),
@@ -506,8 +505,7 @@ static int nvme_update_ana_state(struct nvme_ctrl *ctrl,
 	if (!nr_nsids)
 		return 0;
 
-	down_read(&ctrl->namespaces_rwsem);
-	list_for_each_entry(ns, &ctrl->namespaces, list) {
+	xa_for_each(&ctrl->namespaces, idx, ns) {
 		unsigned nsid = le32_to_cpu(desc->nsids[n]);
 
 		if (ns->head->ns_id < nsid)
@@ -517,7 +515,6 @@ static int nvme_update_ana_state(struct nvme_ctrl *ctrl,
 		if (++n == nr_nsids)
 			break;
 	}
-	up_read(&ctrl->namespaces_rwsem);
 	return 0;
 }
 
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 25dae702e0a2..1e8dedee74df 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -220,8 +220,7 @@ struct nvme_ctrl {
 	int numa_node;
 	struct blk_mq_tag_set *tagset;
 	struct blk_mq_tag_set *admin_tagset;
-	struct list_head namespaces;
-	struct rw_semaphore namespaces_rwsem;
+	struct xarray namespaces;
 	struct device ctrl_device;
 	struct device *device;	/* char device */
 	struct cdev cdev;
@@ -395,8 +394,6 @@ enum nvme_ns_features {
 };
 
 struct nvme_ns {
-	struct list_head list;
-
 	struct nvme_ctrl *ctrl;
 	struct request_queue *queue;
 	struct gendisk *disk;
-- 
2.26.0


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH V3 03/10] nvme: centralize queue action nvme_kill_queues()

[Chaitanya Kulkarni]

There is no point in having 7 different functions which are differing
with just one line.

The helper functions for ctrl queue management can be centralized with
the help of the descriptive enums for action. This avoids code
duplication which spans across the 7 functions as compare to one
function and exporting 7 symbols as compare to one symbol.

This patch merges nvme_kill_queue() into newly introduced
nvme_queue_act() which handles the queue management.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/host/core.c | 37 ++++++++++++++++++++++---------------
 drivers/nvme/host/nvme.h |  6 +++++-
 drivers/nvme/host/pci.c  |  4 ++--
 3 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 297835bda996..b39aafcbdde9 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4107,7 +4107,7 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
 	 * potentially having to clean up the failed sync later.
 	 */
 	if (ctrl->state == NVME_CTRL_DEAD)
-		nvme_kill_queues(ctrl);
+		nvme_queue_act(ctrl, NVME_KILL_QUEUES);
 
 	xa_lock(&ctrl->namespaces);
 	xa_for_each(&ctrl->namespaces, idx, ns) {
@@ -4452,26 +4452,33 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 }
 EXPORT_SYMBOL_GPL(nvme_init_ctrl);
 
-/**
- * nvme_kill_queues(): Ends all namespace queues
- * @ctrl: the dead controller that needs to end
- *
- * Call this function when the driver determines it is unable to get the
- * controller in a state capable of servicing IO.
- */
-void nvme_kill_queues(struct nvme_ctrl *ctrl)
+void nvme_queue_act(struct nvme_ctrl *ctrl, enum nvme_queue_act op)
 {
 	struct nvme_ns *ns;
 	unsigned long idx;
 
-	/* Forcibly unquiesce queues to avoid blocking dispatch */
-	if (ctrl->admin_q && !blk_queue_dying(ctrl->admin_q))
-		blk_mq_unquiesce_queue(ctrl->admin_q);
+	switch (op) {
+	case NVME_KILL_QUEUES:
+		/* Forcibly unquiesce queues to avoid blocking dispatch */
+		if (ctrl->admin_q && !blk_queue_dying(ctrl->admin_q))
+			blk_mq_unquiesce_queue(ctrl->admin_q);
+		break;
+	default:
+		break;
+	}
 
-	xa_for_each(&ctrl->namespaces, idx, ns)
-		nvme_set_queue_dying(ns);
+	xa_for_each(&ctrl->namespaces, idx, ns) {
+		switch (op) {
+		case NVME_KILL_QUEUES:
+			nvme_set_queue_dying(ns);
+			break;
+		default:
+			pr_warn("invalid %s op 0x%x\n", __func__, op);
+			break;
+		}
+	}
 }
-EXPORT_SYMBOL_GPL(nvme_kill_queues);
+EXPORT_SYMBOL_GPL(nvme_queue_act);
 
 void nvme_unfreeze(struct nvme_ctrl *ctrl)
 {
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 1e8dedee74df..b1cde85e9a85 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -545,9 +545,13 @@ int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len,
 void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 		volatile union nvme_result *res);
 
+enum nvme_queue_act {
+	NVME_KILL_QUEUES = 1,
+};
+
+void nvme_queue_act(struct nvme_ctrl *ctrl, enum nvme_queue_act op);
 void nvme_stop_queues(struct nvme_ctrl *ctrl);
 void nvme_start_queues(struct nvme_ctrl *ctrl);
-void nvme_kill_queues(struct nvme_ctrl *ctrl);
 void nvme_sync_queues(struct nvme_ctrl *ctrl);
 void nvme_unfreeze(struct nvme_ctrl *ctrl);
 void nvme_wait_freeze(struct nvme_ctrl *ctrl);
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 19e27dd17108..ce04a76a48af 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2509,7 +2509,7 @@ static void nvme_remove_dead_ctrl(struct nvme_dev *dev)
 	nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_DELETING);
 	nvme_get_ctrl(&dev->ctrl);
 	nvme_dev_disable(dev, false);
-	nvme_kill_queues(&dev->ctrl);
+	nvme_queue_act(&dev->ctrl, NVME_KILL_QUEUES);
 	if (!queue_work(nvme_wq, &dev->remove_work))
 		nvme_put_ctrl(&dev->ctrl);
 }
@@ -2617,7 +2617,7 @@ static void nvme_reset_work(struct work_struct *work)
 	 */
 	if (dev->online_queues < 2) {
 		dev_warn(dev->ctrl.device, "IO queues not created\n");
-		nvme_kill_queues(&dev->ctrl);
+		nvme_queue_act(&dev->ctrl, NVME_KILL_QUEUES);
 		nvme_remove_namespaces(&dev->ctrl);
 		nvme_free_tagset(dev);
 	} else {
-- 
2.26.0


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH V3 04/10] nvme: centralize queue action nvme_unfreeze()

[Chaitanya Kulkarni]

There is no point in having 7 different functions which are differing
with just one line.

The helper functions for ctrl queue management can be centralized with
the help of the descriptive enums for action. This avoids code
duplication which spans across the 7 functions as compare to one
function and exporting 7 symbols as compare to one symbol.

This patch merges nvme_unfreeze() into newly introduced nvme_queue_act()
which handles the queue management.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/host/core.c | 15 ++++-----------
 drivers/nvme/host/nvme.h |  2 +-
 drivers/nvme/host/pci.c  |  4 ++--
 3 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index b39aafcbdde9..fdf47ff7c60c 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1452,7 +1452,7 @@ static void nvme_passthru_end(struct nvme_ctrl *ctrl, u32 effects)
 	if (effects & NVME_CMD_EFFECTS_LBCC)
 		nvme_update_formats(ctrl, &effects);
 	if (effects & (NVME_CMD_EFFECTS_LBCC | NVME_CMD_EFFECTS_CSE_MASK)) {
-		nvme_unfreeze(ctrl);
+		nvme_queue_act(ctrl, NVME_UNFREEZE_QUEUES);
 		nvme_mpath_unfreeze(ctrl->subsys);
 		mutex_unlock(&ctrl->subsys->lock);
 		nvme_remove_invalid_namespaces(ctrl, NVME_NSID_ALL);
@@ -4472,6 +4472,9 @@ void nvme_queue_act(struct nvme_ctrl *ctrl, enum nvme_queue_act op)
 		case NVME_KILL_QUEUES:
 			nvme_set_queue_dying(ns);
 			break;
+		case NVME_UNFREEZE_QUEUES:
+			blk_mq_unfreeze_queue(ns->queue);
+			break;
 		default:
 			pr_warn("invalid %s op 0x%x\n", __func__, op);
 			break;
@@ -4480,16 +4483,6 @@ void nvme_queue_act(struct nvme_ctrl *ctrl, enum nvme_queue_act op)
 }
 EXPORT_SYMBOL_GPL(nvme_queue_act);
 
-void nvme_unfreeze(struct nvme_ctrl *ctrl)
-{
-	struct nvme_ns *ns;
-	unsigned long idx;
-
-	xa_for_each(&ctrl->namespaces, idx, ns)
-		blk_mq_unfreeze_queue(ns->queue);
-}
-EXPORT_SYMBOL_GPL(nvme_unfreeze);
-
 void nvme_wait_freeze_timeout(struct nvme_ctrl *ctrl, long timeout)
 {
 	struct nvme_ns *ns;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index b1cde85e9a85..a1b2364d2bb2 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -547,13 +547,13 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 
 enum nvme_queue_act {
 	NVME_KILL_QUEUES = 1,
+	NVME_UNFREEZE_QUEUES,
 };
 
 void nvme_queue_act(struct nvme_ctrl *ctrl, enum nvme_queue_act op);
 void nvme_stop_queues(struct nvme_ctrl *ctrl);
 void nvme_start_queues(struct nvme_ctrl *ctrl);
 void nvme_sync_queues(struct nvme_ctrl *ctrl);
-void nvme_unfreeze(struct nvme_ctrl *ctrl);
 void nvme_wait_freeze(struct nvme_ctrl *ctrl);
 void nvme_wait_freeze_timeout(struct nvme_ctrl *ctrl, long timeout);
 void nvme_start_freeze(struct nvme_ctrl *ctrl);
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index ce04a76a48af..b87e2f0dcdf2 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2624,7 +2624,7 @@ static void nvme_reset_work(struct work_struct *work)
 		nvme_start_queues(&dev->ctrl);
 		nvme_wait_freeze(&dev->ctrl);
 		nvme_dev_add(dev);
-		nvme_unfreeze(&dev->ctrl);
+		nvme_queue_act(&dev->ctrl, NVME_UNFREEZE_QUEUES);
 	}
 
 	/*
@@ -2997,7 +2997,7 @@ static int nvme_suspend(struct device *dev)
 		ctrl->npss = 0;
 	}
 unfreeze:
-	nvme_unfreeze(ctrl);
+	nvme_queue_act(ctrl, NVME_UNFREEZE_QUEUES);
 	return ret;
 }
 
-- 
2.26.0


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH V3 05/10] nvme: centralize queue action nvme_wait_freeze()

[Chaitanya Kulkarni]

There is no point in having 7 different functions which are differing
with just one line.

The helper functions for ctrl queue management can be centralized with
the help of the descriptive enums for action. This avoids code
duplication which spans across the 7 functions as compare to one
function and exporting 7 symbols as compare to one symbol.

This patch merges nvme_wait_freeze() into newly introduced
nvme_queue_act() which handles the queue management.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/host/core.c | 15 ++++-----------
 drivers/nvme/host/nvme.h |  2 +-
 drivers/nvme/host/pci.c  |  4 ++--
 3 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index fdf47ff7c60c..5d2f85b22061 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1418,7 +1418,7 @@ static u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 		nvme_mpath_start_freeze(ctrl->subsys);
 		nvme_mpath_wait_freeze(ctrl->subsys);
 		nvme_start_freeze(ctrl);
-		nvme_wait_freeze(ctrl);
+		nvme_queue_act(ctrl, NVME_WAIT_FREEZE_QUEUES);
 	}
 	return effects;
 }
@@ -4475,6 +4475,9 @@ void nvme_queue_act(struct nvme_ctrl *ctrl, enum nvme_queue_act op)
 		case NVME_UNFREEZE_QUEUES:
 			blk_mq_unfreeze_queue(ns->queue);
 			break;
+		case NVME_WAIT_FREEZE_QUEUES:
+			blk_mq_freeze_queue_wait(ns->queue);
+			break;
 		default:
 			pr_warn("invalid %s op 0x%x\n", __func__, op);
 			break;
@@ -4496,16 +4499,6 @@ void nvme_wait_freeze_timeout(struct nvme_ctrl *ctrl, long timeout)
 }
 EXPORT_SYMBOL_GPL(nvme_wait_freeze_timeout);
 
-void nvme_wait_freeze(struct nvme_ctrl *ctrl)
-{
-	struct nvme_ns *ns;
-	unsigned long idx;
-
-	xa_for_each(&ctrl->namespaces, idx, ns)
-		blk_mq_freeze_queue_wait(ns->queue);
-}
-EXPORT_SYMBOL_GPL(nvme_wait_freeze);
-
 void nvme_start_freeze(struct nvme_ctrl *ctrl)
 {
 	struct nvme_ns *ns;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index a1b2364d2bb2..349e7cd6dd65 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -548,13 +548,13 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 enum nvme_queue_act {
 	NVME_KILL_QUEUES = 1,
 	NVME_UNFREEZE_QUEUES,
+	NVME_WAIT_FREEZE_QUEUES,
 };
 
 void nvme_queue_act(struct nvme_ctrl *ctrl, enum nvme_queue_act op);
 void nvme_stop_queues(struct nvme_ctrl *ctrl);
 void nvme_start_queues(struct nvme_ctrl *ctrl);
 void nvme_sync_queues(struct nvme_ctrl *ctrl);
-void nvme_wait_freeze(struct nvme_ctrl *ctrl);
 void nvme_wait_freeze_timeout(struct nvme_ctrl *ctrl, long timeout);
 void nvme_start_freeze(struct nvme_ctrl *ctrl);
 
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index b87e2f0dcdf2..71e58d51d1b0 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2622,7 +2622,7 @@ static void nvme_reset_work(struct work_struct *work)
 		nvme_free_tagset(dev);
 	} else {
 		nvme_start_queues(&dev->ctrl);
-		nvme_wait_freeze(&dev->ctrl);
+		nvme_queue_act(&dev->ctrl, NVME_WAIT_FREEZE_QUEUES);
 		nvme_dev_add(dev);
 		nvme_queue_act(&dev->ctrl, NVME_UNFREEZE_QUEUES);
 	}
@@ -2964,7 +2964,7 @@ static int nvme_suspend(struct device *dev)
 		return nvme_disable_prepare_reset(ndev, true);
 
 	nvme_start_freeze(ctrl);
-	nvme_wait_freeze(ctrl);
+	nvme_queue_act(ctrl, NVME_WAIT_FREEZE_QUEUES);
 	nvme_sync_queues(ctrl);
 
 	if (ctrl->state != NVME_CTRL_LIVE)
-- 
2.26.0


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH V3 06/10] nvme: centralize queue action nvme_start_freeze()

[Chaitanya Kulkarni]

There is no point in having 7 different functions which are differing
with just one line.

The helper functions for ctrl queue management can be centralized with
the help of the descriptive enums for action. This avoids code
duplication which spans across the 7 functions as compare to one
function and exporting 7 symbols as compare to one symbol.

This patch merges nvme_start_freeze() into newly introduced
nvme_queue_act() which handles the queue manegement.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/host/core.c | 15 ++++-----------
 drivers/nvme/host/nvme.h |  2 +-
 drivers/nvme/host/pci.c  |  4 ++--
 3 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 5d2f85b22061..2202a13d548d 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1417,7 +1417,7 @@ static u32 nvme_passthru_start(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 		mutex_lock(&ctrl->subsys->lock);
 		nvme_mpath_start_freeze(ctrl->subsys);
 		nvme_mpath_wait_freeze(ctrl->subsys);
-		nvme_start_freeze(ctrl);
+		nvme_queue_act(ctrl, NVME_START_FREEZE_QUEUES);
 		nvme_queue_act(ctrl, NVME_WAIT_FREEZE_QUEUES);
 	}
 	return effects;
@@ -4478,6 +4478,9 @@ void nvme_queue_act(struct nvme_ctrl *ctrl, enum nvme_queue_act op)
 		case NVME_WAIT_FREEZE_QUEUES:
 			blk_mq_freeze_queue_wait(ns->queue);
 			break;
+		case NVME_START_FREEZE_QUEUES:
+			blk_freeze_queue_start(ns->queue);
+			break;
 		default:
 			pr_warn("invalid %s op 0x%x\n", __func__, op);
 			break;
@@ -4499,16 +4502,6 @@ void nvme_wait_freeze_timeout(struct nvme_ctrl *ctrl, long timeout)
 }
 EXPORT_SYMBOL_GPL(nvme_wait_freeze_timeout);
 
-void nvme_start_freeze(struct nvme_ctrl *ctrl)
-{
-	struct nvme_ns *ns;
-	unsigned long idx;
-
-	xa_for_each(&ctrl->namespaces, idx, ns)
-		blk_freeze_queue_start(ns->queue);
-}
-EXPORT_SYMBOL_GPL(nvme_start_freeze);
-
 void nvme_stop_queues(struct nvme_ctrl *ctrl)
 {
 	struct nvme_ns *ns;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 349e7cd6dd65..892154d2a465 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -549,6 +549,7 @@ enum nvme_queue_act {
 	NVME_KILL_QUEUES = 1,
 	NVME_UNFREEZE_QUEUES,
 	NVME_WAIT_FREEZE_QUEUES,
+	NVME_START_FREEZE_QUEUES,
 };
 
 void nvme_queue_act(struct nvme_ctrl *ctrl, enum nvme_queue_act op);
@@ -556,7 +557,6 @@ void nvme_stop_queues(struct nvme_ctrl *ctrl);
 void nvme_start_queues(struct nvme_ctrl *ctrl);
 void nvme_sync_queues(struct nvme_ctrl *ctrl);
 void nvme_wait_freeze_timeout(struct nvme_ctrl *ctrl, long timeout);
-void nvme_start_freeze(struct nvme_ctrl *ctrl);
 
 #define NVME_QID_ANY -1
 struct request *nvme_alloc_request(struct request_queue *q,
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 71e58d51d1b0..92820fdba595 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2405,7 +2405,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 		if (dev->ctrl.state == NVME_CTRL_LIVE ||
 		    dev->ctrl.state == NVME_CTRL_RESETTING) {
 			freeze = true;
-			nvme_start_freeze(&dev->ctrl);
+			nvme_queue_act(&dev->ctrl, NVME_START_FREEZE_QUEUES);
 		}
 		dead = !!((csts & NVME_CSTS_CFS) || !(csts & NVME_CSTS_RDY) ||
 			pdev->error_state  != pci_channel_io_normal);
@@ -2963,7 +2963,7 @@ static int nvme_suspend(struct device *dev)
 	    (ndev->ctrl.quirks & NVME_QUIRK_SIMPLE_SUSPEND))
 		return nvme_disable_prepare_reset(ndev, true);
 
-	nvme_start_freeze(ctrl);
+	nvme_queue_act(ctrl, NVME_START_FREEZE_QUEUES);
 	nvme_queue_act(ctrl, NVME_WAIT_FREEZE_QUEUES);
 	nvme_sync_queues(ctrl);
 
-- 
2.26.0


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH V3 07/10] nvme: centralize queue action nvme_stop_queues()

[Chaitanya Kulkarni]

There is no point in having 7 different functions which are differing
with just one line.

The helper functions for ctrl queue management can be centralized with
the help of the descriptive enums for action. This avoids code
duplication which spans across the 7 functions as compare to one
function and exporting 7 symbols as compare to one symbol.

This patch merges nvme_stop_queue() into newly introduced
nvme_queue_act() which handles the queue management.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/host/core.c   | 15 ++++-----------
 drivers/nvme/host/fc.c     |  2 +-
 drivers/nvme/host/nvme.h   |  2 +-
 drivers/nvme/host/pci.c    |  2 +-
 drivers/nvme/host/rdma.c   |  2 +-
 drivers/nvme/host/tcp.c    |  2 +-
 drivers/nvme/target/loop.c |  2 +-
 7 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 2202a13d548d..e237339616af 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4219,7 +4219,7 @@ static void nvme_fw_act_work(struct work_struct *work)
 		fw_act_timeout = jiffies +
 				msecs_to_jiffies(admin_timeout * 1000);
 
-	nvme_stop_queues(ctrl);
+	nvme_queue_act(ctrl, NVME_STOP_QUEUES);
 	while (nvme_ctrl_pp_status(ctrl)) {
 		if (time_after(jiffies, fw_act_timeout)) {
 			dev_warn(ctrl->device,
@@ -4481,6 +4481,9 @@ void nvme_queue_act(struct nvme_ctrl *ctrl, enum nvme_queue_act op)
 		case NVME_START_FREEZE_QUEUES:
 			blk_freeze_queue_start(ns->queue);
 			break;
+		case NVME_STOP_QUEUES:
+			blk_mq_quiesce_queue(ns->queue);
+			break;
 		default:
 			pr_warn("invalid %s op 0x%x\n", __func__, op);
 			break;
@@ -4502,16 +4505,6 @@ void nvme_wait_freeze_timeout(struct nvme_ctrl *ctrl, long timeout)
 }
 EXPORT_SYMBOL_GPL(nvme_wait_freeze_timeout);
 
-void nvme_stop_queues(struct nvme_ctrl *ctrl)
-{
-	struct nvme_ns *ns;
-	unsigned long idx;
-
-	xa_for_each(&ctrl->namespaces, idx, ns)
-		blk_mq_quiesce_queue(ns->queue);
-}
-EXPORT_SYMBOL_GPL(nvme_stop_queues);
-
 void nvme_start_queues(struct nvme_ctrl *ctrl)
 {
 	struct nvme_ns *ns;
diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 6aa30bb5a762..683b6d1a8e1c 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -3120,7 +3120,7 @@ nvme_fc_delete_association(struct nvme_fc_ctrl *ctrl)
 	 * (but with error status).
 	 */
 	if (ctrl->ctrl.queue_count > 1) {
-		nvme_stop_queues(&ctrl->ctrl);
+		nvme_queue_act(&ctrl->ctrl, NVME_STOP_QUEUES);
 		blk_mq_tagset_busy_iter(&ctrl->tag_set,
 				nvme_fc_terminate_exchange, &ctrl->ctrl);
 		blk_mq_tagset_wait_completed_request(&ctrl->tag_set);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 892154d2a465..59cbd890e38b 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -550,10 +550,10 @@ enum nvme_queue_act {
 	NVME_UNFREEZE_QUEUES,
 	NVME_WAIT_FREEZE_QUEUES,
 	NVME_START_FREEZE_QUEUES,
+	NVME_STOP_QUEUES,
 };
 
 void nvme_queue_act(struct nvme_ctrl *ctrl, enum nvme_queue_act op);
-void nvme_stop_queues(struct nvme_ctrl *ctrl);
 void nvme_start_queues(struct nvme_ctrl *ctrl);
 void nvme_sync_queues(struct nvme_ctrl *ctrl);
 void nvme_wait_freeze_timeout(struct nvme_ctrl *ctrl, long timeout);
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 92820fdba595..1112e6ec5ab1 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2418,7 +2418,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 	if (!dead && shutdown && freeze)
 		nvme_wait_freeze_timeout(&dev->ctrl, NVME_IO_TIMEOUT);
 
-	nvme_stop_queues(&dev->ctrl);
+	nvme_queue_act(&dev->ctrl, NVME_STOP_QUEUES);
 
 	if (!dead && dev->ctrl.queue_count > 0) {
 		nvme_disable_io_queues(dev);
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index e881f879ac63..e2a7608548e5 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -983,7 +983,7 @@ static void nvme_rdma_teardown_io_queues(struct nvme_rdma_ctrl *ctrl,
 		bool remove)
 {
 	if (ctrl->ctrl.queue_count > 1) {
-		nvme_stop_queues(&ctrl->ctrl);
+		nvme_queue_act(&ctrl->ctrl, NVME_STOP_QUEUES);
 		nvme_rdma_stop_io_queues(ctrl);
 		if (ctrl->ctrl.tagset) {
 			blk_mq_tagset_busy_iter(ctrl->ctrl.tagset,
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index b2e73e19ef01..cf3db9025df3 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1884,7 +1884,7 @@ static void nvme_tcp_teardown_io_queues(struct nvme_ctrl *ctrl,
 {
 	if (ctrl->queue_count <= 1)
 		return;
-	nvme_stop_queues(ctrl);
+	nvme_queue_act(ctrl, NVME_STOP_QUEUES);
 	nvme_tcp_stop_io_queues(ctrl);
 	if (ctrl->tagset) {
 		blk_mq_tagset_busy_iter(ctrl->tagset,
diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index f2c80a51985f..d881528e9734 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -406,7 +406,7 @@ static int nvme_loop_configure_admin_queue(struct nvme_loop_ctrl *ctrl)
 static void nvme_loop_shutdown_ctrl(struct nvme_loop_ctrl *ctrl)
 {
 	if (ctrl->ctrl.queue_count > 1) {
-		nvme_stop_queues(&ctrl->ctrl);
+		nvme_queue_act(&ctrl->ctrl, NVME_STOP_QUEUES);
 		blk_mq_tagset_busy_iter(&ctrl->tag_set,
 					nvme_cancel_request, &ctrl->ctrl);
 		blk_mq_tagset_wait_completed_request(&ctrl->tag_set);
-- 
2.26.0


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH V3 08/10] nvme: centralize queue action nvme_start_queues()

[Chaitanya Kulkarni]

There is no point in having 7 different functions which are differing
with just one line.

The helper functions for ctrl queue management can be centralized with
the help of the descriptive enums for action. This avoids code
duplication which spans across the 7 functions as compare to one
function and exporting 7 symbols as compare to one symbol.

This patch merges nvme_start_queues() into newly introduced
nvme_queue_act() which handles the queue management.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/host/core.c | 17 +++++------------
 drivers/nvme/host/fc.c   |  2 +-
 drivers/nvme/host/nvme.h |  2 +-
 drivers/nvme/host/pci.c  |  4 ++--
 drivers/nvme/host/rdma.c |  4 ++--
 drivers/nvme/host/tcp.c  |  4 ++--
 6 files changed, 13 insertions(+), 20 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index e237339616af..6504e713c0fb 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4233,7 +4233,7 @@ static void nvme_fw_act_work(struct work_struct *work)
 	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_LIVE))
 		return;
 
-	nvme_start_queues(ctrl);
+	nvme_queue_act(ctrl, NVME_START_QUEUES);
 	/* read FW slot information to clear the AER */
 	nvme_get_fw_slot_info(ctrl);
 }
@@ -4318,7 +4318,7 @@ void nvme_start_ctrl(struct nvme_ctrl *ctrl)
 
 	if (ctrl->queue_count > 1) {
 		nvme_queue_scan(ctrl);
-		nvme_start_queues(ctrl);
+		nvme_queue_act(ctrl, NVME_START_QUEUES);
 	}
 	ctrl->created = true;
 }
@@ -4484,6 +4484,9 @@ void nvme_queue_act(struct nvme_ctrl *ctrl, enum nvme_queue_act op)
 		case NVME_STOP_QUEUES:
 			blk_mq_quiesce_queue(ns->queue);
 			break;
+		case NVME_START_QUEUES:
+			blk_mq_unquiesce_queue(ns->queue);
+			break;
 		default:
 			pr_warn("invalid %s op 0x%x\n", __func__, op);
 			break;
@@ -4505,16 +4508,6 @@ void nvme_wait_freeze_timeout(struct nvme_ctrl *ctrl, long timeout)
 }
 EXPORT_SYMBOL_GPL(nvme_wait_freeze_timeout);
 
-void nvme_start_queues(struct nvme_ctrl *ctrl)
-{
-	struct nvme_ns *ns;
-	unsigned long idx;
-
-	xa_for_each(&ctrl->namespaces, idx, ns)
-		blk_mq_unquiesce_queue(ns->queue);
-}
-EXPORT_SYMBOL_GPL(nvme_start_queues);
-
 void nvme_sync_queues(struct nvme_ctrl *ctrl)
 {
 	struct nvme_ns *ns;
diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 683b6d1a8e1c..900aece3cb7c 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -3192,7 +3192,7 @@ nvme_fc_delete_association(struct nvme_fc_ctrl *ctrl)
 	blk_mq_unquiesce_queue(ctrl->ctrl.admin_q);
 
 	/* resume the io queues so that things will fast fail */
-	nvme_start_queues(&ctrl->ctrl);
+	nvme_queue_act(&ctrl->ctrl, NVME_START_QUEUES);
 
 	nvme_fc_ctlr_inactive_on_rport(ctrl);
 }
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 59cbd890e38b..419070d0c442 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -551,10 +551,10 @@ enum nvme_queue_act {
 	NVME_WAIT_FREEZE_QUEUES,
 	NVME_START_FREEZE_QUEUES,
 	NVME_STOP_QUEUES,
+	NVME_START_QUEUES,
 };
 
 void nvme_queue_act(struct nvme_ctrl *ctrl, enum nvme_queue_act op);
-void nvme_start_queues(struct nvme_ctrl *ctrl);
 void nvme_sync_queues(struct nvme_ctrl *ctrl);
 void nvme_wait_freeze_timeout(struct nvme_ctrl *ctrl, long timeout);
 
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 1112e6ec5ab1..b7e14b86e00a 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2440,7 +2440,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 	 * deadlocking blk-mq hot-cpu notifier.
 	 */
 	if (shutdown) {
-		nvme_start_queues(&dev->ctrl);
+		nvme_queue_act(&dev->ctrl, NVME_START_QUEUES);
 		if (dev->ctrl.admin_q && !blk_queue_dying(dev->ctrl.admin_q))
 			blk_mq_unquiesce_queue(dev->ctrl.admin_q);
 	}
@@ -2621,7 +2621,7 @@ static void nvme_reset_work(struct work_struct *work)
 		nvme_remove_namespaces(&dev->ctrl);
 		nvme_free_tagset(dev);
 	} else {
-		nvme_start_queues(&dev->ctrl);
+		nvme_queue_act(&dev->ctrl, NVME_START_QUEUES);
 		nvme_queue_act(&dev->ctrl, NVME_WAIT_FREEZE_QUEUES);
 		nvme_dev_add(dev);
 		nvme_queue_act(&dev->ctrl, NVME_UNFREEZE_QUEUES);
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index e2a7608548e5..9218d3e3f065 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -991,7 +991,7 @@ static void nvme_rdma_teardown_io_queues(struct nvme_rdma_ctrl *ctrl,
 			blk_mq_tagset_wait_completed_request(ctrl->ctrl.tagset);
 		}
 		if (remove)
-			nvme_start_queues(&ctrl->ctrl);
+			nvme_queue_act(&ctrl->ctrl, NVME_START_QUEUES);
 		nvme_rdma_destroy_io_queues(ctrl, remove);
 	}
 }
@@ -1129,7 +1129,7 @@ static void nvme_rdma_error_recovery_work(struct work_struct *work)
 
 	nvme_stop_keep_alive(&ctrl->ctrl);
 	nvme_rdma_teardown_io_queues(ctrl, false);
-	nvme_start_queues(&ctrl->ctrl);
+	nvme_queue_act(&ctrl->ctrl, NVME_START_QUEUES);
 	nvme_rdma_teardown_admin_queue(ctrl, false);
 	blk_mq_unquiesce_queue(ctrl->ctrl.admin_q);
 
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index cf3db9025df3..1ea0dc89f677 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1892,7 +1892,7 @@ static void nvme_tcp_teardown_io_queues(struct nvme_ctrl *ctrl,
 		blk_mq_tagset_wait_completed_request(ctrl->tagset);
 	}
 	if (remove)
-		nvme_start_queues(ctrl);
+		nvme_queue_act(ctrl, NVME_START_QUEUES);
 	nvme_tcp_destroy_io_queues(ctrl, remove);
 }
 
@@ -2005,7 +2005,7 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work)
 	nvme_stop_keep_alive(ctrl);
 	nvme_tcp_teardown_io_queues(ctrl, false);
 	/* unquiesce to fail fast pending requests */
-	nvme_start_queues(ctrl);
+	nvme_queue_act(ctrl, NVME_START_QUEUES);
 	nvme_tcp_teardown_admin_queue(ctrl, false);
 	blk_mq_unquiesce_queue(ctrl->admin_q);
 
-- 
2.26.0


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH V3 09/10] nvme: centralize queue action nvme_sync_queues()

[Chaitanya Kulkarni]

There is no point in having 7 different functions which are differing
with just one line.

The helper functions for ctrl queue management can be centralized with
the help of the descriptive enums for action. This avoids code
duplication which spans across the 7 functions as compare to one
function and exporting 7 symbols as compare to one symbol.

This patch merges nvme_sync_queues() into newly introduced
nvme_queue_act() which handles the queue management.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/host/core.c | 13 +++----------
 drivers/nvme/host/nvme.h |  2 +-
 drivers/nvme/host/pci.c  |  6 +++---
 3 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 6504e713c0fb..936cdc7868e2 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4487,6 +4487,9 @@ void nvme_queue_act(struct nvme_ctrl *ctrl, enum nvme_queue_act op)
 		case NVME_START_QUEUES:
 			blk_mq_unquiesce_queue(ns->queue);
 			break;
+		case NVME_SYNC_QUEUES:
+			blk_sync_queue(ns->queue);
+			break;
 		default:
 			pr_warn("invalid %s op 0x%x\n", __func__, op);
 			break;
@@ -4508,16 +4511,6 @@ void nvme_wait_freeze_timeout(struct nvme_ctrl *ctrl, long timeout)
 }
 EXPORT_SYMBOL_GPL(nvme_wait_freeze_timeout);
 
-void nvme_sync_queues(struct nvme_ctrl *ctrl)
-{
-	struct nvme_ns *ns;
-	unsigned long idx;
-
-	xa_for_each(&ctrl->namespaces, idx, ns)
-		blk_sync_queue(ns->queue);
-}
-EXPORT_SYMBOL_GPL(nvme_sync_queues);
-
 /*
  * Check we didn't inadvertently grow the command structure sizes:
  */
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 419070d0c442..8d74eabee41d 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -552,10 +552,10 @@ enum nvme_queue_act {
 	NVME_START_FREEZE_QUEUES,
 	NVME_STOP_QUEUES,
 	NVME_START_QUEUES,
+	NVME_SYNC_QUEUES,
 };
 
 void nvme_queue_act(struct nvme_ctrl *ctrl, enum nvme_queue_act op);
-void nvme_sync_queues(struct nvme_ctrl *ctrl);
 void nvme_wait_freeze_timeout(struct nvme_ctrl *ctrl, long timeout);
 
 #define NVME_QID_ANY -1
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index b7e14b86e00a..238ff4c77de7 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2532,7 +2532,7 @@ static void nvme_reset_work(struct work_struct *work)
 	 */
 	if (dev->ctrl.ctrl_config & NVME_CC_ENABLE)
 		nvme_dev_disable(dev, false);
-	nvme_sync_queues(&dev->ctrl);
+	nvme_queue_act(&dev->ctrl, NVME_SYNC_QUEUES);
 
 	mutex_lock(&dev->shutdown_lock);
 	result = nvme_pci_enable(dev);
@@ -2859,7 +2859,7 @@ static void nvme_reset_prepare(struct pci_dev *pdev)
 	 * with ->remove().
 	 */
 	nvme_disable_prepare_reset(dev, false);
-	nvme_sync_queues(&dev->ctrl);
+	nvme_queue_act(&dev->ctrl, NVME_SYNC_QUEUES);
 }
 
 static void nvme_reset_done(struct pci_dev *pdev)
@@ -2965,7 +2965,7 @@ static int nvme_suspend(struct device *dev)
 
 	nvme_queue_act(ctrl, NVME_START_FREEZE_QUEUES);
 	nvme_queue_act(ctrl, NVME_WAIT_FREEZE_QUEUES);
-	nvme_sync_queues(ctrl);
+	nvme_queue_act(ctrl, NVME_SYNC_QUEUES);
 
 	if (ctrl->state != NVME_CTRL_LIVE)
 		goto unfreeze;
-- 
2.26.0


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH V3 10/10] nvmet: use xarray for ctrl ns storing

[Chaitanya Kulkarni]

This patch replaces the ctrl->namespaces tracking from linked list to
xarray and improves the performance when accessing one namespce :-

XArray vs Default:-

IOPS and BW (more the better) increase BW (~1.8%):-
---------------------------------------------------

 XArray :-
  read:  IOPS=160k,  BW=626MiB/s  (656MB/s)(18.3GiB/30001msec)
  read:  IOPS=160k,  BW=626MiB/s  (656MB/s)(18.3GiB/30001msec)
  read:  IOPS=162k,  BW=631MiB/s  (662MB/s)(18.5GiB/30001msec)

 Default:-
  read:  IOPS=156k,  BW=609MiB/s  (639MB/s)(17.8GiB/30001msec)
  read:  IOPS=157k,  BW=613MiB/s  (643MB/s)(17.0GiB/30001msec)
  read:  IOPS=160k,  BW=626MiB/s  (656MB/s)(18.3GiB/30001msec)

Submission latency (less the better) decrease (~8.3%):-
-------------------------------------------------------

 XArray:-
  slat  (usec):  min=7,  max=8386,  avg=11.19,  stdev=5.96
  slat  (usec):  min=7,  max=441,   avg=11.09,  stdev=4.48
  slat  (usec):  min=7,  max=1088,  avg=11.21,  stdev=4.54

 Default :-
  slat  (usec):  min=8,   max=2826.5k,  avg=23.96,  stdev=3911.50
  slat  (usec):  min=8,   max=503,      avg=12.52,  stdev=5.07
  slat  (usec):  min=8,   max=2384,     avg=12.50,  stdev=5.28

CPU Usage (less the better) decrease (~5.2%):-
----------------------------------------------

 XArray:-
  cpu  :  usr=1.84%,  sys=18.61%,  ctx=949471,  majf=0,  minf=250
  cpu  :  usr=1.83%,  sys=18.41%,  ctx=950262,  majf=0,  minf=237
  cpu  :  usr=1.82%,  sys=18.82%,  ctx=957224,  majf=0,  minf=234

 Default:-
  cpu  :  usr=1.70%,  sys=19.21%,  ctx=858196,  majf=0,  minf=251
  cpu  :  usr=1.82%,  sys=19.98%,  ctx=929720,  majf=0,  minf=227
  cpu  :  usr=1.83%,  sys=20.33%,  ctx=947208,  majf=0,  minf=235.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/target/admin-cmd.c | 17 ++++------
 drivers/nvme/target/core.c      | 58 +++++++++------------------------
 drivers/nvme/target/nvmet.h     |  3 +-
 3 files changed, 23 insertions(+), 55 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 95bb3bc4e335..55918fcef80b 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -113,11 +113,10 @@ static u16 nvmet_get_smart_log_all(struct nvmet_req *req,
 	u64 data_units_read = 0, data_units_written = 0;
 	struct nvmet_ns *ns;
 	struct nvmet_ctrl *ctrl;
+	unsigned long idx;
 
 	ctrl = req->sq->ctrl;
-
-	rcu_read_lock();
-	list_for_each_entry_rcu(ns, &ctrl->subsys->namespaces, dev_link) {
+	xa_for_each(&ctrl->subsys->namespaces, idx, ns) {
 		/* we don't have the right data for file backed ns */
 		if (!ns->bdev)
 			continue;
@@ -127,9 +126,7 @@ static u16 nvmet_get_smart_log_all(struct nvmet_req *req,
 		host_writes += part_stat_read(ns->bdev->bd_part, ios[WRITE]);
 		data_units_written += DIV_ROUND_UP(
 			part_stat_read(ns->bdev->bd_part, sectors[WRITE]), 1000);
-
 	}
-	rcu_read_unlock();
 
 	put_unaligned_le64(host_reads, &slog->host_reads[0]);
 	put_unaligned_le64(data_units_read, &slog->data_units_read[0]);
@@ -230,14 +227,13 @@ static u32 nvmet_format_ana_group(struct nvmet_req *req, u32 grpid,
 {
 	struct nvmet_ctrl *ctrl = req->sq->ctrl;
 	struct nvmet_ns *ns;
+	unsigned long idx;
 	u32 count = 0;
 
 	if (!(req->cmd->get_log_page.lsp & NVME_ANA_LOG_RGO)) {
-		rcu_read_lock();
-		list_for_each_entry_rcu(ns, &ctrl->subsys->namespaces, dev_link)
+		xa_for_each(&ctrl->subsys->namespaces, idx, ns)
 			if (ns->anagrpid == grpid)
 				desc->nsids[count++] = cpu_to_le32(ns->nsid);
-		rcu_read_unlock();
 	}
 
 	desc->grpid = cpu_to_le32(grpid);
@@ -556,6 +552,7 @@ static void nvmet_execute_identify_nslist(struct nvmet_req *req)
 	static const int buf_size = NVME_IDENTIFY_DATA_SIZE;
 	struct nvmet_ctrl *ctrl = req->sq->ctrl;
 	struct nvmet_ns *ns;
+	unsigned long idx;
 	u32 min_nsid = le32_to_cpu(req->cmd->identify.nsid);
 	__le32 *list;
 	u16 status = 0;
@@ -567,15 +564,13 @@ static void nvmet_execute_identify_nslist(struct nvmet_req *req)
 		goto out;
 	}
 
-	rcu_read_lock();
-	list_for_each_entry_rcu(ns, &ctrl->subsys->namespaces, dev_link) {
+	xa_for_each(&ctrl->subsys->namespaces, idx, ns) {
 		if (ns->nsid <= min_nsid)
 			continue;
 		list[i++] = cpu_to_le32(ns->nsid);
 		if (i == buf_size / sizeof(__le32))
 			break;
 	}
-	rcu_read_unlock();
 
 	status = nvmet_copy_to_sgl(req, 0, list, buf_size);
 
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 9cdc39c8b729..2908acc7b1e2 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -115,13 +115,14 @@ u16 nvmet_zero_sgl(struct nvmet_req *req, off_t off, size_t len)
 
 static unsigned int nvmet_max_nsid(struct nvmet_subsys *subsys)
 {
-	struct nvmet_ns *ns;
+	unsigned long nsid = 0;
+	struct nvmet_ns *cur;
+	unsigned long idx;
 
-	if (list_empty(&subsys->namespaces))
-		return 0;
+	xa_for_each(&subsys->namespaces, idx, cur)
+		nsid = cur->nsid;
 
-	ns = list_last_entry(&subsys->namespaces, struct nvmet_ns, dev_link);
-	return ns->nsid;
+	return nsid;
 }
 
 static u32 nvmet_async_event_result(struct nvmet_async_event *aen)
@@ -410,25 +411,12 @@ static void nvmet_stop_keep_alive_timer(struct nvmet_ctrl *ctrl)
 	cancel_delayed_work_sync(&ctrl->ka_work);
 }
 
-static struct nvmet_ns *__nvmet_find_namespace(struct nvmet_ctrl *ctrl,
-		__le32 nsid)
-{
-	struct nvmet_ns *ns;
-
-	list_for_each_entry_rcu(ns, &ctrl->subsys->namespaces, dev_link) {
-		if (ns->nsid == le32_to_cpu(nsid))
-			return ns;
-	}
-
-	return NULL;
-}
-
 struct nvmet_ns *nvmet_find_namespace(struct nvmet_ctrl *ctrl, __le32 nsid)
 {
 	struct nvmet_ns *ns;
 
 	rcu_read_lock();
-	ns = __nvmet_find_namespace(ctrl, nsid);
+	ns = __xa_load(&ctrl->subsys->namespaces, nsid);
 	if (ns)
 		percpu_ref_get(&ns->ref);
 	rcu_read_unlock();
@@ -586,24 +574,10 @@ int nvmet_ns_enable(struct nvmet_ns *ns)
 	if (ns->nsid > subsys->max_nsid)
 		subsys->max_nsid = ns->nsid;
 
-	/*
-	 * The namespaces list needs to be sorted to simplify the implementation
-	 * of the Identify Namepace List subcommand.
-	 */
-	if (list_empty(&subsys->namespaces)) {
-		list_add_tail_rcu(&ns->dev_link, &subsys->namespaces);
-	} else {
-		struct nvmet_ns *old;
-
-		list_for_each_entry_rcu(old, &subsys->namespaces, dev_link,
-					lockdep_is_held(&subsys->lock)) {
-			BUG_ON(ns->nsid == old->nsid);
-			if (ns->nsid < old->nsid)
-				break;
-		}
+	ret = xa_insert(&subsys->namespaces, ns->nsid, ns, GFP_KERNEL);
+	if (ret)
+		goto out_dev_put;
 
-		list_add_tail_rcu(&ns->dev_link, &old->dev_link);
-	}
 	subsys->nr_namespaces++;
 
 	nvmet_ns_changed(subsys, ns->nsid);
@@ -630,7 +604,7 @@ void nvmet_ns_disable(struct nvmet_ns *ns)
 		goto out_unlock;
 
 	ns->enabled = false;
-	list_del_rcu(&ns->dev_link);
+	xa_erase(&ns->subsys->namespaces, ns->nsid);
 	if (ns->nsid == subsys->max_nsid)
 		subsys->max_nsid = nvmet_max_nsid(subsys);
 
@@ -681,7 +655,6 @@ struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid)
 	if (!ns)
 		return NULL;
 
-	INIT_LIST_HEAD(&ns->dev_link);
 	init_completion(&ns->disable_done);
 
 	ns->nsid = nsid;
@@ -1263,14 +1236,14 @@ static void nvmet_setup_p2p_ns_map(struct nvmet_ctrl *ctrl,
 		struct nvmet_req *req)
 {
 	struct nvmet_ns *ns;
+	unsigned long idx;
 
 	if (!req->p2p_client)
 		return;
 
 	ctrl->p2p_client = get_device(req->p2p_client);
 
-	list_for_each_entry_rcu(ns, &ctrl->subsys->namespaces, dev_link,
-				lockdep_is_held(&ctrl->subsys->lock))
+	xa_for_each(&ctrl->subsys->namespaces, idx, ns)
 		nvmet_p2pmem_ns_add_p2p(ctrl, ns);
 }
 
@@ -1523,7 +1496,7 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
 	kref_init(&subsys->ref);
 
 	mutex_init(&subsys->lock);
-	INIT_LIST_HEAD(&subsys->namespaces);
+	xa_init(&subsys->namespaces);
 	INIT_LIST_HEAD(&subsys->ctrls);
 	INIT_LIST_HEAD(&subsys->hosts);
 
@@ -1535,8 +1508,9 @@ static void nvmet_subsys_free(struct kref *ref)
 	struct nvmet_subsys *subsys =
 		container_of(ref, struct nvmet_subsys, ref);
 
-	WARN_ON_ONCE(!list_empty(&subsys->namespaces));
+	WARN_ON_ONCE(!xa_empty(&subsys->namespaces));
 
+	xa_destroy(&subsys->namespaces);
 	kfree(subsys->subsysnqn);
 	kfree_rcu(subsys->model, rcuhead);
 	kfree(subsys);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 6f8bd6a93575..49e14111446c 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -52,7 +52,6 @@
 	(cpu_to_le32(offsetof(struct nvmf_connect_command, x)))
 
 struct nvmet_ns {
-	struct list_head	dev_link;
 	struct percpu_ref	ref;
 	struct block_device	*bdev;
 	struct file		*file;
@@ -219,7 +218,7 @@ struct nvmet_subsys {
 	struct mutex		lock;
 	struct kref		ref;
 
-	struct list_head	namespaces;
+	struct xarray		namespaces;
 	unsigned int		nr_namespaces;
 	unsigned int		max_nsid;
 	u16			cntlid_min;
-- 
2.26.0


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH V3 01/10] xarray: add __xa_load() version

[Keith Busch]

On Tue, Jul 14, 2020 at 04:30:48PM -0700, Chaitanya Kulkarni wrote:
> This patch adds a new xarray API __xa_load() that moves xa_load() core
> to its low level function __xa_load(). Caller is responsible for handling
> RCU locking. This API is needed for NVMe subsystem so that it can take
> an advantage of the RCU locking provided by the XAarray for reference
> counting.

I asked last time: why is this necessary? All the docs say you can nest
RCU read locks so what is wrong with doing the following with existing
xa_load()?

	rcu_read_lock()
	xa_load(..)
	do_something(..)
	rcu_read_unlock()

If there's nothing wrong with the above, then we shouldn't need to
introduce a new API.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH V3 01/10] xarray: add __xa_load() version

[Chaitanya Kulkarni]

On 7/14/20 17:46, Keith Busch wrote:
> On Tue, Jul 14, 2020 at 04:30:48PM -0700, Chaitanya Kulkarni wrote:
>> This patch adds a new xarray API __xa_load() that moves xa_load() core
>> to its low level function __xa_load(). Caller is responsible for handling
>> RCU locking. This API is needed for NVMe subsystem so that it can take
>> an advantage of the RCU locking provided by the XAarray for reference
>> counting.
> I asked last time: why is this necessary? All the docs say you can nest
> RCU read locks so what is wrong with doing the following with existing
> xa_load()?
> 
> 	rcu_read_lock()
> 	xa_load(..)
> 	do_something(..)
> 	rcu_read_unlock()
> 
> If there's nothing wrong with the above, then we shouldn't need to
> introduce a new API.
> 

I see, I'll send V4 with that.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH V3 02/10] nvme-core: use xarray for ctrl ns tracking

[Keith Busch]

On Tue, Jul 14, 2020 at 04:30:49PM -0700, Chaitanya Kulkarni wrote:
> @@ -3933,9 +3918,7 @@ static void nvme_ns_remove(struct nvme_ns *ns)
>  			blk_integrity_unregister(ns->disk);
>  	}
>  
> -	down_write(&ns->ctrl->namespaces_rwsem);
> -	list_del_init(&ns->list);
> -	up_write(&ns->ctrl->namespaces_rwsem);
> +	xa_erase(xa, ns->head->ns_id);

Since you're using rcu to protect the kref_get() on load, we need a
synchronize_rcu() after removing it from the xarray.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH V3 02/10] nvme-core: use xarray for ctrl ns tracking

[Matthew Wilcox]

On Tue, Jul 14, 2020 at 05:55:09PM -0700, Keith Busch wrote:
> On Tue, Jul 14, 2020 at 04:30:49PM -0700, Chaitanya Kulkarni wrote:
> > @@ -3933,9 +3918,7 @@ static void nvme_ns_remove(struct nvme_ns *ns)
> >  			blk_integrity_unregister(ns->disk);
> >  	}
> >  
> > -	down_write(&ns->ctrl->namespaces_rwsem);
> > -	list_del_init(&ns->list);
> > -	up_write(&ns->ctrl->namespaces_rwsem);
> > +	xa_erase(xa, ns->head->ns_id);
> 
> Since you're using rcu to protect the kref_get() on load, we need a
> synchronize_rcu() after removing it from the xarray.

No, synchronize_rcu() is expensive.  We should instead use call_rcu()
to free the ns once the refcount hits zero.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH V3 03/10] nvme: centralize queue action nvme_kill_queues()

[Keith Busch]

On Tue, Jul 14, 2020 at 04:30:50PM -0700, Chaitanya Kulkarni wrote:
> There is no point in having 7 different functions which are differing
> with just one line.
> 
> The helper functions for ctrl queue management can be centralized with
> the help of the descriptive enums for action. This avoids code
> duplication which spans across the 7 functions as compare to one
> function and exporting 7 symbols as compare to one symbol.
> 
> This patch merges nvme_kill_queue() into newly introduced
> nvme_queue_act() which handles the queue management.

This should be separate from the xarray conversion part of this series.

And if we're going to collapse these, I would prefer to create a generic
API to iterate namespaces, something like "nvme_ns_iter()" that takes a
callback function parameter.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH V3 03/10] nvme: centralize queue action nvme_kill_queues()

[Matthew Wilcox]

On Tue, Jul 14, 2020 at 06:36:07PM -0700, Keith Busch wrote:
> On Tue, Jul 14, 2020 at 04:30:50PM -0700, Chaitanya Kulkarni wrote:
> > There is no point in having 7 different functions which are differing
> > with just one line.
> > 
> > The helper functions for ctrl queue management can be centralized with
> > the help of the descriptive enums for action. This avoids code
> > duplication which spans across the 7 functions as compare to one
> > function and exporting 7 symbols as compare to one symbol.
> > 
> > This patch merges nvme_kill_queue() into newly introduced
> > nvme_queue_act() which handles the queue management.
> 
> This should be separate from the xarray conversion part of this series.
> 
> And if we're going to collapse these, I would prefer to create a generic
> API to iterate namespaces, something like "nvme_ns_iter()" that takes a
> callback function parameter.

The XArray iteration API deliberately doesn't do this.  It's not
type-safe, and in this age of spectre/meltdown/l1tf/... indirect
function calls are Bad and Slow.  I'd leave the code the way it is.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH V3 02/10] nvme-core: use xarray for ctrl ns tracking

[Keith Busch]

On Wed, Jul 15, 2020 at 02:33:42AM +0100, Matthew Wilcox wrote:
> On Tue, Jul 14, 2020 at 05:55:09PM -0700, Keith Busch wrote:
> > On Tue, Jul 14, 2020 at 04:30:49PM -0700, Chaitanya Kulkarni wrote:
> > > @@ -3933,9 +3918,7 @@ static void nvme_ns_remove(struct nvme_ns *ns)
> > >  			blk_integrity_unregister(ns->disk);
> > >  	}
> > >  
> > > -	down_write(&ns->ctrl->namespaces_rwsem);
> > > -	list_del_init(&ns->list);
> > > -	up_write(&ns->ctrl->namespaces_rwsem);
> > > +	xa_erase(xa, ns->head->ns_id);
> > 
> > Since you're using rcu to protect the kref_get() on load, we need a
> > synchronize_rcu() after removing it from the xarray.
> 
> No, synchronize_rcu() is expensive.  We should instead use call_rcu()
> to free the ns once the refcount hits zero.

Yeah, that is an even better option, though namespace removal is so
infrequent that cost isn't really a concern: this function already calls
synchronize_rcu() and syncronize_srcu().

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH V3 02/10] nvme-core: use xarray for ctrl ns tracking

[Chaitanya Kulkarni]

On 7/14/20 6:41 PM, Keith Busch wrote:
>>> Since you're using rcu to protect the kref_get() on load, we need a
>>> synchronize_rcu() after removing it from the xarray.
>> No, synchronize_rcu() is expensive.  We should instead use call_rcu()
>> to free the ns once the refcount hits zero.
> Yeah, that is an even better option, though namespace removal is so
> infrequent that cost isn't really a concern: this function already calls
> synchronize_rcu() and syncronize_srcu().
> 

So should keep the synchronize_rcu() ?

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH V3 00/10] nvme: use xarray for ns tracking

[Christoph Hellwig]

On Tue, Jul 14, 2020 at 04:30:47PM -0700, Chaitanya Kulkarni wrote:
> Hi,
> 
> This patch-series uses ctrl->namespaces with an xarray for host-core and
> target-core. We can see following performance improvement when running
> fio with 32 parallel jobs where first 16 namespaces and last 16
> namespaces are used for I/O using NVMeOF (nvme-loop) backed by nulli blk
> devices mapped 1:1 on target namespaces.
> 
> For host even though nvme_find_get_ns() doesn't fall into the fast path
> yet it does for NVMeOF passthru. This prepares us to improve performance
> for future NVMeOF passthru backend which is under review which uses the
> similar data structure as target.
> 
> Following are the performance numbers with NVMeOF (nvme-loop) backed by
> null_blk devices mapped 1:1 on NVMeOF target backend :-

Can you make sure the nvmet patch is the first in the series?  That one
is obviously useful, so as soon as everything is sorted out I'm happy
to apply it.  I'm not quite as sold on the host side.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH V3 10/10] nvmet: use xarray for ctrl ns storing

[Christoph Hellwig]

> index 9cdc39c8b729..2908acc7b1e2 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -115,13 +115,14 @@ u16 nvmet_zero_sgl(struct nvmet_req *req, off_t off, size_t len)
>  
>  static unsigned int nvmet_max_nsid(struct nvmet_subsys *subsys)
>  {
> -	struct nvmet_ns *ns;
> +	unsigned long nsid = 0;
> +	struct nvmet_ns *cur;
> +	unsigned long idx;
>  
> -	if (list_empty(&subsys->namespaces))
> -		return 0;
> +	xa_for_each(&subsys->namespaces, idx, cur)
> +		nsid = cur->nsid;
>  
> -	ns = list_last_entry(&subsys->namespaces, struct nvmet_ns, dev_link);
> -	return ns->nsid;
> +	return nsid;
>  }

If Matthew wants to add a helper to return the highest id for an xarray
this would be the place to use it.


> +	ret = xa_insert(&subsys->namespaces, ns->nsid, ns, GFP_KERNEL);
> +	if (ret)
> +		goto out_dev_put;

I don't think out_dev_put is enough here as that fails to tear down
ns->ref.


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH V3 03/10] nvme: centralize queue action nvme_kill_queues()

[Christoph Hellwig]

On Tue, Jul 14, 2020 at 04:30:50PM -0700, Chaitanya Kulkarni wrote:
> There is no point in having 7 different functions which are differing
> with just one line.

Where the one line is the actual action they do if I follow this.
NAK, don't create pointless multiplexers for these kinds of lines of
code "optimizations" as that makes following the code much harder.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH V3 00/10] nvme: use xarray for ns tracking

[Chaitanya Kulkarni]

On 7/15/20 00:03, Christoph Hellwig wrote:
>> Following are the performance numbers with NVMeOF (nvme-loop) backed by
>> null_blk devices mapped 1:1 on NVMeOF target backend :-
> Can you make sure the nvmet patch is the first in the series?  That one
> is obviously useful, so as soon as everything is sorted out I'm happy
> to apply it.  I'm not quite as sold on the host side.
> 

Okay, will do.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH V3 10/10] nvmet: use xarray for ctrl ns storing

[Chaitanya Kulkarni]

On 7/15/20 00:10, Christoph Hellwig wrote:
>> index 9cdc39c8b729..2908acc7b1e2 100644
>> --- a/drivers/nvme/target/core.c
>> +++ b/drivers/nvme/target/core.c
>> @@ -115,13 +115,14 @@ u16 nvmet_zero_sgl(struct nvmet_req *req, off_t off, size_t len)
>>   
>>   static unsigned int nvmet_max_nsid(struct nvmet_subsys *subsys)
>>   {
>> -	struct nvmet_ns *ns;
>> +	unsigned long nsid = 0;
>> +	struct nvmet_ns *cur;
>> +	unsigned long idx;
>>   
>> -	if (list_empty(&subsys->namespaces))
>> -		return 0;
>> +	xa_for_each(&subsys->namespaces, idx, cur)
>> +		nsid = cur->nsid;
>>   
>> -	ns = list_last_entry(&subsys->namespaces, struct nvmet_ns, dev_link);
>> -	return ns->nsid;
>> +	return nsid;
>>   }
> 
> If Matthew wants to add a helper to return the highest id for an xarray
> this would be the place to use it.
> 
Matthew, any suggestions on how to add a helper for above use case ?
we want to get the highest possible available index for in the XArray.

> 
>> +	ret = xa_insert(&subsys->namespaces, ns->nsid, ns, GFP_KERNEL);
>> +	if (ret)
>> +		goto out_dev_put;
> 
Christoph,
> I don't think out_dev_put is enough here as that fails to tear down
> ns->ref.
>

Yes the call to percpu_ref_exit() is missing here, also it needs to 
restore the subsys->max_nsid when xa_insert() fails (correct me if I'm
wrong), will add it to V4.




_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH V3 00/10] nvme: use xarray for ns tracking

[Chaitanya Kulkarni]

Christoph,

On 7/15/20 00:03, Christoph Hellwig wrote:
>> Following are the performance numbers with NVMeOF (nvme-loop) backed by
>> null_blk devices mapped 1:1 on NVMeOF target backend :-
> Can you make sure the nvmet patch is the first in the series?  That one
> is obviously useful, so as soon as everything is sorted out I'm happy
> to apply it.  I'm not quite as sold on the host side.
> 

Sure.

Can you please elaborate on what is wrong with host side patch ? any
potential bug/race/deadlock/performance regression ?

Cover-letter has basic explanation for host patch. See [1].

In case that is not sufficient at least I'd know what kind of more data
I need to collect to prove the usefulness of the patch.

I've also offered to forward port the passthru and get the performance
numbers with nvme_find_get_ns() with XArray version, but didn't get any
response on that.


[1] Host side patch cover letter reference :-

"For host even though nvme_find_get_ns() doesn't fall into the fast path
yet, it does for NVMeOF passthru. This prepares us to improve 
performance for future NVMeOF passthru backend which is under review 
that uses the similar data structure in host-core as target-core."

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme