| Message ID | 20170925202924.16603-5-bart.vanassche@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> +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.
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.
On Mon, Oct 02, 2017 at 03:56:08PM +0000, Bart Van Assche wrote: > 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. Oh, despite the _unlocked versions existing it doesn't do any atomic ops. Yeah, we'll need the queue lock then. But please split it into one helper for setting the queue blocked and to clear it, especially if the clear needs to do an additional wake_up.
diff --git a/block/blk-core.c b/block/blk-core.c index fef7133f8b0e..9111a8f9c7a1 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -346,6 +346,19 @@ void blk_sync_queue(struct request_queue *q) } EXPORT_SYMBOL(blk_sync_queue); +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); + /** * __blk_run_queue_uncond - run a queue whether or not it has been stopped * @q: The queue to run diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index cd26901a6e25..5bd87599eed0 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -627,6 +627,7 @@ struct request_queue { #define QUEUE_FLAG_REGISTERED 26 /* queue has been registered to a disk */ #define QUEUE_FLAG_SCSI_PASSTHROUGH 27 /* queue supports SCSI commands */ #define QUEUE_FLAG_QUIESCED 28 /* queue has been quiesced */ +#define QUEUE_FLAG_PREEMPT_ONLY 29 /* only process REQ_PREEMPT requests */ #define QUEUE_FLAG_DEFAULT ((1 << QUEUE_FLAG_IO_STAT) | \ (1 << QUEUE_FLAG_STACKABLE) | \ @@ -731,6 +732,10 @@ static inline void queue_flag_clear(unsigned int flag, struct request_queue *q) ((rq)->cmd_flags & (REQ_FAILFAST_DEV|REQ_FAILFAST_TRANSPORT| \ REQ_FAILFAST_DRIVER)) #define blk_queue_quiesced(q) test_bit(QUEUE_FLAG_QUIESCED, &(q)->queue_flags) +#define blk_queue_preempt_only(q) \ + test_bit(QUEUE_FLAG_PREEMPT_ONLY, &(q)->queue_flags) + +extern void blk_set_preempt_only(struct request_queue *q, bool preempt_only); static inline bool blk_account_rq(struct request *rq) {
This flag will be used in the next patch to let the block layer core know whether or not a SCSI request queue has been quiesced. A quiesced SCSI queue namely only processes RQF_PREEMPT requests. Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> Cc: Ming Lei <ming.lei@redhat.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Hannes Reinecke <hare@suse.com> Cc: Johannes Thumshirn <jthumshirn@suse.de> --- block/blk-core.c | 13 +++++++++++++ include/linux/blkdev.h | 5 +++++ 2 files changed, 18 insertions(+)