[PATCH 00/10] mdadm tests fix

[PATCH 00/10] mdadm tests fix

[Xiao Ni]

This is the fourth patch set which enhance/fix mdadm regression tests.

Xiao Ni (10):
  mdadm/Grow: Update new level when starting reshape
  mdadm/Grow: Update reshape_progress to need_back after reshape
    finishes
  mdadm/Grow: Can't open raid when running --grow --continue
  mdadm/Grow: sleep a while after removing disk in impose_level
  mdadm/tests: wait until level changes
  mdadm/tests: 07changelevels fix
  mdadm/tests: Remove 07reshape5intr.broken
  mdadm/tests: 07testreshape5 fix
  mdadm/tests: remove 09imsm-assemble.broken
  mdadm/Manage: record errno

 Grow.c                       | 31 +++++++++++++++++++------
 Manage.c                     |  8 ++++---
 tests/05r6tor0               |  4 ++++
 tests/07changelevels         | 27 ++++++++++------------
 tests/07changelevels.broken  |  9 --------
 tests/07reshape5intr.broken  | 45 ------------------------------------
 tests/07testreshape5         |  1 +
 tests/07testreshape5.broken  | 12 ----------
 tests/09imsm-assemble.broken |  6 -----
 tests/func.sh                |  4 ++++
 10 files changed, 50 insertions(+), 97 deletions(-)
 delete mode 100644 tests/07changelevels.broken
 delete mode 100644 tests/07reshape5intr.broken
 delete mode 100644 tests/07testreshape5.broken
 delete mode 100644 tests/09imsm-assemble.broken

-- 
2.32.0 (Apple Git-132)

[PATCH 01/10] mdadm/Grow: Update new level when starting reshape

[Xiao Ni]

Reshape needs to specify a backup file when it can't update data offset
of member disks. For this situation, first, it starts reshape and then
it kicks off mdadm-grow-continue service which does backup job and
monitors the reshape process. The service is a new process, so it needs
to read superblock from member disks to get information.

But in the first step, it doesn't update new level in superblock. So
it can't change level after reshape finishes, because the new level is
not right. So records the new level in the first step.

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 Grow.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Grow.c b/Grow.c
index 5810b128aa99..97e48d86a33f 100644
--- a/Grow.c
+++ b/Grow.c
@@ -2946,6 +2946,9 @@ static int impose_reshape(struct mdinfo *sra,
 		if (!err && sysfs_set_num(sra, NULL, "layout",
 					  reshape->after.layout) < 0)
 			err = errno;
+		if (!err && sysfs_set_num(sra, NULL, "new_level",
+					info->new_level) < 0)
+			err = errno;
 		if (!err && subarray_set_num(container, sra, "raid_disks",
 					     reshape->after.data_disks +
 					     reshape->parity) < 0)
-- 
2.32.0 (Apple Git-132)

[PATCH 02/10] mdadm/Grow: Update reshape_progress to need_back after reshape finishes

[Xiao Ni]

It tries to update data offset when kicking off reshape. If it can't
change data offset, it needs to use child_monitor to monitor reshape
progress and do back up job. And it needs to update reshape_progress
to need_back when reshape finishes. If not, it will be in a infinite
loop.

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 Grow.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/Grow.c b/Grow.c
index 97e48d86a33f..6b621aea4ecc 100644
--- a/Grow.c
+++ b/Grow.c
@@ -4142,8 +4142,7 @@ int progress_reshape(struct mdinfo *info, struct reshape *reshape,
 		 * waiting forever on a dead array
 		 */
 		char action[SYSFS_MAX_BUF_SIZE];
-		if (sysfs_get_str(info, NULL, "sync_action", action, sizeof(action)) <= 0 ||
-		    strncmp(action, "reshape", 7) != 0)
+		if (sysfs_get_str(info, NULL, "sync_action", action, sizeof(action)) <= 0)
 			break;
 		/* Some kernels reset 'sync_completed' to zero
 		 * before setting 'sync_action' to 'idle'.
@@ -4151,12 +4150,18 @@ int progress_reshape(struct mdinfo *info, struct reshape *reshape,
 		 */
 		if (completed == 0 && advancing &&
 		    strncmp(action, "idle", 4) == 0 &&
-		    info->reshape_progress > 0)
+		    info->reshape_progress > 0) {
+			info->reshape_progress = need_backup;
 			break;
+		}
 		if (completed == 0 && !advancing &&
 		    strncmp(action, "idle", 4) == 0 &&
 		    info->reshape_progress <
-		    (info->component_size * reshape->after.data_disks))
+		    (info->component_size * reshape->after.data_disks)) {
+			info->reshape_progress = need_backup;
+			break;
+		}
+		if (strncmp(action, "reshape", 7) != 0)
 			break;
 		sysfs_wait(fd, NULL);
 		if (sysfs_fd_get_ll(fd, &completed) < 0)
-- 
2.32.0 (Apple Git-132)

[PATCH 03/10] mdadm/Grow: Can't open raid when running --grow --continue

[Xiao Ni]

