nvme: avoid hang on inaccessible paths

Quoted 5 lines

(quoted diff) +0 -1

-	/* XXX: try an inaccessible path as last resort per

8.18.3.3 */

(quoted diff) +2 -0

+	if (!ns)
+		ns = __nvme_find_path(head,

NVME_ANA_INACCESSIBLE);

This at least needs to keep a comment on why we are aiming for an inaccessible path.

Quoted 6 lines

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

@@ -165,10 +166,14 @@ static blk_qc_t
 nvme_ns_head_make_request(struct request_queue *q, 
 	srcu_idx = srcu_read_lock(&head->srcu);
 	ns = nvme_find_path(head);
-	if (likely(ns)) {
+	if (likely(ns && nvme_ns_ana_state(ns) !=

drivers/nvme/host/multipath.c static blk_qc_t nvme_ns_head_make_request(struct request_queue *q, · line 172

Quoted 3 lines

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

 NVME_ANA_INACCESSIBLE)) { bio->bi_disk = ns->disk;
 		bio->bi_opf |= REQ_NVME_MPATH;
 		ret = direct_make_request(bio);  

We should actually sometimes try to issue I/O on an inaccessible path. Please take a look at Section 8.18.3.3 in the ANA TP. Similar handling would also apply to change states, which we'd also need to cover here and above.

drivers/nvme/host/multipath.c static blk_qc_t nvme_ns_head_make_request(struct request_queue *q,

Quoted 8 lines

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

+	} else if (ns) {
+		bio->bi_status = BLK_STS_TRANSPORT;
+		bio_set_flag(bio, BIO_QUIET);
+		bio_endio(bio);
 	} else if (!list_empty_careful(&head->list)) {
 		dev_warn_ratelimited(dev, "no path available -
 requeuing I/O\n");  

But a case where we only have inaccessible / change states isn't really different from the no path available, requeing case.

Now it might make sense to have a (configurable) timeout to give up in all those case, and if my vague memory serves me right you actually volunteered to implement that a while ago.

Quoted 4 lines

Now it might make sense to have a (configurable) timeout to give up in all those case, and if my vague memory serves me right you actually volunteered to implement that a while ago.

The problem is that this particular code path is triggered for the revalidate_disk() case; when opting for requeue (as the original code did) the system will hang during revalidate_disk(), via

As this blocks the nvmf_dev_mutex we don't have any chance to connect the other, optimized path.

I had been thinking of implementing that particular handling from the ANA spec, but that would mean we're adding an ANA TT delay for each inaccessible path, which with the current defaults would mean booting is delayed by 10 seconds per inaccessible path.

From bf729292705351639aa1eac1bde2e1afc25f11b7 Mon Sep 17 00:00:00 2001

And I guess this where the real issue starts. We should not do block I/O under nvmf_dev_mutex.

James has some work on asynchronous connects pending for FC, so I guess we could look into generalizing that and always exectute the real connect work in a different thread.

Quoted 6 lines

And I guess this where the real issue starts. We should not do block I/O under nvmf_dev_mutex.

James has some work on asynchronous connects pending for FC, so I guess we could look into generalizing that and always exectute the real connect work in a different thread.

I still need to ramp up on the ANA patches, but I'd prefer moving create_ctrl outside of nvmf_dev_mutex instead of having it async.

Quoted 7 lines · On Thu, 31 May 2018 02:10:35 +0300 Sagi Grimberg <[email protected]> wrote:

Quoted 6 lines

And I guess this where the real issue starts. We should not do block I/O under nvmf_dev_mutex.

James has some work on asynchronous connects pending for FC, so I guess we could look into generalizing that and always exectute the real connect work in a different thread.

Quoted 2 lines

I still need to ramp up on the ANA patches, but I'd prefer moving create_ctrl outside of nvmf_dev_mutex instead of having it async.

We'd need the async patches still, as they provide a way on how we can call nvme-cli directly from within udev rules.
Without those patches nvme-cli might get stuck (as this issue neatly proves) and would block udev from processing events.

Quoted 6 lines

We'd need the async patches still, as they provide a way on how we can call nvme-cli directly from within udev rules.
Without those patches nvme-cli might get stuck (as this issue neatly proves) and would block udev from processing events.

So just run nvme-cli in the background from udev. Or if udev really is too stupid for that have an option to fork and run in the background in nvme-cli itself..

From udev man:

RUN{type}

...
This can only be used for very short-running foreground tasks. Running an event process for a long period of time may block all further events for this or a dependent device.

Starting daemons or other long running processes is not appropriate for udev; the forked processes, detached or not, will be unconditionally killed after the event handling has finished.

One way around this is to disown the command from udev shell (e.g. $cmd & disown)

Looks like __nvme_find_path is pretty basic one.
Let's say if there are 2 NVME_ANA_OPTIMIZED paths. However it looks like the first found path is chosen. That means having more than one NVME_ANA_OPTIMIZED paths doesn't help much. Any plans to round robin?

Quoted 5 lines

(quoted diff) +0 -1

-	/* XXX: try an inaccessible path as last resort per

8.18.3.3 */

(quoted diff) +2 -0

+	if (!ns)
+		ns = __nvme_find_path(head,

NVME_ANA_INACCESSIBLE);

Also if there are multiple NVME_ANA_INACCESSIBLE paths and only a few(or one) is changing ANA state from INACCESSIBLE to OPTIMIZED and if that happens to be the last entry in your list, then trying above doesn't really help.