please revert a nvme-cli commit

d8f949c21e9d832149518f26c71dfd8de16cd628 does bogus blind retries on failure, and the kernel part of it was rejected. Let's sort this out properly.

Similarly at least for now bb2d87d7f386882005bb87fe9412af23c646c876 is bogus as well. We require fabrics connect to be synchronous, and changing that will require a longer discussion first.

Okay, will do.

Really ?? sigh.? I have lots of consumers that have no issues with these changes and there is nothing that acts "incompatible".?? It's been 1.5 months - where have you been?

These conditions can occur independent of any change in kernel implementation and are significant robustness corrections.

title: nvme-cli: Add ioctl retry support for "connect-all"

This should have been sorted out a long time ago. The current implementation is completely "happy path".? Failure can occur anytime when an error occurs with a discovery controller that causes a reconnect.?? This change is isolated to retrieving discovery log records and is specifically tied to an EAGAIN status - not a "blind failure".

title: nvme-cli: Wait for device file if not present after successful add_ctrl

This, in general, has little to do with kernel implementation which only influences it's likelyhood. There's always the possibility the that the creation of the /dev node in userspace takes time and the return from the create thread could beat it.? It's only more unlikely with the full finish-connect before return as that added time.

thus there's no "synchronous" requirement - what does that even mean ??? What is the difference between a controller being created then immediately failing and going into a reconnect that delays ?

-- james

I don't agree. For one so far we've guaranteed the device node appears before we return from the write to the /dev/nvme-fabrics device. If that changes it is a significant ABI break, and we need to fix that in the kernel.

And if it wasn't the fix is still wrong - we'd need to wait for it to appear using libudev APIs and/or one of the file notification syscalls rather than adding a probing loop that is in a different place than the actual open.

And for retrying the actual I/O we need to decide on the exact semantics we want to support first. Blind n time retry is always a bad idea, we need to build some sort of reliable infrastructure. Be that optionally marking requests as not failfast, and/or some sort of poll notification for a device that is ready.

what provides the guarantee that it appears first ?

Based on what I've seen there is nothing but time that guarantees it, and if that's true, the udev user side always has a potential of getting hung up and exceeding the time.

that's fine, I can certainly go look in that direction.

Ok. Given I've already had complaints that the cli shouldn't hang out for a minute or more - the time it takes for a controller to lose connectivity and fail the loss timeout, and that's not considering the cmds that probe several controllers each of which could do this sequentially resulting in a delay of many minutes - and as the user may want to kill the waiting cli, I'm of the opinion that the kernel request should be terminated not suspended. Thus the ios should continue to be failfast and let the policy be in the cli based on what the command is.? Making the cli use the poll notification to find when the controller is ready again is fine.

what should be the general policy for any command? if receive an EAGAIN (not ready) then poll until ready or device failure ????? or return failure immediately unless command is connect-all (discovery log read) then it waits for ready/failure ?

what should be the policy for the commands that probe multiple controllers ? ?? do commands fail immediately causing command to skip controllers not ready (message when does so) ??? note: the skip is already there on command failure, but if we suspend as a general policy, it would affect it.

-- james

Then we need to use the proper apis to wait for it to appear instead of just trying a few times. I suspect those are going to be either libudev or libsysfs APIs, but I'll need to look into the details.

The passthrough ioctls can set a per-command timeout, so we should set that to the time we want to wait for. And yes, we should do a killable wait. Right now the block layer doesn't offer an API for that, but we really should switch all of NVMe over to that. I can take it as an action item to add that support.

We should only ever return EAGAIN if opened with O_NONBLOCK for a start. And then preferable we should allow to poll for the controller to be ready.

I think we should have the ioctl blocking until the sent timeout (or killed) and just skip the controllers that time out. But I'd be happy to listen to other idea if they sound more sensible.