[PATCH 00/10] nvme: ANA fixups

[PATCH 00/10] nvme: ANA fixups

[Hannes Reinecke]

Hi Christoph,

here's a set of patches and updates for the ANA support, both host and target.
It's mainly small issues I've found during testing.
Patches are relative to your nvme-ana branch.

As usual, comments and reviews are welcome.

Hannes Reinecke (10):
  nvme: add missing kfree() in nvme_configure_ana()
  nvme: ANA transition timeout handling
  nvme: Only update capacity for optimized or non-optimized paths
  nvme: clear current path on ANA state change
  nvme: retry nvme_get_log_ext() when processing ANA log
  nvme: simplify check for ANA in nvme_ns_id_attrs_are_visible()
  nvmet: make ANATT configurable
  nvmet: Set nanagrpid correctly
  nvmet: Set mnan correctly
  nvmet: set 'nuse' and 'nsze' to zero for inaccessible paths

 drivers/nvme/host/core.c        | 17 ++++++++++----
 drivers/nvme/host/multipath.c   | 51 +++++++++++++++++++++++++++++------------
 drivers/nvme/host/nvme.h        | 17 +++++++++++++-
 drivers/nvme/target/admin-cmd.c | 43 +++++++++++++++++++++++++++-------
 drivers/nvme/target/configfs.c  | 28 ++++++++++++++++++++++
 drivers/nvme/target/core.c      |  1 +
 drivers/nvme/target/nvmet.h     |  2 ++
 7 files changed, 131 insertions(+), 28 deletions(-)

-- 
2.12.3

[PATCH 01/10] nvme: add missing kfree() in nvme_configure_ana()

[Hannes Reinecke]

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 drivers/nvme/host/multipath.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 0d3ef3098063..1a8791340862 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -421,6 +421,7 @@ int nvme_configure_ana(struct nvme_ctrl *ctrl)
 out_free_ana_log_buf:
 	kfree(ctrl->ana_log_buf);
 out_free_ana_state:
+	kfree(ctrl->ana_state);
 	return -ENOMEM;
 }
 
-- 
2.12.3

[PATCH 02/10] nvme: ANA transition timeout handling

[Hannes Reinecke]

Use delayed work for processing ANA log pages, and resubmit the
workqueue function in case of an error or if a state is found to
be in 'transitioning'.

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 drivers/nvme/host/core.c      | 11 +++++++++--
 drivers/nvme/host/multipath.c | 33 +++++++++++++++++++++++++++------
 drivers/nvme/host/nvme.h      |  4 +++-
 3 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index b1e4f56a55ed..1be424053f47 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1516,6 +1516,12 @@ static int nvme_revalidate_disk(struct gendisk *disk)
 		goto out;
 	}
 
+	if (ns->anagrpid != le32_to_cpu(id->anagrpid)) {
+		dev_warn(ctrl->device, "nsid %d ANA group id changed\n",
+			 ns->head->ns_id);
+		queue_delayed_work(nvme_wq, &ctrl->ana_work, 0);
+	}
+
 	__nvme_revalidate_disk(disk, id);
 	nvme_report_ns_ids(ctrl, ns->head->ns_id, id, &ids);
 	if (!nvme_ns_ids_equal(&ns->head->ids, &ids)) {
@@ -2390,6 +2396,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 	ctrl->kas = le16_to_cpu(id->kas);
 	ctrl->max_namespaces = le32_to_cpu(id->mnan);
 	ctrl->anacap = id->anacap;
+	ctrl->anatt = id->anatt;
 	ctrl->nanagrpid = le32_to_cpu(id->nanagrpid);
 	ctrl->anagrpmax = le32_to_cpu(id->anagrpmax);
 
@@ -3415,7 +3422,7 @@ static void nvme_handle_aen_notice(struct nvme_ctrl *ctrl, u32 result)
 	case NVME_AER_NOTICE_ANA:
 		if (WARN_ON_ONCE(!ctrl->ana_log_buf))
 			break;
-		queue_work(nvme_wq, &ctrl->ana_work);
+		queue_delayed_work(nvme_wq, &ctrl->ana_work, 0);
 		break;
 	default:
 		dev_warn(ctrl->device, "async event result %08x\n", result);
@@ -3452,7 +3459,7 @@ void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
 	nvme_stop_keep_alive(ctrl);
 	flush_work(&ctrl->async_event_work);
 	flush_work(&ctrl->scan_work);
-	cancel_work_sync(&ctrl->ana_work);
+	cancel_delayed_work_sync(&ctrl->ana_work);
 	cancel_work_sync(&ctrl->fw_act_work);
 	if (ctrl->ops->stop_ctrl)
 		ctrl->ops->stop_ctrl(ctrl);
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 1a8791340862..2fcaf50d84e2 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -69,6 +69,8 @@ void nvme_failover_req(struct request *req)
 		 * entirely trivial..
 		 */
 		nvme_update_ana_state(ns, NVME_ANA_CHANGE);
+		queue_delayed_work(nvme_wq, &ns->ctrl->ana_work,
+				   ns->ctrl->anatt * HZ);
 		break;
 	case NVME_SC_ANA_PERSISTENT_LOSS:
 		nvme_update_ana_state(ns, NVME_ANA_PERSISTENT_LOSS);
@@ -309,7 +311,7 @@ static int nvme_process_ana_log(struct nvme_ctrl *ctrl, bool groups_only)
 {
 	void *base = ctrl->ana_log_buf;
 	size_t offset = sizeof(struct nvme_ana_rsp_hdr);
-	int error, i;
+	int error = 0, i;
 
 	/*
 	 * If anagrpid never changes we don't need to process the namespace
@@ -323,7 +325,7 @@ static int nvme_process_ana_log(struct nvme_ctrl *ctrl, bool groups_only)
 			ctrl->ana_log_buf, ctrl->ana_log_size, 0);
 	if (error) {
 		dev_warn(ctrl->device, "Failed to get ANA log: %d\n", error);
-		return error;
+		return -EIO;
 	}
 
 	for (i = 0; i < le16_to_cpu(ctrl->ana_log_buf->ngrps); i++) {
@@ -345,6 +347,8 @@ static int nvme_process_ana_log(struct nvme_ctrl *ctrl, bool groups_only)
 		dev_info(ctrl->device, "ANA group %d: %s.\n",
 				grpid, nvme_ana_state_names[desc->state]);
 		WRITE_ONCE(ctrl->ana_state[grpid], desc->state);
+		if (desc->state == NVME_ANA_CHANGE)
+			error = -EAGAIN;
 		offset += sizeof(*desc);
 		if (!nr_nsids)
 			continue;
@@ -372,14 +376,31 @@ static int nvme_process_ana_log(struct nvme_ctrl *ctrl, bool groups_only)
 			return -EINVAL;
 	}
 
-	return 0;
+	return error;
 }
 
 static void nvme_ana_work(struct work_struct *work)
 {
-	struct nvme_ctrl *ctrl = container_of(work, struct nvme_ctrl, ana_work);
+	struct nvme_ctrl *ctrl = container_of(work, struct nvme_ctrl,
+					      ana_work.work);
+	int ret;
+
+	if (!ctrl->ana_log_buf)
+		return;
+	if (ctrl->state != NVME_CTRL_LIVE)
+		return;
+	ret = nvme_process_ana_log(ctrl, false);
+	if (ret == -EAGAIN || ret == -EIO) {
+		unsigned long log_delay = ctrl->anatt * HZ;
 
-	nvme_process_ana_log(ctrl, false);
+		/*
+		 * In case of an I/O error just add a small delay to not hit
+		 * the target too hard
+		 */
+		if (ret == -EIO)
+			log_delay = msecs_to_jiffies(NVME_ANA_LOG_DELAY);
+		queue_delayed_work(nvme_wq, &ctrl->ana_work, log_delay);
+	}
 	nvme_kick_requeue_lists(ctrl);
 }
 
