[PATCH 04/47] block: provide a new BLK_EH_QUIESCED timeout return value

[hch@lst.de (Christoph Hellwig)]

On Tue, Nov 24, 2015@10:51:04AM -0500, Jeff Moyer wrote:
> >> 
> >> blk_complete_request:
> >> A  if (!blk_mark_rq_complete(rq) ||
> >> B      test_and_cleart_bit(REQ_ATOM_QUIESCED, &req->atomic_flags)) {
> >> C        __blk_mq_complete_request(rq);
> >> 
> >> could run alongside of:
> >> 
> >> blk_rq_check_expired:
> >> 1 if (!blk_mark_rq_complete(rq))
> >> 2   blk_rq_timed_out(rq);
> >> 
> >> So, if 1 comes before A, we have two cases to consider:
> >> 
> >> i.  the expiration path does not yet set REQ_ATOM_QUIESCED before the
> >>     completion code runs, and so the completion code does nothing.
> >
> > The command has timed out and sets REQ_ATOM_COMPLETED first,
> > the the actual completion comes in and does indeed nothing.  We now
> > set REQ_ATOM_QUIESCED and kick off a controller reset, which will
> > ultimatively complete all commands using blk_mq_complete_request.
> > Now REQ_ATOM_QUIESCED is set on the command that caused the timeout,
> > so it will be completed as well.
> >
> >> ii. the expiration path *does* SET REQ_ATOM_QUIESCED.  In this instance,
> >>     will we get yet another completion for the request when the command
> >>     is ultimately retired by the adapter reset?
> >
> > The command has timed out and sets REQ_ATOM_COMPLETED first, then
> > REQ_ATOM_QUIESCED as well.  Now the actual completion comes in and does
> > nothing because REQ_ATOM_COMPLETED was set.  We will then kick off the
> 
> See B above.  REQ_ATOM_COMPLETE is set, so the first half of that
> statement is false, but then test_and_clear_bit(REQ_ATOM_QUIESCED...)
> returns true, so we call __blk_complete_request.  So the question is,
> will we get a double completion for that request after the reset is
> performed?

We always set REQ_ATOM_COMPLETE before calling into ->timeout and thus
even having a chance to REQ_ATOM_QUIESCED.  Maybe we're talking past
each other, so if it feels like I'm off track try to explain the
race in a bit more detail.