Create xpt_sim_poll, refactor and use it in mps / mpr.

[imp]

Summary

cam: create xpt_sim_poll and refactor

xpt_sim_poll takes the sim to poll as an argument. It will do the
proper locking protocol, call the SIM polling routine, and then call
camisr_runqueue to process completions on any CCBs the SIM's poll
routine completed. It will be used during late shutdown when a SIM is
waiting for CCBs it sent during shutdown to finish and the scheduler
isn't running because we've panic'd.

This sequence was used twice in cam_xpt, so refactor those to use this
new function.

mps: Call xpt_sim_poll in shutdown_final handler.

When we're shutting down, we send a number of start/stop commands to
the known targets. We have to wait for them to complete. During a
panic, the interrupts are off, and using pause to wait for them to
fire and complete won't work: we have to poll after pause returns so
the completion routines of the CCBs run so we decrement work
outstanding counts.

mpr: Port the mps panic-safe shutdown_final handling

r330951 by smh fixed the mps driver to avoid deadlocks when panicing.
The same code is needed for mpr, so port it here, along with the fix
which allows the CCBs scheduled to complete avoiding at least a scary
message and likely other unintended consequences.

[cem]

sys/cam/cam_xpt.c

3214

I don't know about the prevalent style in the file, but this can just be sim->sim_poll(sim); in C. The extra parens and nominal dereference are functionally unnecessary boilerplate.

(I don't recall style(9) providing any particular guidance on function pointer invocation either way, and especially if this is the dominant style in CAM, I'm ok with it as-is.)

(Also, now I see that this was just moved up from below, and that's another justification for just leaving it as-is.)

3261

BDE hat: It's kind of curious that timeout is in units of 0.1 ms. Not related to this change.

[imp]

sys/cam/cam_xpt.c

3261

Yea, would have to audit callers to change that... It is an odd unit.

[scottl]

I think that this is going in the right direction. The SSU/shutdown code in mpr/mps is still not great, IMHO. There's a completion handler for the SSU CCB that could take care of decrementing the SSU_refcount, but instead every I/O is checked for the SSU state in the normal command completion path, and the refcount decrement happens there. The cost of checking every I/O for the uncommon shutdown case is probably in the noise right now, but if we ever want these drivers to do millions of IOPS, it'll need to be addressed. Also, error recovery is going to be highly problematic. Protocol-level recovery is non-existent because the completion handler doesn't call cam_periph_error(), but transport-level error recovery (for things like timeouts) will likely still be called from the driver. Do we want error recovery to be triggered during a system shutdown, possibly triggering extra delays? If an error does happen, does ignoring it make the device problematic the next time the system starts up? If we want the driver to ignore transport level error recovery, how should that be flagged?

sys/dev/mps/mps_sas_lsi.c

1215

Should the pause() always be done, or should it only be done if SCHEDULER_STOPPED is false? Same question applies to both mpr and mps, it was just easier to comment down here.

[cem]

sys/dev/mps/mps_sas_lsi.c

1215

In r300489 (May 2016), pause() gained a fallback mode that invokes DELAY() instead of _sleep() if SCHEDULER_STOPPED is true.

[markj]

sys/dev/mpr/mpr_sas_lsi.c

1464

Looks like this comment ought to be updated, ditto below.

[imp]

In D16663#354801, @scottl wrote:

I think that this is going in the right direction. The SSU/shutdown code in mpr/mps is still not great, IMHO. There's a completion handler for the SSU CCB that could take care of decrementing the SSU_refcount, but instead every I/O is checked for the SSU state in the normal command completion path, and the refcount decrement happens there. The cost of checking every I/O for the uncommon shutdown case is probably in the noise right now, but if we ever want these drivers to do millions of IOPS, it'll need to be addressed. Also, error recovery is going to be highly problematic. Protocol-level recovery is non-existent because the completion handler doesn't call cam_periph_error(), but transport-level error recovery (for things like timeouts) will likely still be called from the driver. Do we want error recovery to be triggered during a system shutdown, possibly triggering extra delays? If an error does happen, does ignoring it make the device problematic the next time the system starts up? If we want the driver to ignore transport level error recovery, how should that be flagged?

I tend to agree with your assessment. But CAM right now isn't great about knowing we're in 'shutdown' mode where error recovery might not be a good idea (but it might... It's unclear to me that we can say never here). In the mean time, doing the polling I think is a good 'make it better w/o letting the perfect be the enemy of the better' compromise. If we want to try it in the future, we'll need a comprehensive design so we know where we need to add changes for the error recovery and what the split we'd need and how much we want to drive in shutdown...

[imp]

sys/cam/cam_xpt.c

3214

I'm just moving lines here... If I were to do style changes, there's a lot more than just this that I'd do...

[scottl]

But CAM right now isn't great about knowing we're in 'shutdown' mode where error recovery might not be a good idea (but it might... It's unclear to me that we can say never here). In the mean time, doing the polling I think is a good 'make it better w/o letting the perfect be the enemy of the better' compromise.

I think I wasn't clear, sorry. CAM is not really involved at this point. Well, it is to a certain extent because we're asking it to do goofy things with devq scheduling in xpt_poll_setup(), but that's besides the point. For error recovery in this shutdown case, whether a normal shutdown or a panic, the ball is completely in the hands of the mps/mpr drivers. Protocol-level error recovery, i.e. getting back a completed CDB and examining the SCSI error and Sense codes, is completely bypassed and ignored. Transport level error recovery, i.e. timeouts on commands and command return status from the HBA itself, is still potentially active in this case, and still has the potential to put the system into either a hung or highly delayed state. The new polling method that you're proposing is good, but it's not enough to ensure reliable shutdown. My point was to start outlining what needs to be done to take the next steps. So a basic question is whether transport and protocol error recovery are necessary to ensure that devices that are having problems don't hang the system on shutdown and don't hang the system on restart. As a side note, I'd like to also get rid of the code in mpssas_scsiio_complete() that checks every I/O for the shutdown condition, and move that codes refcount manipulation to mpssas_stop_unit_done(). That'll start cleaning up the mess and making it more clear where further work is needed to advance or roll back error recovery decisions.