On Tue, Dec 04, 2018 at 11:33:33AM -0800, James Smart wrote: > > > On 12/4/2018 9:48 AM, Keith Busch wrote: > > On Tue, Dec 04, 2018 at 09:38:29AM -0800, Sagi Grimberg wrote: > > > > > > Yes, I'm very much in favour of this, too. > > > > > > We always have this IMO slightly weird notion of stopping the queue, set > > > > > > some error flags in the driver, then _restarting_ the queue, just so > > > > > > that the driver then sees the error flag and terminates the requests. > > > > > > Which I always found quite counter-intuitive. > > > > > What about requests that come in after the iteration runs? how are those > > > > > terminated? > > > > If we've reached a dead state, I think you'd want to start a queue freeze > > > > before running the terminating iterator. > > > Its not necessarily dead, in fabrics we need to handle disconnections > > > that last for a while before we are able to reconnect (for a variety of > > > reasons) and we need a way to fail I/O for failover (or requeue, or > > > block its up to the upper layer). Its less of a "last resort" action > > > like in the pci case. > > > > > > Does this guarantee that after freeze+iter we won't get queued with any > > > other request? If not then we still need to unfreeze and fail at > > > queue_rq. > > It sounds like there are different scenarios to consider. > > > > For the dead controller, we call blk_cleanup_queue() at the end which > > ends callers who blocked on entering. > > > > If you're doing a failover, you'd replace the freeze with a current path > > update in order to prevent new requests from entering. > and if you're not multipath ? I assume you want the io queues to be > frozen so they queue there - which can block threads such as ns > verification. It's good to have them live, as todays checks bounce the io, > letting the thread terminate as its in a reset/reconnect state, which allows > those threads to exit out or finish before a new reconnect kicks them off > again. We've already been fighting deadlocks with the reset/delete/rescan > paths and these io paths. suspending the queues completely over the > reconnect will likely create more issues in this area. > > > > In either case, you don't need checks in queue_rq. The queue_rq check > > is redundant with the quiesce state that blk-mq already provides. > > I disagree. The cases I've run into are on the admin queue - where we are > sending io to initialize the controller when another error/reset occurs, and > the checks are required to identify/reject the "old" initialization > commands, with another state check allowing them to proceed on the "new" > initialization commands. And there are also cases for ioctls and other > things that occur during the middle of those initialization steps that need > to be weeded out. The Admin queue has to be kept live to allow the > initialization commands on the new controller. > > state checks are also needed for those namespace validation cases.... > > > > > Once quiesced, the proposed iterator can handle the final termination > > of the request, perform failover, or some other lld specific action > > depending on your situation. > > I don't believe they can remain frozen, definitely not for the admin queue. > -- james Quiesced and frozen carry different semantics. My understanding of the nvme-fc implementation is that it returns BLK_STS_RESOURCE in the scenario you've described where the admin command can't be executed at the moment. That just has the block layer requeue it for later resubmission 3 milliseconds later, which will continue to return the same status code until you're really ready for it. What I'm proposing is that instead of using that return code, you may have nvme-fc control when to dispatch those queued requests by utilizing the blk-mq quiesce on/off states. Is there a reason that wouldn't work?