It passes 'array' as devname in Grow_continue. So it fails to
open raid device. Use mdinfo to open raid device.

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 Grow.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/Grow.c b/Grow.c
index 6b621aea4ecc..2a7587315817 100644
--- a/Grow.c
+++ b/Grow.c
@@ -3688,9 +3688,12 @@ started:
 		set_array_size(st, info, info->text_version);
 
 	if (info->new_level != reshape.level) {
-		if (fd < 0)
-			fd = open(devname, O_RDONLY);
-		impose_level(fd, info->new_level, devname, verbose);
+		fd = open_dev(sra->sys_name);
+		if (fd < 0) {
+			pr_err("Can't open %s\n", sra->sys_name);
+			goto out;
+		}
+		impose_level(fd, info->new_level, sra->sys_name, verbose);
 		close(fd);
 		if (info->new_level == 0)
 			st->update_tail = NULL;
-- 
2.32.0 (Apple Git-132)

[PATCH 04/10] mdadm/Grow: sleep a while after removing disk in impose_level

[Xiao Ni]

It needs to remove disks when reshaping from raid456 to raid0. In
kernel space it sets MD_RECOVERY_RUNNING. And it will fail to change
level. So wait sometime to let md thread to clear this flag.

This is found by test case 05r6tor0.

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 Grow.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Grow.c b/Grow.c
index 2a7587315817..aaf349e9722f 100644
--- a/Grow.c
+++ b/Grow.c
@@ -3028,6 +3028,12 @@ static int impose_level(int fd, int level, char *devname, int verbose)
 			      makedev(disk.major, disk.minor));
 			hot_remove_disk(fd, makedev(disk.major, disk.minor), 1);
 		}
