| Message ID | 20170527142126.26079-7-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 2017-05-27 at 22:21 +0800, Ming Lei wrote: > diff --git a/block/blk-mq.c b/block/blk-mq.c > index 032045841856..84cce67caee3 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -168,8 +168,6 @@ void blk_mq_quiesce_queue(struct request_queue *q) > unsigned int i; > bool rcu = false; > > - __blk_mq_stop_hw_queues(q, true); > - > spin_lock_irq(q->queue_lock); > queue_flag_set(QUEUE_FLAG_QUIESCED, q); > spin_unlock_irq(q->queue_lock); > @@ -198,7 +196,12 @@ void blk_mq_unquiesce_queue(struct request_queue *q) > queue_flag_clear(QUEUE_FLAG_QUIESCED, q); > spin_unlock_irq(q->queue_lock); > > - blk_mq_start_stopped_hw_queues(q, true); > + /* > + * During quiescing, requests can be inserted > + * to scheduler's queue or sw queue, so we run > + * queues for dispatching these requests. > + */ > + blk_mq_start_hw_queues(q); > } > EXPORT_SYMBOL_GPL(blk_mq_unquiesce_queue); Hello Ming, This looks really weird to me. If blk_mq_quiesce_queue() doesn't stop any hardware queues, why should blk_mq_unquiesce_queue() start any hardware queues? Bart.
On Sat, May 27, 2017 at 09:49:27PM +0000, Bart Van Assche wrote: > On Sat, 2017-05-27 at 22:21 +0800, Ming Lei wrote: > > diff --git a/block/blk-mq.c b/block/blk-mq.c > > index 032045841856..84cce67caee3 100644 > > --- a/block/blk-mq.c > > +++ b/block/blk-mq.c > > @@ -168,8 +168,6 @@ void blk_mq_quiesce_queue(struct request_queue *q) > > unsigned int i; > > bool rcu = false; > > > > - __blk_mq_stop_hw_queues(q, true); > > - > > spin_lock_irq(q->queue_lock); > > queue_flag_set(QUEUE_FLAG_QUIESCED, q); > > spin_unlock_irq(q->queue_lock); > > @@ -198,7 +196,12 @@ void blk_mq_unquiesce_queue(struct request_queue *q) > > queue_flag_clear(QUEUE_FLAG_QUIESCED, q); > > spin_unlock_irq(q->queue_lock); > > > > - blk_mq_start_stopped_hw_queues(q, true); > > + /* > > + * During quiescing, requests can be inserted > > + * to scheduler's queue or sw queue, so we run > > + * queues for dispatching these requests. > > + */ > > + blk_mq_start_hw_queues(q); > > } > > EXPORT_SYMBOL_GPL(blk_mq_unquiesce_queue); > > Hello Ming, Hello Bart, > > This looks really weird to me. If blk_mq_quiesce_queue() doesn't stop any > hardware queues, why should blk_mq_unquiesce_queue() start any hardware > queues? Please see the above comment, especially in direct issue path, we have to insert the request into sw/scheduler queue when queue is quiesced, and these queued requests have to be dispatched out during unquiescing. I considered to sleep the current context under this situation, but turns out it is more complicated to handle, given we have very limited queue depth, just let applications consumes all, then wait. Thanks, Ming
On Sun, 2017-05-28 at 18:50 +0800, Ming Lei wrote: > On Sat, May 27, 2017 at 09:49:27PM +0000, Bart Van Assche wrote: > > On Sat, 2017-05-27 at 22:21 +0800, Ming Lei wrote: > > > diff --git a/block/blk-mq.c b/block/blk-mq.c > > > index 032045841856..84cce67caee3 100644 > > > --- a/block/blk-mq.c > > > +++ b/block/blk-mq.c > > > @@ -168,8 +168,6 @@ void blk_mq_quiesce_queue(struct request_queue *q) > > > unsigned int i; > > > bool rcu = false; > > > > > > - __blk_mq_stop_hw_queues(q, true); > > > - > > > spin_lock_irq(q->queue_lock); > > > queue_flag_set(QUEUE_FLAG_QUIESCED, q); > > > spin_unlock_irq(q->queue_lock); > > > @@ -198,7 +196,12 @@ void blk_mq_unquiesce_queue(struct request_queue *q) > > > queue_flag_clear(QUEUE_FLAG_QUIESCED, q); > > > spin_unlock_irq(q->queue_lock); > > > > > > - blk_mq_start_stopped_hw_queues(q, true); > > > + /* > > > + * During quiescing, requests can be inserted > > > + * to scheduler's queue or sw queue, so we run > > > + * queues for dispatching these requests. > > > + */ > > > + blk_mq_start_hw_queues(q); > > > } > > > EXPORT_SYMBOL_GPL(blk_mq_unquiesce_queue); > > > > This looks really weird to me. If blk_mq_quiesce_queue() doesn't stop any > > hardware queues, why should blk_mq_unquiesce_queue() start any hardware > > queues? > > Please see the above comment, especially in direct issue path, we have to > insert the request into sw/scheduler queue when queue is quiesced, > and these queued requests have to be dispatched out during unquiescing. > > I considered to sleep the current context under this situation, but > turns out it is more complicated to handle, given we have very limited > queue depth, just let applications consumes all, then wait. Hello Ming, The above seems to me to be an argument to *run* the queue from inside blk_mq_unquiesce_queue() instead of *starting* stopped queues from inside that function. Bart.
On Sun, May 28, 2017 at 04:03:20PM +0000, Bart Van Assche wrote: > On Sun, 2017-05-28 at 18:50 +0800, Ming Lei wrote: > > On Sat, May 27, 2017 at 09:49:27PM +0000, Bart Van Assche wrote: > > > On Sat, 2017-05-27 at 22:21 +0800, Ming Lei wrote: > > > > diff --git a/block/blk-mq.c b/block/blk-mq.c > > > > index 032045841856..84cce67caee3 100644 > > > > --- a/block/blk-mq.c > > > > +++ b/block/blk-mq.c > > > > @@ -168,8 +168,6 @@ void blk_mq_quiesce_queue(struct request_queue *q) > > > > unsigned int i; > > > > bool rcu = false; > > > > > > > > - __blk_mq_stop_hw_queues(q, true); > > > > - > > > > spin_lock_irq(q->queue_lock); > > > > queue_flag_set(QUEUE_FLAG_QUIESCED, q); > > > > spin_unlock_irq(q->queue_lock); > > > > @@ -198,7 +196,12 @@ void blk_mq_unquiesce_queue(struct request_queue *q) > > > > queue_flag_clear(QUEUE_FLAG_QUIESCED, q); > > > > spin_unlock_irq(q->queue_lock); > > > > > > > > - blk_mq_start_stopped_hw_queues(q, true); > > > > + /* > > > > + * During quiescing, requests can be inserted > > > > + * to scheduler's queue or sw queue, so we run > > > > + * queues for dispatching these requests. > > > > + */ > > > > + blk_mq_start_hw_queues(q); > > > > } > > > > EXPORT_SYMBOL_GPL(blk_mq_unquiesce_queue); > > > > > > This looks really weird to me. If blk_mq_quiesce_queue() doesn't stop any > > > hardware queues, why should blk_mq_unquiesce_queue() start any hardware > > > queues? > > > > Please see the above comment, especially in direct issue path, we have to > > insert the request into sw/scheduler queue when queue is quiesced, > > and these queued requests have to be dispatched out during unquiescing. > > > > I considered to sleep the current context under this situation, but > > turns out it is more complicated to handle, given we have very limited > > queue depth, just let applications consumes all, then wait. > > Hello Ming, > > The above seems to me to be an argument to *run* the queue from inside > blk_mq_unquiesce_queue() instead of *starting* stopped queues from inside > that function. Yes, it is run queues, as you can see in my comment. The reality is that we can't run queue without clearing the STOPPED state. Thanks, Ming
On Tue, 2017-05-30 at 08:27 +0800, Ming Lei wrote: > On Sun, May 28, 2017 at 04:03:20PM +0000, Bart Van Assche wrote: > > On Sun, 2017-05-28 at 18:50 +0800, Ming Lei wrote: > > > On Sat, May 27, 2017 at 09:49:27PM +0000, Bart Van Assche wrote: > > > > On Sat, 2017-05-27 at 22:21 +0800, Ming Lei wrote: > > > > > diff --git a/block/blk-mq.c b/block/blk-mq.c > > > > > index 032045841856..84cce67caee3 100644 > > > > > --- a/block/blk-mq.c > > > > > +++ b/block/blk-mq.c > > > > > @@ -168,8 +168,6 @@ void blk_mq_quiesce_queue(struct request_queue *q) > > > > > unsigned int i; > > > > > bool rcu = false; > > > > > > > > > > - __blk_mq_stop_hw_queues(q, true); > > > > > - > > > > > spin_lock_irq(q->queue_lock); > > > > > queue_flag_set(QUEUE_FLAG_QUIESCED, q); > > > > > spin_unlock_irq(q->queue_lock); > > > > > @@ -198,7 +196,12 @@ void blk_mq_unquiesce_queue(struct request_queue *q) > > > > > queue_flag_clear(QUEUE_FLAG_QUIESCED, q); > > > > > spin_unlock_irq(q->queue_lock); > > > > > > > > > > - blk_mq_start_stopped_hw_queues(q, true); > > > > > + /* > > > > > + * During quiescing, requests can be inserted > > > > > + * to scheduler's queue or sw queue, so we run > > > > > + * queues for dispatching these requests. > > > > > + */ > > > > > + blk_mq_start_hw_queues(q); > > > > > } > > > > > EXPORT_SYMBOL_GPL(blk_mq_unquiesce_queue); > > > > > > > > This looks really weird to me. If blk_mq_quiesce_queue() doesn't stop any > > > > hardware queues, why should blk_mq_unquiesce_queue() start any hardware > > > > queues? > > > > > > Please see the above comment, especially in direct issue path, we have to > > > insert the request into sw/scheduler queue when queue is quiesced, > > > and these queued requests have to be dispatched out during unquiescing. > > > > > > I considered to sleep the current context under this situation, but > > > turns out it is more complicated to handle, given we have very limited > > > queue depth, just let applications consumes all, then wait. > > > > Hello Ming, > > > > The above seems to me to be an argument to *run* the queue from inside > > blk_mq_unquiesce_queue() instead of *starting* stopped queues from inside > > that function. > > Yes, it is run queues, as you can see in my comment. > > The reality is that we can't run queue without clearing the STOPPED state. Hello Ming, I think it's completely wrong to make blk_mq_unquiesce_queue() to clear the STOPPED state. Stopping a queue is typically used to tell the block layer to stop dispatching requests and not to resume dispatching requests until the STOPPED flag is cleared. If stopping and quiescing a queue happen simultaneously then dispatching should not occur before both blk_mq_unquiesce_queue() and blk_mq_start_hw_queue() have been called. Your patch would make dispatching start too early. Bart.
On Tue, May 30, 2017 at 05:02:23PM +0000, Bart Van Assche wrote: > On Tue, 2017-05-30 at 08:27 +0800, Ming Lei wrote: > > On Sun, May 28, 2017 at 04:03:20PM +0000, Bart Van Assche wrote: > > > On Sun, 2017-05-28 at 18:50 +0800, Ming Lei wrote: > > > > On Sat, May 27, 2017 at 09:49:27PM +0000, Bart Van Assche wrote: > > > > > On Sat, 2017-05-27 at 22:21 +0800, Ming Lei wrote: > > > > > > diff --git a/block/blk-mq.c b/block/blk-mq.c > > > > > > index 032045841856..84cce67caee3 100644 > > > > > > --- a/block/blk-mq.c > > > > > > +++ b/block/blk-mq.c > > > > > > @@ -168,8 +168,6 @@ void blk_mq_quiesce_queue(struct request_queue *q) > > > > > > unsigned int i; > > > > > > bool rcu = false; > > > > > > > > > > > > - __blk_mq_stop_hw_queues(q, true); > > > > > > - > > > > > > spin_lock_irq(q->queue_lock); > > > > > > queue_flag_set(QUEUE_FLAG_QUIESCED, q); > > > > > > spin_unlock_irq(q->queue_lock); > > > > > > @@ -198,7 +196,12 @@ void blk_mq_unquiesce_queue(struct request_queue *q) > > > > > > queue_flag_clear(QUEUE_FLAG_QUIESCED, q); > > > > > > spin_unlock_irq(q->queue_lock); > > > > > > > > > > > > - blk_mq_start_stopped_hw_queues(q, true); > > > > > > + /* > > > > > > + * During quiescing, requests can be inserted > > > > > > + * to scheduler's queue or sw queue, so we run > > > > > > + * queues for dispatching these requests. > > > > > > + */ > > > > > > + blk_mq_start_hw_queues(q); > > > > > > } > > > > > > EXPORT_SYMBOL_GPL(blk_mq_unquiesce_queue); > > > > > > > > > > This looks really weird to me. If blk_mq_quiesce_queue() doesn't stop any > > > > > hardware queues, why should blk_mq_unquiesce_queue() start any hardware > > > > > queues? > > > > > > > > Please see the above comment, especially in direct issue path, we have to > > > > insert the request into sw/scheduler queue when queue is quiesced, > > > > and these queued requests have to be dispatched out during unquiescing. > > > > > > > > I considered to sleep the current context under this situation, but > > > > turns out it is more complicated to handle, given we have very limited > > > > queue depth, just let applications consumes all, then wait. > > > > > > Hello Ming, > > > > > > The above seems to me to be an argument to *run* the queue from inside > > > blk_mq_unquiesce_queue() instead of *starting* stopped queues from inside > > > that function. > > > > Yes, it is run queues, as you can see in my comment. > > > > The reality is that we can't run queue without clearing the STOPPED state. > > Hello Ming, > > I think it's completely wrong to make blk_mq_unquiesce_queue() to clear the > STOPPED state. Stopping a queue is typically used to tell the block layer to > stop dispatching requests and not to resume dispatching requests until the > STOPPED flag is cleared. If stopping and quiescing a queue happen > simultaneously then dispatching should not occur before both > blk_mq_unquiesce_queue() and blk_mq_start_hw_queue() have been called. Your > patch would make dispatching start too early. Yes, I thought of it too, now we need to sleep the context of direct issue, then restart in blk_mq_unquiesce_queue() can be avoided. Thanks, Ming
diff --git a/block/blk-mq.c b/block/blk-mq.c index 032045841856..84cce67caee3 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -168,8 +168,6 @@ void blk_mq_quiesce_queue(struct request_queue *q) unsigned int i; bool rcu = false; - __blk_mq_stop_hw_queues(q, true); - spin_lock_irq(q->queue_lock); queue_flag_set(QUEUE_FLAG_QUIESCED, q); spin_unlock_irq(q->queue_lock); @@ -198,7 +196,12 @@ void blk_mq_unquiesce_queue(struct request_queue *q) queue_flag_clear(QUEUE_FLAG_QUIESCED, q); spin_unlock_irq(q->queue_lock); - blk_mq_start_stopped_hw_queues(q, true); + /* + * During quiescing, requests can be inserted + * to scheduler's queue or sw queue, so we run + * queues for dispatching these requests. + */ + blk_mq_start_hw_queues(q); } EXPORT_SYMBOL_GPL(blk_mq_unquiesce_queue);
Queue can be started by other blk-mq APIs and can be used in different cases, this limits uses of blk_mq_quiesce_queue() if it is based on stopping queue, and make its usage very difficult, especially users have to use the stop queue APIs carefully for avoiding to break blk_mq_quiesce_queue(). We have applied the QUIESCED flag for draining and blocking dispatch, so it isn't necessary to stop queue any more. After stopping queue is removed, blk_mq_quiesce_queue() can be used safely and easily, then users won't worry about queue restarting during quiescing at all. Signed-off-by: Ming Lei <ming.lei@redhat.com> --- block/blk-mq.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)