Skip to contentSkip to conversation

nvme: ANA fixups

PATCH New Hannes Reinecke 4 participants 32 messages May 29, 2018 – May 31, 2018

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover nvme: ANA fixups

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(-)
Signature · 1 line
2.12.3
#1 lore raw

PATCH 1/10 nvme: add missing kfree() in nvme_configure_ana()

Signed-off-by Hannes Reinecke <hare at suse.com>
---
 drivers/nvme/host/multipath.c | 1 +
 1 file changed, 1 insertion(+)
drivers/nvme/host/multipath.c +1 -0
index 0d3ef3098063..1a8791340862 100644
@@ -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;
 }
 
Signature · 1 line
2.12.3
#2 lore raw

PATCH 1/10 nvme: add missing kfree() in nvme_configure_ana()

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>

Signature · 6 lines
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
#4 lore raw

PATCH 2/10 nvme: ANA transition timeout handling

In reply to 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(-)
drivers/nvme/host/core.c +9 -2
index b1e4f56a55ed..1be424053f47 100644
@@ -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);
drivers/nvme/host/multipath.c +27 -6
index 1a8791340862..2fcaf50d84e2 100644
@@ -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)
drivers/nvme/host/nvme.h +3 -1
index 4a29c736370f..53cb7f8a6267 100644
@@ -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;
Signature · 1 line
2.12.3
#5 lore raw

PATCH 2/10 nvme: ANA transition timeout handling

drivers/nvme/host/core.c static int nvme_revalidate_disk(struct gendisk *disk) · line 1523

Quoted 5 lines
drivers/nvme/host/core.c +5 -0
+	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.

drivers/nvme/host/multipath.c void nvme_failover_req(struct request *req) · line 73

Quoted 10 lines
drivers/nvme/host/multipath.c +2 -0
index 1a8791340862..2fcaf50d84e2 100644
@@ -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.

Quoted 10 lines
drivers/nvme/host/multipath.c +1 -1
@@ -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++) {

drivers/nvme/host/multipath.c static int nvme_process_ana_log(struct nvme_ctrl *ctrl, bool groups_only) · line 351

Quoted 5 lines
drivers/nvme/host/multipath.c +2 -0
@@ -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.

drivers/nvme/host/multipath.c static int nvme_process_ana_log(struct nvme_ctrl *ctrl, bool groups_only) · line 353

Quoted 2 lines
drivers/nvme/host/multipath.c +2 -0
+	if (!ctrl->ana_log_buf)
+		return;

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

drivers/nvme/host/multipath.c static int nvme_process_ana_log(struct nvme_ctrl *ctrl, bool groups_only) · line 391

Quoted 2 lines
drivers/nvme/host/multipath.c +2 -0
+	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.

drivers/nvme/host/multipath.c static int nvme_process_ana_log(struct nvme_ctrl *ctrl, bool groups_only) · line 402

Quoted 7 lines
drivers/nvme/host/multipath.c +7 -0
+		/*
+		 * 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.

#6 lore raw

PATCH 2/10 nvme: ANA transition timeout handling

Quoted 10 lines · On Tue, 29 May 2018 14:47:29 +0200 Christoph Hellwig <[email protected]> wrote:

drivers/nvme/host/core.c static int nvme_revalidate_disk(struct gendisk *disk) · line 1523

Quoted 6 lines
(quoted diff) +2 -0
+	if (ns->anagrpid != le32_to_cpu(id->anagrpid)) {
+		dev_warn(ctrl->device, "nsid %d ANA group id

changed\n",

drivers/nvme/host/core.c +3 -0
+			 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.

Quoted 16 lines

drivers/nvme/host/multipath.c void nvme_failover_req(struct request *req) · line 73

Quoted 10 lines
(quoted diff) +0 -0

b/drivers/nvme/host/multipath.c index 1a8791340862..2fcaf50d84e2 100644 --- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c

drivers/nvme/host/multipath.c +2 -0
@@ -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?

Quoted 9 lines
Quoted 9 lines
drivers/nvme/host/multipath.c +1 -1
@@ -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;
 	}
 
Quoted 12 lines

drivers/nvme/host/multipath.c static int nvme_process_ana_log(struct nvme_ctrl *ctrl, bool groups_only) · line 351

Quoted 7 lines

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]);

drivers/nvme/host/multipath.c +2 -0
 		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.

Quoted 6 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+	if (!ctrl->ana_log_buf)
+		return;  

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

Quoted 6 lines

drivers/nvme/host/multipath.c static int nvme_process_ana_log(struct nvme_ctrl *ctrl, bool groups_only) · line 391

Quoted 2 lines
drivers/nvme/host/multipath.c +2 -0
+	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.

Quoted 6 lines
Quoted 6 lines
(quoted diff) +2 -0
+		/*
+		 * In case of an I/O error just add a small delay

to not hit

drivers/nvme/host/multipath.c +3 -0
+		 * the target too hard
+		 */
+		if (ret == -EIO)
Quoted 9 lines
Quoted 4 lines
(quoted diff) +1 -0
+			log_delay =

msecs_to_jiffies(NVME_ANA_LOG_DELAY);

(quoted diff) +1 -0
+		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

#7 lore raw

PATCH 2/10 nvme: ANA transition timeout handling

Quoted 2 lines · Hannes Reinecke

In an ideal world, yes.
But what happens if we don't?

Then the whole ANA scheme doesn't work.

Quoted 2 lines

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.

Quoted 5 lines

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.

#8 lore raw

PATCH 3/10 nvme: Only update capacity for optimized or non-optimized paths

In reply to 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(-)
drivers/nvme/host/core.c +3 -1
index 1be424053f47..cc43bb685e63 100644
@@ -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);
 }
