RFC: what to do about abort?

I've recently spent some time on the Abort implementation for the Fabrics host and target drivers, and given how vaguele Abort is defined in the spec, I fail to see how sending an abort actually buys us anything. The controller doesn't even have to respond, and in fact many don't, so we simply end up escalation to a reset after the abort command times out. Similarly the amount of abort commands is severly limited, and I've not seen a controller exceeding the recommended 4 allowed abort commands, leading to sever pressure on the error handling code for the typical case of a somehow stuck controller that has multiple outstanding commands beyoned the allowed timeout.

Based on that I came to the conclusion that we'd be much better off to just use the 60 second timeout for I/O command as well, and simply avoid ever sending the abort command. RFC patch below:

index 2de248b..21b10ed 100644
@@ -38,7 +38,7 @@ module_param(admin_timeout, byte, 0644);
 MODULE_PARM_DESC(admin_timeout, "timeout in seconds for admin commands");
 EXPORT_SYMBOL_GPL(admin_timeout);
 
-unsigned char nvme_io_timeout = 30;
+unsigned char nvme_io_timeout = 60;
 module_param_named(io_timeout, nvme_io_timeout, byte, 0644);
 MODULE_PARM_DESC(io_timeout, "timeout in seconds for I/O");
 EXPORT_SYMBOL_GPL(nvme_io_timeout);

@@ -1100,7 +1100,6 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 
 	ctrl->vid = le16_to_cpu(id->vid);
 	ctrl->oncs = le16_to_cpup(&id->oncs);
-	atomic_set(&ctrl->abort_limit, id->acl + 1);
 	ctrl->vwc = id->vwc;
 	ctrl->cntlid = le16_to_cpup(&id->cntlid);
 	memcpy(ctrl->serial, id->sn, sizeof(id->sn));

index 114b928..4ea0914 100644
@@ -102,7 +102,6 @@ struct nvme_ctrl {
 	u32 stripe_size;
 	u16 oncs;
 	u16 vid;
-	atomic_t abort_limit;
 	u8 event_limit;
 	u8 vwc;
 	u32 vs;

Failure to respond to an abort command?
IE you send the abort and never ever get a completion for the abort? Uh-oh.
How would be able to figure out if the card has been able to process the abort?

Doesn't really help if the command has been dropped into a black hole somewhere on the way, right?
In general you _do_ want to handle aborts; there might be long-running commands or the array might be genuinely stuck. In which case you do want to send an abort to inform the array that it should stop processing the command.

If and how the aborts are handled from the initiator side is another story, but for the transport you do need them.
And increasing the timeout is just deferring the issue to another time; why should any command return within the increased timeout, if it already failed to return within the original timeout?

Cheers,

Hannes

Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare at suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)

The NVMe spec is extremely vague about Abort doing anything:

"An Abort command is a best effort command; the command to abort may
have already completed, currently be in execution, or may be deeply queued. It is implementation specific if/when a controller chooses to complete the command when the command to abort is not found."

And various controllers make full use of the ambiguity offered.

We don't. Once the abort command itself times out we finally reset the controller. And I've not seen a Abort command doing anything but timing out in the wild.

It does. We still end up resetting the controller about 60 seconds after the black hole appeared, but we put a whole lot less stress on the host and controller in the meantime.

There might be special cases where we want to abort a long running command without touching the rest of the controller state. However for the current NVMe driver there are no such long running commands except for the asynchronous event requests (which never respond to aborts anyway in practice), and there is no infrastructure to abort commands except for from the timeout handler. This mail and patch should not be interpreted as blocking that use case of abort in the long run.

Which transport? NVMe aborts are protocol level aborts that are just another command as far as the transport is concerned.

No good reason. I'm just trying to keep existing behavior as much as possible, and the existing behavior is:

60 second timeouts for admin command, then reset the controller 30 second timeouts for I/O commands, then:

a) send a abort command if under the abort limit
b) reset the timer for another 30 seconds

The I/O command behavior essentially is a 60 to 90 second timeout due to the way aborts are actually implemented. Setting a consistent 60 second timeout and do the deterministic reset will provide us with a much more consistent behavior, and put a lot less stress on the host and controller in the even of the timeout.

That's an unfortunate way to the spec describes this command. All my controllers immediately post success status with the CQE's DWORD0 set to 0;abort is essentially a no-op.

Even though it's a no-op for me, the main benefit is that abort makes for an awesome trigger for analyzers and firmware. It's been a valuable tool for debugging issues, and I'd hate to lose it.

Can we propose changing the spec instead?

That's one of the most common behaviors instead.

You're the maintainer, so it's up to you - it just doesn't seem very useful to me from both the host and device perspectice.

Good luck changing existing NVMe behavior. We could probably introduce an optional well defined abort, but I'm not sure lots of vendors would even bother to implement it unless there is a hard requirement for it.

If you don't find a deterministic event on failure to be useful for the device side, you must have better visibility into your internal device state than most others do. :)

The spec so badly defines abort, though, I have to agree you're right to remove it. Since having a dependable event greatly helps debugging field issues when device makers desperately want to blame the driver for command timeouts, does it sound okay to have a module param toggle its use, defaulting to off?

I get a deterministic error: the controller reset.

Let's just keep it. I don't think it's helpful, but it's not that bad that we have to get rid of it.