[PATCHv4] nvme: allow to re-attach namespaces after all paths are down

[PATCHv4] nvme: allow to re-attach namespaces after all paths are down

[Hannes Reinecke]

We should only remove the ns head from the list of heads per
subsystem if the reference count drops to zero. Removing it
at the start of nvme_ns_remove() will prevent us from reattaching
the namespace to the correct ns head once a path becomes available
again.

Changes to v3:
- Simplify if() clause to detect duplicate namespaces
Changes to v2:
- Drop memcpy() statement
Changes to v1:
- Always check NSIDs after reattach

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/host/core.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 522c9b229f80..a60144a4054a 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -546,6 +546,9 @@ static void nvme_free_ns_head(struct kref *ref)
 	struct nvme_ns_head *head =
 		container_of(ref, struct nvme_ns_head, ref);
 
+	mutex_lock(&head->subsys->lock);
+	list_del_init(&head->entry);
+	mutex_unlock(&head->subsys->lock);
 	nvme_mpath_remove_disk(head);
 	ida_simple_remove(&head->subsys->ns_ida, head->instance);
 	cleanup_srcu_struct(&head->srcu);
@@ -3605,7 +3608,11 @@ static int nvme_init_ns_head(struct nvme_ns *ns, unsigned nsid,
 		head->shared = is_shared;
 	} else {
 		ret = -EINVAL;
-		if (!is_shared || !head->shared) {
+		/*
+		 * If multipath is enabled we might hit an ns head with no
+		 * paths, but that doesn't indicate it's a shared namespace.
+		 */
+		if (!list_empty(&head->list) && (!is_shared || !head->shared)) {
 			dev_err(ctrl->device,
 				"Duplicate unshared namespace %d\n", nsid);
 			goto out_put_ns_head;
@@ -3764,8 +3771,6 @@ static void nvme_ns_remove(struct nvme_ns *ns)
 
 	mutex_lock(&ns->ctrl->subsys->lock);
 	list_del_rcu(&ns->siblings);
-	if (list_empty(&ns->head->list))
-		list_del_init(&ns->head->entry);
 	mutex_unlock(&ns->ctrl->subsys->lock);
 
 	synchronize_rcu(); /* guarantee not available in head->list */
-- 
2.29.2


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCHv4] nvme: allow to re-attach namespaces after all paths are down

[Keith Busch]

On Mon, May 17, 2021 at 10:18:09AM +0200, Hannes Reinecke wrote:
> We should only remove the ns head from the list of heads per
> subsystem if the reference count drops to zero. Removing it
> at the start of nvme_ns_remove() will prevent us from reattaching
> the namespace to the correct ns head once a path becomes available
> again.

This looks fine to me

Reviewed-by: Keith Busch <kbusch@kernel.org>

There are potential corner cases associated with namespace management
that can cause problems here, but it would certainly be a user error
that needs manual intervention anyway. I suspect those scenarios are
very uncommon too, so I think we can deal with that later if it becomes
a problem.

> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/nvme/host/core.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 522c9b229f80..a60144a4054a 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -546,6 +546,9 @@ static void nvme_free_ns_head(struct kref *ref)
>  	struct nvme_ns_head *head =
>  		container_of(ref, struct nvme_ns_head, ref);
>  
> +	mutex_lock(&head->subsys->lock);
> +	list_del_init(&head->entry);
> +	mutex_unlock(&head->subsys->lock);
>  	nvme_mpath_remove_disk(head);
>  	ida_simple_remove(&head->subsys->ns_ida, head->instance);
>  	cleanup_srcu_struct(&head->srcu);
> @@ -3605,7 +3608,11 @@ static int nvme_init_ns_head(struct nvme_ns *ns, unsigned nsid,
>  		head->shared = is_shared;
>  	} else {
>  		ret = -EINVAL;
> -		if (!is_shared || !head->shared) {
> +		/*
> +		 * If multipath is enabled we might hit an ns head with no
> +		 * paths, but that doesn't indicate it's a shared namespace.
> +		 */
> +		if (!list_empty(&head->list) && (!is_shared || !head->shared)) {
>  			dev_err(ctrl->device,
>  				"Duplicate unshared namespace %d\n", nsid);
>  			goto out_put_ns_head;
> @@ -3764,8 +3771,6 @@ static void nvme_ns_remove(struct nvme_ns *ns)
>  
>  	mutex_lock(&ns->ctrl->subsys->lock);
>  	list_del_rcu(&ns->siblings);
> -	if (list_empty(&ns->head->list))
> -		list_del_init(&ns->head->entry);
>  	mutex_unlock(&ns->ctrl->subsys->lock);
>  
>  	synchronize_rcu(); /* guarantee not available in head->list */
> -- 

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCHv4] nvme: allow to re-attach namespaces after all paths are down

[Hannes Reinecke]

On 5/17/21 5:26 PM, Keith Busch wrote:
> On Mon, May 17, 2021 at 10:18:09AM +0200, Hannes Reinecke wrote:
>> We should only remove the ns head from the list of heads per
>> subsystem if the reference count drops to zero. Removing it
>> at the start of nvme_ns_remove() will prevent us from reattaching
>> the namespace to the correct ns head once a path becomes available
>> again.
> 
> This looks fine to me
> 
> Reviewed-by: Keith Busch <kbusch@kernel.org>
> 
> There are potential corner cases associated with namespace management
> that can cause problems here, but it would certainly be a user error
> that needs manual intervention anyway. I suspect those scenarios are
> very uncommon too, so I think we can deal with that later if it becomes
> a problem.
> 
How so?
We're keeping a copy of the NS IDs around, so after reconnect the
namespace would have to provide same set of NS IDs.

If the namespace is different yet providing the same NS IDs we'll have a
problem even with the current code, as we're using the ND ID to identify
multipathed namespaces.

Or do you have a different scenario in mind?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		        Kernel Storage Architect
hare@suse.de			               +49 911 74053 688
SUSE Software Solutions Germany GmbH, 90409 Nürnberg
GF: F. Imendörffer, HRB 36809 (AG Nürnberg)

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCHv4] nvme: allow to re-attach namespaces after all paths are down

[Keith Busch]

On Mon, May 17, 2021 at 05:36:39PM +0200, Hannes Reinecke wrote:
> On 5/17/21 5:26 PM, Keith Busch wrote:
> > On Mon, May 17, 2021 at 10:18:09AM +0200, Hannes Reinecke wrote:
> >> We should only remove the ns head from the list of heads per
> >> subsystem if the reference count drops to zero. Removing it
> >> at the start of nvme_ns_remove() will prevent us from reattaching
> >> the namespace to the correct ns head once a path becomes available
> >> again.
> > 
> > This looks fine to me
> > 
> > Reviewed-by: Keith Busch <kbusch@kernel.org>
> > 
> > There are potential corner cases associated with namespace management
> > that can cause problems here, but it would certainly be a user error
> > that needs manual intervention anyway. I suspect those scenarios are
> > very uncommon too, so I think we can deal with that later if it becomes
> > a problem.
> > 
> How so?
> We're keeping a copy of the NS IDs around, so after reconnect the
> namespace would have to provide same set of NS IDs.
> 
> If the namespace is different yet providing the same NS IDs we'll have a
> problem even with the current code, as we're using the ND ID to identify
> multipathed namespaces.
> 
> Or do you have a different scenario in mind?

Sorry, I didn't mean to imply this creates a new problem. It's already a
problem. :)

This just provides a chance for a recycled ID for a newly created
namespace to attach to an in-use head associated with a previously
deleted namespace. I am not really concerned about this scenario,
though, since it wouldn't work today anyway.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCHv4] nvme: allow to re-attach namespaces after all paths are down

[Sagi Grimberg]

>>> On Mon, May 17, 2021 at 10:18:09AM +0200, Hannes Reinecke wrote:
>>>> We should only remove the ns head from the list of heads per
>>>> subsystem if the reference count drops to zero. Removing it
>>>> at the start of nvme_ns_remove() will prevent us from reattaching
>>>> the namespace to the correct ns head once a path becomes available
>>>> again.
>>>
>>> This looks fine to me
>>>
>>> Reviewed-by: Keith Busch <kbusch@kernel.org>
>>>
>>> There are potential corner cases associated with namespace management
>>> that can cause problems here, but it would certainly be a user error
>>> that needs manual intervention anyway. I suspect those scenarios are
>>> very uncommon too, so I think we can deal with that later if it becomes
>>> a problem.
>>>
>> How so?
>> We're keeping a copy of the NS IDs around, so after reconnect the
>> namespace would have to provide same set of NS IDs.
>>
>> If the namespace is different yet providing the same NS IDs we'll have a
>> problem even with the current code, as we're using the ND ID to identify
>> multipathed namespaces.
>>
>> Or do you have a different scenario in mind?
> 
> Sorry, I didn't mean to imply this creates a new problem. It's already a
> problem. :)
> 
> This just provides a chance for a recycled ID for a newly created
> namespace to attach to an in-use head associated with a previously
> deleted namespace. I am not really concerned about this scenario,
> though, since it wouldn't work today anyway.

That is because we keep the device node around even though it doesn't
have any bottom devices. Which is what I think Hannes tried to fix
in an earlier patch.

Also, if an NVM subsystem happens to recycle nsids, it will trigger
pretty surely. So I'm not sure this is an obscure case...

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCHv4] nvme: allow to re-attach namespaces after all paths are down