drivers/nvme/host/multipath.c +0 -9
index 2fcaf50d84e2..a5f43d164b19 100644
@@ -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",
drivers/nvme/host/nvme.h +13 -0
index 53cb7f8a6267..8572f106d6ac 100644
@@ -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
Signature · 1 line
2.12.3
#9 lore raw

PATCH 3/10 nvme: Only update capacity for optimized or non-optimized paths

Quoted 6 lines · 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.

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

Quoted 76 lines
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(-)
drivers/nvme/host/core.c +3 -1
index 1be424053f47..cc43bb685e63 100644
@@ -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);
 }
drivers/nvme/host/multipath.c +0 -9
index 2fcaf50d84e2..a5f43d164b19 100644
@@ -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",
drivers/nvme/host/nvme.h +13 -0
index 53cb7f8a6267..8572f106d6ac 100644
@@ -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
Signature · 1 line
2.12.3

---end quoted text---

#10 lore raw

PATCH 4/10 nvme: clear current path on ANA state change

In reply to 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(+)
drivers/nvme/host/multipath.c +1 -0
index a5f43d164b19..a520582811f8 100644
@@ -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)
Signature · 1 line
2.12.3
#11 lore raw

PATCH 4/10 nvme: clear current path on ANA state change

Quoted 13 lines · 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(+)
drivers/nvme/host/multipath.c +0 -0
index a5f43d164b19..a520582811f8 100644

drivers/nvme/host/multipath.c void nvme_set_disk_name(char *disk_name, struct nvme_ns *ns, · line 47

Quoted 4 lines
drivers/nvme/host/multipath.c +1 -0
@@ -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

Signature · 6 lines
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
#12 lore raw

PATCH 4/10 nvme: clear current path on ANA state change

Quoted 9 lines · On Tue, 29 May 2018 14:22:12 +0200 "Johannes Thumshirn" <[email protected]>…
Quoted 8 lines · 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(+)
Quoted 16 lines

drivers/nvme/host/multipath.c void nvme_set_disk_name(char *disk_name, struct · line 47

Quoted 9 lines
(quoted diff) +0 -0

b/drivers/nvme/host/multipath.c index a5f43d164b19..a520582811f8 100644 --- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c

drivers/nvme/host/multipath.c +1 -0
@@ -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

#13 lore raw

PATCH 4/10 nvme: clear current path on ANA state change

In reply to Hannes Reinecke
Quoted 2 lines · Hannes Reinecke

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.

#14 lore raw

PATCH 5/10 nvme: retry nvme_get_log_ext() when processing ANA log

In reply to 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(-)
drivers/nvme/host/multipath.c +8 -1
index a520582811f8..73bd4ff6ab06 100644
@@ -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;
 	}
Signature · 1 line
2.12.3
#15 lore raw

PATCH 5/10 nvme: retry nvme_get_log_ext() when processing ANA log

Quoted 2 lines · Hannes Reinecke

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

Why?

#16 lore raw

PATCH 6/10 nvme: simplify check for ANA in nvme_ns_id_attrs_are_visible()

In reply to 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(-)
drivers/nvme/host/core.c +1 -1
index cc43bb685e63..194cce7e63ce 100644
@@ -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
Signature · 1 line
2.12.3
#17 lore raw

PATCH 6/10 nvme: simplify check for ANA in nvme_ns_id_attrs_are_visible()

Quoted 3 lines · 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.

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

#18 lore raw

PATCH 7/10 nvmet: make ANATT configurable

In reply to 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(-)
drivers/nvme/target/admin-cmd.c +1 -1
index c2fba1c92cd8..f2eedb13482a 100644
@@ -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);
 
