diff mbox

[V7,5/6] block: support PREEMPT_ONLY

Message ID 20170930061214.10622-6-ming.lei@redhat.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Ming Lei Sept. 30, 2017, 6:12 a.m. UTC 
When queue is in PREEMPT_ONLY mode, only RQF_PREEMPT request
can be allocated and dispatched, other requests won't be allowed
to enter I/O path.

This is useful for supporting safe SCSI quiesce.

Part of this patch is from Bart's '[PATCH v4 4∕7] block: Add the QUEUE_FLAG_PREEMPT_ONLY
request queue flag'.

Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name>
Tested-by: Martin Steigerwald <martin@lichtvoll.de>
Cc: Bart Van Assche <Bart.VanAssche@wdc.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-core.c       | 26 ++++++++++++++++++++++++--
 include/linux/blkdev.h |  5 +++++
 2 files changed, 29 insertions(+), 2 deletions(-)

Comments

Christoph Hellwig Oct. 2, 2017, 1:50 p.m. UTC | #1 
> +void blk_set_preempt_only(struct request_queue *q, bool preempt_only)
> +{
> +	blk_mq_freeze_queue(q);
> +	if (preempt_only)
> +		queue_flag_set_unlocked(QUEUE_FLAG_PREEMPT_ONLY, q);
> +	else
> +		queue_flag_clear_unlocked(QUEUE_FLAG_PREEMPT_ONLY, q);
> +	blk_mq_unfreeze_queue(q);
> +}
> +EXPORT_SYMBOL(blk_set_preempt_only);

What do you need the queue freeze for?

> +		/*
> +		 * preempt_only flag has to be set after queue is frozen,
> +		 * so it can be checked here lockless and safely
> +		 */
> +		if (blk_queue_preempt_only(q)) {

We can always check a single bit flag safely, so I really don't
understand that comment.

> +			if (!(flags & BLK_REQ_PREEMPT))
> +				goto slow_path;
> +		}
> +
>  		if (percpu_ref_tryget_live(&q->q_usage_counter))
>  			return 0;
> -
> + slow_path:

Also this looks a very spaghetti, why not:


	if (!blk_queue_preempt_only(q) || (flags & BLK_MQ_REQ_PREEMPT)) {
		if (percpu_ref_tryget_live(&q->q_usage_counter))
			return 0;
	}
Bart Van Assche Oct. 2, 2017, 4:27 p.m. UTC | #2 
On Sat, 2017-09-30 at 14:12 +0800, Ming Lei wrote:
> +void blk_set_preempt_only(struct request_queue *q, bool preempt_only)

> +{

> +	blk_mq_freeze_queue(q);

> +	if (preempt_only)

> +		queue_flag_set_unlocked(QUEUE_FLAG_PREEMPT_ONLY, q);

> +	else

> +		queue_flag_clear_unlocked(QUEUE_FLAG_PREEMPT_ONLY, q);

> +	blk_mq_unfreeze_queue(q);

> +}

> +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

> @@ -771,9 +782,18 @@ int blk_queue_enter(struct request_queue *q, unsigned flags)