[Keith Busch]

On Mon, May 17, 2021 at 10:58:38AM -0700, Sagi Grimberg wrote:
> 
> > > > On Mon, May 17, 2021 at 10:18:09AM +0200, Hannes Reinecke wrote:
> > > > > We should only remove the ns head from the list of heads per
> > > > > subsystem if the reference count drops to zero. Removing it
> > > > > at the start of nvme_ns_remove() will prevent us from reattaching
> > > > > the namespace to the correct ns head once a path becomes available
> > > > > again.
> > > > 
> > > > This looks fine to me
> > > > 
> > > > Reviewed-by: Keith Busch <kbusch@kernel.org>
> > > > 
> > > > There are potential corner cases associated with namespace management
> > > > that can cause problems here, but it would certainly be a user error
> > > > that needs manual intervention anyway. I suspect those scenarios are
> > > > very uncommon too, so I think we can deal with that later if it becomes
> > > > a problem.
> > > > 
> > > How so?
> > > We're keeping a copy of the NS IDs around, so after reconnect the
> > > namespace would have to provide same set of NS IDs.
> > > 
> > > If the namespace is different yet providing the same NS IDs we'll have a
> > > problem even with the current code, as we're using the ND ID to identify
> > > multipathed namespaces.
> > > 
> > > Or do you have a different scenario in mind?
> > 
> > Sorry, I didn't mean to imply this creates a new problem. It's already a
> > problem. :)
> > 
> > This just provides a chance for a recycled ID for a newly created
> > namespace to attach to an in-use head associated with a previously
> > deleted namespace. I am not really concerned about this scenario,
> > though, since it wouldn't work today anyway.
> 
> That is because we keep the device node around even though it doesn't
> have any bottom devices. Which is what I think Hannes tried to fix
> in an earlier patch.
> 
> Also, if an NVM subsystem happens to recycle nsids, it will trigger
> pretty surely. So I'm not sure this is an obscure case...

