[PATCH mdadm v3 0/2] Couple more testing fixes

[PATCH mdadm v3 0/2] Couple more testing fixes

[Logan Gunthorpe]

Hey,

The first commit is a new race condition seen with more recent
md/md-next kernels. udevadm settle needs to be called in a test
to avoid the race.

The second patch I have already sent a couple times[1], but have
reworked it based on feedback from Mariusz. The new version of the
patch keeps the checks on the device if it already exists but
changes them so it doesn't require openning the device (which has
annoying side effects).

Logan

[1] https://lkml.kernel.org/r/20220714223749.17250-1-logang@deltatee.com

--


Logan Gunthorpe (2):
  tests/00readonly: Run udevadm settle before setting ro
  mdadm: Don't open md device for CREATE and ASSEMBLE

 lib.c            | 12 ++++++++++++
 mdadm.c          | 40 ++++++++++++++++++++--------------------
 mdadm.h          |  1 +
 tests/00readonly |  1 +
 4 files changed, 34 insertions(+), 20 deletions(-)

--
2.30.2

[PATCH mdadm v3 1/2] tests/00readonly: Run udevadm settle before setting ro

[Logan Gunthorpe]

In some recent kernel versions, 00readonly fails with:

  mdadm: failed to set readonly for /dev/md0: Device or resource busy
  ERROR: array is not read-only!

This was traced down to a race condition with udev holding a reference
to the block device at the same time as trying to set it read only.

To fix this, call udevadm settle before setting the array read only.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 tests/00readonly | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/00readonly b/tests/00readonly
index 39202487f614..afe243b3a0b0 100644
--- a/tests/00readonly
+++ b/tests/00readonly
@@ -12,6 +12,7 @@ do
 			$dev1 $dev2 $dev3 $dev4 --assume-clean
 		check nosync
 		check $level
+		udevadm settle
 		mdadm -ro $md0
 		check readonly
 		state=$(cat /sys/block/md0/md/array_state)
-- 
2.30.2

[PATCH mdadm v3 2/2] mdadm: Don't open md device for CREATE and ASSEMBLE

[Logan Gunthorpe]

The mdadm command tries to open the md device for most modes, first
thing, no matter what. When running to create or assemble an array,
in most cases, the md device will not exist, the open call will fail
and everything will proceed correctly.

However, when running tests, a create or assembly command may be run
shortly after stopping an array and the old md device file may still
be around. Then, if create_on_open is set in the kernel, a new md
device will be created when mdadm does its initial open.

When mdadm gets around to creating the new device with the new_array
parameter it issues this error:

   mdadm: Fail to create md0 when using
   /sys/module/md_mod/parameters/new_array, fallback to creation via node

This is because an mddev was already created by the kernel with the
earlier open() call and thus the new one being created will fail with
EEXIST. The mdadm command will still successfully be created due to
falling back to the node creation method. However, the error message
itself will fail any test that's running it.

This issue is a race condition that is very rare, but a recent change
in the kernel caused this to happen more frequently: about 1 in 50
times.

To fix this, don't bother trying to open the md device for CREATE,
ASSEMBLE and BUILD commands, as the file descriptor will never be used
anyway even if it is successfully openned. The mdfd has not been used
for these commands since:

   7f91af49ad09 ("Delay creation of array devices for assemble/build/create")

The checks that were done on the open device can be changed to being
done with stat.

Side note: it would be nice to disable create_on_open as well to help
solve this, but it seems the work for this was never finished. By default,
mdadm will create using the old node interface when a name is specified
unless the user specifically puts names=yes in a config file, which
doesn't seem to be common or desirable to require this..

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 lib.c   | 12 ++++++++++++
 mdadm.c | 40 ++++++++++++++++++++--------------------
 mdadm.h |  1 +
 3 files changed, 33 insertions(+), 20 deletions(-)

diff --git a/lib.c b/lib.c
index 7e3e3d473995..e395b28d7d07 100644
--- a/lib.c
+++ b/lib.c
@@ -164,6 +164,18 @@ char *stat2devnm(struct stat *st)
 	return devid2devnm(st->st_rdev);
 }
 
