nvme: Pass malloc flags to request allocation functions

[markj]

Summary

There are some contexts where it is safe to sleep, so we should pass
M_WAITOK to ensure that a null pointer dereference can't happen.

A few places allocate with M_NOWAIT but have no way to signal an error.
Flag those with an XXX comment.

PR: 276770

[jhb]

sys/dev/nvme/nvme_ctrlr.c

823–829

Clearly the code doesn't handle failure here or in the other XXX places. I think two of them (this and the logpage command) are both due to the AER handler? That is, when submitting the initial back of AER requests during attach, it's ok to use M_WAITOK here, but presumably when an AER command is completed, the handler needs to fetch log pages and then submit a new AER request and that is where the M_NOWAIT requirement comes from?

[markj]

sys/dev/nvme/nvme_ctrlr.c

823–829

Yes, I believe request completion handlers are not sleepable, so calls via nvme_ctrlr_async_event_log_page_cb() and nvme_ctrlr_async_event_cb() must not sleep.

[jhb]

Hmm, I

sys/dev/nvme/nvme_ctrlr.c

823–829

FWIW, for nvmf(4) I use a deferred task on a taskqueue to handle AER completions (specifically fetching the log page and re-submitting a new AER command)

[imp]

sys/dev/nvme/nvme_ctrlr.c

823–829

(I had this tagged, but didn't submit it)

I was thinking we need to do the same thing here.

And we need to have comments about the XXX not just XXX

824

Why is this non sleepable?

sys/dev/nvme/nvme_ctrlr_cmd.c

263

Why is this unsleepable?

327–330

Why is this unsleepable (though maybe we use it for recovery from an interrupt context... which maybe we should defer to a thread/workqueue)

[imp]

A sleepable context to do AER and error handling would help quite a bit. We have vague plans for needing this, but if someone else implements it before me, I'll happily review the changes.

sys/dev/nvme/nvme_ctrlr_cmd.c

263

like jhb said: this is to get the log page from aer completions
It will be needed in the future too for some enhanced error reporting that we're contemplating.

[markj]

Add some info to XXX comments.

[markj]

In D47307#1079305, @imp wrote:

A sleepable context to do AER and error handling would help quite a bit. We have vague plans for needing this, but if someone else implements it before me, I'll happily review the changes.

I don't have any plan to implement that. I'm just trying to fix some of the problems reported in the bugzilla PR.

[imp]

In D47307#1079325, @markj wrote:

In D47307#1079305, @imp wrote:

A sleepable context to do AER and error handling would help quite a bit. We have vague plans for needing this, but if someone else implements it before me, I'll happily review the changes.

I don't have any plan to implement that. I'm just trying to fix some of the problems reported in the bugzilla PR.

Sounds reasonable. If you can add a comments from john and my feedback rather than the bare XXX, then I'm happy with this. I know the deferred context is well beyond the scope of this change.

[markj]

Tweak a comment some more.

[imp]

I like the first one, it's specific enough, but the other two need to be more specific since I think there's too vague.

sys/dev/nvme/nvme_ctrlr_cmd.c

264

This one should be more specific: "Currently called from AER completion processing, which is a non sleepable context"

328

The reason for this one is different: It's because we do reset from non-sleepable context and abort commands as part of that.

[markj]

Flesh out comments some more.

[imp]

Great. Thanks for doing thia and veing patient with my feedback