+		/*
+		 * hot_remove_disk lets kernel set MD_RECOVERY_RUNNING
+		 * and it can't set level. It needs to wait sometime
+		 * to let md thread to clear the flag.
+		 */
+		sleep_for(5, 0, true);
 	}
 	c = map_num(pers, level);
 	if (c) {
-- 
2.32.0 (Apple Git-132)

[PATCH 05/10] mdadm/tests: wait until level changes

[Xiao Ni]

check wait waits reshape finishes, but it doesn't wait level changes.
The level change happens in a forked child progress. So we need to
search the child progress and monitor it.

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 tests/05r6tor0 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tests/05r6tor0 b/tests/05r6tor0
index 2fd51f2ea4bb..b2685b721c2e 100644
--- a/tests/05r6tor0
+++ b/tests/05r6tor0
@@ -13,6 +13,10 @@ check raid5
 testdev $md0 3 19456 512
 mdadm -G $md0 -l0
 check wait; sleep 1
+while ps auxf | grep "mdadm -G" | grep -v grep
+do
+        sleep 1
+done
 check raid0
 testdev $md0 3 19456 512
 mdadm -G $md0 -l5 --add $dev3 $dev4
-- 
2.32.0 (Apple Git-132)

[PATCH 06/10] mdadm/tests: 07changelevels fix

[Xiao Ni]

There are five changes to this case.

1. remove testdev check. It can't work anymore and check if it's a
block device directly.

2. It can't change level and chunk size at the same time

3. Sleep more than 10s before check wait.
The test devices are small. Sometimes it can finish so quickly once
the reshape just starts. mdadm will be stuck before it waits reshape
to start. So the sync speed is limited. And it restores the sync speed
when it waits reshape to finish. It's good for case without backup
file.

It uses systemd service mdadm-grow-continue to monitor reshape
progress when specifying backup file. If reshape finishes so quickly
before it starts monitoring reshape progress, the daemon will be stuck
too. Because reshape_progress is 0 which means the reshape hasn't been
started. So give more time to let service can get right information
from kernel space.

But before getting these information. It needs to suspend array. At
the same time the reshape is running. The kernel reshape daemon will
update metadata 10s. So it needs to limit the sync speed more than 10s
before restoring sync speed. Then systemd service can suspend array
and start monitoring reshape progress.

4. Wait until mdadm-grow-continue service exits
mdadm --wait doesn't wait systemd service. For the case that needs
backup file, systemd service deletes the backup file after reshape
finishes. In this test case, it runs next case when reshape finishes.
And it fails because it can't create backup file because the backup
file exits.

5. Don't reshape from raid5 to raid1. It can't work now.

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 tests/07changelevels        | 27 ++++++++++++---------------
 tests/07changelevels.broken |  9 ---------
 tests/func.sh               |  4 ++++
 3 files changed, 16 insertions(+), 24 deletions(-)
 delete mode 100644 tests/07changelevels.broken

diff --git a/tests/07changelevels b/tests/07changelevels
index a328874ac43f..3df8660e6bae 100644
--- a/tests/07changelevels
+++ b/tests/07changelevels
@@ -10,7 +10,6 @@ export MDADM_GROW_VERIFY=1
 dotest() {
  sleep 2
  check wait
- testdev $md0 $1 19968 64 nd
  blockdev --flushbufs $md0
  cmp -s -n $[textK*1024] $md0 /tmp/RandFile || { echo cmp failed; exit 2; }
  # write something new - shift chars 4 space
@@ -24,7 +23,7 @@ checkgeo() {
  # level raid_disks chunk_size layout
  dev=$1
  shift
- sleep 0.5
+ sleep 15
  check wait
  sleep 1
  for attr in level raid_disks chunk_size layout
@@ -43,22 +42,25 @@ checkgeo() {
 
 bu=/tmp/md-test-backup
 rm -f $bu
-mdadm -CR $md0 -l1 -n2 -x1 $dev0 $dev1 $dev2 -z 19968
-testdev $md0 1 $mdsize1a 64
+mdadm -CR $md0 -l1 -n2 -x1 $dev0 $dev1 $dev2
+[ -b $md0 ] || die "$1 isn't a block device."
 dd if=/tmp/RandFile of=$md0
 dotest 1
 
-mdadm --grow $md0 -l5 -n3 --chunk 64
+mdadm --grow $md0 -l5 -n3
+checkgeo md0 raid5 3
 dotest 2
 
 mdadm $md0 --add $dev3 $dev4
 mdadm --grow $md0 -n4 --chunk 32
+checkgeo md0 raid5 4 $[32*1024]
 dotest 3
 
 mdadm -G $md0 -l6 --backup-file $bu
+checkgeo md0 raid6 5 $[32*1024]
 dotest 3
 
-mdadm -G /dev/md0 --array-size 39936
+mdadm -G /dev/md0 --array-size 37888
 mdadm -G $md0 -n4 --backup-file $bu
 checkgeo md0 raid6 4 $[32*1024]
 dotest 2
@@ -67,14 +69,11 @@ mdadm -G $md0 -l5 --backup-file $bu
 checkgeo md0 raid5 3 $[32*1024]
 dotest 2
 
-mdadm -G /dev/md0 --array-size 19968
+mdadm -G /dev/md0 --array-size 18944
 mdadm -G $md0 -n2 --backup-file $bu
 checkgeo md0 raid5 2 $[32*1024]
 dotest 1
 
-mdadm -G --level=1 $md0
-dotest 1
-
 # now repeat that last few steps only with a degraded array.
 mdadm -S $md0
 mdadm -CR $md0 -l6 -n5 $dev0 $dev1 $dev2 $dev3 $dev4
@@ -83,7 +82,7 @@ dotest 3
 
 mdadm $md0 --fail $dev0
 
-mdadm -G /dev/md0 --array-size 37888
+mdadm -G /dev/md0 --array-size 35840
 mdadm -G $md0 -n4 --backup-file $bu
 dotest 2
 checkgeo md0 raid6 4 $[512*1024]
@@ -103,12 +102,10 @@ dotest 2
 mdadm -G $md0 -l5 --backup-file $bu
 dotest 2
 
-mdadm -G /dev/md0 --array-size 18944
+mdadm -G /dev/md0 --array-size 17920
 mdadm -G $md0 -n2 --backup-file $bu
 dotest 1
 checkgeo md0 raid5 2 $[512*1024]
 mdadm $md0 --fail $dev2
 
-mdadm -G --level=1 $md0
-dotest 1
-checkgeo md0 raid1 2
+mdadm -S $md0
diff --git a/tests/07changelevels.broken b/tests/07changelevels.broken
index 9b930d932c48..000000000000
--- a/tests/07changelevels.broken
+++ /dev/null
@@ -1,9 +0,0 @@
-always fails
-
-Fails with errors:
-
-    mdadm: /dev/loop0 is smaller than given size. 18976K < 19968K + metadata
-    mdadm: /dev/loop1 is smaller than given size. 18976K < 19968K + metadata
-    mdadm: /dev/loop2 is smaller than given size. 18976K < 19968K + metadata
-
-    ERROR: /dev/md0 isn't a block device.
diff --git a/tests/func.sh b/tests/func.sh
index e7ccc4fc66eb..567d91d9173e 100644
--- a/tests/func.sh
+++ b/tests/func.sh
@@ -362,6 +362,10 @@ check() {
 		do
 			sleep 0.5
 		done
+		while ps auxf | grep "mdadm --grow --continue" | grep -v grep
+		do
+			sleep 1
+		done
 		echo $min > /proc/sys/dev/raid/speed_limit_min
 		echo $max > /proc/sys/dev/raid/speed_limit_max
 	;;
-- 
2.32.0 (Apple Git-132)

[PATCH 07/10] mdadm/tests: Remove 07reshape5intr.broken

[Xiao Ni]

07reshape5intr can run successfully now.

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 tests/07reshape5intr.broken | 45 -------------------------------------
 1 file changed, 45 deletions(-)
 delete mode 100644 tests/07reshape5intr.broken

diff --git a/tests/07reshape5intr.broken b/tests/07reshape5intr.broken
index efe52a667172..000000000000
--- a/tests/07reshape5intr.broken
+++ /dev/null
@@ -1,45 +0,0 @@
-always fails
-
-This patch, recently added to md-next causes the test to always fail:
-
-7e6ba434cc60 ("md: don't unregister sync_thread with reconfig_mutex
-held")
-
-The new error is simply:
-
-   ERROR: no reshape happening
-
-Before the patch, the error seen is below.
-
---
-
-fails infrequently
-
-Fails roughly 1 in 4 runs with errors:
-
-    mdadm: Merging with already-assembled /dev/md/0
-    mdadm: cannot re-read metadata from /dev/loop6 - aborting
-
-    ERROR: no reshape happening
-
-Also have seen a random deadlock:
-
-     INFO: task mdadm:109702 blocked for more than 30 seconds.
-           Not tainted 5.18.0-rc3-eid-vmlocalyes-dbg-00095-g3c2b5427979d #2040
-     "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
-     task:mdadm           state:D stack:    0 pid:109702 ppid:     1 flags:0x00004000
-     Call Trace:
-      <TASK>
-      __schedule+0x67e/0x13b0
-      schedule+0x82/0x110
-      mddev_suspend+0x2e1/0x330
-      suspend_lo_store+0xbd/0x140
-      md_attr_store+0xcb/0x130
-      sysfs_kf_write+0x89/0xb0
-      kernfs_fop_write_iter+0x202/0x2c0
-      new_sync_write+0x222/0x330
-      vfs_write+0x3bc/0x4d0
-      ksys_write+0xd9/0x180
-      __x64_sys_write+0x43/0x50
-      do_syscall_64+0x3b/0x90
-      entry_SYSCALL_64_after_hwframe+0x44/0xae
-- 
2.32.0 (Apple Git-132)

[PATCH 08/10] mdadm/tests: 07testreshape5 fix

[Xiao Ni]

Init dir to avoid test failure.

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 tests/07testreshape5        |  1 +
 tests/07testreshape5.broken | 12 ------------
 2 files changed, 1 insertion(+), 12 deletions(-)
 delete mode 100644 tests/07testreshape5.broken

diff --git a/tests/07testreshape5 b/tests/07testreshape5
index 0e1f25f98bc8..d90fd15e0e61 100644
--- a/tests/07testreshape5
+++ b/tests/07testreshape5
@@ -4,6 +4,7 @@
 # kernel md code to move data into and out of variously
 # shaped md arrays.
 set -x
+dir="."
 layouts=(la ra ls rs)
 for level in 5 6
 do
diff --git a/tests/07testreshape5.broken b/tests/07testreshape5.broken
index a8ce03e491b3..000000000000
--- a/tests/07testreshape5.broken
+++ /dev/null
@@ -1,12 +0,0 @@
-always fails
-
-Test seems to run 'test_stripe' at $dir directory, but $dir is never
-set. If $dir is adjusted to $PWD, the test still fails with:
-
-    mdadm: /dev/loop2 is not suitable for this array.
-    mdadm: create aborted
-    ++ return 1
-    ++ cmp -s -n 8192 /dev/md0 /tmp/RandFile
-    ++ echo cmp failed
-    cmp failed
-    ++ exit 2
-- 
2.32.0 (Apple Git-132)

[PATCH 09/10] mdadm/tests: remove 09imsm-assemble.broken

[Xiao Ni]

09imsm-assemble can run successfully.

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 tests/09imsm-assemble.broken | 6 ------
 1 file changed, 6 deletions(-)
 delete mode 100644 tests/09imsm-assemble.broken

diff --git a/tests/09imsm-assemble.broken b/tests/09imsm-assemble.broken
index a6d4d5cf911b..000000000000
--- a/tests/09imsm-assemble.broken
+++ /dev/null
@@ -1,6 +0,0 @@
-fails infrequently
-
-Fails roughly 1 in 10 runs with errors:
-
-    mdadm: /dev/loop2 is still in use, cannot remove.
-    /dev/loop2 removal from /dev/md/container should have succeeded
-- 
2.32.0 (Apple Git-132)

[PATCH 10/10] mdadm/Manage: record errno

[Xiao Ni]

Sometimes it reports:
mdadm: failed to stop array /dev/md0: Success
It's the reason the errno is reset. So record errno during the loop.

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 Manage.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/Manage.c b/Manage.c
index 241de05520d6..aba97df8e122 100644
--- a/Manage.c
+++ b/Manage.c
@@ -238,13 +238,14 @@ int Manage_stop(char *devname, int fd, int verbose, int will_retry)
 					    "array_state",
 					    "inactive")) < 0 &&
 		       errno == EBUSY) {
+			err = errno;
 			sleep_for(0, MSEC_TO_NSEC(200), true);
 			count--;
 		}
 		if (err) {
 			if (verbose >= 0)
 				pr_err("failed to stop array %s: %s\n",
-				       devname, strerror(errno));
+				       devname, strerror(err));
 			rv = 1;
 			goto out;
 		}
@@ -438,14 +439,15 @@ done:
 	count = 25; err = 0;
 	while (count && fd >= 0 &&
 	       (err = ioctl(fd, STOP_ARRAY, NULL)) < 0 && errno == EBUSY) {
+		err = errno;
 		sleep_for(0, MSEC_TO_NSEC(200), true);
 		count --;
 	}
 	if (fd >= 0 && err) {
 		if (verbose >= 0) {
 			pr_err("failed to stop array %s: %s\n",
-			       devname, strerror(errno));
-			if (errno == EBUSY)
+			       devname, strerror(err));
+			if (err == EBUSY)
 				cont_err("Perhaps a running process, mounted filesystem or active volume group?\n");
 		}
 		rv = 1;
-- 
2.32.0 (Apple Git-132)

Re: [PATCH 01/10] mdadm/Grow: Update new level when starting reshape

[Mariusz Tkaczyk]

Hi Xiao,
Thanks for patches.

On Wed, 28 Aug 2024 10:11:41 +0800
Xiao Ni <xni@redhat.com> wrote:

> Reshape needs to specify a backup file when it can't update data offset
> of member disks. For this situation, first, it starts reshape and then
> it kicks off mdadm-grow-continue service which does backup job and
> monitors the reshape process. The service is a new process, so it needs
> to read superblock from member disks to get information.

Looks like kernel is fine with reset the same level so I don't see a risk in
this change for other scenarios but please mention that.

> 
> But in the first step, it doesn't update new level in superblock. So
> it can't change level after reshape finishes, because the new level is
> not right. So records the new level in the first step.

> 
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>  Grow.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Grow.c b/Grow.c
> index 5810b128aa99..97e48d86a33f 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -2946,6 +2946,9 @@ static int impose_reshape(struct mdinfo *sra,
>  		if (!err && sysfs_set_num(sra, NULL, "layout",
>  					  reshape->after.layout) < 0)
>  			err = errno;
> +		if (!err && sysfs_set_num(sra, NULL, "new_level",
> +					info->new_level) < 0)
> +			err = errno;

Please add empty line before and after and please merge if statement to one
line (we support up to 100).


>  		if (!err && subarray_set_num(container, sra, "raid_disks",
>  					     reshape->after.data_disks +
>  					     reshape->parity) < 0)


Thanks,
Mariusz

Re: [PATCH 02/10] mdadm/Grow: Update reshape_progress to need_back after reshape finishes

[Mariusz Tkaczyk]

On Wed, 28 Aug 2024 10:11:42 +0800
Xiao Ni <xni@redhat.com> wrote:

> It tries to update data offset when kicking off reshape. If it can't
> change data offset, it needs to use child_monitor to monitor reshape
> progress and do back up job. And it needs to update reshape_progress
> to need_back when reshape finishes. If not, it will be in a infinite
> loop.
> 
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>  Grow.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/Grow.c b/Grow.c
> index 97e48d86a33f..6b621aea4ecc 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -4142,8 +4142,7 @@ int progress_reshape(struct mdinfo *info, struct
> reshape *reshape,
>  		 * waiting forever on a dead array
>  		 */
>  		char action[SYSFS_MAX_BUF_SIZE];
> -		if (sysfs_get_str(info, NULL, "sync_action", action,
> sizeof(action)) <= 0 ||
> -		    strncmp(action, "reshape", 7) != 0)
> +		if (sysfs_get_str(info, NULL, "sync_action", action,
> sizeof(action)) <= 0) break;

There must be empty line after declaration.


>  		/* Some kernels reset 'sync_completed' to zero
>  		 * before setting 'sync_action' to 'idle'.
> @@ -4151,12 +4150,18 @@ int progress_reshape(struct mdinfo *info, struct
> reshape *reshape, */
>  		if (completed == 0 && advancing &&
>  		    strncmp(action, "idle", 4) == 0 &&
> -		    info->reshape_progress > 0)
> +		    info->reshape_progress > 0) {
> +			info->reshape_progress = need_backup;
>  			break;
> +		}
>  		if (completed == 0 && !advancing &&
>  		    strncmp(action, "idle", 4) == 0 &&
>  		    info->reshape_progress <
> -		    (info->component_size * reshape->after.data_disks))
> +		    (info->component_size * reshape->after.data_disks)) {
> +			info->reshape_progress = need_backup;
> +			break;

This look weird to assign need_backup (suggests boolean field) to
reshape_progress but it is not your code so you have my ack.

Thanks,
Mariusz

> +		}
> +		if (strncmp(action, "reshape", 7) != 0)
>  			break;
>  		sysfs_wait(fd, NULL);
>  		if (sysfs_fd_get_ll(fd, &completed) < 0)

Re: [PATCH 03/10] mdadm/Grow: Can't open raid when running --grow --continue

[Mariusz Tkaczyk]

On Wed, 28 Aug 2024 10:11:43 +0800
Xiao Ni <xni@redhat.com> wrote:

> It passes 'array' as devname in Grow_continue. So it fails to
> open raid device. Use mdinfo to open raid device.
> 
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>  Grow.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/Grow.c b/Grow.c
> index 6b621aea4ecc..2a7587315817 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -3688,9 +3688,12 @@ started:
>  		set_array_size(st, info, info->text_version);
>  
>  	if (info->new_level != reshape.level) {
> -		if (fd < 0)
> -			fd = open(devname, O_RDONLY);
> -		impose_level(fd, info->new_level, devname, verbose);
> +		fd = open_dev(sra->sys_name);
> +		if (fd < 0) {
> +			pr_err("Can't open %s\n", sra->sys_name);
> +			goto out;
> +		}
> +		impose_level(fd, info->new_level, sra->sys_name, verbose);
>  		close(fd);
>  		if (info->new_level == 0)
>  			st->update_tail = NULL;

You can consider declaring fd locally (inside if) but it is fine anyway.
You can also switch close(fd) to close_fd(&fd); because this resource is
probably reused later in the function body.

Anyway, LGTM but I will run my internal IMSM test suite to confirm that there is
no regression for IMSM before I will merge patchset.

Thanks,
Mariusz

Re: [PATCH 01/10] mdadm/Grow: Update new level when starting reshape

[Mariusz Tkaczyk]

On Mon, 2 Sep 2024 11:50:13 +0200
Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com> wrote:

> Hi Xiao,
> Thanks for patches.
> 
> On Wed, 28 Aug 2024 10:11:41 +0800
> Xiao Ni <xni@redhat.com> wrote:
> 
> > Reshape needs to specify a backup file when it can't update data offset
> > of member disks. For this situation, first, it starts reshape and then
> > it kicks off mdadm-grow-continue service which does backup job and
> > monitors the reshape process. The service is a new process, so it needs
> > to read superblock from member disks to get information.  
> 
> Looks like kernel is fine with reset the same level so I don't see a risk in
> this change for other scenarios but please mention that.
> 

Sorry, I didn't notice that it is new field. We should not update it if it
doesn't exist. Perhaps, we should print message that kernel patch is needed? 

> > 
> > But in the first step, it doesn't update new level in superblock. So
> > it can't change level after reshape finishes, because the new level is
> > not right. So records the new level in the first step.  
> 
> > 
> > Signed-off-by: Xiao Ni <xni@redhat.com>
> > ---
> >  Grow.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/Grow.c b/Grow.c
> > index 5810b128aa99..97e48d86a33f 100644
> > --- a/Grow.c
> > +++ b/Grow.c
> > @@ -2946,6 +2946,9 @@ static int impose_reshape(struct mdinfo *sra,
> >  		if (!err && sysfs_set_num(sra, NULL, "layout",
> >  					  reshape->after.layout) < 0)
> >  			err = errno;
> > +		if (!err && sysfs_set_num(sra, NULL, "new_level",
> > +					info->new_level) < 0)
> > +			err = errno;  
> 
> Please add empty line before and after and please merge if statement to one
> line (we support up to 100).
> 
> 
> >  		if (!err && subarray_set_num(container, sra, "raid_disks",
> >  					     reshape->after.data_disks +
> >  					     reshape->parity) < 0)  
> 
> 
> Thanks,
> Mariusz
> 

Re: [PATCH 01/10] mdadm/Grow: Update new level when starting reshape

[Xiao Ni]

On Mon, Sep 2, 2024 at 5:50 PM Mariusz Tkaczyk
<mariusz.tkaczyk@linux.intel.com> wrote:
>
> Hi Xiao,
> Thanks for patches.
>
> On Wed, 28 Aug 2024 10:11:41 +0800
> Xiao Ni <xni@redhat.com> wrote:
>
> > Reshape needs to specify a backup file when it can't update data offset
> > of member disks. For this situation, first, it starts reshape and then
> > it kicks off mdadm-grow-continue service which does backup job and
> > monitors the reshape process. The service is a new process, so it needs
> > to read superblock from member disks to get information.
>
Hi Mariusz

> Looks like kernel is fine with reset the same level so I don't see a risk in
> this change for other scenarios but please mention that.

impose_level can't be called if the new level is the same as the old
level. It already checks it before calling impose_level.

>
> >
> > But in the first step, it doesn't update new level in superblock. So
> > it can't change level after reshape finishes, because the new level is
> > not right. So records the new level in the first step.
>
> >
> > Signed-off-by: Xiao Ni <xni@redhat.com>
> > ---
> >  Grow.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/Grow.c b/Grow.c
> > index 5810b128aa99..97e48d86a33f 100644
> > --- a/Grow.c
> > +++ b/Grow.c
> > @@ -2946,6 +2946,9 @@ static int impose_reshape(struct mdinfo *sra,
> >               if (!err && sysfs_set_num(sra, NULL, "layout",
> >                                         reshape->after.layout) < 0)
> >                       err = errno;
> > +             if (!err && sysfs_set_num(sra, NULL, "new_level",
> > +                                     info->new_level) < 0)
> > +                     err = errno;
>
> Please add empty line before and after and please merge if statement to one
> line (we support up to 100).

Ok
>
>
> >               if (!err && subarray_set_num(container, sra, "raid_disks",
> >                                            reshape->after.data_disks +
> >                                            reshape->parity) < 0)
>
>
> Thanks,
> Mariusz
>

Re: [PATCH 01/10] mdadm/Grow: Update new level when starting reshape

[Xiao Ni]

On Mon, Sep 2, 2024 at 6:10 PM Mariusz Tkaczyk
<mariusz.tkaczyk@linux.intel.com> wrote:
>
> On Mon, 2 Sep 2024 11:50:13 +0200
> Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com> wrote:
>
> > Hi Xiao,
> > Thanks for patches.
> >
> > On Wed, 28 Aug 2024 10:11:41 +0800
> > Xiao Ni <xni@redhat.com> wrote:
> >
> > > Reshape needs to specify a backup file when it can't update data offset
> > > of member disks. For this situation, first, it starts reshape and then
> > > it kicks off mdadm-grow-continue service which does backup job and
> > > monitors the reshape process. The service is a new process, so it needs
> > > to read superblock from member disks to get information.
> >
> > Looks like kernel is fine with reset the same level so I don't see a risk in
> > this change for other scenarios but please mention that.
> >
>
> Sorry, I didn't notice that it is new field. We should not update it if it
> doesn't exist. Perhaps, we should print message that kernel patch is needed?

We can merge this patch set after the kernel patch is merged. Because
this change depends on the kernel change. If the kernel patch is
rejected, we need to find another way to fix this problem.

Regards
Xiao
>
> > >
> > > But in the first step, it doesn't update new level in superblock. So
> > > it can't change level after reshape finishes, because the new level is
> > > not right. So records the new level in the first step.
> >
> > >
> > > Signed-off-by: Xiao Ni <xni@redhat.com>
> > > ---
> > >  Grow.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/Grow.c b/Grow.c
> > > index 5810b128aa99..97e48d86a33f 100644
> > > --- a/Grow.c
> > > +++ b/Grow.c
> > > @@ -2946,6 +2946,9 @@ static int impose_reshape(struct mdinfo *sra,
> > >             if (!err && sysfs_set_num(sra, NULL, "layout",
> > >                                       reshape->after.layout) < 0)
> > >                     err = errno;
> > > +           if (!err && sysfs_set_num(sra, NULL, "new_level",
> > > +                                   info->new_level) < 0)
> > > +                   err = errno;
> >
> > Please add empty line before and after and please merge if statement to one
> > line (we support up to 100).
> >
> >
> > >             if (!err && subarray_set_num(container, sra, "raid_disks",
> > >                                          reshape->after.data_disks +
> > >                                          reshape->parity) < 0)
> >
> >
> > Thanks,
> > Mariusz
> >
>

Re: [PATCH 04/10] mdadm/Grow: sleep a while after removing disk in impose_level

[Xiao Ni]

On Mon, Sep 2, 2024 at 6:14 PM Mariusz Tkaczyk
<mariusz.tkaczyk@linux.intel.com> wrote:
>
> On Wed, 28 Aug 2024 10:11:44 +0800
> Xiao Ni <xni@redhat.com> wrote:
>
> > It needs to remove disks when reshaping from raid456 to raid0. In
> > kernel space it sets MD_RECOVERY_RUNNING. And it will fail to change
> > level. So wait sometime to let md thread to clear this flag.
> >
> > This is found by test case 05r6tor0.
> >
> > Signed-off-by: Xiao Ni <xni@redhat.com>
> > ---
> >  Grow.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/Grow.c b/Grow.c
> > index 2a7587315817..aaf349e9722f 100644
> > --- a/Grow.c
> > +++ b/Grow.c
> > @@ -3028,6 +3028,12 @@ static int impose_level(int fd, int level, char
> > *devname, int verbose) makedev(disk.major, disk.minor));
> >                       hot_remove_disk(fd, makedev(disk.major, disk.minor),
> > 1); }
> > +             /*
> > +              * hot_remove_disk lets kernel set MD_RECOVERY_RUNNING
> > +              * and it can't set level. It needs to wait sometime
> > +              * to let md thread to clear the flag.
> > +              */
> > +             sleep_for(5, 0, true);
>

Hi Mariusz

> Shouldn't we check sysfs is shorter intervals? I know that is the simplest way but
> big sleeps are generally not good.
>
> I will merge it if you don't want to rework it but you need to add log that we
> are waiting 5 second for the user to not panic that it is frozen.

Which sysfs do you mean? If we have a better way, I want to choose it.

Best Regards
Xiao

>
> Thanks,
> Mariusz
>

Re: [PATCH 04/10] mdadm/Grow: sleep a while after removing disk in impose_level

[Mariusz Tkaczyk]

On Tue, 3 Sep 2024 08:53:42 +0800
Xiao Ni <xni@redhat.com> wrote:

> On Mon, Sep 2, 2024 at 6:14 PM Mariusz Tkaczyk
> <mariusz.tkaczyk@linux.intel.com> wrote:
> >
> > On Wed, 28 Aug 2024 10:11:44 +0800
> > Xiao Ni <xni@redhat.com> wrote:
> >  
> > > It needs to remove disks when reshaping from raid456 to raid0. In
> > > kernel space it sets MD_RECOVERY_RUNNING. And it will fail to change
> > > level. So wait sometime to let md thread to clear this flag.
> > >
> > > This is found by test case 05r6tor0.
> > >
> > > Signed-off-by: Xiao Ni <xni@redhat.com>
> > > ---
> > >  Grow.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/Grow.c b/Grow.c
> > > index 2a7587315817..aaf349e9722f 100644
> > > --- a/Grow.c
> > > +++ b/Grow.c
> > > @@ -3028,6 +3028,12 @@ static int impose_level(int fd, int level, char
> > > *devname, int verbose) makedev(disk.major, disk.minor));
> > >                       hot_remove_disk(fd, makedev(disk.major, disk.minor),
> > > 1); }
> > > +             /*
> > > +              * hot_remove_disk lets kernel set MD_RECOVERY_RUNNING
> > > +              * and it can't set level. It needs to wait sometime
> > > +              * to let md thread to clear the flag.
> > > +              */
> > > +             sleep_for(5, 0, true);  
> >  
> 
> Hi Mariusz
> 
> > Shouldn't we check sysfs is shorter intervals? I know that is the simplest
> > way but big sleeps are generally not good.
> >
> > I will merge it if you don't want to rework it but you need to add log that
> > we are waiting 5 second for the user to not panic that it is frozen.  
> 
> Which sysfs do you mean? If we have a better way, I want to choose it.
> 

If we are sending hot remove to the disk, we can check if there is path
available: /sys/block/<mddev>/md/dev-{devnm}
if not, then device has been finally removed.
Eventually, we can see same in mdstat but checking path looks simpler to me.

Thanks,
Mariusz

Re: [PATCH 01/10] mdadm/Grow: Update new level when starting reshape

[Mariusz Tkaczyk]

On Tue, 3 Sep 2024 08:34:46 +0800
Xiao Ni <xni@redhat.com> wrote:

> On Mon, Sep 2, 2024 at 6:10 PM Mariusz Tkaczyk
> <mariusz.tkaczyk@linux.intel.com> wrote:
> >
> > On Mon, 2 Sep 2024 11:50:13 +0200
> > Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com> wrote:
> >  
> > > Hi Xiao,
> > > Thanks for patches.
> > >
> > > On Wed, 28 Aug 2024 10:11:41 +0800
> > > Xiao Ni <xni@redhat.com> wrote:
> > >  
> > > > Reshape needs to specify a backup file when it can't update data offset
> > > > of member disks. For this situation, first, it starts reshape and then
> > > > it kicks off mdadm-grow-continue service which does backup job and
> > > > monitors the reshape process. The service is a new process, so it needs
> > > > to read superblock from member disks to get information.  
> > >
> > > Looks like kernel is fine with reset the same level so I don't see a risk
> > > in this change for other scenarios but please mention that.
> > >  
> >
> > Sorry, I didn't notice that it is new field. We should not update it if it
> > doesn't exist. Perhaps, we should print message that kernel patch is
> > needed?  
> 
> We can merge this patch set after the kernel patch is merged. Because
> this change depends on the kernel change. If the kernel patch is
> rejected, we need to find another way to fix this problem.

We have to mention kernel commit in description then.


Let say that it is merged, we should probably notify user,
"hey your kernel is missing the kernel patch that allow this functionality to
work reliably, issue may occur". What do you think? Is it valuable?

Thanks,
Mariusz

Re: [PATCH 01/10] mdadm/Grow: Update new level when starting reshape

[Xiao Ni]

On Tue, Sep 3, 2024 at 3:34 PM Mariusz Tkaczyk
<mariusz.tkaczyk@linux.intel.com> wrote:
>
> On Tue, 3 Sep 2024 08:34:46 +0800
> Xiao Ni <xni@redhat.com> wrote:
>
> > On Mon, Sep 2, 2024 at 6:10 PM Mariusz Tkaczyk
> > <mariusz.tkaczyk@linux.intel.com> wrote:
> > >
> > > On Mon, 2 Sep 2024 11:50:13 +0200
> > > Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com> wrote:
> > >
> > > > Hi Xiao,
> > > > Thanks for patches.
> > > >
> > > > On Wed, 28 Aug 2024 10:11:41 +0800
> > > > Xiao Ni <xni@redhat.com> wrote:
> > > >
> > > > > Reshape needs to specify a backup file when it can't update data offset
> > > > > of member disks. For this situation, first, it starts reshape and then
> > > > > it kicks off mdadm-grow-continue service which does backup job and
> > > > > monitors the reshape process. The service is a new process, so it needs
> > > > > to read superblock from member disks to get information.
> > > >
> > > > Looks like kernel is fine with reset the same level so I don't see a risk
> > > > in this change for other scenarios but please mention that.
> > > >
> > >
> > > Sorry, I didn't notice that it is new field. We should not update it if it
> > > doesn't exist. Perhaps, we should print message that kernel patch is
> > > needed?
> >
> > We can merge this patch set after the kernel patch is merged. Because
> > this change depends on the kernel change. If the kernel patch is
> > rejected, we need to find another way to fix this problem.
>
> We have to mention kernel commit in description then.
>
>
> Let say that it is merged, we should probably notify user,
> "hey your kernel is missing the kernel patch that allow this functionality to
> work reliably, issue may occur". What do you think? Is it valuable?

Hi Mariusz

It makes sense. I'll add this in V2 once kernel patch is merged.

Regards
Xiao
>
> Thanks,
> Mariusz
>

Re: [PATCH 04/10] mdadm/Grow: sleep a while after removing disk in impose_level

[Xiao Ni]

在 2024/9/3 下午3:22, Mariusz Tkaczyk 写道:
> On Tue, 3 Sep 2024 08:53:42 +0800
> Xiao Ni <xni@redhat.com> wrote:
>
>> On Mon, Sep 2, 2024 at 6:14 PM Mariusz Tkaczyk
>> <mariusz.tkaczyk@linux.intel.com> wrote:
>>> On Wed, 28 Aug 2024 10:11:44 +0800
>>> Xiao Ni <xni@redhat.com> wrote:
>>>   
>>>> It needs to remove disks when reshaping from raid456 to raid0. In
>>>> kernel space it sets MD_RECOVERY_RUNNING. And it will fail to change
>>>> level. So wait sometime to let md thread to clear this flag.
>>>>
>>>> This is found by test case 05r6tor0.
>>>>
>>>> Signed-off-by: Xiao Ni <xni@redhat.com>
>>>> ---
>>>>   Grow.c | 6 ++++++
>>>>   1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/Grow.c b/Grow.c
>>>> index 2a7587315817..aaf349e9722f 100644
>>>> --- a/Grow.c
>>>> +++ b/Grow.c
>>>> @@ -3028,6 +3028,12 @@ static int impose_level(int fd, int level, char
>>>> *devname, int verbose) makedev(disk.major, disk.minor));
>>>>                        hot_remove_disk(fd, makedev(disk.major, disk.minor),
>>>> 1); }
>>>> +             /*
>>>> +              * hot_remove_disk lets kernel set MD_RECOVERY_RUNNING
>>>> +              * and it can't set level. It needs to wait sometime
>>>> +              * to let md thread to clear the flag.
>>>> +              */
>>>> +             sleep_for(5, 0, true);
>>>   
>> Hi Mariusz
>>
>>> Shouldn't we check sysfs is shorter intervals? I know that is the simplest
>>> way but big sleeps are generally not good.
>>>
>>> I will merge it if you don't want to rework it but you need to add log that
>>> we are waiting 5 second for the user to not panic that it is frozen.
>> Which sysfs do you mean? If we have a better way, I want to choose it.
>>
> If we are sending hot remove to the disk, we can check if there is path
> available: /sys/block/<mddev>/md/dev-{devnm}
> if not, then device has been finally removed.
> Eventually, we can see same in mdstat but checking path looks simpler to me.
>
> Thanks,
> Mariusz