@@ -404,7 +425,7 @@ int nvme_configure_ana(struct nvme_ctrl *ctrl)
 		return 0;
 	}
 
-	INIT_WORK(&ctrl->ana_work, nvme_ana_work);
+	INIT_DELAYED_WORK(&ctrl->ana_work, nvme_ana_work);
 	ctrl->ana_state = kcalloc(ctrl->anagrpmax, sizeof(*ctrl->ana_state),
 			GFP_KERNEL);
 	if (!ctrl->ana_state)
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 4a29c736370f..53cb7f8a6267 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -32,6 +32,7 @@ extern unsigned int admin_timeout;
 
 #define NVME_DEFAULT_KATO	5
 #define NVME_KATO_GRACE		10
+#define NVME_ANA_LOG_DELAY	500
 
 extern struct workqueue_struct *nvme_wq;
 extern struct workqueue_struct *nvme_reset_wq;
@@ -195,12 +196,13 @@ struct nvme_ctrl {
 
 	/* asymmetric namespace access: */
 	u8 anacap;
+	u8 anatt;
 	u32 anagrpmax;
 	u32 nanagrpid;
 	enum nvme_ana_state *ana_state;
 	size_t ana_log_size;
 	struct nvme_ana_rsp_hdr *ana_log_buf;
-	struct work_struct ana_work;
+	struct delayed_work ana_work;
 
 	/* Power saving configuration */
 	u64 ps_max_latency_us;
-- 
2.12.3

[PATCH 03/10] nvme: Only update capacity for optimized or non-optimized paths

[Hannes Reinecke]

As per ANA spec paths in 'inaccessible' or 'persistent-loss' state
might return a capacity of '0'. So when an inaccessible path is
detected last it might overwrite the actual capacity with '0',
rendering the device unuseable.
Hence we should only be updating the capacity if we are sure that
we've read the correct one.

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 drivers/nvme/host/core.c      |  4 +++-
 drivers/nvme/host/multipath.c |  9 ---------
 drivers/nvme/host/nvme.h      | 13 +++++++++++++
 3 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 1be424053f47..cc43bb685e63 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1457,7 +1457,9 @@ static void nvme_update_disk_info(struct gendisk *disk,
 	if (ns->ms && !nvme_ns_has_pi(ns) && !blk_get_integrity(disk))
 		capacity = 0;
 
-	set_capacity(disk, capacity);
+	if (nvme_ns_ana_state(ns) == NVME_ANA_OPTIMIZED ||
+	    nvme_ns_ana_state(ns) == NVME_ANA_NONOPTIMIZED)
+		set_capacity(disk, capacity);
 	nvme_config_discard(ns);
 	blk_mq_unfreeze_queue(disk->queue);
 }
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 2fcaf50d84e2..a5f43d164b19 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -108,15 +108,6 @@ void nvme_kick_requeue_lists(struct nvme_ctrl *ctrl)
 	up_read(&ctrl->namespaces_rwsem);
 }
 
-static inline enum nvme_ana_state nvme_ns_ana_state(struct nvme_ns *ns)
-{
-	if (!nvme_ctrl_has_ana(ns->ctrl))
-		return NVME_ANA_OPTIMIZED;
-	if (WARN_ON_ONCE(ns->anagrpid > ns->ctrl->anagrpmax))
-		return 0;
-	return READ_ONCE(ns->ctrl->ana_state[ns->anagrpid]);
-}
-
 static const char *nvme_ana_state_names[] = {
 	[0]				= "invalid state",
 	[NVME_ANA_OPTIMIZED]		= "optimized",
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 53cb7f8a6267..8572f106d6ac 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -485,6 +485,15 @@ static inline void nvme_mpath_check_last_path(struct nvme_ns *ns)
 		kblockd_schedule_work(&head->requeue_work);
 }
 
+static inline enum nvme_ana_state nvme_ns_ana_state(struct nvme_ns *ns)
+{
+	if (!nvme_ctrl_has_ana(ns->ctrl))
+		return NVME_ANA_OPTIMIZED;
+	if (WARN_ON_ONCE(ns->anagrpid > ns->ctrl->anagrpmax))
+		return 0;
+	return READ_ONCE(ns->ctrl->ana_state[ns->anagrpid]);
+}
+
 extern struct device_attribute dev_attr_ana_grpid;
 extern struct device_attribute dev_attr_ana_state;
 
