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.



Reply via email to