Hi Mariusz

I check you method and it doesn't work. There are two steps in kernel 
space and they are async.

1. remove disk including remove the sysfs directory, set 
MD_RECOVERY_NEEDED and wake up md thread

2. Because MD_RECOVERY_NEEDED is set, kernel space sets 
MD_RECOVERY_RUNNING and queue a sync work. It doesn't do anything and 
clear MD_RECOVERY_RUNNING

So there is a time window. It depends on machines. Sometimes it fails 
when setting new level because MD_RECOVERY_RUNNING is set. Maybe we can 
add some check when removing disk. If it doesn't need to do 
sync/recovery, we don't need to set MD_RECOVERY_NEEDED. But now, we can 
add a sleep here as a solution. I'll add a log here to give admin.

Best Regards

Xiao

[PATCH 03/10] mdadm/Grow: Can't open raid when running --grow --continue

[Xiao Ni]

It passes 'array' as devname in Grow_continue. So it fails to
open raid device. Use mdinfo to open raid device.

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 Grow.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/Grow.c b/Grow.c
index 6b621aea4ecc..2a7587315817 100644
--- a/Grow.c
+++ b/Grow.c
@@ -3688,9 +3688,12 @@ started:
 		set_array_size(st, info, info->text_version);
 
 	if (info->new_level != reshape.level) {
-		if (fd < 0)
-			fd = open(devname, O_RDONLY);
-		impose_level(fd, info->new_level, devname, verbose);
+		fd = open_dev(sra->sys_name);
+		if (fd < 0) {
+			pr_err("Can't open %s\n", sra->sys_name);
+			goto out;
+		}
+		impose_level(fd, info->new_level, sra->sys_name, verbose);
 		close(fd);
 		if (info->new_level == 0)
 			st->update_tail = NULL;
-- 
2.32.0 (Apple Git-132)