@@ -534,6 +543,10 @@ static inline int nvme_configure_ana(struct nvme_ctrl *ctrl)
 static inline void nvme_deconfigure_ana(struct nvme_ctrl *ctrl)
 {
 }
+static inline enum nvme_ana_state nvme_ns_ana_state(struct nvme_ns *ns)
+{
+	return NVME_ANA_OPTIMIZED;
+}
 #endif /* CONFIG_NVME_MULTIPATH */
 
 #ifdef CONFIG_NVM
-- 
2.12.3

[PATCH 04/10] nvme: clear current path on ANA state change

[Hannes Reinecke]

Whenever ANA state changes the path selection needs to be retriggered,
so we should be clearing the current path here.

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 drivers/nvme/host/multipath.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index a5f43d164b19..a520582811f8 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -44,6 +44,7 @@ void nvme_set_disk_name(char *disk_name, struct nvme_ns *ns,
 static void nvme_update_ana_state(struct nvme_ns *ns, enum nvme_ana_state state)
 {
 	WRITE_ONCE(ns->ctrl->ana_state[ns->anagrpid], state);
+	nvme_mpath_clear_current_path(ns);
 }
 
 void nvme_failover_req(struct request *req)
-- 
2.12.3

[PATCH 05/10] nvme: retry nvme_get_log_ext() when processing ANA log

[Hannes Reinecke]

When we can't fetch the ANA log page we should retry to ensure
that we really have nothing to read.

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 drivers/nvme/host/multipath.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index a520582811f8..73bd4ff6ab06 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -19,6 +19,10 @@ module_param(multipath, bool, 0444);
 MODULE_PARM_DESC(multipath,
 	"turn on native support for multiple controllers per subsystem");
 
+static u8 ana_log_retries = 5;
+module_param(ana_log_retries, byte, 0644);
+MODULE_PARM_DESC(ana_log_retries, "max number of retries retrieving ANA log");
+
 /*
  * If multipathing is enabled we need to always use the subsystem instance
  * number for numbering our devices to avoid conflicts between subsystems that
@@ -303,7 +307,7 @@ static int nvme_process_ana_log(struct nvme_ctrl *ctrl, bool groups_only)
 {
 	void *base = ctrl->ana_log_buf;
 	size_t offset = sizeof(struct nvme_ana_rsp_hdr);
-	int error = 0, i;
+	int error = 0, i, retries = 0;
 
 	/*
 	 * If anagrpid never changes we don't need to process the namespace
@@ -312,10 +316,13 @@ static int nvme_process_ana_log(struct nvme_ctrl *ctrl, bool groups_only)
 	if (ctrl->anacap & (1 << 6))
 		groups_only = true;
 
+resubmit:
 	error = nvme_get_log_ext(ctrl, NULL, NVME_LOG_ANA,
 			groups_only ? NVME_ANA_LOG_RGO : 0,
 			ctrl->ana_log_buf, ctrl->ana_log_size, 0);
 	if (error) {
+		if (++retries < ana_log_retries)
+			goto resubmit;
 		dev_warn(ctrl->device, "Failed to get ANA log: %d\n", error);
 		return -EIO;
 	}
-- 
2.12.3

[PATCH 06/10] nvme: simplify check for ANA in nvme_ns_id_attrs_are_visible()

[Hannes Reinecke]

Instead of adding another indirection to the controller we can
as well check the ANA group id of the namespace to figure out
if ANA is supported.

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 drivers/nvme/host/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index cc43bb685e63..194cce7e63ce 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2714,7 +2714,7 @@ static umode_t nvme_ns_id_attrs_are_visible(struct kobject *kobj,
 	if (a == &dev_attr_ana_grpid.attr || a == &dev_attr_ana_state.attr) {
 		if (dev_to_disk(dev)->fops != &nvme_fops) /* per-path attr */
 			return 0;
-		if (!nvme_ctrl_has_ana(nvme_get_ns_from_dev(dev)->ctrl))
+		if (!nvme_get_ns_from_dev(dev)->anagrpid)
 			return 0;
 	}
 #endif
-- 
2.12.3

[PATCH 07/10] nvmet: make ANATT configurable

[Hannes Reinecke]

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 drivers/nvme/target/admin-cmd.c |  2 +-
 drivers/nvme/target/configfs.c  | 28 ++++++++++++++++++++++++++++
 drivers/nvme/target/core.c      |  1 +
 drivers/nvme/target/nvmet.h     |  2 ++
 4 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index c2fba1c92cd8..f2eedb13482a 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -312,7 +312,7 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 	id->msdbd = ctrl->ops->msdbd;
 
 	id->anacap = (1 << 0) | (1 << 1) | (1 << 2) | (1 << 3) | (1 << 4);
-	id->anatt = 10; /* random value */
+	id->anatt = ctrl->subsys->anatt;
 	id->anagrpmax = cpu_to_le32(NVMET_MAX_ANAGRPS);
 	id->nanagrpid = cpu_to_le32(NVMET_MAX_ANAGRPS);
 
diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index ddc3fa9fd92f..2c6de9961ef4 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -738,10 +738,38 @@ static ssize_t nvmet_subsys_attr_serial_store(struct config_item *item,
 }
 CONFIGFS_ATTR(nvmet_subsys_, attr_serial);
 
+static ssize_t nvmet_subsys_attr_anatt_show(struct config_item *item,
+					    char *page)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+
+	return snprintf(page, PAGE_SIZE, "%d\n", subsys->anatt);
+}
+
+static ssize_t nvmet_subsys_attr_anatt_store(struct config_item *item,
+					     const char *page, size_t count)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+	int ret;
+	u8 ana_tt;
+
+	ret = kstrtou8(page, 0, &ana_tt);
+	if (ret || ana_tt == 0) {
+		return -EINVAL;
+	}
+	down_write(&nvmet_config_sem);
+	subsys->anatt = ana_tt;
+	up_write(&nvmet_config_sem);
+
+	return count;
+}
+CONFIGFS_ATTR(nvmet_subsys_, attr_anatt);
+
 static struct configfs_attribute *nvmet_subsys_attrs[] = {
 	&nvmet_subsys_attr_attr_allow_any_host,
 	&nvmet_subsys_attr_attr_version,
 	&nvmet_subsys_attr_attr_serial,
+	&nvmet_subsys_attr_attr_anatt,
 	NULL,
 };
 
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 1301a7e8b354..f47916481d8b 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1103,6 +1103,7 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
 		return NULL;
 	}
 	subsys->type = type;
