[PATCHv2 0/2] nvme-multipath: path selection fixes

[PATCHv2 0/2] nvme-multipath: path selection fixes

[Hannes Reinecke]

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.

Changes to v1:
- Use common helper nvme_path_is_disabled()

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 | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

-- 
2.16.4

[PATCH 1/2] nvme-multipath: check singular list in vme_round_robin_path()

[Hannes Reinecke]

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 | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 499acf07d61a..9b6dc11fa559 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -123,14 +123,19 @@ void nvme_mpath_clear_current_path(struct nvme_ns *ns)
 	}
 }
 
+static bool nvme_path_is_disabled(struct nvme_ns *ns)
+{
+	return ns->ctrl->state != NVME_CTRL_LIVE ||
+		test_bit(NVME_NS_ANA_PENDING, &ns->flags);
+}
+
 static struct nvme_ns *__nvme_find_path(struct nvme_ns_head *head, int node)
 {
 	int found_distance = INT_MAX, fallback_distance = INT_MAX, distance;
 	struct nvme_ns *found = NULL, *fallback = NULL, *ns;
 
 	list_for_each_entry_rcu(ns, &head->list, siblings) {
-		if (ns->ctrl->state != NVME_CTRL_LIVE ||
-		    test_bit(NVME_NS_ANA_PENDING, &ns->flags))
+		if (nvme_path_is_disabled(ns))
 			continue;
 
 		if (READ_ONCE(head->subsys->iopolicy) == NVME_IOPOLICY_NUMA)
@@ -178,14 +183,16 @@ 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 (nvme_path_is_disabled(old))
+			return NULL;
 		return old;
+	}
 
 	for (ns = nvme_next_ns(head, old);
 	     ns != old;
 	     ns = nvme_next_ns(head, ns)) {
-		if (ns->ctrl->state != NVME_CTRL_LIVE ||
-		    test_bit(NVME_NS_ANA_PENDING, &ns->flags))
+		if (nvme_path_is_disabled(ns))
 			continue;
 
 		if (ns->ana_state == NVME_ANA_OPTIMIZED) {
-- 
2.16.4

[PATCH 2/2] nvme-multipath: do not select namespaces which are about to be removed

[Hannes Reinecke]

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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 9b6dc11fa559..a9a927677970 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -126,7 +126,8 @@ void nvme_mpath_clear_current_path(struct nvme_ns *ns)
 static bool nvme_path_is_disabled(struct nvme_ns *ns)
 {
 	return 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);
 }
 
 static struct nvme_ns *__nvme_find_path(struct nvme_ns_head *head, int node)
-- 
2.16.4

[PATCHv2 0/2] nvme-multipath: path selection fixes

[Christoph Hellwig]

Thanks,

I've split out the addition of the new nvme_path_is_disabled helper into
a separate patch and then applied there series to the nvme-5.3 branch.

[PATCH 0/2] nvme-multipath: path selection fixes

[Hannes Reinecke]

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

[PATCH 2/2] nvme-multipath: do not select namespaces which are about to be removed

[Hannes Reinecke]

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

[PATCH 2/2] nvme-multipath: do not select namespaces which are about to be removed

[Christoph Hellwig]

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?

[PATCH 2/2] nvme-multipath: do not select namespaces which are about to be removed

[Sagi Grimberg]

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

[PATCH 2/2] nvme-multipath: do not select namespaces which are about to be removed

[Hannes Reinecke]

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)