| Message ID | 20200727231022.307602-3-sagi@grimberg.me (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | improve nvme quiesce time for large amount of namespaces | expand |
On 7/27/20 4:10 PM, Sagi Grimberg wrote: > All controller namespaces share the same tagset, so we > can use this interface which does the optimal operation > for parallel quiesce based on the tagset type (e.g. > blocking tagsets and non-blocking tagsets). > > Signed-off-by: Sagi Grimberg <sagi@grimberg.me> > --- > drivers/nvme/host/core.c | 14 ++------------ > 1 file changed, 2 insertions(+), 12 deletions(-) > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c > index 05aa568a60af..c41df20996d7 100644 > --- a/drivers/nvme/host/core.c > +++ b/drivers/nvme/host/core.c > @@ -4557,23 +4557,13 @@ EXPORT_SYMBOL_GPL(nvme_start_freeze); > > void nvme_stop_queues(struct nvme_ctrl *ctrl) > { > - struct nvme_ns *ns; > - > - down_read(&ctrl->namespaces_rwsem); > - list_for_each_entry(ns, &ctrl->namespaces, list) > - blk_mq_quiesce_queue(ns->queue); > - up_read(&ctrl->namespaces_rwsem); > + blk_mq_quiesce_tagset(ctrl->tagset); Rrr.. this one is slightly annoying. We have the connect_q in fabrics that we use to issue the connect command which is now quiesced too... If we will use this interface, we can unquiesce it right away, but that seems kinda backwards... -- diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 05aa568a60af..70af0ff63e7f 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -4557,23 +4557,15 @@ EXPORT_SYMBOL_GPL(nvme_start_freeze); void nvme_stop_queues(struct nvme_ctrl *ctrl) { - struct nvme_ns *ns; - - down_read(&ctrl->namespaces_rwsem); - list_for_each_entry(ns, &ctrl->namespaces, list) - blk_mq_quiesce_queue(ns->queue); - up_read(&ctrl->namespaces_rwsem); + blk_mq_quiesce_tagset(ctrl->tagset); + if (ctrl->connect_q) + blk_mq_unquiesce_queue(ctrl->connect_q); } EXPORT_SYMBOL_GPL(nvme_stop_queues); -- Thoughts?
On 2020/7/28 8:54, Sagi Grimberg wrote: > > > On 7/27/20 4:10 PM, Sagi Grimberg wrote: >> All controller namespaces share the same tagset, so we >> can use this interface which does the optimal operation >> for parallel quiesce based on the tagset type (e.g. >> blocking tagsets and non-blocking tagsets). >> >> Signed-off-by: Sagi Grimberg <sagi@grimberg.me> >> --- >> drivers/nvme/host/core.c | 14 ++------------ >> 1 file changed, 2 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c >> index 05aa568a60af..c41df20996d7 100644 >> --- a/drivers/nvme/host/core.c >> +++ b/drivers/nvme/host/core.c >> @@ -4557,23 +4557,13 @@ EXPORT_SYMBOL_GPL(nvme_start_freeze); >> void nvme_stop_queues(struct nvme_ctrl *ctrl) >> { >> - struct nvme_ns *ns; >> - >> - down_read(&ctrl->namespaces_rwsem); >> - list_for_each_entry(ns, &ctrl->namespaces, list) >> - blk_mq_quiesce_queue(ns->queue); >> - up_read(&ctrl->namespaces_rwsem); >> + blk_mq_quiesce_tagset(ctrl->tagset); > > Rrr.. this one is slightly annoying. We have the connect_q in > fabrics that we use to issue the connect command which is now > quiesced too... > > If we will use this interface, we can unquiesce it right away, > but that seems kinda backwards..Io queue and admin queue has different treat mechanism, introduce blk_mq_quiesce_tagset may make the mechanism unclear. So this is probably not a good choice. > > -- > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c > index 05aa568a60af..70af0ff63e7f 100644 > --- a/drivers/nvme/host/core.c > +++ b/drivers/nvme/host/core.c > @@ -4557,23 +4557,15 @@ EXPORT_SYMBOL_GPL(nvme_start_freeze); > > void nvme_stop_queues(struct nvme_ctrl *ctrl) > { > - struct nvme_ns *ns; > - > - down_read(&ctrl->namespaces_rwsem); > - list_for_each_entry(ns, &ctrl->namespaces, list) > - blk_mq_quiesce_queue(ns->queue); > - up_read(&ctrl->namespaces_rwsem); > + blk_mq_quiesce_tagset(ctrl->tagset); > + if (ctrl->connect_q) > + blk_mq_unquiesce_queue(ctrl->connect_q); > } > EXPORT_SYMBOL_GPL(nvme_stop_queues); > -- > > Thoughts? > .
>>> All controller namespaces share the same tagset, so we >>> can use this interface which does the optimal operation >>> for parallel quiesce based on the tagset type (e.g. >>> blocking tagsets and non-blocking tagsets). >>> >>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me> >>> --- >>> drivers/nvme/host/core.c | 14 ++------------ >>> 1 file changed, 2 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c >>> index 05aa568a60af..c41df20996d7 100644 >>> --- a/drivers/nvme/host/core.c >>> +++ b/drivers/nvme/host/core.c >>> @@ -4557,23 +4557,13 @@ EXPORT_SYMBOL_GPL(nvme_start_freeze); >>> void nvme_stop_queues(struct nvme_ctrl *ctrl) >>> { >>> - struct nvme_ns *ns; >>> - >>> - down_read(&ctrl->namespaces_rwsem); >>> - list_for_each_entry(ns, &ctrl->namespaces, list) >>> - blk_mq_quiesce_queue(ns->queue); >>> - up_read(&ctrl->namespaces_rwsem); >>> + blk_mq_quiesce_tagset(ctrl->tagset); >> >> Rrr.. this one is slightly annoying. We have the connect_q in >> fabrics that we use to issue the connect command which is now >> quiesced too... >> >> If we will use this interface, we can unquiesce it right away, >> but that seems kinda backwards..Io queue and admin queue has different >> treat mechanism, introduce > blk_mq_quiesce_tagset may make the mechanism unclear. So this is > probably not a good choice. The meaning of blk_mq_quiesce_tagset is clear, the awkward thing is that we need to unquiesce the connect_q after blk_mq_quiesce_tagset quiesced it. I'm thinking of resorting from this abstraction...
On 2020/7/28 11:34, Sagi Grimberg wrote: > >>>> All controller namespaces share the same tagset, so we >>>> can use this interface which does the optimal operation >>>> for parallel quiesce based on the tagset type (e.g. >>>> blocking tagsets and non-blocking tagsets). >>>> >>>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me> >>>> --- >>>> drivers/nvme/host/core.c | 14 ++------------ >>>> 1 file changed, 2 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c >>>> index 05aa568a60af..c41df20996d7 100644 >>>> --- a/drivers/nvme/host/core.c >>>> +++ b/drivers/nvme/host/core.c >>>> @@ -4557,23 +4557,13 @@ EXPORT_SYMBOL_GPL(nvme_start_freeze); >>>> void nvme_stop_queues(struct nvme_ctrl *ctrl) >>>> { >>>> - struct nvme_ns *ns; >>>> - >>>> - down_read(&ctrl->namespaces_rwsem); >>>> - list_for_each_entry(ns, &ctrl->namespaces, list) >>>> - blk_mq_quiesce_queue(ns->queue); >>>> - up_read(&ctrl->namespaces_rwsem); >>>> + blk_mq_quiesce_tagset(ctrl->tagset); >>> >>> Rrr.. this one is slightly annoying. We have the connect_q in >>> fabrics that we use to issue the connect command which is now >>> quiesced too... >>> >>> If we will use this interface, we can unquiesce it right away, >>> but that seems kinda backwards..Io queue and admin queue has different treat mechanism, introduce >> blk_mq_quiesce_tagset may make the mechanism unclear. So this is >> probably not a good choice. > > The meaning of blk_mq_quiesce_tagset is clear, the awkward thing is > that we need to unquiesce the connect_q after blk_mq_quiesce_tagset > quiesced it. Io queue and admin queue has different treat mechanism. If we switch to blk_mq_quiesce_tagset, maybe we need do more extras such as unquiesce the connect_q, thus the mechanism maybe unclear. Surely if we introduce blk_mq_quiesce_tagset for new feature, the abstraction is great. > > I'm thinking of resorting from this abstraction... > .
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 05aa568a60af..c41df20996d7 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -4557,23 +4557,13 @@ EXPORT_SYMBOL_GPL(nvme_start_freeze); void nvme_stop_queues(struct nvme_ctrl *ctrl) { - struct nvme_ns *ns; - - down_read(&ctrl->namespaces_rwsem); - list_for_each_entry(ns, &ctrl->namespaces, list) - blk_mq_quiesce_queue(ns->queue); - up_read(&ctrl->namespaces_rwsem); + blk_mq_quiesce_tagset(ctrl->tagset); } EXPORT_SYMBOL_GPL(nvme_stop_queues); void nvme_start_queues(struct nvme_ctrl *ctrl) { - struct nvme_ns *ns; - - down_read(&ctrl->namespaces_rwsem); - list_for_each_entry(ns, &ctrl->namespaces, list) - blk_mq_unquiesce_queue(ns->queue); - up_read(&ctrl->namespaces_rwsem); + blk_mq_unquiesce_tagset(ctrl->tagset); } EXPORT_SYMBOL_GPL(nvme_start_queues);
All controller namespaces share the same tagset, so we can use this interface which does the optimal operation for parallel quiesce based on the tagset type (e.g. blocking tagsets and non-blocking tagsets). Signed-off-by: Sagi Grimberg <sagi@grimberg.me> --- drivers/nvme/host/core.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)