On Mon, 2018-06-04 at 15:42 +0200, hch@xxxxxx wrote:
> On Mon, Jun 04, 2018 at 06:56:22AM +0000, Bart Van Assche wrote:
> > Can you explain why you prefer this approach? I think this patch introduces
> > more new atomic operations in the hot path than the approach I had proposed
> > ("blk-mq: Rework blk-mq timeout handling again"). In other words, I think the
> > patch that I had proposed will yield better performance.
>
> Primarily because the generation counter is fundamentally the wrong
> thing to do. If we know the tag has some pending action on it we
> should not reuse it insted of working around races that stem from the
> fact that we reuse it.
We may each have a different opinion about this. Anyway, let's not get
distracted by a discussion about theoretical arguments.
> We can look at the performance numbers and tune as needed, but for now
> we need to get the high level approach right first. I think once
> we've done a proper audit of the SCSI EH code and blk_abort_request
> users we can probably drop the cmpxchg and only have a single
> atomic_dec_and_test left, which in terms of atomic operations isn't
> the world.
I think that the race between request completion and request timeout
handling is fundamental so I'm not sure that it is possible to eliminate
the cmpxchg() calls. Additionally, even if the cmpxchg() calls would be
eliminated, struct request will still have two variables that represent
the request state (rq->state and rq->ref) and that are updated separately.
With my approach all request state information is stored in a single
variable which makes it easy to update the individual fields of that
variable simultaneously.
Bart.