+bool stat_is_md_dev(struct stat *st)
+{
+	if ((S_IFMT & st->st_mode) != S_IFBLK)
+		return false;
+	if (major(st->st_rdev) == MD_MAJOR)
+		return true;
+	if (major(st->st_rdev) == (unsigned)get_mdp_major())
+		return true;
+
+	return false;
+}
+
 char *fd2devnm(int fd)
 {
 	struct stat stb;
diff --git a/mdadm.c b/mdadm.c
index 56722ed997a2..db6524a9994a 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -1349,6 +1349,9 @@ int main(int argc, char *argv[])
 
 	if (mode == MANAGE || mode == BUILD || mode == CREATE ||
 	    mode == GROW || (mode == ASSEMBLE && ! c.scan)) {
+		struct stat stb;
+		int ret;
+
 		if (devs_found < 1) {
 			pr_err("an md device must be given in this mode\n");
 			exit(2);
@@ -1361,6 +1364,12 @@ int main(int argc, char *argv[])
 			mdfd = open_mddev(devlist->devname, 1);
 			if (mdfd < 0)
 				exit(1);
+
+			ret = fstat(mdfd, &stb);
+			if (ret) {
+				pr_err("fstat failed on %s.\n", devlist->devname);
+				exit(1);
+			}
 		} else {
 			char *bname = basename(devlist->devname);
 
@@ -1368,30 +1377,21 @@ int main(int argc, char *argv[])
 				pr_err("Name %s is too long.\n", devlist->devname);
 				exit(1);
 			}
-			/* non-existent device is OK */
-			mdfd = open_mddev(devlist->devname, 0);
-		}
-		if (mdfd == -2) {
-			pr_err("device %s exists but is not an md array.\n", devlist->devname);
-			exit(1);
-		}
-		if ((int)ident.super_minor == -2) {
-			struct stat stb;
-			if (mdfd < 0) {
+
+			ret = stat(devlist->devname, &stb);
+			if (ident.super_minor == -2 && ret != 0) {
 				pr_err("--super-minor=dev given, and listed device %s doesn't exist.\n",
-					devlist->devname);
+				       devlist->devname);
+				exit(1);
+			}
+
+			if (!ret && !stat_is_md_dev(&stb)) {
+				pr_err("device %s exists but is not an md array.\n", devlist->devname);
 				exit(1);
 			}
-			fstat(mdfd, &stb);
-			ident.super_minor = minor(stb.st_rdev);
-		}
-		if (mdfd >= 0 && mode != MANAGE && mode != GROW) {
-			/* We don't really want this open yet, we just might
-			 * have wanted to check some things
-			 */
-			close(mdfd);
-			mdfd = -1;
 		}
+		if (ident.super_minor == -2)
+			ident.super_minor = minor(stb.st_rdev);
 	}
 
 	if (s.raiddisks) {
diff --git a/mdadm.h b/mdadm.h
index 05ef881f4709..69f498b4ff59 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1664,6 +1664,7 @@ void *super1_make_v0(struct supertype *st, struct mdinfo *info, mdp_super_t *sb0
 extern char *stat2kname(struct stat *st);
 extern char *fd2kname(int fd);
 extern char *stat2devnm(struct stat *st);
+bool stat_is_md_dev(struct stat *st);
 extern char *fd2devnm(int fd);
 extern void udev_block(char *devnm);
 extern void udev_unblock(void);
-- 
2.30.2

Re: [PATCH mdadm v3 1/2] tests/00readonly: Run udevadm settle before setting ro

[Jes Sorensen]

On 7/27/22 17:52, Logan Gunthorpe wrote:
> In some recent kernel versions, 00readonly fails with:
> 
>   mdadm: failed to set readonly for /dev/md0: Device or resource busy
>   ERROR: array is not read-only!
> 
> This was traced down to a race condition with udev holding a reference
> to the block device at the same time as trying to set it read only.
> 
> To fix this, call udevadm settle before setting the array read only.
> 
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> ---
>  tests/00readonly | 1 +
>  1 file changed, 1 insertion(+)

Applied,

Thanks,
Jes