drivers/nvme/target/configfs.c +28 -0
index ddc3fa9fd92f..2c6de9961ef4 100644
@@ -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,
 };
 
drivers/nvme/target/core.c +1 -0
index 1301a7e8b354..f47916481d8b 100644
@@ -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) {
drivers/nvme/target/nvmet.h +2 -0
index 7195e313b375..04dec38ffe74 100644
@@ -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
Signature · 1 line
2.12.3
#19 lore raw

PATCH 7/10 nvmet: make ANATT configurable

Quoted 17 lines · Hannes Reinecke
drivers/nvme/target/configfs.c +9 -0
index ddc3fa9fd92f..2c6de9961ef4 100644
@@ -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,

drivers/nvme/target/configfs.c static ssize_t nvmet_subsys_attr_serial_store(struct config_item *item, · line 759

Quoted 10 lines
drivers/nvme/target/configfs.c +10 -0
+					     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.

#20 lore raw

PATCH 8/10 nvmet: Set nanagrpid correctly

In reply to 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(-)
drivers/nvme/target/admin-cmd.c +22 -7
index f2eedb13482a..a9d05eb03117 100644
@@ -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.
Signature · 1 line
2.12.3
#21 lore raw

PATCH 8/10 nvmet: Set nanagrpid correctly

Quoted 1 line · Hannes Reinecke

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.

#22 lore raw

PATCH 8/10 nvmet: Set nanagrpid correctly

Quoted 9 lines · On Tue, 29 May 2018 14:51:51 +0200 Christoph Hellwig <[email protected]> wrote:
Quoted 2 lines · Hannes Reinecke

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

#23 lore raw

PATCH 8/10 nvmet: Set nanagrpid correctly

Quoted 2 lines · Hannes Reinecke

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.

Quoted 4 lines

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.

#24 lore raw

PATCH 9/10 nvmet: Set mnan correctly

In reply to 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(-)
drivers/nvme/target/admin-cmd.c +1 -1
index a9d05eb03117..99592c7b8bc7 100644
@@ -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);
 
Signature · 1 line
2.12.3
#25 lore raw

PATCH 9/10 nvmet: Set mnan correctly

drivers/nvme/target/admin-cmd.c static void nvmet_execute_identify_ctrl(struct nvmet_req *req) · line 304

Quoted 3 lines · Hannes Reinecke
drivers/nvme/target/admin-cmd.c +1 -1
 	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

#26 lore raw

PATCH 9/10 nvmet: Set mnan correctly

Quoted 10 lines · On Tue, 29 May 2018 14:52:49 +0200 Christoph Hellwig <[email protected]> wrote:

drivers/nvme/target/admin-cmd.c static void nvmet_execute_identify_ctrl(struct nvmet_req *req) · line 304

Quoted 3 lines · Hannes Reinecke
drivers/nvme/target/admin-cmd.c +1 -1
 	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

#27 lore raw

PATCH 9/10 nvmet: Set mnan correctly

Quoted 11 lines · Hannes Reinecke
Quoted 8 lines · On Tue, 29 May 2018 14:52:49 +0200 Christoph Hellwig <[email protected]> wrote:

drivers/nvme/target/admin-cmd.c static void nvmet_execute_identify_ctrl(struct nvmet_req *req) · line 304

Quoted 3 lines · Hannes Reinecke
drivers/nvme/target/admin-cmd.c +1 -1
 	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."

Quoted 5 lines
Quoted 2 lines

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.

#28 lore raw

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

In reply to 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(+)
drivers/nvme/target/admin-cmd.c +12 -0
index 99592c7b8bc7..0ae25478e3eb 100644
@@ -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.
Signature · 1 line
2.12.3
#29 lore raw

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

Quoted 2 lines · 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.

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

Quoted 6 lines
drivers/nvme/target/admin-cmd.c +0 -0
@@ -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);
 

drivers/nvme/target/admin-cmd.c static void nvmet_execute_identify_ns(struct nvmet_req *req) · line 383

Quoted 10 lines
drivers/nvme/target/admin-cmd.c +10 -0
+	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;
	}
maintainer
#30 lore raw

cover nvme: ANA fixups

In reply to Hannes Reinecke
Quoted 6 lines

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?

Quoted 1 line

As usual, comments and reviews are welcome.

:(

#31 lore raw

cover nvme: ANA fixups

Quoted 9 lines · Sagi Grimberg
Quoted 6 lines

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.