[PATCH 0/4] nvme: NSHEAD_DISK_LIVE fixes

[PATCH 0/4] nvme: NSHEAD_DISK_LIVE fixes

[Hannes Reinecke]

Hi all,

I'm have a testcase which does a repeated map/unmap of namespaces and
changing the UUID for each namespace after unmapping.
This leads to an unrecoverable system where the scanning processes
are stuck in 'device_add_disk()' waiting for I/O which will never come
as the I/O is doing ping-pong between the namespace (which cannot
do I/O as the path is inaccessible) and the ns_head (which is not
live and hence can't do I/O, either).
With this patchset (and the queue freeze patchset from hch) the problem
is resolved and the testcase runs without issues.
I see to get the testcase added to blktests.

As usual, comments and reviews are welcome.

Hannes Reinecke (4):
  nvme-multipath: fixup typo when clearing DISK_LIVE
  nvme-multipath: clear 'NVME_NSHEAD_DISK_LIVE' bit on shutdown
  nvme-multipath: check for NVME_NSHEAD_DISK_LIVE when selecting paths
  nvme: remove existing namespace on ID mismatch

 drivers/nvme/host/core.c      |  4 +++-
 drivers/nvme/host/multipath.c | 10 ++++++++--
 2 files changed, 11 insertions(+), 3 deletions(-)

-- 
2.35.3

[PATCH 1/4] nvme-multipath: fixup typo when clearing DISK_LIVE

[Hannes Reinecke]

NVME_NSHEAD_DISK_LIVE is a flag for 'struct nvme_ns_head', not
'struct nvme_ns'.

Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
 drivers/nvme/host/multipath.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 91d9eb3c22ef..c9d23b1b8efc 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -646,7 +646,7 @@ static void nvme_mpath_set_live(struct nvme_ns *ns)
 		rc = device_add_disk(&head->subsys->dev, head->disk,
 				     nvme_ns_attr_groups);
 		if (rc) {
-			clear_bit(NVME_NSHEAD_DISK_LIVE, &ns->flags);
+			clear_bit(NVME_NSHEAD_DISK_LIVE, &head->flags);
 			return;
 		}
 		nvme_add_ns_head_cdev(head);
-- 
2.35.3

Re: [PATCH 1/4] nvme-multipath: fixup typo when clearing DISK_LIVE

[Sagi Grimberg]

This was already sent wasn't it?


On 02/09/2024 14:15, Hannes Reinecke wrote:
> NVME_NSHEAD_DISK_LIVE is a flag for 'struct nvme_ns_head', not
> 'struct nvme_ns'.
>
> Signed-off-by: Hannes Reinecke <hare@kernel.org>

And again,
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

[PATCH 2/4] nvme-multipath: clear 'NVME_NSHEAD_DISK_LIVE' bit on shutdown

[Hannes Reinecke]

When calling 'nvme_mpath_shutdown_disk()' we should be clearing
the 'NVME_NSHEAD_DISK_LIVE' bit to be consistent with
nvme_mpath_set_disk_live().

Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
 drivers/nvme/host/multipath.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index c9d23b1b8efc..01dbbe866d61 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -968,7 +968,7 @@ void nvme_mpath_shutdown_disk(struct nvme_ns_head *head)
 	if (!head->disk)
 		return;
 	kblockd_schedule_work(&head->requeue_work);
-	if (test_bit(NVME_NSHEAD_DISK_LIVE, &head->flags)) {
+	if (test_and_clear_bit(NVME_NSHEAD_DISK_LIVE, &head->flags)) {
 		nvme_cdev_del(&head->cdev, &head->cdev_device);
 		del_gendisk(head->disk);
 	}
-- 
2.35.3

Re: [PATCH 2/4] nvme-multipath: clear 'NVME_NSHEAD_DISK_LIVE' bit on shutdown

[Sagi Grimberg]

On 02/09/2024 14:15, Hannes Reinecke wrote:
> When calling 'nvme_mpath_shutdown_disk()' we should be clearing
> the 'NVME_NSHEAD_DISK_LIVE' bit to be consistent with
> nvme_mpath_set_disk_live().

I have to say, this looks redundant on its own. The nshead is
going away so it shouldn't matter.

Your cover letter suggest that there is a hang, which I assume
this is essential to, please describe how and say what is this fixing.

[PATCH 3/4] nvme-multipath: check for NVME_NSHEAD_DISK_LIVE when selecting paths

[Hannes Reinecke]

During repetitive namespace remapping operations on the target the
namespace might have changed between the time the initial scan
was performed, and partition scan was invoked by device_add_disk()
in nvme_mpath_set_live(). We then end up with a stuck scanning process:

[<0>] folio_wait_bit_common+0x12a/0x310
[<0>] filemap_read_folio+0x97/0xd0
[<0>] do_read_cache_folio+0x108/0x390
[<0>] read_part_sector+0x31/0xa0
[<0>] read_lba+0xc5/0x160
[<0>] efi_partition+0xd9/0x8f0
[<0>] bdev_disk_changed+0x23d/0x6d0
[<0>] blkdev_get_whole+0x78/0xc0
[<0>] bdev_open+0x2c6/0x3b0
[<0>] bdev_file_open_by_dev+0xcb/0x120
[<0>] disk_scan_partitions+0x5d/0x100
[<0>] device_add_disk+0x402/0x420
[<0>] nvme_mpath_set_live+0x4f/0x1f0 [nvme_core]
[<0>] nvme_mpath_add_disk+0x107/0x120 [nvme_core]
[<0>] nvme_alloc_ns+0xac6/0xe60 [nvme_core]
[<0>] nvme_scan_ns+0x2dd/0x3e0 [nvme_core]
[<0>] nvme_scan_work+0x1a3/0x490 [nvme_core]

and another scanning process (triggered by AEN) trying to remove the namespaces:

[<0>] __synchronize_srcu+0x17c/0x1b0
[<0>] nvme_ns_remove+0x12e/0x210 [nvme_core]
[<0>] nvme_ns_remove_by_nsid+0x21/0x70 [nvme_core]
[<0>] nvme_scan_work+0x1b7/0x490 [nvme_core]
[<0>] process_scheduled_works+0x37d/0x6d0

none of which is able to make progress as the first process cannot submit I/O
(as all the namespace inforation is stale), and the second process is stalled
waiting on the srcu to be released by the first process.

This patch checks for the NVME_NSHEAD_DISK_LIVE bit when selecting a path,
causing I/O to be aborted and allowing the first process to make progress.

Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
 drivers/nvme/host/multipath.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 01dbbe866d61..f5f2e1667c64 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -407,6 +407,9 @@ static struct nvme_ns *nvme_numa_path(struct nvme_ns_head *head)
 
 inline struct nvme_ns *nvme_find_path(struct nvme_ns_head *head)
 {
+	if (!test_bit(NVME_NSHEAD_DISK_LIVE, &head->flags))
+		return NULL;
+
 	switch (READ_ONCE(head->subsys->iopolicy)) {
 	case NVME_IOPOLICY_QD:
 		return nvme_queue_depth_path(head);
@@ -421,6 +424,9 @@ static bool nvme_available_path(struct nvme_ns_head *head)
 {
 	struct nvme_ns *ns;
 
+	if (!test_bit(NVME_NSHEAD_DISK_LIVE, &head->flags))
+		return NULL;
+
 	list_for_each_entry_rcu(ns, &head->list, siblings) {
 		if (test_bit(NVME_CTRL_FAILFAST_EXPIRED, &ns->ctrl->flags))
 			continue;
-- 
2.35.3

Re: [PATCH 3/4] nvme-multipath: check for NVME_NSHEAD_DISK_LIVE when selecting paths

[Sagi Grimberg]

The patch title should describe what it is fixing rather than what it is 
doing.


On 02/09/2024 14:15, Hannes Reinecke wrote:
> During repetitive namespace remapping operations on the target the
> namespace might have changed between the time the initial scan
> was performed, and partition scan was invoked by device_add_disk()
> in nvme_mpath_set_live(). We then end up with a stuck scanning process:

I'm not sure I understand what "remapping operations" are?

> [<0>] folio_wait_bit_common+0x12a/0x310
> [<0>] filemap_read_folio+0x97/0xd0
> [<0>] do_read_cache_folio+0x108/0x390
> [<0>] read_part_sector+0x31/0xa0
> [<0>] read_lba+0xc5/0x160
> [<0>] efi_partition+0xd9/0x8f0
> [<0>] bdev_disk_changed+0x23d/0x6d0
> [<0>] blkdev_get_whole+0x78/0xc0
> [<0>] bdev_open+0x2c6/0x3b0
> [<0>] bdev_file_open_by_dev+0xcb/0x120
> [<0>] disk_scan_partitions+0x5d/0x100
> [<0>] device_add_disk+0x402/0x420
> [<0>] nvme_mpath_set_live+0x4f/0x1f0 [nvme_core]
> [<0>] nvme_mpath_add_disk+0x107/0x120 [nvme_core]
> [<0>] nvme_alloc_ns+0xac6/0xe60 [nvme_core]
> [<0>] nvme_scan_ns+0x2dd/0x3e0 [nvme_core]
> [<0>] nvme_scan_work+0x1a3/0x490 [nvme_core]
>
> and another scanning process (triggered by AEN) trying to remove the namespaces:
>
> [<0>] __synchronize_srcu+0x17c/0x1b0
> [<0>] nvme_ns_remove+0x12e/0x210 [nvme_core]
> [<0>] nvme_ns_remove_by_nsid+0x21/0x70 [nvme_core]
> [<0>] nvme_scan_work+0x1b7/0x490 [nvme_core]
> [<0>] process_scheduled_works+0x37d/0x6d0
>
> none of which is able to make progress as the first process cannot submit I/O
> (as all the namespace inforation is stale), and the second process is stalled
> waiting on the srcu to be released by the first process.
>
> This patch checks for the NVME_NSHEAD_DISK_LIVE bit when selecting a path,
> causing I/O to be aborted and allowing the first process to make progress.
>
> Signed-off-by: Hannes Reinecke <hare@kernel.org>
> ---
>   drivers/nvme/host/multipath.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 01dbbe866d61..f5f2e1667c64 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -407,6 +407,9 @@ static struct nvme_ns *nvme_numa_path(struct nvme_ns_head *head)
>   
>   inline struct nvme_ns *nvme_find_path(struct nvme_ns_head *head)
>   {
> +	if (!test_bit(NVME_NSHEAD_DISK_LIVE, &head->flags))
> +		return NULL;
> +
>   	switch (READ_ONCE(head->subsys->iopolicy)) {
>   	case NVME_IOPOLICY_QD:
>   		return nvme_queue_depth_path(head);
> @@ -421,6 +424,9 @@ static bool nvme_available_path(struct nvme_ns_head *head)
>   {
>   	struct nvme_ns *ns;
>   
> +	if (!test_bit(NVME_NSHEAD_DISK_LIVE, &head->flags))
> +		return NULL;
> +
>   	list_for_each_entry_rcu(ns, &head->list, siblings) {
>   		if (test_bit(NVME_CTRL_FAILFAST_EXPIRED, &ns->ctrl->flags))
>   			continue;

This looks like together with the last patch address the hang, so they 
need to be squashed together.

[PATCH 4/4] nvme: remove existing namespace on ID mismatch

[Hannes Reinecke]

When the namespace IDs mismatch during rescan we clearly ran out of
sync with the target, but we really can't know whether our local copy
of the NSID is correct, and that one found by scanning is not.
But clearly something is wrong with the namespaces, so we should remove
the existing one, too, to avoid accidental data corruption.

Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
 drivers/nvme/host/core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 0dc8bcc664f2..626718bbf3a5 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -136,6 +136,7 @@ static void nvme_remove_invalid_namespaces(struct nvme_ctrl *ctrl,
 					   unsigned nsid);
 static void nvme_update_keep_alive(struct nvme_ctrl *ctrl,
 				   struct nvme_command *cmd);
+static void nvme_ns_remove_by_nsid(struct nvme_ctrl *ctrl, u32 nsid);
 
 void nvme_queue_scan(struct nvme_ctrl *ctrl)
 {
@@ -3746,8 +3747,9 @@ static int nvme_init_ns_head(struct nvme_ns *ns, struct nvme_ns_info *info)
 		}
 		if (!nvme_ns_ids_equal(&head->ids, &info->ids)) {
 			dev_err(ctrl->device,
-				"IDs don't match for shared namespace %d\n",
+				"IDs don't match for shared namespace %d, removing\n",
 					info->nsid);
+			nvme_ns_remove_by_nsid(ctrl, info->nsid);
 			goto out_put_ns_head;
 		}
 
-- 
2.35.3

Re: [PATCH 4/4] nvme: remove existing namespace on ID mismatch

[Sagi Grimberg]

Again, patch title should describe what it is fixing.


On 02/09/2024 14:15, Hannes Reinecke wrote:
> When the namespace IDs mismatch during rescan we clearly ran out of
> sync with the target,

Not clear at all. When can this happen and how?

>   but we really can't know whether our local copy
> of the NSID is correct, and that one found by scanning is not.
> But clearly something is wrong with the namespaces, so we should remove
> the existing one, too, to avoid accidental data corruption.

Umm. So you see a bogus ns and conflicting with another ns which is
seemingly fine and you remove it?

I feel like you are describing something "generically useful/correct" 
while you address
a very specific problem but you don't describe it to us...

Re: [PATCH 4/4] nvme: remove existing namespace on ID mismatch

[Hannes Reinecke]

On 9/2/24 19:01, Sagi Grimberg wrote:
> Again, patch title should describe what it is fixing.
> 
> 
> On 02/09/2024 14:15, Hannes Reinecke wrote:
>> When the namespace IDs mismatch during rescan we clearly ran out of
>> sync with the target,
> 
> Not clear at all. When can this happen and how?
> 
>>   but we really can't know whether our local copy
>> of the NSID is correct, and that one found by scanning is not.
>> But clearly something is wrong with the namespaces, so we should remove
>> the existing one, too, to avoid accidental data corruption.
> 
> Umm. So you see a bogus ns and conflicting with another ns which is
> seemingly fine and you remove it?
> 
> I feel like you are describing something "generically useful/correct" 
> while you address
> a very specific problem but you don't describe it to us...
> 
Turns out that this patch was actually pointless, as the code in 
question can only be reached from the 'nvme_alloc_ns()' flow, which
means that no namespace for this nsid on that controller is present
(and hence nothing to remove).

But I seem to have found the root cause now (I/O stall as we don't
kick the requeue list in all cases), will be sending a new patchset.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

[PATCHv3 0/4] nvme: NSHEAD_DISK_LIVE fixes

[Hannes Reinecke]

Hi all,

I'm having a testcase which repeatedly deletes namespaces on the target
and creates new namespaces, and aggressively re-using NSIDs for the
new namespaces.
To throw in more fun these namespaces are created on different nodes
in the cluster, where only the paths local to the cluster node are
active, and all other paths are inaccessible.

Essentially it's doing something like:

echo 0 > ${ns}/enable
rm ${ns}
<random delay>
mkdir ${ns}
echo "<dev>" > ${ns}/device_path
echo "<grpid>" > ${ns}/ana_grpid
uuidgen > ${ns}/device_uuid
echo 1 > ${ns}/enable

repeatedly with several namespaces and several ANA groups.
This leads to an unrecoverable system where the scanning processes
are stuck in the partition scanning code triggered via
'device_add_disk()' waiting for I/O which will never
come.
There are two parts to fixing this:
We need to ensure the NSHEAD_DISK_LIVE is properly set when the
ns_head is live, and unset when the last path is gone.
And we need to trigger the requeue list after NSHEAD_DISK_LIVE
has been cleared to flush all outstanding I/O.

Turns out there's another corner case; when running the same test
but not removing the namespaces while changing the UUID we end up
with I/Os constantly being retried, and we are unable to even
disconnect the controller. To fix this we should set the
'failfast' flag for the controller when disconnecting to ensure
that all I/O is aborted.

With these patches (and the queue freeze patchset from hch) the problem
is resolved and the testcase runs without issues.
I see to get the testcase added to blktests.

As usual, comments and reviews are welcome.

Changes to v2:
- Include reviews from Sagi
- Drop the check for NSHEAD_DISK_LIVE in nvme_available_path()
- Add a patch to requeue I/O if the ANA state changed
- Set the 'failfast' flag when removing controllers

Changes to the original submission:
- Drop patch to remove existing namespaces on ID mismatch
- Combine patches updating NSHEAD_DISK_LIVE handling
- requeue I/O after NSHEAD_DISK_LIVE has been cleared

Hannes Reinecke (4):
  nvme-multipath: fixup typo when clearing DISK_LIVE
  nvme-multipath: check for NVME_NSHEAD_DISK_LIVE when selecting paths
  nvme-multipath: always requeue I/O when updating the ANA state
  nvme: set 'failfast_expired' in nvme_remove_namespaces()

 drivers/nvme/host/core.c      |  7 +++++++
 drivers/nvme/host/multipath.c | 23 +++++++++++++++++------
 2 files changed, 24 insertions(+), 6 deletions(-)

-- 
2.35.3

[PATCH 1/4] nvme-multipath: fixup typo when clearing DISK_LIVE

[Hannes Reinecke]

NVME_NSHEAD_DISK_LIVE is a flag for 'struct nvme_ns_head', not
'struct nvme_ns'.

Signed-off-by: Hannes Reinecke <hare@kernel.org>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/host/multipath.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 91d9eb3c22ef..c9d23b1b8efc 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -646,7 +646,7 @@ static void nvme_mpath_set_live(struct nvme_ns *ns)
 		rc = device_add_disk(&head->subsys->dev, head->disk,
 				     nvme_ns_attr_groups);
 		if (rc) {
-			clear_bit(NVME_NSHEAD_DISK_LIVE, &ns->flags);
+			clear_bit(NVME_NSHEAD_DISK_LIVE, &head->flags);
 			return;
 		}
 		nvme_add_ns_head_cdev(head);
-- 
2.35.3

Re: [PATCH 1/4] nvme-multipath: fixup typo when clearing DISK_LIVE

[Sagi Grimberg]

On 06/09/2024 10:18, Hannes Reinecke wrote:
> NVME_NSHEAD_DISK_LIVE is a flag for 'struct nvme_ns_head', not
> 'struct nvme_ns'.
>
> Signed-off-by: Hannes Reinecke <hare@kernel.org>
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Christoph also gave a Reviewed-by tag iirc