I'm not very concerned because (1) deleting an in-use namespace is
surely a user error, and (2) it currently doesn't work either, and no
one's complained yet AFAIK.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCHv4] nvme: allow to re-attach namespaces after all paths are down

[Sagi Grimberg]

>> That is because we keep the device node around even though it doesn't
>> have any bottom devices. Which is what I think Hannes tried to fix
>> in an earlier patch.
>>
>> Also, if an NVM subsystem happens to recycle nsids, it will trigger
>> pretty surely. So I'm not sure this is an obscure case...
> 
> I'm not very concerned because (1) deleting an in-use namespace is
> surely a user error, and (2) it currently doesn't work either, and no
> one's complained yet AFAIK.

You're not wrong. It's just that one error (on ns X) can manifest in
other potentially completely different namespaces created. I guess
this kind of thing can happen when ns administration is done by
adminX (or scriptX) and the ns mounting is done by userY where what each
is doing is unknown to each other.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCHv4] nvme: allow to re-attach namespaces after all paths are down

[Christoph Hellwig]

On Mon, May 17, 2021 at 11:20:36AM -0700, Sagi Grimberg wrote:
>
>>> That is because we keep the device node around even though it doesn't
>>> have any bottom devices. Which is what I think Hannes tried to fix
>>> in an earlier patch.
>>>
>>> Also, if an NVM subsystem happens to recycle nsids, it will trigger
>>> pretty surely. So I'm not sure this is an obscure case...
>>
>> I'm not very concerned because (1) deleting an in-use namespace is
>> surely a user error, and (2) it currently doesn't work either, and no
>> one's complained yet AFAIK.
>
> You're not wrong. It's just that one error (on ns X) can manifest in
> other potentially completely different namespaces created. I guess
> this kind of thing can happen when ns administration is done by
> adminX (or scriptX) and the ns mounting is done by userY where what each
> is doing is unknown to each other.

We also check for the matching identifiers in nvme_init_ns_head, so
we'll error out there.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCHv4] nvme: allow to re-attach namespaces after all paths are down

[Hannes Reinecke]

On 5/17/21 8:20 PM, Sagi Grimberg wrote:
> 
>>> That is because we keep the device node around even though it doesn't
>>> have any bottom devices. Which is what I think Hannes tried to fix
>>> in an earlier patch.
>>>
>>> Also, if an NVM subsystem happens to recycle nsids, it will trigger
>>> pretty surely. So I'm not sure this is an obscure case...
>>
>> I'm not very concerned because (1) deleting an in-use namespace is
>> surely a user error, and (2) it currently doesn't work either, and no
>> one's complained yet AFAIK.
> 
> You're not wrong. It's just that one error (on ns X) can manifest in
> other potentially completely different namespaces created. I guess
> this kind of thing can happen when ns administration is done by
> adminX (or scriptX) and the ns mounting is done by userY where what each
> is doing is unknown to each other.

This can only happen if NSFEAT bit 3 is not set and the namespace does 
not provide a UUID.
So if we are concerned about that we can restrict nvme multipathing to 
exclude these scenarios.
But in all other cases CNS 03 _is_ identifying the namespace, and cannot 
be re-used for other namespaces.
And we are keeping a copy of CNS 03 in nshead, so we'll be safe there.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCHv4] nvme: allow to re-attach namespaces after all paths are down

[Christoph Hellwig]

Thanks,

applied to nvme-5.14.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme