nvme: Close a race in destroying qpair and timeouts

[imp]

Summary

While we should have cleared all the pending I/O prior to calling
nvme_qpair_destroy, which should ensure that if the callout_drain causes
a call to nvme_qpair_timeout(), it won't schedule any new
timeout. However, it doesn't hurt to set timeout_pending to false in
nvme_qpair_destroy() and have nvme_qpair_timeout() exit early if it sees
it w/o scheduling a timeout. Since we don't otherwise stop the timeout
until we're about to destroy the qpair, this ensures we fail safe. The
lock/unlock also ensures the callout_drain will either remove the callout,
or wait for it to run with the early bailout.

Sponsored by: Netflix

[jhb]

sys/dev/nvme/nvme_qpair.c

901

This sort of thing is what callout_init_mtx is designed to solve. I'm also somewhat nervous about using mtx_initialized here. Are there really cases where that isn't true? Note that callout_drain on a non-initialized callout will break as well. Usually the best practice is to always ensure things like mutexes and callouts are initialized (e.g. doing it early when creating an object so they are always initialized even for error cases).

[imp]

fix per jhb: this will always be initialized. move callout init to make sure it is too.

[imp]

sys/dev/nvme/nvme_qpair.c

901

You are correct: it's always initialized as far as I can tell. Moved the callout init too so it too is always init'd here.

[jhb]

sys/dev/nvme/nvme_qpair.c

894

I see this is using callout_init_mtx, so actually you can just do this:

mtx_lock(&qpair->recovery);
callout_stop(&qpair->recovery);
mtx_unlock(&qpair->recovery);

You would still want a callout_drain() before the mtx_destroy though. Maybe do what you have for now, but I suspect you can remove qpair->timer_armed entirely as a followup commit and for that you would want to use callout_stop() under the lock here.

[imp]

sys/dev/nvme/nvme_qpair.c

894

I'll go ahead and commit what I have now then. It's better than what's in the tree, even if it isn't quite perfect yet.
I'll look to moving to the callout_stop in the next round.
the timer_armed stuff is for starting up the heartbeat timeout that polls the status of incomplete commands to see if they are timing out or not (rather than having one timeout per I/O, which helps in recovery), so I'm not sure I can get rid of it entirely. I'll have to think about it and look at different scenarios.