* [PATCH 0/2] nvme-multipath: path selection fixes @ 2019-07-03 13:12 Hannes Reinecke 2019-07-03 13:12 ` [PATCH 1/2] nvme-multipath: check singular list in vme_round_robin_path() Hannes Reinecke 2019-07-03 13:12 ` [PATCH 2/2] nvme-multipath: do not select namespaces which are about to be removed Hannes Reinecke 0 siblings, 2 replies; 6+ messages in thread From: Hannes Reinecke @ 2019-07-03 13:12 UTC (permalink / raw) Hi all, during analysing a mysterious failure due to blk_queue_split() (again) I've found that the multipath path selection might return invalid namespaces under certain circumstances. This patchset fixes those scenarios. As usual, reviews and comments are welcome. Hannes Reinecke (2): nvme-multipath: check singular list in vme_round_robin_path() nvme-multipath: do not select namespaces which are about to be removed drivers/nvme/host/multipath.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) -- 2.16.4 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] nvme-multipath: check singular list in vme_round_robin_path() 2019-07-03 13:12 [PATCH 0/2] nvme-multipath: path selection fixes Hannes Reinecke @ 2019-07-03 13:12 ` Hannes Reinecke 2019-07-03 13:12 ` [PATCH 2/2] nvme-multipath: do not select namespaces which are about to be removed Hannes Reinecke 1 sibling, 0 replies; 6+ messages in thread From: Hannes Reinecke @ 2019-07-03 13:12 UTC (permalink / raw) When we have a singular list in nvme_round_robin_path() we still need to check its validity. Signed-off-by: Hannes Reinecke <hare at suse.com> --- drivers/nvme/host/multipath.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c index 499acf07d61a..c8cc82639327 100644 --- a/drivers/nvme/host/multipath.c +++ b/drivers/nvme/host/multipath.c @@ -178,8 +178,12 @@ static struct nvme_ns *nvme_round_robin_path(struct nvme_ns_head *head, { struct nvme_ns *ns, *found, *fallback = NULL; - if (list_is_singular(&head->list)) + if (list_is_singular(&head->list)) { + if (old->ctrl->state != NVME_CTRL_LIVE || + test_bit(NVME_NS_ANA_PENDING, &old->flags)) + return NULL; return old; + } for (ns = nvme_next_ns(head, old); ns != old; -- 2.16.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] nvme-multipath: do not select namespaces which are about to be removed 2019-07-03 13:12 [PATCH 0/2] nvme-multipath: path selection fixes Hannes Reinecke 2019-07-03 13:12 ` [PATCH 1/2] nvme-multipath: check singular list in vme_round_robin_path() Hannes Reinecke @ 2019-07-03 13:12 ` Hannes Reinecke 2019-07-03 13:21 ` Christoph Hellwig 1 sibling, 1 reply; 6+ messages in thread From: Hannes Reinecke @ 2019-07-03 13:12 UTC (permalink / raw) nvme_ns_remove() will first set the NVME_NS_REMOVING flag before removing it from the list at the very last step. So to avoid selecting a namespace in nvme_find_path() which is about to be removed check the NVME_NS_REMOVING flag, too, when selecting a new path. Signed-off-by: Hannes Reinecke <hare at suse.com> --- drivers/nvme/host/multipath.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c index c8cc82639327..1f6105a5c596 100644 --- a/drivers/nvme/host/multipath.c +++ b/drivers/nvme/host/multipath.c @@ -130,7 +130,8 @@ static struct nvme_ns *__nvme_find_path(struct nvme_ns_head *head, int node) list_for_each_entry_rcu(ns, &head->list, siblings) { if (ns->ctrl->state != NVME_CTRL_LIVE || - test_bit(NVME_NS_ANA_PENDING, &ns->flags)) + test_bit(NVME_NS_ANA_PENDING, &ns->flags) || + test_bit(NVME_NS_REMOVING, &ns->flags)) continue; if (READ_ONCE(head->subsys->iopolicy) == NVME_IOPOLICY_NUMA) @@ -180,7 +181,8 @@ static struct nvme_ns *nvme_round_robin_path(struct nvme_ns_head *head, if (list_is_singular(&head->list)) { if (old->ctrl->state != NVME_CTRL_LIVE || - test_bit(NVME_NS_ANA_PENDING, &old->flags)) + test_bit(NVME_NS_ANA_PENDING, &old->flags)|| + test_bit(NVME_NS_REMOVING, &old->flags)) return NULL; return old; } @@ -189,7 +191,8 @@ static struct nvme_ns *nvme_round_robin_path(struct nvme_ns_head *head, ns != old; ns = nvme_next_ns(head, ns)) { if (ns->ctrl->state != NVME_CTRL_LIVE || - test_bit(NVME_NS_ANA_PENDING, &ns->flags)) + test_bit(NVME_NS_ANA_PENDING, &ns->flags) || + test_bit(NVME_NS_REMOVING, &ns->flags)) continue; if (ns->ana_state == NVME_ANA_OPTIMIZED) { -- 2.16.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] nvme-multipath: do not select namespaces which are about to be removed 2019-07-03 13:12 ` [PATCH 2/2] nvme-multipath: do not select namespaces which are about to be removed Hannes Reinecke @ 2019-07-03 13:21 ` Christoph Hellwig 2019-07-03 20:33 ` Sagi Grimberg 0 siblings, 1 reply; 6+ messages in thread From: Christoph Hellwig @ 2019-07-03 13:21 UTC (permalink / raw) On Wed, Jul 03, 2019@03:12:32PM +0200, Hannes Reinecke wrote: > diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c > index c8cc82639327..1f6105a5c596 100644 > --- a/drivers/nvme/host/multipath.c > +++ b/drivers/nvme/host/multipath.c > @@ -130,7 +130,8 @@ static struct nvme_ns *__nvme_find_path(struct nvme_ns_head *head, int node) > > list_for_each_entry_rcu(ns, &head->list, siblings) { > if (ns->ctrl->state != NVME_CTRL_LIVE || > - test_bit(NVME_NS_ANA_PENDING, &ns->flags)) > + test_bit(NVME_NS_ANA_PENDING, &ns->flags) || > + test_bit(NVME_NS_REMOVING, &ns->flags)) > continue; > > if (READ_ONCE(head->subsys->iopolicy) == NVME_IOPOLICY_NUMA) > @@ -180,7 +181,8 @@ static struct nvme_ns *nvme_round_robin_path(struct nvme_ns_head *head, > > if (list_is_singular(&head->list)) { > if (old->ctrl->state != NVME_CTRL_LIVE || > - test_bit(NVME_NS_ANA_PENDING, &old->flags)) > + test_bit(NVME_NS_ANA_PENDING, &old->flags)|| > + test_bit(NVME_NS_REMOVING, &old->flags)) > return NULL; > return old; > } > @@ -189,7 +191,8 @@ static struct nvme_ns *nvme_round_robin_path(struct nvme_ns_head *head, > ns != old; > ns = nvme_next_ns(head, ns)) { > if (ns->ctrl->state != NVME_CTRL_LIVE || > - test_bit(NVME_NS_ANA_PENDING, &ns->flags)) > + test_bit(NVME_NS_ANA_PENDING, &ns->flags) || > + test_bit(NVME_NS_REMOVING, &ns->flags)) > continue; I think we clearly need a patch before the two patches in your series that factors this check into a little helper with a descriptive name. Also doesn't nvme_path_is_optimized also need to have these checks while we are at it? ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] nvme-multipath: do not select namespaces which are about to be removed 2019-07-03 13:21 ` Christoph Hellwig @ 2019-07-03 20:33 ` Sagi Grimberg 2019-07-04 6:00 ` Hannes Reinecke 0 siblings, 1 reply; 6+ messages in thread From: Sagi Grimberg @ 2019-07-03 20:33 UTC (permalink / raw) >> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c >> index c8cc82639327..1f6105a5c596 100644 >> --- a/drivers/nvme/host/multipath.c >> +++ b/drivers/nvme/host/multipath.c >> @@ -130,7 +130,8 @@ static struct nvme_ns *__nvme_find_path(struct nvme_ns_head *head, int node) >> >> list_for_each_entry_rcu(ns, &head->list, siblings) { >> if (ns->ctrl->state != NVME_CTRL_LIVE || >> - test_bit(NVME_NS_ANA_PENDING, &ns->flags)) >> + test_bit(NVME_NS_ANA_PENDING, &ns->flags) || >> + test_bit(NVME_NS_REMOVING, &ns->flags)) >> continue; >> >> if (READ_ONCE(head->subsys->iopolicy) == NVME_IOPOLICY_NUMA) >> @@ -180,7 +181,8 @@ static struct nvme_ns *nvme_round_robin_path(struct nvme_ns_head *head, >> >> if (list_is_singular(&head->list)) { >> if (old->ctrl->state != NVME_CTRL_LIVE || >> - test_bit(NVME_NS_ANA_PENDING, &old->flags)) >> + test_bit(NVME_NS_ANA_PENDING, &old->flags)|| >> + test_bit(NVME_NS_REMOVING, &old->flags)) >> return NULL; >> return old; >> } >> @@ -189,7 +191,8 @@ static struct nvme_ns *nvme_round_robin_path(struct nvme_ns_head *head, >> ns != old; >> ns = nvme_next_ns(head, ns)) { >> if (ns->ctrl->state != NVME_CTRL_LIVE || >> - test_bit(NVME_NS_ANA_PENDING, &ns->flags)) >> + test_bit(NVME_NS_ANA_PENDING, &ns->flags) || >> + test_bit(NVME_NS_REMOVING, &ns->flags)) >> continue; > > I think we clearly need a patch before the two patches in your > series that factors this check into a little helper with a > descriptive name. Also doesn't nvme_path_is_optimized also need > to have these checks while we are at it? I definitely agree here. BTW, while we're on this, do we need the flags check to be atomic? or can we micro-optimize with a simple flag arithmetic? ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] nvme-multipath: do not select namespaces which are about to be removed 2019-07-03 20:33 ` Sagi Grimberg @ 2019-07-04 6:00 ` Hannes Reinecke 0 siblings, 0 replies; 6+ messages in thread From: Hannes Reinecke @ 2019-07-04 6:00 UTC (permalink / raw) On 7/3/19 10:33 PM, Sagi Grimberg wrote: > >>> diff --git a/drivers/nvme/host/multipath.c >>> b/drivers/nvme/host/multipath.c >>> index c8cc82639327..1f6105a5c596 100644 >>> --- a/drivers/nvme/host/multipath.c >>> +++ b/drivers/nvme/host/multipath.c >>> @@ -130,7 +130,8 @@ static struct nvme_ns *__nvme_find_path(struct >>> nvme_ns_head *head, int node) >>> ? ????? list_for_each_entry_rcu(ns, &head->list, siblings) { >>> ????????? if (ns->ctrl->state != NVME_CTRL_LIVE || >>> -??????????? test_bit(NVME_NS_ANA_PENDING, &ns->flags)) >>> +??????????? test_bit(NVME_NS_ANA_PENDING, &ns->flags) || >>> +??????????? test_bit(NVME_NS_REMOVING, &ns->flags)) >>> ????????????? continue; >>> ? ????????? if (READ_ONCE(head->subsys->iopolicy) == NVME_IOPOLICY_NUMA) >>> @@ -180,7 +181,8 @@ static struct nvme_ns >>> *nvme_round_robin_path(struct nvme_ns_head *head, >>> ? ????? if (list_is_singular(&head->list)) { >>> ????????? if (old->ctrl->state != NVME_CTRL_LIVE || >>> -??????????? test_bit(NVME_NS_ANA_PENDING, &old->flags)) >>> +??????????? test_bit(NVME_NS_ANA_PENDING, &old->flags)|| >>> +??????????? test_bit(NVME_NS_REMOVING, &old->flags)) >>> ????????????? return NULL; >>> ????????? return old; >>> ????? } >>> @@ -189,7 +191,8 @@ static struct nvme_ns >>> *nvme_round_robin_path(struct nvme_ns_head *head, >>> ?????????? ns != old; >>> ?????????? ns = nvme_next_ns(head, ns)) { >>> ????????? if (ns->ctrl->state != NVME_CTRL_LIVE || >>> -??????????? test_bit(NVME_NS_ANA_PENDING, &ns->flags)) >>> +??????????? test_bit(NVME_NS_ANA_PENDING, &ns->flags) || >>> +??????????? test_bit(NVME_NS_REMOVING, &ns->flags)) >>> ????????????? continue; >> >> I think we clearly need a patch before the two patches in your >> series that factors this check into a little helper with a >> descriptive name.? Also doesn't nvme_path_is_optimized also need >> to have these checks while we are at it? > > I definitely agree here. > > BTW, while we're on this, do we need the flags check to be > atomic? or can we micro-optimize with a simple flag arithmetic? Depends on your definition of 'atomic' here. No, the _combined_ check does not need to be atomic (ie we do not need to take a lock around the check here) as the flags are set in two wildly different places. Yes, the check itself needs to be atomic (ie any update to the flags bitmap needs to be visible) as this check protect against a race condition. We could try to micro-optimize here with using bitmap_and(), but then this would probably amount to out-guess the compiler, and I'd rather not attempt it for the sake of readability. Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking hare at suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Mary Higgins, Sri Rasiah HRB 21284 (AG N?rnberg) ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-07-04 6:00 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-07-03 13:12 [PATCH 0/2] nvme-multipath: path selection fixes Hannes Reinecke 2019-07-03 13:12 ` [PATCH 1/2] nvme-multipath: check singular list in vme_round_robin_path() Hannes Reinecke 2019-07-03 13:12 ` [PATCH 2/2] nvme-multipath: do not select namespaces which are about to be removed Hannes Reinecke 2019-07-03 13:21 ` Christoph Hellwig 2019-07-03 20:33 ` Sagi Grimberg 2019-07-04 6:00 ` Hannes Reinecke
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox