On Wed, 2018-04-11 at 17:32 +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.
>
> I understand why it is safe, I just didn't understand why it is needed.
The only reason is that it keeps the line with the blk_mq_change_rq_state() call
below the 80 character limit :-)
Bart.