nvme: ANATT handling

Needs a description. Especially to explain why you want it per-port. In the end the transition timeout is something determined by the failover backend, which in no way maps to a port. In fact it doesn't map to anything, which is why I would suggest to just make it global. This still won't cover the case of using different backed storage types with different failover characteristics, but at least it ?sn't too confusing.

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

Quoted 3 lines

drivers/nvme/target/configfs.c +3 -0

+	ret = kstrtou8(page, 0, &ana_tt);
+	if (ret || ana_tt == 0)
+		return -EINVAL;  

Please propagate the actual error returned by kstrtou8.

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

Quoted 9 lines

drivers/nvme/target/configfs.c +3 -0

+
+	if (port->enabled) {
+		pr_err("Cannot modify ANA transition timeout while

enabled\n");

drivers/nvme/target/configfs.c +5 -0

+		return -EACCES;
+	}
+	down_write(&nvmet_config_sem);
+	port->anatt = ana_tt;
+	up_write(&nvmet_config_sem);  

The enabled check needs to be under nvmet_config_sem to be race free.

Whenever an ANA state change is triggered the ANA state for that group ID is set to 'state change' and a delayed work is started, which will be setting the port to the actual state after anatt has expired.

Quoted 4 lines · Hannes Reinecke

Whenever an ANA state change is triggered the ANA state for that group ID is set to 'state change' and a delayed work is started, which will be setting the port to the actual state after anatt has expired.

What is the point? We want to move to the actual state as soon as we can, where as soon is defined by a backend. So we need manual control over exact states, including change state.

Even for testing it makes much more sense to have entirely manual control, so that your userspace test framework can trivial inject too long transition faults.

But we would _require_ userspace interaction to facilitate correct operation. And with a purely manual configuration on the target any ANATT value becomes meaningless as we have no way of enforcing it.

drivers/nvme/target/core.c +3 -0

+	INIT_DELAYED_WORK(&ev->work, nvmet_ana_state_change_work);
+	ev->port = port;
+	ev->grpid = grpid;

drivers/nvme/target/core.c +3 -0

+}
+
  int nvmet_register_transport(const struct nvmet_fabrics_ops *ops)
  {
  	int ret = 0;

@@ -276,11 +336,22 @@ int nvmet_enable_port(struct nvmet_port *port)
  void nvmet_disable_port(struct nvmet_port *port)
  {
  	const struct nvmet_fabrics_ops *ops;
+	struct nvmet_ana_change_event *ev, *tmp;
 

drivers/nvme/target/core.c +4 -0

+		}
+	}
+	mutex_unlock(&port->anatt_list_lock);
+
  	ops = nvmet_transports[port->disc_addr.trtype];
  	ops->remove_port(port);
  	module_put(ops->owner);

drivers/nvme/host/core.c static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid) · line 3029

Quoted 1 line

drivers/nvme/host/core.c +1 -0

+	timer_setup(&ns->anatt_timer, nvme_anatt_timedout, 0);  

ANATT is per-group, so having a timer in the namespace is wrong.

Quoted 4 lines · On Thu, 7 Jun 2018 14:09:41 +0200 Christoph Hellwig <[email protected]> wrote:

drivers/nvme/host/core.c static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid) · line 3029

Quoted 1 line

drivers/nvme/host/core.c +1 -0

+	timer_setup(&ns->anatt_timer, nvme_anatt_timedout, 0);  

ANATT is per-group, so having a timer in the namespace is wrong.

But would requiring us to create a separate ana group structure on the host. Could be looking into that if you insist, but this implementation is way easier.

Add a timer for tracking ANA transition timeout, and reset the controller once the timeout expires.

Signed-off-by Hannes Reinecke <hare at suse.com>

---
  drivers/nvme/host/core.c      |  3 +++
  drivers/nvme/host/multipath.c | 34 ++++++++++++++++++++++++++++------
  drivers/nvme/host/nvme.h      |  6 ++++++
  3 files changed, 37 insertions(+), 6 deletions(-)

drivers/nvme/host/core.c +3 -0

index e62de51209b2..3b578ae62cb5 100644
@@ -2378,6 +2378,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);
 

@@ -3025,6 +3026,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
  	blk_queue_flag_set(QUEUE_FLAG_NONROT, ns->queue);
  	ns->queue->queuedata = ns;
  	ns->ctrl = ctrl;
+	timer_setup(&ns->anatt_timer, nvme_anatt_timedout, 0);
 
  	kref_init(&ns->kref);
  	ns->lba_shift = 9; /* set to a default value for 512 until disk is validated */

@@ -3113,6 +3115,7 @@ static void nvme_ns_remove(struct nvme_ns *ns)
  			blk_integrity_unregister(ns->disk);
  	}
 
+	del_timer_sync(&ns->anatt_timer);
  	mutex_lock(&ns->ctrl->subsys->lock);
  	nvme_mpath_clear_current_path(ns);
  	list_del_rcu(&ns->siblings);

