| Message ID | 20211119021849.2259254-1-ming.lei@redhat.com |
|---|---|
| Headers | show |
| Series | blk-mq: quiesce improvement | expand |
On 11/30/21 4:33 AM, Ming Lei wrote: > On Tue, Nov 23, 2021 at 11:00:45AM +0200, Sagi Grimberg wrote: >> >>>>>>> Add helper of blk_mq_global_quiesce_wait() for supporting to quiesce >>>>>>> queues in parallel, then we can just wait once if global quiesce wait >>>>>>> is allowed. >>>>>> >>>>>> blk_mq_global_quiesce_wait() is a poor name... global is scope-less and >>>>>> obviously it has a scope. >>>>> >>>>> How about blk_mq_shared_quiesce_wait()? or any suggestion? >>>> >>>> Shared between what? >>> >>> All request queues in one host-wide, both scsi and nvme has such >>> requirement. >>> >>>> >>>> Maybe if the queue has a non-blocking tagset, it can have a "quiesced" >>>> flag that is cleared in unquiesce? then the callers can just continue >>>> to iterate but will only wait the rcu grace period once. >>> >>> Yeah, that is what these patches try to implement. >> >> I was suggesting to "hide" it in the interface. >> Maybe something like: >> -- >> diff --git a/block/blk-mq.c b/block/blk-mq.c >> index 8799fa73ef34..627b631db1f9 100644 >> --- a/block/blk-mq.c >> +++ b/block/blk-mq.c >> @@ -263,14 +263,18 @@ void blk_mq_wait_quiesce_done(struct request_queue *q) >> unsigned int i; >> bool rcu = false; >> >> + if (!q->has_srcu && q->quiesced) >> + return; >> queue_for_each_hw_ctx(q, hctx, i) { >> if (hctx->flags & BLK_MQ_F_BLOCKING) >> synchronize_srcu(hctx->srcu); >> else >> rcu = true; >> } >> - if (rcu) >> + if (rcu) { >> synchronize_rcu(); >> + q->quiesced = true; >> + } >> } >> EXPORT_SYMBOL_GPL(blk_mq_wait_quiesce_done); >> >> @@ -308,6 +312,7 @@ void blk_mq_unquiesce_queue(struct request_queue *q) >> } else if (!--q->quiesce_depth) { >> blk_queue_flag_clear(QUEUE_FLAG_QUIESCED, q); >> run_queue = true; >> + q->quiesced = false; > > Different request queues are passed to blk_mq_wait_quiesce_done() during > the iteration, so marking 'quiesced' doesn't make any difference here. I actually meant q->tag_set->quiesced, such that the flag will be used in the tag_set reference. This way this sharing will be kept hidden from the callers.
On Wed, Dec 08, 2021 at 02:49:25PM +0200, Sagi Grimberg wrote: > > > On 11/30/21 4:33 AM, Ming Lei wrote: > > On Tue, Nov 23, 2021 at 11:00:45AM +0200, Sagi Grimberg wrote: > > > > > > > > > > > Add helper of blk_mq_global_quiesce_wait() for supporting to quiesce > > > > > > > > queues in parallel, then we can just wait once if global quiesce wait > > > > > > > > is allowed. > > > > > > > > > > > > > > blk_mq_global_quiesce_wait() is a poor name... global is scope-less and > > > > > > > obviously it has a scope. > > > > > > > > > > > > How about blk_mq_shared_quiesce_wait()? or any suggestion? > > > > > > > > > > Shared between what? > > > > > > > > All request queues in one host-wide, both scsi and nvme has such > > > > requirement. > > > > > > > > > > > > > > Maybe if the queue has a non-blocking tagset, it can have a "quiesced" > > > > > flag that is cleared in unquiesce? then the callers can just continue > > > > > to iterate but will only wait the rcu grace period once. > > > > > > > > Yeah, that is what these patches try to implement. > > > > > > I was suggesting to "hide" it in the interface. > > > Maybe something like: > > > -- > > > diff --git a/block/blk-mq.c b/block/blk-mq.c > > > index 8799fa73ef34..627b631db1f9 100644 > > > --- a/block/blk-mq.c > > > +++ b/block/blk-mq.c > > > @@ -263,14 +263,18 @@ void blk_mq_wait_quiesce_done(struct request_queue *q) > > > unsigned int i; > > > bool rcu = false; > > > > > > + if (!q->has_srcu && q->quiesced) > > > + return; > > > queue_for_each_hw_ctx(q, hctx, i) { > > > if (hctx->flags & BLK_MQ_F_BLOCKING) > > > synchronize_srcu(hctx->srcu); > > > else > > > rcu = true; > > > } > > > - if (rcu) > > > + if (rcu) { > > > synchronize_rcu(); > > > + q->quiesced = true; > > > + } > > > } > > > EXPORT_SYMBOL_GPL(blk_mq_wait_quiesce_done); > > > > > > @@ -308,6 +312,7 @@ void blk_mq_unquiesce_queue(struct request_queue *q) > > > } else if (!--q->quiesce_depth) { > > > blk_queue_flag_clear(QUEUE_FLAG_QUIESCED, q); > > > run_queue = true; > > > + q->quiesced = false; > > > > Different request queues are passed to blk_mq_wait_quiesce_done() during > > the iteration, so marking 'quiesced' doesn't make any difference here. > > I actually meant q->tag_set->quiesced, such that the flag will be > used in the tag_set reference. This way this sharing will be kept > hidden from the callers. That looks easy, but not true really from API viewpoint, q->tag_set has different lifetime which is covered by driver, not mention other request queues may touch q->tag_set->quiesced from other code path, so things will become much complicated, also what is the meaning of quiesced state for tagset wide? Here I prefer to philosophy of 'Worse is better'[1], and simplicity over perfection by adding one simple helper of blk_mq_shared_quiesce_wait() for improving the case very easily. [1] ihttps://ipfs.fleek.co/ipfs/QmXoypizjW3WknFiJnKLwHCnL72vedxjQkDDP1mXWo6uco/wiki/Unix_philosophy.html Thanks, Ming