+	subsys->anatt = NVMET_DEFAULT_ANATT;
 	subsys->subsysnqn = kstrndup(subsysnqn, NVMF_NQN_SIZE,
 			GFP_KERNEL);
 	if (!subsys->subsysnqn) {
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 7195e313b375..04dec38ffe74 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -166,6 +166,7 @@ struct nvmet_subsys {
 	struct list_head	namespaces;
 	unsigned int		nr_namespaces;
 	unsigned int		max_nsid;
+	u8			anatt;
 
 	struct list_head	ctrls;
 
@@ -386,6 +387,7 @@ u32 nvmet_get_log_page_len(struct nvme_command *cmd);
  */
 #define NVMET_MAX_ANAGRPS	128
 #define NVMET_DEFAULT_ANA_GRPID	1
+#define NVMET_DEFAULT_ANATT	10
 
 #define NVMET_KAS		10
 #define NVMET_DISC_KATO		120
-- 
2.12.3

[PATCH 08/10] nvmet: Set nanagrpid correctly

[Hannes Reinecke]

We should be returning the actual number of namespaces for nanagrpid.

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 drivers/nvme/target/admin-cmd.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index f2eedb13482a..a9d05eb03117 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -217,6 +217,8 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 	struct nvme_id_ctrl *id;
 	u16 status = 0;
 	const char model[] = "Linux";
+	u32 grpid;
+	u16 ngrps = 0;
 
 	id = kzalloc(sizeof(*id), GFP_KERNEL);
 	if (!id) {
@@ -242,8 +244,26 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 	 * the safest is to leave it as zeroes.
 	 */
 
-	/* we support multiple ports, multiples hosts and ANA: */
-	id->cmic = (1 << 0) | (1 << 1) | (1 << 3);
+	/* we support multiple ports and multiple hosts: */
+	id->cmic = (1 << 0) | (1 << 1);
+
+	/* ANA Support */
+	down_read(&nvmet_ana_sem);
+	for (grpid = 1; grpid <= NVMET_MAX_ANAGRPS; grpid++) {
+		if (nvmet_ana_group_enabled[grpid])
+			ngrps++;
+	}
+	up_read(&nvmet_ana_sem);
+
+	if (ngrps) {
+		id->cmic |= (1 << 3);
+		/* All ANA states are supported */
+		id->anacap = (1 << 0) | (1 << 1) | (1 << 2) | \
+			(1 << 3) | (1 << 4);
+		id->anatt = ctrl->subsys->anatt;
+		id->anagrpmax = cpu_to_le32(NVMET_MAX_ANAGRPS);
+		id->nanagrpid = cpu_to_le32(ngrps);
+	}
 
 	/* no limit on data transfer sizes for now */
 	id->mdts = 0;
@@ -311,11 +331,6 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 
 	id->msdbd = ctrl->ops->msdbd;
 
-	id->anacap = (1 << 0) | (1 << 1) | (1 << 2) | (1 << 3) | (1 << 4);
-	id->anatt = ctrl->subsys->anatt;
-	id->anagrpmax = cpu_to_le32(NVMET_MAX_ANAGRPS);
-	id->nanagrpid = cpu_to_le32(NVMET_MAX_ANAGRPS);
-
 	/*
 	 * Meh, we don't really support any power state.  Fake up the same
 	 * values that qemu does.
-- 
2.12.3

[PATCH 09/10] nvmet: Set mnan correctly

[Hannes Reinecke]

We already have the number of namespaces per subsystem, so we can as well
use that for the 'mnan' value.

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 drivers/nvme/target/admin-cmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index a9d05eb03117..99592c7b8bc7 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -301,7 +301,7 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 	id->maxcmd = cpu_to_le16(NVMET_MAX_CMD);
 
 	id->nn = cpu_to_le32(ctrl->subsys->max_nsid);
-	id->mnan = cpu_to_le32(NVMET_MAX_NAMESPACES);
+	id->mnan = cpu_to_le32(ctrl->subsys->nr_namespaces);
 	id->oncs = cpu_to_le16(NVME_CTRL_ONCS_DSM |
 			NVME_CTRL_ONCS_WRITE_ZEROES);
 
-- 
2.12.3

[PATCH 10/10] nvmet: set 'nuse' and 'nsze' to zero for inaccessible paths

[Hannes Reinecke]

According to the ANA base specification the 'nuse' and 'nsze' values
can be set to '0' for 'inaccessible' or 'persistent-loss' path states.

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 drivers/nvme/target/admin-cmd.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 99592c7b8bc7..0ae25478e3eb 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -371,6 +371,18 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 	id->ncap = id->nuse = id->nsze =
 		cpu_to_le64(ns->size >> ns->blksize_shift);
 
+	if (ns->anagrpid) {
+		enum nvme_ana_state ana_state =
+			req->port->ana_state[ns->anagrpid];
+
+		/*
+		 * nuse and nsze should be zero for inaccessible or
+		 * persistent loss ANA state.
+		 */
+		if ((ana_state == NVME_ANA_INACCESSIBLE) ||
+		    (ana_state == NVME_ANA_PERSISTENT_LOSS))
+			id->nuse = id->nsze = 0;
+	}
 	/*
 	 * We just provide a single LBA format that matches what the
 	 * underlying device reports.
-- 
2.12.3

[PATCH 01/10] nvme: add missing kfree() in nvme_configure_ana()

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 04/10] nvme: clear current path on ANA state change

[Johannes Thumshirn]

On Tue, May 29, 2018@12:14:25PM +0200, Hannes Reinecke wrote:
> Whenever ANA state changes the path selection needs to be retriggered,
> so we should be clearing the current path here.
> 
> Signed-off-by: Hannes Reinecke <hare at suse.com>
> ---
>  drivers/nvme/host/multipath.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index a5f43d164b19..a520582811f8 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -44,6 +44,7 @@ void nvme_set_disk_name(char *disk_name, struct nvme_ns *ns,
>  static void nvme_update_ana_state(struct nvme_ns *ns, enum nvme_ana_state state)
>  {
>  	WRITE_ONCE(ns->ctrl->ana_state[ns->anagrpid], state);
> +	nvme_mpath_clear_current_path(ns);

Don't we need the subsys lock when calling
nvme_mpath_clear_current_path()? At least that's how I interpreted
Paul here:
http://lists.infradead.org/pipermail/linux-nvme/2018-May/017484.html


-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 03/10] nvme: Only update capacity for optimized or non-optimized paths

[Christoph Hellwig]

On Tue, May 29, 2018@12:14:24PM +0200, Hannes Reinecke wrote:
> As per ANA spec paths in 'inaccessible' or 'persistent-loss' state
> might return a capacity of '0'. So when an inaccessible path is
> detected last it might overwrite the actual capacity with '0',
> rendering the device unuseable.
> Hence we should only be updating the capacity if we are sure that
> we've read the correct one.

That clause in the ANA spec only affects the NUSE and NVMCAP fields,
which we never look at in the linux nvme driver.


> 
> Signed-off-by: Hannes Reinecke <hare at suse.com>
> ---
>  drivers/nvme/host/core.c      |  4 +++-
>  drivers/nvme/host/multipath.c |  9 ---------
>  drivers/nvme/host/nvme.h      | 13 +++++++++++++
>  3 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 1be424053f47..cc43bb685e63 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1457,7 +1457,9 @@ static void nvme_update_disk_info(struct gendisk *disk,
>  	if (ns->ms && !nvme_ns_has_pi(ns) && !blk_get_integrity(disk))
>  		capacity = 0;
>  
> -	set_capacity(disk, capacity);
> +	if (nvme_ns_ana_state(ns) == NVME_ANA_OPTIMIZED ||
> +	    nvme_ns_ana_state(ns) == NVME_ANA_NONOPTIMIZED)
> +		set_capacity(disk, capacity);
>  	nvme_config_discard(ns);
>  	blk_mq_unfreeze_queue(disk->queue);
>  }
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 2fcaf50d84e2..a5f43d164b19 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -108,15 +108,6 @@ void nvme_kick_requeue_lists(struct nvme_ctrl *ctrl)
>  	up_read(&ctrl->namespaces_rwsem);
>  }
>  
> -static inline enum nvme_ana_state nvme_ns_ana_state(struct nvme_ns *ns)
> -{
> -	if (!nvme_ctrl_has_ana(ns->ctrl))
> -		return NVME_ANA_OPTIMIZED;
> -	if (WARN_ON_ONCE(ns->anagrpid > ns->ctrl->anagrpmax))
> -		return 0;
> -	return READ_ONCE(ns->ctrl->ana_state[ns->anagrpid]);
> -}
> -
>  static const char *nvme_ana_state_names[] = {
>  	[0]				= "invalid state",
>  	[NVME_ANA_OPTIMIZED]		= "optimized",
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 53cb7f8a6267..8572f106d6ac 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -485,6 +485,15 @@ static inline void nvme_mpath_check_last_path(struct nvme_ns *ns)
>  		kblockd_schedule_work(&head->requeue_work);
>  }
>  
> +static inline enum nvme_ana_state nvme_ns_ana_state(struct nvme_ns *ns)
> +{
> +	if (!nvme_ctrl_has_ana(ns->ctrl))
> +		return NVME_ANA_OPTIMIZED;
> +	if (WARN_ON_ONCE(ns->anagrpid > ns->ctrl->anagrpmax))
> +		return 0;
> +	return READ_ONCE(ns->ctrl->ana_state[ns->anagrpid]);
> +}
> +
>  extern struct device_attribute dev_attr_ana_grpid;
>  extern struct device_attribute dev_attr_ana_state;
>  
> @@ -534,6 +543,10 @@ static inline int nvme_configure_ana(struct nvme_ctrl *ctrl)
>  static inline void nvme_deconfigure_ana(struct nvme_ctrl *ctrl)
>  {
>  }
> +static inline enum nvme_ana_state nvme_ns_ana_state(struct nvme_ns *ns)
> +{
> +	return NVME_ANA_OPTIMIZED;
> +}
>  #endif /* CONFIG_NVME_MULTIPATH */
>  
>  #ifdef CONFIG_NVM
> -- 
> 2.12.3
---end quoted text---

[PATCH 01/10] nvme: add missing kfree() in nvme_configure_ana()

[Christoph Hellwig]

Thanks, applied.

[PATCH 04/10] nvme: clear current path on ANA state change

[Hannes Reinecke]

On Tue, 29 May 2018 14:22:12 +0200
"Johannes Thumshirn" <jthumshirn@suse.de> wrote:

> On Tue, May 29, 2018@12:14:25PM +0200, Hannes Reinecke wrote:
> > Whenever ANA state changes the path selection needs to be
> > retriggered, so we should be clearing the current path here.
> > 
> > Signed-off-by: Hannes Reinecke <hare at suse.com>
> > ---
> >  drivers/nvme/host/multipath.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/nvme/host/multipath.c
> > b/drivers/nvme/host/multipath.c index a5f43d164b19..a520582811f8
> > 100644 --- a/drivers/nvme/host/multipath.c
> > +++ b/drivers/nvme/host/multipath.c
> > @@ -44,6 +44,7 @@ void nvme_set_disk_name(char *disk_name, struct
> > nvme_ns *ns, static void nvme_update_ana_state(struct nvme_ns *ns,
> > enum nvme_ana_state state) {
> >  	WRITE_ONCE(ns->ctrl->ana_state[ns->anagrpid], state);
> > +	nvme_mpath_clear_current_path(ns);  
> 
> Don't we need the subsys lock when calling
> nvme_mpath_clear_current_path()? At least that's how I interpreted
> Paul here:
> http://lists.infradead.org/pipermail/linux-nvme/2018-May/017484.html
> 
> 

I'm aware of that, and interpreted it differently.
In the end we're only _clearing_ the pointer, not assigning a new one.
And clearing is always an atomic update, so that's okay.
We might be racing with another update, but then this is not a
hard requirement but rather a hint to the path selector. So if it's
being overwritten later on, fine.

Cheers,

Hannes

[PATCH 02/10] nvme: ANA transition timeout handling

[Christoph Hellwig]

> +	if (ns->anagrpid != le32_to_cpu(id->anagrpid)) {
> +		dev_warn(ctrl->device, "nsid %d ANA group id changed\n",
> +			 ns->head->ns_id);
> +		queue_delayed_work(nvme_wq, &ctrl->ana_work, 0);
> +	}

No need to queue any work if an anagrpid changed.  We'll automatically
index into the right group once it has changed.

> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 1a8791340862..2fcaf50d84e2 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -69,6 +69,8 @@ void nvme_failover_req(struct request *req)
>  		 * entirely trivial..
>  		 */
>  		nvme_update_ana_state(ns, NVME_ANA_CHANGE);
> +		queue_delayed_work(nvme_wq, &ns->ctrl->ana_work,
> +				   ns->ctrl->anatt * HZ);

This doesn't make much sense.  Once we get the ana transitioning
status we should either retry the command up to ANATT or try another
path.  There is no point in scheduling a read of the log page after
ANATT, as we'll already get an AEN when that log page is ready.

> @@ -323,7 +325,7 @@ static int nvme_process_ana_log(struct nvme_ctrl *ctrl, bool groups_only)
>  			ctrl->ana_log_buf, ctrl->ana_log_size, 0);
>  	if (error) {
>  		dev_warn(ctrl->device, "Failed to get ANA log: %d\n", error);
> -		return error;
> +		return -EIO;
>  	}
>  
>  	for (i = 0; i < le16_to_cpu(ctrl->ana_log_buf->ngrps); i++) {
> @@ -345,6 +347,8 @@ static int nvme_process_ana_log(struct nvme_ctrl *ctrl, bool groups_only)
>  		dev_info(ctrl->device, "ANA group %d: %s.\n",
>  				grpid, nvme_ana_state_names[desc->state]);
>  		WRITE_ONCE(ctrl->ana_state[grpid], desc->state);
> +		if (desc->state == NVME_ANA_CHANGE)
> +			error = -EAGAIN;

Huh?  Why would be stop processing our log when we see a change state?
This looks extremely dubious to me, and does not match the changelog
either.

> +	if (!ctrl->ana_log_buf)
> +		return;

How would the log buf disappear?  Even if it does please does this
in a separate, documented patch.

> +	if (ctrl->state != NVME_CTRL_LIVE)
> +		return;

This looks sensible, but it should probably also check for ADMIN_LIVE
for completeness, and be a seprate, properly documented patch.

> +		/*
> +		 * In case of an I/O error just add a small delay to not hit
> +		 * the target too hard
> +		 */
> +		if (ret == -EIO)
> +			log_delay = msecs_to_jiffies(NVME_ANA_LOG_DELAY);
> +		queue_delayed_work(nvme_wq, &ctrl->ana_work, log_delay);

What is the rationale for this I/O error handling?  In NVMe over
Fabrics transport errors tear down the association, so I really
don't see why we should handle errors here.

[PATCH 04/10] nvme: clear current path on ANA state change

[Christoph Hellwig]

On Tue, May 29, 2018@12:14:25PM +0200, Hannes Reinecke wrote:
> Whenever ANA state changes the path selection needs to be retriggered,
> so we should be clearing the current path here.

No, it doesn't.  We always check that the current path is optimized
in nvme_find_path, and trigger a reselection there if it isn't.

[PATCH 05/10] nvme: retry nvme_get_log_ext() when processing ANA log

[Christoph Hellwig]

On Tue, May 29, 2018@12:14:26PM +0200, Hannes Reinecke wrote:
> When we can't fetch the ANA log page we should retry to ensure
> that we really have nothing to read.

Why?

[PATCH 06/10] nvme: simplify check for ANA in nvme_ns_id_attrs_are_visible()

[Christoph Hellwig]

On Tue, May 29, 2018@12:14:27PM +0200, Hannes Reinecke wrote:
> Instead of adding another indirection to the controller we can
> as well check the ANA group id of the namespace to figure out
> if ANA is supported.

Not sure why this is so much better, but it does look correct to me.

[PATCH 07/10] nvmet: make ANATT configurable

[Christoph Hellwig]

On Tue, May 29, 2018@12:14:28PM +0200, Hannes Reinecke wrote:
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index ddc3fa9fd92f..2c6de9961ef4 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -738,10 +738,38 @@ static ssize_t nvmet_subsys_attr_serial_store(struct config_item *item,
>  }
>  CONFIGFS_ATTR(nvmet_subsys_, attr_serial);
>  
> +static ssize_t nvmet_subsys_attr_anatt_show(struct config_item *item,
> +					    char *page)
> +{
> +	struct nvmet_subsys *subsys = to_subsys(item);
> +
> +	return snprintf(page, PAGE_SIZE, "%d\n", subsys->anatt);
> +}
> +
> +static ssize_t nvmet_subsys_attr_anatt_store(struct config_item *item,
> +					     const char *page, size_t count)
> +{
> +	struct nvmet_subsys *subsys = to_subsys(item);
> +	int ret;
> +	u8 ana_tt;
> +
> +	ret = kstrtou8(page, 0, &ana_tt);
> +	if (ret || ana_tt == 0) {
> +		return -EINVAL;
> +	}

No need for the braces and please propagate the original return value.

Otherwise this looks ok.

[PATCH 08/10] nvmet: Set nanagrpid correctly

[Christoph Hellwig]

On Tue, May 29, 2018@12:14:29PM +0200, Hannes Reinecke wrote:
> We should be returning the actual number of namespaces for nanagrpid.

"This field indicates the number
of ANA groups supported by this controller."

Nothing indicates this field is dynamic, especially as dynamic values
in Identify are very rare and avoided.

[PATCH 09/10] nvmet: Set mnan correctly

[Christoph Hellwig]

On Tue, May 29, 2018@12:14:30PM +0200, Hannes Reinecke wrote:
>  	id->nn = cpu_to_le32(ctrl->subsys->max_nsid);
> -	id->mnan = cpu_to_le32(NVMET_MAX_NAMESPACES);
> +	id->mnan = cpu_to_le32(ctrl->subsys->nr_namespaces);

"Maximum Number of Allocated Namespaces (MNAN): This field indicates the
maximum number of allocated namespaces."

maximum != current

[PATCH 10/10] nvmet: set 'nuse' and 'nsze' to zero for inaccessible paths

[Christoph Hellwig]

On Tue, May 29, 2018@12:14:31PM +0200, Hannes Reinecke wrote:
> According to the ANA base specification the 'nuse' and 'nsze' values
> can be set to '0' for 'inaccessible' or 'persistent-loss' path states.

No.  It applies to NUSE and NVMCAP.  We don't report NVMCAP, so we
only need this tweak for NUSE.

> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -371,6 +371,18 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
>  	id->ncap = id->nuse = id->nsze =
>  		cpu_to_le64(ns->size >> ns->blksize_shift);
>  
> +	if (ns->anagrpid) {
> +		enum nvme_ana_state ana_state =
> +			req->port->ana_state[ns->anagrpid];
> +
> +		/*
> +		 * nuse and nsze should be zero for inaccessible or
> +		 * persistent loss ANA state.
> +		 */
> +		if ((ana_state == NVME_ANA_INACCESSIBLE) ||
> +		    (ana_state == NVME_ANA_PERSISTENT_LOSS))

Please remove the inner braces.

We don't need the anagrpid check, because we a) always set it to
a non-zero value, and b) would not be able to have a non-optimized
state anyway.

So just something like:

	id->ncap = id->nsze = cpu_to_le64(ns->size >> ns->blksize_shift);

	switch (req->port->ana_state[ns->anagrpid]) {
	case NVME_ANA_INACCESSIBLE:
	case NVME_ANA_PERSISTENT_LOSS:
		break;
	default:
		id->nuse = id->nsze;
		break;
	}

[PATCH 08/10] nvmet: Set nanagrpid correctly

[Hannes Reinecke]

On Tue, 29 May 2018 14:51:51 +0200
Christoph Hellwig <hch@lst.de> wrote:

> On Tue, May 29, 2018@12:14:29PM +0200, Hannes Reinecke wrote:
> > We should be returning the actual number of namespaces for
> > nanagrpid.  
> 
> "This field indicates the number
> of ANA groups supported by this controller."
> 
> Nothing indicates this field is dynamic, especially as dynamic values
> in Identify are very rare and avoided.

Yes, but it also doesn't _forbid_ us to set it to the value which is
currently active.

Plus we already have dynamic fields in the identify output; the
'mnan' field is specifically for the number of allocated namespaces.
And I can easily create new namespaces while a controller is attached.

I do agree that we don't have a good method of signalling any
configuration change to the attached controllers; maybe we should be
dropping the connection and for the attached hosts to do a reset.
But this should be a separate discussion.

Cheers,

Hannes

[PATCH 09/10] nvmet: Set mnan correctly

[Hannes Reinecke]

On Tue, 29 May 2018 14:52:49 +0200
Christoph Hellwig <hch@lst.de> wrote:

> On Tue, May 29, 2018@12:14:30PM +0200, Hannes Reinecke wrote:
> >  	id->nn = cpu_to_le32(ctrl->subsys->max_nsid);
> > -	id->mnan = cpu_to_le32(NVMET_MAX_NAMESPACES);
> > +	id->mnan = cpu_to_le32(ctrl->subsys->nr_namespaces);  
> 
> "Maximum Number of Allocated Namespaces (MNAN): This field indicates
> the maximum number of allocated namespaces."
> 
> maximum != current
> 

What exactly is the difference between 'allocated' and 'current'
namespaces in the context of nvmet?

It's not that we have _allocated_ all possible namespace. And the mnan
field was specifically designed to give a hint to the host on how many
namespaces are expected to be available to it.

Cheers,

Hannes

[PATCH 02/10] nvme: ANA transition timeout handling

[Hannes Reinecke]

On Tue, 29 May 2018 14:47:29 +0200
Christoph Hellwig <hch@lst.de> wrote:

> > +	if (ns->anagrpid != le32_to_cpu(id->anagrpid)) {
> > +		dev_warn(ctrl->device, "nsid %d ANA group id
> > changed\n",
> > +			 ns->head->ns_id);
> > +		queue_delayed_work(nvme_wq, &ctrl->ana_work, 0);
> > +	}  
> 
> No need to queue any work if an anagrpid changed.  We'll automatically
> index into the right group once it has changed.
> 
> > diff --git a/drivers/nvme/host/multipath.c
> > b/drivers/nvme/host/multipath.c index 1a8791340862..2fcaf50d84e2
> > 100644 --- a/drivers/nvme/host/multipath.c
> > +++ b/drivers/nvme/host/multipath.c
> > @@ -69,6 +69,8 @@ void nvme_failover_req(struct request *req)
> >  		 * entirely trivial..
> >  		 */
> >  		nvme_update_ana_state(ns, NVME_ANA_CHANGE);
> > +		queue_delayed_work(nvme_wq, &ns->ctrl->ana_work,
> > +				   ns->ctrl->anatt * HZ);  
> 
> This doesn't make much sense.  Once we get the ana transitioning
> status we should either retry the command up to ANATT or try another
> path.  There is no point in scheduling a read of the log page after
> ANATT, as we'll already get an AEN when that log page is ready.
> 
In an ideal world, yes.
But what happens if we don't?

> > @@ -323,7 +325,7 @@ static int nvme_process_ana_log(struct
> > nvme_ctrl *ctrl, bool groups_only) ctrl->ana_log_buf,
> > ctrl->ana_log_size, 0); if (error) {
> >  		dev_warn(ctrl->device, "Failed to get ANA log:
> > %d\n", error);
> > -		return error;
> > +		return -EIO;
> >  	}
> >  
> >  	for (i = 0; i < le16_to_cpu(ctrl->ana_log_buf->ngrps);
> > i++) { @@ -345,6 +347,8 @@ static int nvme_process_ana_log(struct
> > nvme_ctrl *ctrl, bool groups_only) dev_info(ctrl->device, "ANA
> > group %d: %s.\n", grpid, nvme_ana_state_names[desc->state]);
> >  		WRITE_ONCE(ctrl->ana_state[grpid], desc->state);
> > +		if (desc->state == NVME_ANA_CHANGE)
> > +			error = -EAGAIN;  
> 
> Huh?  Why would be stop processing our log when we see a change state?
> This looks extremely dubious to me, and does not match the changelog
> either.
> 
We don't stop processing. We just record the error so that we can
retrigger the ANA log page scan.

> > +	if (!ctrl->ana_log_buf)
> > +		return;  
> 
> How would the log buf disappear?  Even if it does please does this
> in a separate, documented patch.
> 
> > +	if (ctrl->state != NVME_CTRL_LIVE)
> > +		return;  
> 
> This looks sensible, but it should probably also check for ADMIN_LIVE
> for completeness, and be a seprate, properly documented patch.
> 
Ok, will be doing so.

> > +		/*
> > +		 * In case of an I/O error just add a small delay
> > to not hit
> > +		 * the target too hard
> > +		 */
> > +		if (ret == -EIO)
> > +			log_delay =
> > msecs_to_jiffies(NVME_ANA_LOG_DELAY);
> > +		queue_delayed_work(nvme_wq, &ctrl->ana_work,
> > log_delay);  
> 
> What is the rationale for this I/O error handling?  In NVMe over
> Fabrics transport errors tear down the association, so I really
> don't see why we should handle errors here.
> 
The idea of the patch is to start off a delayed workqueue function to
ensure we're catching ANA transition timeout errors.

The workqueue function will be cancelled if we get an AEN, but gives it
another go at reading the ANA log if the transition timeout expires.

I do agree on the EIO error, though. That can be removed.

Cheers,

Hannes

[PATCH 08/10] nvmet: Set nanagrpid correctly

[Christoph Hellwig]

On Tue, May 29, 2018@03:04:14PM +0200, Hannes Reinecke wrote:
> Yes, but it also doesn't _forbid_ us to set it to the value which is
> currently active.

It doesn't.  The maximum naming however forbids you to ever add
one that is higher than the current max one if you did that,
severly limiting the amount of dynamic changes.

> 
> Plus we already have dynamic fields in the identify output; the
> 'mnan' field is specifically for the number of allocated namespaces.
> And I can easily create new namespaces while a controller is attached.

mnan is static as well.

[PATCH 09/10] nvmet: Set mnan correctly

[Christoph Hellwig]

On Tue, May 29, 2018@03:06:35PM +0200, Hannes Reinecke wrote:
> On Tue, 29 May 2018 14:52:49 +0200
> Christoph Hellwig <hch@lst.de> wrote:
> 
> > On Tue, May 29, 2018@12:14:30PM +0200, Hannes Reinecke wrote:
> > >  	id->nn = cpu_to_le32(ctrl->subsys->max_nsid);
> > > -	id->mnan = cpu_to_le32(NVMET_MAX_NAMESPACES);
> > > +	id->mnan = cpu_to_le32(ctrl->subsys->nr_namespaces);  
> > 
> > "Maximum Number of Allocated Namespaces (MNAN): This field indicates
> > the maximum number of allocated namespaces."
> > 
> > maximum != current
> > 
> 
> What exactly is the difference between 'allocated' and 'current'
> namespaces in the context of nvmet?

NVMe differciates between allocated namespaces that exist in a given
subsystem, and attached namespaces that are attached to a given
controller.  I can't see how that distinction actually matters
here.

[PATCH 02/10] nvme: ANA transition timeout handling

[Christoph Hellwig]

On Tue, May 29, 2018@03:34:41PM +0200, Hannes Reinecke wrote:
> In an ideal world, yes.
> But what happens if we don't?

Then the whole ANA scheme doesn't work.

> We don't stop processing. We just record the error so that we can
> retrigger the ANA log page scan.

But showing a change state is in no way an error.

> The idea of the patch is to start off a delayed workqueue function to
> ensure we're catching ANA transition timeout errors.
> 
> The workqueue function will be cancelled if we get an AEN, but gives it
> another go at reading the ANA log if the transition timeout expires.

But a delayed work on it's own isn't going to work.  For each group
we need to record when it did transition to the change state, and then
check for ANATT expiry vs that particular value.  And we really don't
need to queue reading a log page for that.  We need to do this algorithm:

 1) when transitioning any group to change state set a timer to fire
   at now + ANATT
 2) when transitioning away from change state cancel the timer
 3) when the timer fires reset the controller as it is toast

Given that nvme_reset_ctrl is irq save it might be easiest to actually
use real Linux timers for that.  It sounds like a waste to have an
array of those for each group, but opencoding time expiration like
in the blk-mq eh code just seems like an invitation for subtle bugs.

[PATCH 00/10] nvme: ANA fixups

[Sagi Grimberg]

> Hi Christoph,
> 
> here's a set of patches and updates for the ANA support, both host and target.
> It's mainly small issues I've found during testing.
> Patches are relative to your nvme-ana branch.

Hannes, This is hard to review. Not sure what branch this is exactly
nor what this is incremental to?

> As usual, comments and reviews are welcome.

:(

[PATCH 00/10] nvme: ANA fixups

[Christoph Hellwig]

On Thu, May 31, 2018@01:30:55PM +0300, Sagi Grimberg wrote:
>
>> Hi Christoph,
>>
>> here's a set of patches and updates for the ANA support, both host and target.
>> It's mainly small issues I've found during testing.
>> Patches are relative to your nvme-ana branch.
>
> Hannes, This is hard to review. Not sure what branch this is exactly
> nor what this is incremental to?

Sorry we had a bit of an offlist conversation.  Here is the current
code:

	http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/nvme-ana

I'm going to repost it once I'll get a little time.