On Wed, 2018-04-11 at 16:19 +0300, Sagi Grimberg wrote:
> > static void __blk_mq_requeue_request(struct request *rq)
> > {
> > struct request_queue *q = rq->q;
> > + enum mq_rq_state old_state = blk_mq_rq_state(rq);
> >
> > blk_mq_put_driver_tag(rq);
> >
> > trace_block_rq_requeue(q, rq);
> > wbt_requeue(q->rq_wb, &rq->issue_stat);
> >
> > - if (blk_mq_rq_state(rq) != MQ_RQ_IDLE) {
> > - blk_mq_rq_update_state(rq, MQ_RQ_IDLE);
> > + if (old_state != MQ_RQ_IDLE) {
> > + if (!blk_mq_change_rq_state(rq, old_state, MQ_RQ_IDLE))
> > + WARN_ON_ONCE(true);
> > if (q->dma_drain_size && blk_rq_bytes(rq))
> > rq->nr_phys_segments--;
> > }
>
> Can you explain why was old_state kept as a local variable?
Hello Sagi,
Since blk_mq_requeue_request() must be called after a request has completed
the timeout handler will ignore requests that are being requeued. Hence it
is safe in this function to cache the request state in a local variable.
> > +static inline bool blk_mq_change_rq_state(struct request *rq,
> > + enum mq_rq_state old_state,
> > + enum mq_rq_state new_state)
> > {
> > - u64 old_val = READ_ONCE(rq->gstate);
> > - u64 new_val = (old_val & ~MQ_RQ_STATE_MASK) | state;
> > -
> > - if (state == MQ_RQ_IN_FLIGHT) {
> > - WARN_ON_ONCE((old_val & MQ_RQ_STATE_MASK) != MQ_RQ_IDLE);
> > - new_val += MQ_RQ_GEN_INC;
> > - }
> > + unsigned long old_val = (READ_ONCE(rq->__deadline) & ~RQ_STATE_MASK) |
> > + old_state;
> > + unsigned long new_val = (old_val & ~RQ_STATE_MASK) | new_state;
> >
> > - /* avoid exposing interim values */
> > - WRITE_ONCE(rq->gstate, new_val);
> > + return cmpxchg(&rq->__deadline, old_val, new_val) == old_val;
> > }
>
> Can you explain why this takes the old_state of the request?
Can you clarify your question? The purpose of this function is to change
the request state only into @new_state if it matches @old_state. I think
that is also what the implementation of this function does.
Thanks,
Bart.