drivers/nvme/host/multipath.c +0 -2

index d30229a21c97..7a37f5959ad4 100644
@@ -62,13 +62,12 @@ void nvme_failover_req(struct request *req)
  	 */
  	switch (nvme_req(req)->status & 0x7ff) {
  	case NVME_SC_ANA_TRANSITION:
-		/*
-		 * XXX: We should verify the controller doesn't die on during

drivers/nvme/host/multipath.c +4 -0

  		break;
  	case NVME_SC_ANA_PERSISTENT_LOSS:
  		nvme_update_ana_state(ns, NVME_ANA_PERSISTENT_LOSS);

@@ -390,11 +389,34 @@ static int nvme_process_ana_log(struct nvme_ctrl *ctrl, bool groups_only)
  static void nvme_ana_work(struct work_struct *work)
  {
  	struct nvme_ctrl *ctrl = container_of(work, struct nvme_ctrl, ana_work);
+	struct nvme_ns *ns;
 
  	nvme_process_ana_log(ctrl, false);
+	down_write(&ctrl->namespaces_rwsem);
+	list_for_each_entry(ns, &ctrl->namespaces, list) {
+		enum nvme_ana_state state = nvme_ns_ana_state(ns);

Quoted 6 lines

drivers/nvme/host/multipath.c +0 -6

-		/*
-		 * XXX: We should verify the controller doesn't die on during
-		 * the transition.  But that means we per-group timeout from
-		 * when we first hit the change state, so this won't be
-		 * entirely trivial..
-		 */

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

Quoted 6 lines

  		nvme_update_ana_state(ns, NVME_ANA_CHANGE);

drivers/nvme/host/multipath.c +5 -0

+		if (!timer_pending(&ns->anatt_timer)) {
+			ns->anatt_timer.expires =
+				ns->ctrl->anatt * HZ + jiffies;
+			add_timer(&ns->anatt_timer);
+		}

Shouldn't this del and re-add the timer if its pending?

drivers/nvme/host/multipath.c +2 -1

 	case NVME_SC_ANA_PERSISTENT_LOSS:
-		nvme_update_ana_state(ns, NVME_ANA_PERSISTENT_LOSS);
+		if (ana_state != NVME_ANA_PERSISTENT_LOSS) {
+			nvme_update_ana_state(ns, NVME_ANA_PERSISTENT_LOSS);

Quoted 5 lines

drivers/nvme/host/multipath.c +1 -2

+	ana_state = READ_ONCE(ns->ctrl->ana_state[ns->anagrpid]);
 	switch (nvme_req(req)->status & 0x7ff) {
 	case NVME_SC_ANA_TRANSITION:
-		nvme_update_ana_state(ns, NVME_ANA_CHANGE);
-		if (!timer_pending(&ns->anatt_timer)) {

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

Quoted 6 lines

drivers/nvme/host/multipath.c +3 -3

-			ns->anatt_timer.expires =
-				ns->ctrl->anatt * HZ + jiffies;
-			add_timer(&ns->anatt_timer);
+		if (ana_state != NVME_ANA_CHANGE) {
+			nvme_update_ana_state(ns, NVME_ANA_CHANGE);
+			ana_state_changed = true;  

Needs an xchg operation to be atomic.

Quoted 12 lines

(quoted diff) +0 -1

 	case NVME_SC_ANA_PERSISTENT_LOSS:
-		nvme_update_ana_state(ns,

NVME_ANA_PERSISTENT_LOSS);

(quoted diff) +2 -0

+		if (ana_state != NVME_ANA_PERSISTENT_LOSS) {
+			nvme_update_ana_state(ns,

NVME_ANA_PERSISTENT_LOSS);

drivers/nvme/host/multipath.c +3 -1

+			ana_state_changed = true;
+		}
 		break;
 	case NVME_SC_ANA_INACCESSIBLE:
-		nvme_update_ana_state(ns, NVME_ANA_INACCESSIBLE);
+		if (ana_state != NVME_ANA_INACCESSIBLE) {

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

Quoted 5 lines

(quoted diff) +1 -0

+			nvme_update_ana_state(ns,

NVME_ANA_INACCESSIBLE);

drivers/nvme/host/multipath.c +2 -0

+			ana_state_changed = true;
+		}
 		break;  

We don't need to start the ANA timer for these states, it should only happen for the CHANGE state.

Quoted 3 lines

We don't need to start the ANA timer for these states, it should only happen for the CHANGE state.

Hmm. When we're getting these error it means that there is a divergence between the ANA state on the host and the target.

Which really shouldn't happen, as we _should_ have received an ANA AEN signalling us this state. As there is a race condition here (ANA AENs are being send via the admin connection, and it might be that we haven't gotten around to processing the ANA state yet) we should be starting the ANATT timer to catch any outstanding AENs.

And if we're not getting an ANA AEN within ANATT the controller should be considered hosed, and we need to reset it.

If you disagree, what _should_ be the appropriate handling in this case, ie we have the most current ANA log page, _and_ get this error indicating a state mismatch?

Quoted 4 lines · Hannes Reinecke

Quoted 3 lines

We don't need to start the ANA timer for these states, it should only happen for the CHANGE state.

Quoted 8 lines

Hmm. When we're getting these error it means that there is a divergence between the ANA state on the host and the target.

Which really shouldn't happen, as we _should_ have received an ANA AEN signalling us this state. As there is a race condition here (ANA AENs are being send via the admin connection, and it might be that we haven't gotten around to processing the ANA state yet) we should be starting the ANATT timer to catch any outstanding AENs.

We should receive an AEN, yes. But there is absolutely no ordering guarantees either in the ANA spec, or in NVMe in general for multiple queues (admin vs I/O). But that was intentional in the specification, and has nothing to do with ANATT at all.

If no controllers are reporting ANA Optimized state or ANA Non-Optimized state, then a transition may be occurring such that a controller reporting the Inaccessible state may become accessible and the host should retry the command on the controller reporting Inaccessible state for at least ANATT seconds (refer to Figure 109).

Quoted 2 lines

And if we're not getting an ANA AEN within ANATT the controller should be considered hosed, and we need to reset it.

I can't find anything in the spec saying that.

Quoted 3 lines

If you disagree, what _should_ be the appropriate handling in this case, ie we have the most current ANA log page, _and_ get this error indicating a state mismatch?

Update our in-memory state ASAP, and then just way for the AEN to happen eventually. As the current code does.

Precisely.
Looking closer, there are not even guarantees within which time any target should be sending the AEN.
So we have to start somewhere when defining a sensible timeframe during which we should have received an ANA AEN. And the one timer we already have is ANATT, so it looked logical to use that.

Especially due to this paragraph:

Quoted 5 lines

If no controllers are reporting ANA Optimized state or ANA Non-Optimized state, then a transition may be occurring such that a controller reporting the Inaccessible state may become accessible and the host should retry the command on the controller reporting Inaccessible state for at least ANATT seconds (refer to Figure 109).

which seems to imply to me that we can have an implicit transition on the target while reporting the inaccessible state error, and the use of ANATT here is indeed applicable.

This code is essentially error handling.

On Thu, Jun 07, 2018@03:20:36PM +0200, Hannes Reinecke wrote:

Quoted 9 lines

Especially due to this paragraph:

Quoted 6 lines

If no controllers are reporting ANA Optimized state or ANA Non-Optimized state, then a transition may be occurring such that a controller reporting the Inaccessible state may become accessible and the host should retry the command on the controller reporting Inaccessible state for at least ANATT seconds (refer to Figure 109).

Quoted 3 lines

which seems to imply to me that we can have an implicit transition on the target while reporting the inaccessible state error, and the use of ANATT here is indeed applicable.

But we'll still get the AEN. The whole point is the above is that if we had a queue_if_no_path=0 mode we should not give up before ANATT.

There is no guarantee that we will get the AEN.
Yes, the spec says we should, but it's just a single message which might easily be lost if the corresponding frame is dropped. Think of FC, which supposedly is a lossless fabric, yet a sizeable piece of the spec deals with recovery from lost frames. It _will_ happen.

Quoted 7 lines

There is no guarantee that we will get the AEN.
Yes, the spec says we should, but it's just a single message which might easily be lost if the corresponding frame is dropped. Think of FC, which supposedly is a lossless fabric, yet a sizeable piece of the spec deals with recovery from lost frames. It _will_ happen.

NVMe requires a reliable transport. The Fabrics spec very explicitly states that, and PCIe provides the gurantees as well.

If we lose random admin command replies we have much, much deeper problems. E.g. if we lose the AEN reply we would never get any other AEN ever because we are not going to resubmit the command.

If I/O is terminated due to an invalid ANA state we should be starting the ANATT timer as we really should have received an ANA AEN signalling the state change.

Signed-off-by Hannes Reinecke <hare at suse.com>

---
  drivers/nvme/host/multipath.c | 36 +++++++++++++++++++++++++++++-------
  1 file changed, 29 insertions(+), 7 deletions(-)

drivers/nvme/host/multipath.c +3 -0

index 7a37f5959ad4..7f34ae260ca9 100644
@@ -49,7 +49,10 @@ static void nvme_update_ana_state(struct nvme_ns *ns, enum nvme_ana_state state)
  void nvme_failover_req(struct request *req)
  {
  	struct nvme_ns *ns = req->q->queuedata;
+	struct device *dev = disk_to_dev(ns->disk);
  	unsigned long flags;
+	enum nvme_ana_state ana_state;
+	bool ana_state_changed = false;
 
  	spin_lock_irqsave(&ns->head->requeue_lock, flags);
  	blk_steal_bios(&ns->head->requeue_list, req);

@@ -60,26 +63,45 @@ void nvme_failover_req(struct request *req)
  	 * Reset the controller for any non-ANA error as we don't know what