On Mon, 2017-10-02 at 15:47 +0200, Christoph Hellwig wrote:
> > +void blk_set_preempt_only(struct request_queue *q, bool preempt_only)
> > +{
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(q->queue_lock, flags);
> > + if (preempt_only)
> > + queue_flag_set(QUEUE_FLAG_PREEMPT_ONLY, q);
> > + else
> > + queue_flag_clear(QUEUE_FLAG_PREEMPT_ONLY, q);
> > + spin_unlock_irqrestore(q->queue_lock, flags);
> > +}
> > +EXPORT_SYMBOL(blk_set_preempt_only);
>
> Why do we even need this helper? The lock doesn't make sense to me,
> and it would just much easier to set/clear the flag from the driver.
Hello Christoph,
Since queue_flag_set() and queue_flag_clear() use non-atomic primitives to
set and clear flags it is essential to protect calls of these functions with
the queue lock. Otherwise flag updates could get lost due to concurrent
queue_flag_set() or queue_flag_clear() calls.
One of the reasons why I introduced this helper function is to keep the
wake_up_all(&q->mq_freeze_wq) call that is added to this function by a later
patch in the block layer.
Bart.