>  	while (true) {

>  		int ret;

>  

> +		/*

> +		 * preempt_only flag has to be set after queue is frozen,

> +		 * so it can be checked here lockless and safely

> +		 */

> +		if (blk_queue_preempt_only(q)) {

> +			if (!(flags & BLK_REQ_PREEMPT))

> +				goto slow_path;

> +		}

> +

>  		if (percpu_ref_tryget_live(&q->q_usage_counter))

>  			return 0;


Sorry but I don't think that it is possible with these changes to prevent
that a non-preempt request gets allocated after a (SCSI) queue has been
quiesced. If the CPU that calls blk_queue_enter() observes the set of the
PREEMPT_ONLY flag after the queue has been unfrozen and after the SCSI
device state has been changed into QUIESCED then blk_queue_enter() can
succeed for a non-preempt request. I think this is exactly the scenario
we want to avoid. This is why a synchronize_rcu() call is present in my
patch before the queue is unfrozen and also why in my patch the
percpu_ref_tryget_live() call occurs before the test of the PREEMPT_ONLY
flag.

Bart.
Ming Lei Oct. 3, 2017, 8:13 a.m. UTC | #3 
On Mon, Oct 02, 2017 at 04:27:59PM +0000, Bart Van Assche wrote:
> On Sat, 2017-09-30 at 14:12 +0800, Ming Lei wrote:
> > +void blk_set_preempt_only(struct request_queue *q, bool preempt_only)
> > +{
> > +	blk_mq_freeze_queue(q);
> > +	if (preempt_only)
> > +		queue_flag_set_unlocked(QUEUE_FLAG_PREEMPT_ONLY, q);
> > +	else
> > +		queue_flag_clear_unlocked(QUEUE_FLAG_PREEMPT_ONLY, q);
> > +	blk_mq_unfreeze_queue(q);
> > +}
> > +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
> > @@ -771,9 +782,18 @@ int blk_queue_enter(struct request_queue *q, unsigned flags)
> >  	while (true) {
> >  		int ret;
> >  
> > +		/*
> > +		 * preempt_only flag has to be set after queue is frozen,
> > +		 * so it can be checked here lockless and safely
> > +		 */
> > +		if (blk_queue_preempt_only(q)) {
> > +			if (!(flags & BLK_REQ_PREEMPT))
> > +				goto slow_path;
> > +		}
> > +
> >  		if (percpu_ref_tryget_live(&q->q_usage_counter))
> >  			return 0;
> 
> Sorry but I don't think that it is possible with these changes to prevent
> that a non-preempt request gets allocated after a (SCSI) queue has been
> quiesced. If the CPU that calls blk_queue_enter() observes the set of the
> PREEMPT_ONLY flag after the queue has been unfrozen and after the SCSI
> device state has been changed into QUIESCED then blk_queue_enter() can
> succeed for a non-preempt request. I think this is exactly the scenario

OK, looks one issue, will address it in next version.
Ming Lei Oct. 3, 2017, 8:22 a.m. UTC | #4 
On Mon, Oct 02, 2017 at 06:50:24AM -0700, Christoph Hellwig wrote:
> > +void blk_set_preempt_only(struct request_queue *q, bool preempt_only)
> > +{
> > +	blk_mq_freeze_queue(q);
> > +	if (preempt_only)
> > +		queue_flag_set_unlocked(QUEUE_FLAG_PREEMPT_ONLY, q);
> > +	else
> > +		queue_flag_clear_unlocked(QUEUE_FLAG_PREEMPT_ONLY, q);
> > +	blk_mq_unfreeze_queue(q);
> > +}
> > +EXPORT_SYMBOL(blk_set_preempt_only);
> 
> What do you need the queue freeze for?

The main reason is for draining requests in queue before setting PREEMP_ONLY.

> 
> > +		/*
> > +		 * preempt_only flag has to be set after queue is frozen,
> > +		 * so it can be checked here lockless and safely
> > +		 */
> > +		if (blk_queue_preempt_only(q)) {
> 
> We can always check a single bit flag safely, so I really don't
> understand that comment.
> 
> > +			if (!(flags & BLK_REQ_PREEMPT))
> > +				goto slow_path;
> > +		}
> > +
> >  		if (percpu_ref_tryget_live(&q->q_usage_counter))
> >  			return 0;
> > -
> > + slow_path:
> 
> Also this looks a very spaghetti, why not:
> 
> 
> 	if (!blk_queue_preempt_only(q) || (flags & BLK_MQ_REQ_PREEMPT)) {
> 		if (percpu_ref_tryget_live(&q->q_usage_counter))
> 			return 0;
> 	}

Looks fine, will do it in next version.
diff mbox

Patch

diff --git a/block/blk-core.c b/block/blk-core.c
index 95b1c5e50be3..bb683bfe37b2 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -346,6 +346,17 @@  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)
+{
+	blk_mq_freeze_queue(q);
+	if (preempt_only)
+		queue_flag_set_unlocked(QUEUE_FLAG_PREEMPT_ONLY, q);
+	else
+		queue_flag_clear_unlocked(QUEUE_FLAG_PREEMPT_ONLY, q);
+	blk_mq_unfreeze_queue(q);
+}
+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
@@ -771,9 +782,18 @@  int blk_queue_enter(struct request_queue *q, unsigned flags)
 	while (true) {
 		int ret;
 
+		/*
+		 * preempt_only flag has to be set after queue is frozen,
+		 * so it can be checked here lockless and safely
+		 */
+		if (blk_queue_preempt_only(q)) {
+			if (!(flags & BLK_REQ_PREEMPT))
+				goto slow_path;
+		}
+
 		if (percpu_ref_tryget_live(&q->q_usage_counter))
 			return 0;
-
+ slow_path:
 		if (flags & BLK_REQ_NOWAIT)
 			return -EBUSY;
 
@@ -787,7 +807,9 @@  int blk_queue_enter(struct request_queue *q, unsigned flags)
 		smp_rmb();
 
 		ret = wait_event_interruptible(q->mq_freeze_wq,
-				!atomic_read(&q->mq_freeze_depth) ||
+				(!atomic_read(&q->mq_freeze_depth) &&
+				((flags & BLK_REQ_PREEMPT) ||
+				 !blk_queue_preempt_only(q))) ||
 				blk_queue_dying(q));
 		if (blk_queue_dying(q))
 			return -ENODEV;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 68445adc8765..b01a0c6bb1f0 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -631,6 +631,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)	|	\
@@ -735,6 +736,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)
 {