[PATCH 00/13] Custom drives policies verification

[PATCH 00/13] Custom drives policies verification

[Mariusz Tkaczyk]

validate_geometry_imsm() over the years became huge and complicated.
It is extremely hard to develop or optimize this code now. What the
most important, it doesn't address all scenarios. For example if
container contains disks under different controllers (spare container),
"autolayout" feature allows to create raid array. This code has a lot
of dependencies and it is almost impossible to add support of this
scenario without breaking something else.

There is also get_disk_controller_domain() which in my understating is a
part of validate_geometry_imsm() functionality moved outside, fit ideally
to mdmonitor needs.

Drive encryption determining will be added to IMSM soon. The encryption
status of the drive will be determined for every drive. There is no
simple way to add it.

The solution added in this serie addresses those problems by making one
easily extendable api to analyze every disk separately, outside
validate_geometry().

First five patches are optimizations with no functional changes. New
functionality replaces get_disk_controller_domain(). It should also
cover some verifications done in validate_geometry_imsm() and
add_to_super_imsm() but to lower regression risk these parts are
not removed yet.

Mariusz Tkaczyk (13):
  mdadm: Add functions for spare criteria verification
  mdadm: drop get_required_spare_criteria()
  Manage: fix check after dereference issue
  Manage: implement manage_add_external()
  mdadm: introduce sysfs_get_container_devnm()
  mdadm.h: Introduce custom device policies
  mdadm: test_and_add device policies implementation
  Create: Use device policies
  Manage: check device policies in manage_add_external()
  Monitor, Incremental: use device policies
  imsm: test_and_add_device_policies() implementation
  mdadm: drop get_disk_controller_domain()
  Revert "policy.c: Avoid to take spare without defined domain by imsm"

 Create.c         |  48 +++++++---
 Incremental.c    |  77 ++++++++++-----
 Manage.c         | 195 +++++++++++++++++++++----------------
 Monitor.c        |  50 ++--------
 mdadm.h          |  90 +++++++++--------
 platform-intel.h |   1 -
 policy.c         | 110 +++++++++++++++++----
 super-intel.c    | 245 ++++++++++++++++++++++++++++++++---------------
 sysfs.c          |  23 +++++
 util.c           | 117 ++++++++++++----------
 10 files changed, 606 insertions(+), 350 deletions(-)

-- 
2.35.3

[PATCH 01/13] mdadm: Add functions for spare criteria verification

[Mariusz Tkaczyk]

It is done similar way in few places. As a result, two almost identical
functions (dev_size_from_id() and dev_sector_size_from_id()) are
removed. Now, it uses same file descriptor to send two ioctls.

Two extern functions are added, in next patches
disk_fd_matches_criteria() is used.

Next optimization is inline zeroing struct spare_criteria. With that,
we don't need to reset values in get_spare_criteria_imsm().

Dedicated boolean field for checking if criteria are filled is added.
We don't need to execute the code if it is not set.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 Incremental.c |   2 +-
 Monitor.c     |  14 +------
 mdadm.h       |   6 ++-
 super-intel.c |   4 +-
 util.c        | 112 ++++++++++++++++++++++++++------------------------
 5 files changed, 67 insertions(+), 71 deletions(-)

diff --git a/Incremental.c b/Incremental.c
index 30c07c037028..2b5a5859ced7 100644
--- a/Incremental.c
+++ b/Incremental.c
@@ -874,7 +874,7 @@ static int array_try_spare(char *devname, int *dfdp, struct dev_policy *pol,
 		struct domainlist *dl = NULL;
 		struct mdinfo *sra;
 		unsigned long long devsize, freesize = 0;
-		struct spare_criteria sc = {0, 0};
+		struct spare_criteria sc = {0};
 
 		if (is_subarray(mp->metadata))
 			continue;
diff --git a/Monitor.c b/Monitor.c
index 7cee95d4487a..6917ae6c8e6d 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -1065,22 +1065,12 @@ static dev_t choose_spare(struct state *from, struct state *to,
 	for (d = from->raid; !dev && d < MAX_DISKS; d++) {
 		if (from->devid[d] > 0 && from->devstate[d] == 0) {
 			struct dev_policy *pol;
-			unsigned long long dev_size;
-			unsigned int dev_sector_size;
 
 			if (to->metadata->ss->external &&
 			    test_partition_from_id(from->devid[d]))
 				continue;
 
-			if (sc->min_size &&
-			    dev_size_from_id(from->devid[d], &dev_size) &&
-			    dev_size < sc->min_size)
-				continue;
-
-			if (sc->sector_size &&
-			    dev_sector_size_from_id(from->devid[d],
-						    &dev_sector_size) &&
-			    sc->sector_size != dev_sector_size)
+			if (devid_matches_criteria(from->devid[d], sc) == false)
 				continue;
 
 			pol = devid_policy(from->devid[d]);
@@ -1165,12 +1155,12 @@ static void try_spare_migration(struct state *statelist)
 {
 	struct state *from;
 	struct state *st;
-	struct spare_criteria sc;
 
 	link_containers_with_subarrays(statelist);
 	for (st = statelist; st; st = st->next)
 		if (st->active < st->raid && st->spare == 0 && !st->err) {
 			struct domainlist *domlist = NULL;
+			struct spare_criteria sc = {0};
 			int d;
 			struct state *to = st;
 
diff --git a/mdadm.h b/mdadm.h
index 75c887e4c64c..e8abd7309412 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -430,6 +430,7 @@ struct createinfo {
 };
 
 struct spare_criteria {
+	bool criteria_set;
 	unsigned long long min_size;
 	unsigned int sector_size;
 };
@@ -1368,8 +1369,6 @@ extern struct supertype *dup_super(struct supertype *st);
 extern int get_dev_size(int fd, char *dname, unsigned long long *sizep);
 extern int get_dev_sector_size(int fd, char *dname, unsigned int *sectsizep);
 extern int must_be_container(int fd);
-extern int dev_size_from_id(dev_t id, unsigned long long *size);
-extern int dev_sector_size_from_id(dev_t id, unsigned int *size);
 void wait_for(char *dev, int fd);
 
 /*
@@ -1708,6 +1707,9 @@ extern int assemble_container_content(struct supertype *st, int mdfd,
 #define	INCR_UNSAFE	2
 #define	INCR_ALREADY	4
 #define	INCR_YES	8
+
+extern bool devid_matches_criteria(dev_t devid, struct spare_criteria *sc);
+extern bool disk_fd_matches_criteria(int disk_fd, struct spare_criteria *sc);
 extern struct mdinfo *container_choose_spares(struct supertype *st,
 					      struct spare_criteria *criteria,
 					      struct domainlist *domlist,
diff --git a/super-intel.c b/super-intel.c
index e61f3f6ff662..e22a4bd7de6b 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -1748,9 +1748,6 @@ int get_spare_criteria_imsm(struct supertype *st, struct spare_criteria *c)
 	int i;
 	unsigned long long size = 0;
 
-	c->min_size = 0;
-	c->sector_size = 0;
-
 	if (!super)
 		return -EINVAL;
 	/* find first active disk in array */
@@ -1774,6 +1771,7 @@ int get_spare_criteria_imsm(struct supertype *st, struct spare_criteria *c)
 
 	c->min_size = size * 512;
 	c->sector_size = super->sector_size;
+	c->criteria_set = true;
 
 	return 0;
 }
diff --git a/util.c b/util.c
index b145447370b3..041e78cf5426 100644
--- a/util.c
+++ b/util.c
@@ -1266,40 +1266,6 @@ struct supertype *super_by_fd(int fd, char **subarrayp)
 	return st;
 }
 
-int dev_size_from_id(dev_t id, unsigned long long *size)
-{
-	char buf[20];
-	int fd;
-
-	sprintf(buf, "%d:%d", major(id), minor(id));
-	fd = dev_open(buf, O_RDONLY);
-	if (fd < 0)
-		return 0;
-	if (get_dev_size(fd, NULL, size)) {
-		close(fd);
-		return 1;
-	}
-	close(fd);
-	return 0;
-}
-
-int dev_sector_size_from_id(dev_t id, unsigned int *size)
-{
-	char buf[20];
-	int fd;
-
-	sprintf(buf, "%d:%d", major(id), minor(id));
-	fd = dev_open(buf, O_RDONLY);
-	if (fd < 0)
-		return 0;
-	if (get_dev_sector_size(fd, NULL, size)) {
-		close(fd);
-		return 1;
-	}
-	close(fd);
-	return 0;
-}
-
 struct supertype *dup_super(struct supertype *orig)
 {
 	struct supertype *st;
@@ -2088,6 +2054,60 @@ void append_metadata_update(struct supertype *st, void *buf, int len)
 unsigned int __invalid_size_argument_for_IOC = 0;
 #endif
 
+/**
+ * disk_fd_matches_criteria() - check if device matches spare criteria.
+ * @disk_fd: file descriptor of the disk.
+ * @sc: criteria to test.
+ *
+ * Return: true if disk matches criteria, false otherwise.
+ */
+bool disk_fd_matches_criteria(int disk_fd, struct spare_criteria *sc)
+{
+	unsigned int dev_sector_size = 0;
+	unsigned long long dev_size = 0;
+
+	if (!sc->criteria_set)
+		return true;
+
+	if (!get_dev_size(disk_fd, NULL, &dev_size) || dev_size < sc->min_size)
+		return false;
+
+	if (!get_dev_sector_size(disk_fd, NULL, &dev_sector_size) ||
+	    sc->sector_size != dev_sector_size)
+		return false;
+
+	return true;
+}
+
+/**
+ * devid_matches_criteria() - check if device referenced by devid matches spare criteria.
+ * @devid: devid of the device to check.
+ * @sc: criteria to test.
+ *
+ * Return: true if disk matches criteria, false otherwise.
+ */
+bool devid_matches_criteria(dev_t devid, struct spare_criteria *sc)
+{
+	char buf[NAME_MAX];
+	bool ret;
+	int fd;
+
+	if (!sc->criteria_set)
+		return true;
+
+	snprintf(buf, NAME_MAX, "%d:%d", major(devid), minor(devid));
+
+	fd = dev_open(buf, O_RDONLY);
+	if (!is_fd_valid(fd))
+		return false;
+
+	/* Error code inherited */
+	ret = disk_fd_matches_criteria(fd, sc);
+
+	close(fd);
+	return ret;
+}
+
 /* Pick all spares matching given criteria from a container
  * if min_size == 0 do not check size
  * if domlist == NULL do not check domains
@@ -2111,28 +2131,13 @@ struct mdinfo *container_choose_spares(struct supertype *st,
 	dp = &disks->devs;
 	disks->array.spare_disks = 0;
 	while (*dp) {
-		int found = 0;
+		bool found = false;
+
 		d = *dp;
 		if (d->disk.state == 0) {
-			/* check if size is acceptable */
-			unsigned long long dev_size;
-			unsigned int dev_sector_size;
-			int size_valid = 0;
-			int sector_size_valid = 0;
-
 			dev_t dev = makedev(d->disk.major,d->disk.minor);
 
-			if (!criteria->min_size ||
-			   (dev_size_from_id(dev,  &dev_size) &&
-			    dev_size >= criteria->min_size))
-				size_valid = 1;
-
-			if (!criteria->sector_size ||
-			    (dev_sector_size_from_id(dev, &dev_sector_size) &&
-			     criteria->sector_size == dev_sector_size))
-				sector_size_valid = 1;
-
-			found = size_valid && sector_size_valid;
+			found = devid_matches_criteria(dev, criteria);
 
 			/* check if domain matches */
 			if (found && domlist) {
@@ -2141,7 +2146,8 @@ struct mdinfo *container_choose_spares(struct supertype *st,
 					pol_add(&pol, pol_domain,
 						spare_group, NULL);
 				if (domain_test(domlist, pol, metadata) != 1)
-					found = 0;
+					found = false;
+
 				dev_policy_free(pol);
 			}
 		}
-- 
2.35.3

[PATCH 02/13] mdadm: drop get_required_spare_criteria()

[Mariusz Tkaczyk]

Only IMSM implements get_spare_criteria, so load_super() in
get_required_spare_criteria() is dead code. It is moved inside
metadata handler, because only IMSM implements it.

Give possibility to provide devnode to be opened. With that we can hide
load_container() used only to fill spare criteria inside handler
and simplify implementation in generic code.

Add helper function for testing spare criteria in Incremental and
error messages.

File descriptor in get_spare_criteria_imsm() is always opened on purpose.
New functionality added in next patches will require it. For the same
reason, function is moved to other place.

No functional changes.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 Incremental.c |  77 ++++++++++++++++++++++----------
 Monitor.c     |  35 +++------------
 mdadm.h       |   5 +--
 super-intel.c | 120 +++++++++++++++++++++++++++++++++-----------------
 4 files changed, 140 insertions(+), 97 deletions(-)

diff --git a/Incremental.c b/Incremental.c
index 2b5a5859ced7..66c2cc86dc5a 100644
--- a/Incremental.c
+++ b/Incremental.c
@@ -833,6 +833,53 @@ container_members_max_degradation(struct map_ent *map, struct map_ent *me)
 	return max_degraded;
 }
 
+/**
+ * incremental_external_test_spare_criteria() - helper to test spare criteria.
+ * @st: supertype, must be not NULL, it is duplicated here.
+ * @container_devnm: devnm of the container.
+ * @disk_fd: file descriptor of device to tested.
+ * @verbose: verbose flag.
+ *
+ * The function is used on new drive verification path to check if it can be added to external
+ * container. To test spare criteria, metadata must be loaded. It duplicates super to not mess in
+ * original one.
+ * Function is executed if superblock supports get_spare_criteria(), otherwise success is returned.
+ */
+mdadm_status_t incremental_external_test_spare_criteria(struct supertype *st, char *container_devnm,
+							int disk_fd, int verbose)
+{
+	mdadm_status_t rv = MDADM_STATUS_ERROR;
+	char container_devname[PATH_MAX];
+	struct spare_criteria sc = {0};
+	struct supertype *dup;
+
+	if (!st->ss->get_spare_criteria)
+		return MDADM_STATUS_SUCCESS;
+
+	dup = dup_super(st);
+	snprintf(container_devname, PATH_MAX, "/dev/%s", container_devnm);
+
+	if (dup->ss->get_spare_criteria(dup, container_devname, &sc) != 0) {
+		if (verbose > 1)
+			pr_err("Failed to get spare criteria for %s\n", container_devname);
+		goto out;
+	}
+
+	if (!disk_fd_matches_criteria(disk_fd, &sc)) {
+		if (verbose > 1)
+			pr_err("Disk does not match spare criteria for %s\n", container_devname);
+		goto out;
+	}
+
+	rv = MDADM_STATUS_SUCCESS;
+
+out:
+	dup->ss->free_super(dup);
+	free(dup);
+
+	return rv;
+}
+
 static int array_try_spare(char *devname, int *dfdp, struct dev_policy *pol,
 			   struct map_ent *target, int bare,
 			   struct supertype *st, int verbose)
@@ -873,8 +920,7 @@ static int array_try_spare(char *devname, int *dfdp, struct dev_policy *pol,
 		struct supertype *st2;
 		struct domainlist *dl = NULL;
 		struct mdinfo *sra;
-		unsigned long long devsize, freesize = 0;
-		struct spare_criteria sc = {0};
+		unsigned long long freesize = 0;
 
 		if (is_subarray(mp->metadata))
 			continue;
@@ -925,34 +971,19 @@ static int array_try_spare(char *devname, int *dfdp, struct dev_policy *pol,
 		if (sra->array.failed_disks == -1)
 			sra->array.failed_disks = container_members_max_degradation(map, mp);
 
-		get_dev_size(dfd, NULL, &devsize);
 		if (sra->component_size == 0) {
-			/* true for containers, here we must read superblock
-			 * to obtain minimum spare size */
-			struct supertype *st3 = dup_super(st2);
-			int mdfd = open_dev(mp->devnm);
-			if (mdfd < 0) {
-				free(st3);
+			/* true for containers */
+			if (incremental_external_test_spare_criteria(st2, mp->devnm, dfd, verbose))
 				goto next;
-			}
-			if (st3->ss->load_container &&
-			    !st3->ss->load_container(st3, mdfd, mp->path)) {
-				if (st3->ss->get_spare_criteria)
-					st3->ss->get_spare_criteria(st3, &sc);
-				st3->ss->free_super(st3);
-			}
-			free(st3);
-			close(mdfd);
 		}
-		if ((sra->component_size > 0 &&
-		     st2->ss->validate_geometry(st2, sra->array.level, sra->array.layout,
+
+		if (sra->component_size > 0 &&
+		    st2->ss->validate_geometry(st2, sra->array.level, sra->array.layout,
 						sra->array.raid_disks, &sra->array.chunk_size,
 						sra->component_size,
 						sra->devs ? sra->devs->data_offset : INVALID_SECTORS,
 						devname, &freesize, sra->consistency_policy,
-						0) &&
-		     freesize < sra->component_size) ||
-		    (sra->component_size == 0 && devsize < sc.min_size)) {
+						0) && freesize < sra->component_size) {
 			if (verbose > 1)
 				pr_err("not adding %s to %s as it is too small\n",
 					devname, mp->path);
diff --git a/Monitor.c b/Monitor.c
index 6917ae6c8e6d..2167523ca3e2 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -1003,34 +1003,6 @@ static int add_new_arrays(struct mdstat_ent *mdstat, struct state **statelist)
 	return new_found;
 }
 
-static int get_required_spare_criteria(struct state *st,
-				       struct spare_criteria *sc)
-{
-	int fd;
-
-	if (!st->metadata || !st->metadata->ss->get_spare_criteria) {
-		sc->min_size = 0;
-		sc->sector_size = 0;
-		return 0;
-	}
-
-	fd = open(st->devname, O_RDONLY);
-	if (fd < 0)
-		return 1;
-	if (st->metadata->ss->external)
-		st->metadata->ss->load_container(st->metadata, fd, st->devname);
-	else
-		st->metadata->ss->load_super(st->metadata, fd, st->devname);
-	close(fd);
-	if (!st->metadata->sb)
-		return 1;
-
-	st->metadata->ss->get_spare_criteria(st->metadata, sc);
-	st->metadata->ss->free_super(st->metadata);
-
-	return 0;
-}
-
 static int check_donor(struct state *from, struct state *to)
 {
 	struct state *sub;
@@ -1173,8 +1145,11 @@ static void try_spare_migration(struct state *statelist)
 				/* member of a container */
 				to = to->parent;
 
-			if (get_required_spare_criteria(to, &sc))
-				continue;
+			if (to->metadata->ss->get_spare_criteria)
+				if (to->metadata->ss->get_spare_criteria(to->metadata, to->devname,
+									 &sc))
+					continue;
+
 			if (to->metadata->ss->external) {
 				/* We must make sure there is
 				 * no suitable spare in container already.
diff --git a/mdadm.h b/mdadm.h
index e8abd7309412..cbc586f5e3ef 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1116,10 +1116,9 @@ extern struct superswitch {
 	 * Return spare criteria for array:
 	 * - minimum disk size can be used in array;
 	 * - sector size can be used in array.
-	 * Return values: 0 - for success and -EINVAL on error.
 	 */
-	int (*get_spare_criteria)(struct supertype *st,
-				  struct spare_criteria *sc);
+	mdadm_status_t (*get_spare_criteria)(struct supertype *st, char *mddev_path,
+					     struct spare_criteria *sc);
 	/* Find somewhere to put a bitmap - possibly auto-size it - and
 	 * update the metadata to record this.  The array may be newly
 	 * created, in which case data_size may be updated, or it might
diff --git a/super-intel.c b/super-intel.c
index e22a4bd7de6b..90928dce722e 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -1736,46 +1736,6 @@ static __u32 imsm_min_reserved_sectors(struct intel_super *super)
 	return  (remainder < rv) ? remainder : rv;
 }
 
-/*
- * Return minimum size of a spare and sector size
- * that can be used in this array
- */
-int get_spare_criteria_imsm(struct supertype *st, struct spare_criteria *c)
-{
-	struct intel_super *super = st->sb;
-	struct dl *dl;
-	struct extent *e;
-	int i;
-	unsigned long long size = 0;
-
-	if (!super)
-		return -EINVAL;
-	/* find first active disk in array */
-	dl = super->disks;
-	while (dl && (is_failed(&dl->disk) || dl->index == -1))
-		dl = dl->next;
-	if (!dl)
-		return -EINVAL;
-	/* find last lba used by subarrays */
-	e = get_extents(super, dl, 0);
-	if (!e)
-		return -EINVAL;
-	for (i = 0; e[i].size; i++)
-		continue;
-	if (i > 0)
-		size = e[i-1].start + e[i-1].size;
-	free(e);
-
-	/* add the amount of space needed for metadata */
-	size += imsm_min_reserved_sectors(super);
-
-	c->min_size = size * 512;
-	c->sector_size = super->sector_size;
-	c->criteria_set = true;
-
-	return 0;
-}
-
 static bool is_gen_migration(struct imsm_dev *dev);
 
 #define IMSM_4K_DIV 8
@@ -11295,6 +11255,84 @@ static const char *imsm_get_disk_controller_domain(const char *path)
 	return drv;
 }
 
+/**
+ * get_spare_criteria_imsm() - set spare criteria.
+ * @st: supertype.
+ * @mddev_path: path to md device devnode, it must be container.
+ * @c: spare_criteria struct to fill, not NULL.
+ *
+ * If superblock is not loaded, use mddev_path to load_container. It must be given in this case.
+ * Filles size and sector size accordingly to superblock.
+ */
+mdadm_status_t get_spare_criteria_imsm(struct supertype *st, char *mddev_path,
+				       struct spare_criteria *c)
+{
+	mdadm_status_t ret = MDADM_STATUS_ERROR;
+	bool free_superblock = false;
+	unsigned long long size = 0;
+	struct intel_super *super;
+	struct extent *e;
+	struct dl *dl;
+	int i;
+
+	/* If no superblock and no mddev_path, we cannot load superblock. */
+	assert(st->sb || mddev_path);
+
+	if (mddev_path) {
+		int fd = open(mddev_path, O_RDONLY);
+
+		if (!is_fd_valid(fd))
+			return MDADM_STATUS_ERROR;
+
+		if (!st->sb) {
+			if (load_container_imsm(st, fd, st->devnm)) {
+				close(fd);
+				return MDADM_STATUS_ERROR;
+			}
+			free_superblock = true;
+		}
+		close(fd);
+	}
+
+	super = st->sb;
+
+	/* find first active disk in array */
+	dl = super->disks;
+	while (dl && (is_failed(&dl->disk) || dl->index == -1))
+		dl = dl->next;
+
+	if (!dl)
+		goto out;
+
+	/* find last lba used by subarrays */
+	e = get_extents(super, dl, 0);
+	if (!e)
+		goto out;
+
+	for (i = 0; e[i].size; i++)
+		continue;
+	if (i > 0)
+		size = e[i - 1].start + e[i - 1].size;
+	free(e);
+
+	/* add the amount of space needed for metadata */
+	size += imsm_min_reserved_sectors(super);
+
+	c->min_size = size * 512;
+	c->sector_size = super->sector_size;
+	c->criteria_set = true;
+	ret = MDADM_STATUS_SUCCESS;
+
+out:
+	if (free_superblock)
+		free_super_imsm(st);
+
+	if (ret != MDADM_STATUS_SUCCESS)
+		c->criteria_set = false;
+
+	return ret;
+}
+
 static char *imsm_find_array_devnm_by_subdev(int subdev, char *container)
 {
 	static char devnm[32];
@@ -11425,7 +11463,7 @@ static struct mdinfo *get_spares_for_grow(struct supertype *st)
 {
 	struct spare_criteria sc;
 
-	get_spare_criteria_imsm(st, &sc);
+	get_spare_criteria_imsm(st, NULL, &sc);
 	return container_choose_spares(st, &sc, NULL, NULL, NULL, 0);
 }
 
-- 
2.35.3

[PATCH 03/13] Manage: fix check after dereference issue

[Mariusz Tkaczyk]

The code dereferences dev_st earlier without checking, it gives SAST
problem.

dev_st is needed for attempt_re_add(), but it is executed only if
dv->disposition != 'S', so move disposition check up.

tst is a must to reach this place, dup_super() have to return valid
pointer, all it needs to check is if load_super() returns superblock.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 Manage.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/Manage.c b/Manage.c
index 30302ac833f2..77b79cf57554 100644
--- a/Manage.c
+++ b/Manage.c
@@ -794,25 +794,23 @@ int Manage_add(int fd, int tfd, struct mddev_dev *dv,
 		 * simply re-add it.
 		 */
 
-		if (array->not_persistent == 0) {
+		if (array->not_persistent == 0 && dv->disposition != 'S') {
+			int rv = 0;
+
 			dev_st = dup_super(tst);
 			dev_st->ss->load_super(dev_st, tfd, NULL);
-			if (dev_st->sb && dv->disposition != 'S') {
-				int rv;
 
-				rv = attempt_re_add(fd, tfd, dv, dev_st, tst,
-						    rdev, update, devname,
-						    verbose, array);
-				dev_st->ss->free_super(dev_st);
-				if (rv) {
-					free(dev_st);
-					return rv;
-				}
-			}
-			if (dev_st) {
+			if (dev_st->sb) {
+				rv = attempt_re_add(fd, tfd, dv, dev_st, tst, rdev, update,
+						    devname, verbose, array);
+
 				dev_st->ss->free_super(dev_st);
-				free(dev_st);
 			}
+
+			free(dev_st);
+
+			if (rv)
+				return rv;
 		}
 		if (dv->disposition == 'M') {
 			if (verbose > 0)
-- 
2.35.3

[PATCH 04/13] Manage: implement manage_add_external()

[Mariusz Tkaczyk]

Move external add code to separate function. It is easier to control
error path now. Error messages are adjusted.

No functional changes.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 Manage.c | 147 ++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 86 insertions(+), 61 deletions(-)

diff --git a/Manage.c b/Manage.c
index 77b79cf57554..b3e216cbcec6 100644
--- a/Manage.c
+++ b/Manage.c
@@ -695,6 +695,91 @@ skip_re_add:
 	return 0;
 }
 
+/**
+ * manage_add_external() - Add disk to external container.
+ * @st: external supertype pointer, must not be NULL, superblock is released here.
+ * @fd: container file descriptor, must not have O_EXCL mode.
+ * @disk_fd: device to add file descriptor.
+ * @disk_name: name of the device to add.
+ * @disc: disk info.
+ *
+ * Superblock is released here because any open fd with O_EXCL will block sysfs_add_disk().
+ */
+mdadm_status_t manage_add_external(struct supertype *st, int fd, char *disk_name,
+				   mdu_disk_info_t *disc)
+{
+	mdadm_status_t rv = MDADM_STATUS_ERROR;
+	char container_devpath[MD_NAME_MAX];
+	struct mdinfo new_mdi;
+	struct mdinfo *sra = NULL;
+	int container_fd;
+	int disk_fd = -1;
+
+	snprintf(container_devpath, MD_NAME_MAX, "%s", fd2devnm(fd));
+
+	container_fd = open_dev_excl(container_devpath);
+	if (!is_fd_valid(container_fd)) {
+		pr_err("Failed to get exclusive access to container %s\n", container_devpath);
+		return MDADM_STATUS_ERROR;
+	}
+
+	/* Check if metadata handler is able to accept the drive */
+	if (!st->ss->validate_geometry(st, LEVEL_CONTAINER, 0, 1, NULL, 0, 0, disk_name, NULL,
+				       0, 1))
+		goto out;
+
+	Kill(disk_name, NULL, 0, -1, 0);
+
+	disk_fd = dev_open(disk_name, O_RDWR | O_EXCL | O_DIRECT);
+	if (!is_fd_valid(disk_fd)) {
+		pr_err("Failed to exclusively open %s\n", disk_name);
+		goto out;
+	}
+
+	if (st->ss->add_to_super(st, disc, disk_fd, disk_name, INVALID_SECTORS))
+		goto out;
+
+	if (!mdmon_running(st->container_devnm))
+		st->ss->sync_metadata(st);
+
+	sra = sysfs_read(container_fd, NULL, 0);
+	if (!sra) {
+		pr_err("Failed to read sysfs for %s\n", disk_name);
+		goto out;
+	}
+
+	sra->array.level = LEVEL_CONTAINER;
+	/* Need to set data_offset and component_size */
+	st->ss->getinfo_super(st, &new_mdi, NULL);
+	new_mdi.disk.major = disc->major;
+	new_mdi.disk.minor = disc->minor;
+	new_mdi.recovery_start = 0;
+
+	st->ss->free_super(st);
+
+	if (sysfs_add_disk(sra, &new_mdi, 0) != 0) {
+		pr_err("Failed to add %s to container %s\n", disk_name, container_devpath);
+		goto out;
+	}
+	ping_monitor(container_devpath);
+	rv = MDADM_STATUS_SUCCESS;
+
+out:
+	close(container_fd);
+
+	if (sra)
+		sysfs_free(sra);
+
+	if (rv != MDADM_STATUS_SUCCESS && is_fd_valid(disk_fd))
+		/* Metadata handler records this descriptor, so release it only on failure. */
+		close(disk_fd);
+
+	if (st->sb)
+		st->ss->free_super(st);
+
+	return rv;
+}
+
 int Manage_add(int fd, int tfd, struct mddev_dev *dv,
 	       struct supertype *tst, mdu_array_info_t *array,
 	       int force, int verbose, char *devname,
@@ -966,68 +1051,8 @@ int Manage_add(int fd, int tfd, struct mddev_dev *dv,
 	if (dv->failfast == FlagSet)
 		disc.state |= (1 << MD_DISK_FAILFAST);
 	if (tst->ss->external) {
-		/* add a disk
-		 * to an external metadata container */
-		struct mdinfo new_mdi;
-		struct mdinfo *sra;
-		int container_fd;
-		char devnm[32];
-		int dfd;
-
-		strcpy(devnm, fd2devnm(fd));
-
-		container_fd = open_dev_excl(devnm);
-		if (container_fd < 0) {
-			pr_err("add failed for %s: could not get exclusive access to container\n",
-			       dv->devname);
-			tst->ss->free_super(tst);
+		if (manage_add_external(tst, fd, dv->devname, &disc) != MDADM_STATUS_SUCCESS)
 			goto unlock;
-		}
-
-		/* Check if metadata handler is able to accept the drive */
-		if (!tst->ss->validate_geometry(tst, LEVEL_CONTAINER, 0, 1, NULL,
-		    0, 0, dv->devname, NULL, 0, 1)) {
-			close(container_fd);
-			goto unlock;
-		}
-
-		Kill(dv->devname, NULL, 0, -1, 0);
-		dfd = dev_open(dv->devname, O_RDWR | O_EXCL|O_DIRECT);
-		if (tst->ss->add_to_super(tst, &disc, dfd,
-					  dv->devname, INVALID_SECTORS)) {
-			close(dfd);
-			close(container_fd);
-			goto unlock;
-		}
-		if (!mdmon_running(tst->container_devnm))
-			tst->ss->sync_metadata(tst);
-
-		sra = sysfs_read(container_fd, NULL, 0);
-		if (!sra) {
-			pr_err("add failed for %s: sysfs_read failed\n",
-			       dv->devname);
-			close(container_fd);
-			tst->ss->free_super(tst);
-			goto unlock;
-		}
-		sra->array.level = LEVEL_CONTAINER;
-		/* Need to set data_offset and component_size */
-		tst->ss->getinfo_super(tst, &new_mdi, NULL);
-		new_mdi.disk.major = disc.major;
-		new_mdi.disk.minor = disc.minor;
-		new_mdi.recovery_start = 0;
-		/* Make sure fds are closed as they are O_EXCL which
-		 * would block add_disk */
-		tst->ss->free_super(tst);
-		if (sysfs_add_disk(sra, &new_mdi, 0) != 0) {
-			pr_err("add new device to external metadata failed for %s\n", dv->devname);
-			close(container_fd);
-			sysfs_free(sra);
-			goto unlock;
-		}
-		ping_monitor(devnm);
-		sysfs_free(sra);
-		close(container_fd);
 	} else {
 		tst->ss->free_super(tst);
 		if (ioctl(fd, ADD_NEW_DISK, &disc)) {
-- 
2.35.3

[PATCH 05/13] mdadm: introduce sysfs_get_container_devnm()

[Mariusz Tkaczyk]

There at least two places where it is done directly, so replace them
with function. Print message about creating external array, add "/dev/"
prefix to refer directly to devnode.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 Create.c | 21 ++++++++++-----------
 Manage.c | 14 ++++----------
 mdadm.h  |  2 ++
 sysfs.c  | 23 +++++++++++++++++++++++
 4 files changed, 39 insertions(+), 21 deletions(-)

diff --git a/Create.c b/Create.c
index 7e9170b6a1ac..0b7762661c76 100644
--- a/Create.c
+++ b/Create.c
@@ -1142,24 +1142,23 @@ int Create(struct supertype *st, struct mddev_ident *ident, int subdevs,
 
 	if (did_default && c->verbose >= 0) {
 		if (is_subarray(info.text_version)) {
-			char devnm[32];
-			char *ep;
+			char devnm[MD_NAME_MAX];
 			struct mdinfo *mdi;
 
-			strncpy(devnm, info.text_version+1, 32);
-			devnm[31] = 0;
-			ep = strchr(devnm, '/');
-			if (ep)
-				*ep = 0;
+			sysfs_get_container_devnm(&info, devnm);
 
 			mdi = sysfs_read(-1, devnm, GET_VERSION);
+			if (!mdi) {
+				pr_err("Cannot open sysfs for container %s\n", devnm);
+				goto abort_locked;
+			}
+
+			pr_info("Creating array inside %s container /dev/%s\n", mdi->text_version,
+				devnm);
 
-			pr_info("Creating array inside %s container %s\n",
-				mdi?mdi->text_version:"managed", devnm);
 			sysfs_free(mdi);
 		} else
-			pr_info("Defaulting to version %s metadata\n",
-				info.text_version);
+			pr_info("Defaulting to version %s metadata\n", info.text_version);
 	}
 
 	map_update(&map, fd2devnm(mdfd), info.text_version,
diff --git a/Manage.c b/Manage.c
index b3e216cbcec6..969d0ea9d81f 100644
--- a/Manage.c
+++ b/Manage.c
@@ -178,7 +178,7 @@ int Manage_stop(char *devname, int fd, int verbose, int will_retry)
 	struct map_ent *map = NULL;
 	struct mdinfo *mdi;
 	char devnm[32];
-	char container[32];
+	char container[MD_NAME_MAX] = {0};
 	int err;
 	int count;
 	char buf[SYSFS_MAX_BUF_SIZE];
@@ -192,15 +192,9 @@ int Manage_stop(char *devname, int fd, int verbose, int will_retry)
 	 * to stop is probably a bad idea.
 	 */
 	mdi = sysfs_read(fd, NULL, GET_LEVEL|GET_COMPONENT|GET_VERSION);
-	if (mdi && is_subarray(mdi->text_version)) {
-		char *sl;
-		strncpy(container, mdi->text_version+1, sizeof(container));
-		container[sizeof(container)-1] = 0;
-		sl = strchr(container, '/');
-		if (sl)
-			*sl = 0;
-	} else
-		container[0] = 0;
+	if (mdi && is_subarray(mdi->text_version))
+		sysfs_get_container_devnm(mdi, container);
+
 	close(fd);
 	count = 5;
 	while (((fd = ((devname[0] == '/')
diff --git a/mdadm.h b/mdadm.h
index cbc586f5e3ef..39b86bd08029 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -777,6 +777,8 @@ enum sysfs_read_flags {
 
 #define SYSFS_MAX_BUF_SIZE 64
 
+extern void sysfs_get_container_devnm(struct mdinfo *mdi, char *buf);
+
 /* If fd >= 0, get the array it is open on,
  * else use devnm.
  */
diff --git a/sysfs.c b/sysfs.c
index f95ef7013e84..230b842e4117 100644
--- a/sysfs.c
+++ b/sysfs.c
@@ -74,6 +74,29 @@ void sysfs_free(struct mdinfo *sra)
 	}
 }
 
+/**
+ * sysfs_get_container_devnm() - extract container device name.
+ * @mdi: md_info describes member array, with GET_VERSION option.
+ * @buf: buf to fill, must be MD_NAME_MAX.
+ *
+ * External array version is in format {/,-}<container_devnm>/<array_index>
+ * Extract container_devnm from it and safe it in @buf.
+ */
+void sysfs_get_container_devnm(struct mdinfo *mdi, char *buf)
+{
+	char *p;
+
+	assert(is_subarray(mdi->text_version));
+
+	/* Skip first special sign */
+	snprintf(buf, MD_NAME_MAX, "%s", mdi->text_version + 1);
+
+	/* Remove array index */
+	p = strchr(buf, '/');
+	if (p)
+		*p = 0;
+}
+
 int sysfs_open(char *devnm, char *devname, char *attr)
 {
 	char fname[MAX_SYSFS_PATH_LEN];
-- 
2.35.3

[PATCH 06/13] mdadm.h: Introduce custom device policies

[Mariusz Tkaczyk]

The approach proposed here is to test drive policies outside
validate_geometry() separately per every drive and add determined
policies to list. The implementation reuses dev_policy we have in
mdadm.

This concept addresses following problems:
- test drives if they fit together to criteria required by metadata
  handler,
- test all drives assigned to the container even if some of them are not
  target of the request, mdmon is free to use any drive in the same
  container,
- extensibility, new policies can be added to handler easy,
- fix issues related to imsm controller domain verifying.

Add superswitch function. It is used in next patches.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 mdadm.h | 54 ++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 18 deletions(-)

diff --git a/mdadm.h b/mdadm.h
index 39b86bd08029..889f4a0f1ecf 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -940,6 +940,23 @@ struct reshape {
 	unsigned long long new_size; /* New size of array in sectors */
 };
 
+/**
+ * struct dev_policy - Data structure for policy management.
+ * @next: pointer to next dev_policy.
+ * @name: policy name, category.
+ * @metadata: the metadata type it affects.
+ * @value: value of the policy.
+ *
+ * The functions to manipulate dev_policy lists do not free elements, so they must be statically
+ * allocated. @name and @metadata can be compared by address.
+ */
+typedef struct dev_policy {
+	struct dev_policy *next;
+	char *name;
+	const char *metadata;
+	const char *value;
+} dev_policy_t;
+
 /* A superswitch provides entry point to a metadata handler.
  *
  * The superswitch primarily operates on some "metadata" that
@@ -1168,6 +1185,25 @@ extern struct superswitch {
 				 char *subdev, unsigned long long *freesize,
 				 int consistency_policy, int verbose);
 
+	/**
+	 * test_and_add_drive_policies() - test new and add custom policies from metadata handler.
+	 * @pols: list of currently recorded policies.
+	 * @disk_fd: file descriptor of the device to check.
+	 * @verbose: verbose flag.
+	 *
+	 * Used by IMSM to verify all drives in container/array, against requirements not recored
+	 * in superblock, like controller type for IMSM. It should check all drives even if
+	 * they are not actually used, because mdmon or kernel are free to use any drive assigned to
+	 * container automatically.
+	 *
+	 * Generating and comparison methods belong to metadata handler. It is not mandatory to be
+	 * implemented.
+	 *
+	 * Return: MDADM_STATUS_SUCCESS is expected on success.
+	 */
+	mdadm_status_t (*test_and_add_drive_policies)(dev_policy_t **pols, int disk_fd,
+						      const int verbose);
+
 	/* Return a linked list of 'mdinfo' structures for all arrays
 	 * in the container.  For non-containers, it is like
 	 * getinfo_super with an allocated mdinfo.*/
@@ -1372,23 +1408,6 @@ extern int get_dev_sector_size(int fd, char *dname, unsigned int *sectsizep);
 extern int must_be_container(int fd);
 void wait_for(char *dev, int fd);
 
-/*
- * Data structures for policy management.
- * Each device can have a policy structure that lists
- * various name/value pairs each possibly with a metadata associated.
- * The policy list is sorted by name/value/metadata
- */
-struct dev_policy {
-	struct dev_policy *next;
-	char *name;	/* None of these strings are allocated.  They are
-			 * all just references to strings which are known
-			 * to exist elsewhere.
-			 * name and metadata can be compared by address equality.
-			 */
-	const char *metadata;
-	const char *value;
-};
-
 extern char pol_act[], pol_domain[], pol_metadata[], pol_auto[];
 
 /* iterate over the sublist starting at list, having the same
@@ -1430,7 +1449,6 @@ extern struct dev_policy *disk_policy(struct mdinfo *disk);
 extern struct dev_policy *devid_policy(int devid);
 extern void dev_policy_free(struct dev_policy *p);
 
-//extern void pol_new(struct dev_policy **pol, char *name, char *val, char *metadata);
 extern void pol_add(struct dev_policy **pol, char *name, char *val, char *metadata);
 extern struct dev_policy *pol_find(struct dev_policy *pol, char *name);
 
-- 
2.35.3

[PATCH 07/13] mdadm: test_and_add device policies implementation

[Mariusz Tkaczyk]

Add support for three scenarios:
- obtaining array wide policies via fd,
- obtaining array wide policies via struct mdinfo,
- getting policies for particular drive from the request.

Add proper functions and make them extern. These functions are used
in next patches.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 mdadm.h  |  7 +++++
 policy.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 100 insertions(+)

diff --git a/mdadm.h b/mdadm.h
index 889f4a0f1ecf..af2bc714bacb 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1452,6 +1452,13 @@ extern void dev_policy_free(struct dev_policy *p);
 extern void pol_add(struct dev_policy **pol, char *name, char *val, char *metadata);
 extern struct dev_policy *pol_find(struct dev_policy *pol, char *name);
 
+extern mdadm_status_t drive_test_and_add_policies(struct supertype *st, dev_policy_t **pols,
+						  int fd, const int verbose);
+extern mdadm_status_t sysfs_test_and_add_drive_policies(struct supertype *st, dev_policy_t **pols,
+							struct mdinfo *mdi, const int verbose);
+extern mdadm_status_t mddev_test_and_add_drive_policies(struct supertype *st, dev_policy_t **pols,
+							int array_fd, const int verbose);
+
 enum policy_action {
 	act_default,
 	act_include,
diff --git a/policy.c b/policy.c
index eee9ef63adda..4b85f62d9675 100644
--- a/policy.c
+++ b/policy.c
@@ -397,6 +397,99 @@ struct dev_policy *path_policy(char **paths, char *type)
 	return pol;
 }
 
+/**
+ * drive_test_and_add_policies() - get policies for drive and add them to pols.
+ * @st: supertype.
+ * @pols: pointer to pointer of first list entry, cannot be NULL, may point to NULL.
+ * @fd: device descriptor.
+ * @verbose: verbose flag.
+ *
+ * If supertype doesn't support this functionality return success. Use metadata handler to get
+ * policies.
+ */
+mdadm_status_t drive_test_and_add_policies(struct supertype *st, dev_policy_t **pols, int fd,
+					   const int verbose)
+{
+	if (!st->ss->test_and_add_drive_policies)
+		return MDADM_STATUS_SUCCESS;
+
+	if (st->ss->test_and_add_drive_policies(pols, fd, verbose) == MDADM_STATUS_SUCCESS) {
+		/* After successful call list cannot be empty */
+		assert(*pols);
+		return MDADM_STATUS_SUCCESS;
+	}
+
+	return MDADM_STATUS_ERROR;
+}
+
+/**
+ * sysfs_test_and_add_policies() - get policies for mddev and add them to pols.
+ * @st: supertype.
+ * @pols: pointer to pointer of first list entry, cannot be NULL, may point to NULL.
+ * @mdi: mdinfo describes the MD array, must have GET_DISKS option.
+ * @verbose: verbose flag.
+ *
+ * If supertype doesn't support this functionality return success. To get policies, all disks
+ * connected to mddev are analyzed.
+ */
+mdadm_status_t sysfs_test_and_add_drive_policies(struct supertype *st, dev_policy_t **pols,
+						 struct mdinfo *mdi, const int verbose)
+{
+	struct mdinfo *sd;
+
+	if (!st->ss->test_and_add_drive_policies)
+		return MDADM_STATUS_SUCCESS;
+
+	for (sd = mdi->devs; sd; sd = sd->next) {
+		char *devpath = map_dev(sd->disk.major, sd->disk.minor, 0);
+		int fd = dev_open(devpath, O_RDONLY);
+		int rv;
+
+		if (!is_fd_valid(fd)) {
+			pr_err("Cannot open fd for %s\n", devpath);
+			return MDADM_STATUS_ERROR;
+		}
+
+		rv = drive_test_and_add_policies(st, pols, fd, verbose);
+		close(fd);
+
+		if (rv)
+			return MDADM_STATUS_ERROR;
+	}
+
+	return MDADM_STATUS_SUCCESS;
+}
+
+/**
+ * mddev_test_and_add_policies() - get policies for mddev and add them to pols.
+ * @st: supertype.
+ * @pols: pointer to pointer of first list entry, cannot be NULL, may point to NULL.
+ * @array_fd: MD device descriptor.
+ * @verbose: verbose flag.
+ *
+ * If supertype doesn't support this functionality return success. Use fd to extract disks.
+ */
+mdadm_status_t mddev_test_and_add_drive_policies(struct supertype *st, dev_policy_t **pols,
+						 int array_fd, const int verbose)
+{
+	struct mdinfo *sra;
+	int ret;
+
+	if (!st->ss->test_and_add_drive_policies)
+		return MDADM_STATUS_SUCCESS;
+
+	sra = sysfs_read(array_fd, NULL, GET_DEVS);
+	if (!sra) {
+		pr_err("Cannot load sysfs for %s\n", fd2devnm(array_fd));
+		return MDADM_STATUS_ERROR;
+	}
+
+	ret = sysfs_test_and_add_drive_policies(st, pols, sra, verbose);
+
+	sysfs_free(sra);
+	return ret;
+}
+
 void pol_add(struct dev_policy **pol,
 		    char *name, char *val,
 		    char *metadata)
-- 
2.35.3

[PATCH 08/13] Create: Use device policies

[Mariusz Tkaczyk]

Generate and compare policies, abort if policies do not match.
It is tested for both create modes, with container and disk list
specified directly. It is used if supertype supports it.

For a case when disk list is specified, container may contain more
devices, so additional check on container is done to analyze all disks.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 Create.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/Create.c b/Create.c
index 0b7762661c76..4397ff49554d 100644
--- a/Create.c
+++ b/Create.c
@@ -497,6 +497,7 @@ int Create(struct supertype *st, struct mddev_ident *ident, int subdevs,
 	 */
 	int mdfd;
 	unsigned long long minsize = 0, maxsize = 0;
+	dev_policy_t *custom_pols = NULL;
 	char *mindisc = NULL;
 	char *maxdisc = NULL;
 	char *name = ident->name;
@@ -588,6 +589,9 @@ int Create(struct supertype *st, struct mddev_ident *ident, int subdevs,
 				first_missing = subdevs * 2;
 				second_missing = subdevs * 2;
 				insert_point = subdevs * 2;
+
+				if (mddev_test_and_add_drive_policies(st, &custom_pols, fd, 1))
+					exit(1);
 			}
 		}
 		if (fd >= 0)
@@ -739,7 +743,7 @@ int Create(struct supertype *st, struct mddev_ident *ident, int subdevs,
 			close(dfd);
 			exit(2);
 		}
-		close(dfd);
+
 		info.array.working_disks++;
 		if (dnum < s->raiddisks && dv->disposition != 'j')
 			info.array.active_disks++;
@@ -812,6 +816,11 @@ int Create(struct supertype *st, struct mddev_ident *ident, int subdevs,
 			}
 		}
 
+		if (drive_test_and_add_policies(st, &custom_pols, dfd, 1))
+			exit(1);
+
+		close(dfd);
+
 		if (dv->disposition == 'j')
 			goto skip_size_check;  /* skip write journal for size check */
 
@@ -886,6 +895,7 @@ int Create(struct supertype *st, struct mddev_ident *ident, int subdevs,
 			close(fd);
 		}
 	}
+
 	if (missing_disks == dnum && !have_container) {
 		pr_err("Subdevs can't be all missing\n");
 		return 1;
@@ -1140,25 +1150,30 @@ int Create(struct supertype *st, struct mddev_ident *ident, int subdevs,
 		goto abort_locked;
 	}
 
-	if (did_default && c->verbose >= 0) {
+	if (did_default) {
 		if (is_subarray(info.text_version)) {
 			char devnm[MD_NAME_MAX];
 			struct mdinfo *mdi;
 
 			sysfs_get_container_devnm(&info, devnm);
 
-			mdi = sysfs_read(-1, devnm, GET_VERSION);
+			mdi = sysfs_read(-1, devnm, GET_VERSION | GET_DEVS);
 			if (!mdi) {
 				pr_err("Cannot open sysfs for container %s\n", devnm);
 				goto abort_locked;
 			}
 
-			pr_info("Creating array inside %s container /dev/%s\n", mdi->text_version,
-				devnm);
+			if (sysfs_test_and_add_drive_policies(st, &custom_pols, mdi, 1))
+				goto abort_locked;
+
+			if (c->verbose >= 0)
+				pr_info("Creating array inside %s container /dev/%s\n",
+					mdi->text_version, devnm);
 
 			sysfs_free(mdi);
-		} else
+		} else if (c->verbose >= 0) {
 			pr_info("Defaulting to version %s metadata\n", info.text_version);
+		}
 	}
 
 	map_update(&map, fd2devnm(mdfd), info.text_version,
@@ -1328,6 +1343,8 @@ int Create(struct supertype *st, struct mddev_ident *ident, int subdevs,
 	udev_unblock();
 	close(mdfd);
 	sysfs_uevent(&info, "change");
+	dev_policy_free(custom_pols);
+
 	return 0;
 
  abort:
@@ -1339,5 +1356,7 @@ int Create(struct supertype *st, struct mddev_ident *ident, int subdevs,
 
 	if (mdfd >= 0)
 		close(mdfd);
+
+	dev_policy_free(custom_pols);
 	return 1;
 }
-- 
2.35.3

[PATCH 09/13] Manage: check device policies in manage_add_external()

[Mariusz Tkaczyk]

Only IMSM is going to use device policies so it is added to
manage_add_external(). Test policies before adding the drive to
container.

The change blocks adding new device to the container which already
contains not matching devices

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 Manage.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Manage.c b/Manage.c
index 969d0ea9d81f..96e5ee5427a2 100644
--- a/Manage.c
+++ b/Manage.c
@@ -704,6 +704,7 @@ mdadm_status_t manage_add_external(struct supertype *st, int fd, char *disk_name
 {
 	mdadm_status_t rv = MDADM_STATUS_ERROR;
 	char container_devpath[MD_NAME_MAX];
+	struct dev_policy *pols = NULL;
 	struct mdinfo new_mdi;
 	struct mdinfo *sra = NULL;
 	int container_fd;
@@ -722,6 +723,9 @@ mdadm_status_t manage_add_external(struct supertype *st, int fd, char *disk_name
 				       0, 1))
 		goto out;
 
+	if (mddev_test_and_add_drive_policies(st, &pols, container_fd, 1))
+		goto out;
+
 	Kill(disk_name, NULL, 0, -1, 0);
 
 	disk_fd = dev_open(disk_name, O_RDWR | O_EXCL | O_DIRECT);
@@ -730,6 +734,9 @@ mdadm_status_t manage_add_external(struct supertype *st, int fd, char *disk_name
 		goto out;
 	}
 
+	if (drive_test_and_add_policies(st, &pols, disk_fd, 1))
+		goto out;
+
 	if (st->ss->add_to_super(st, disc, disk_fd, disk_name, INVALID_SECTORS))
 		goto out;
 
@@ -760,6 +767,7 @@ mdadm_status_t manage_add_external(struct supertype *st, int fd, char *disk_name
 
 out:
 	close(container_fd);
+	dev_policy_free(pols);
 
 	if (sra)
 		sysfs_free(sra);
-- 
2.35.3

[PATCH 10/13] Monitor, Incremental: use device policies

[Mariusz Tkaczyk]

spare_criteria is expanded to contain policies which will be generated
by handler's get_spare_criteria() function. It provides a way to
test device for metadata specific policies earlier than during
add_do_super(), when device is already removed from previous
array/container for Monitor.

For Incremental, it ensures that all criteria are tested when trying
spare. It is not tested when device contains valid metadata.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 Incremental.c |  2 +-
 Monitor.c     |  3 ++-
 mdadm.h       |  5 +++--
 util.c        | 13 +++++++++----
 4 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/Incremental.c b/Incremental.c
index 66c2cc86dc5a..958ba9ba7851 100644
--- a/Incremental.c
+++ b/Incremental.c
@@ -865,7 +865,7 @@ mdadm_status_t incremental_external_test_spare_criteria(struct supertype *st, ch
 		goto out;
 	}
 
-	if (!disk_fd_matches_criteria(disk_fd, &sc)) {
+	if (!disk_fd_matches_criteria(dup, disk_fd, &sc)) {
 		if (verbose > 1)
 			pr_err("Disk does not match spare criteria for %s\n", container_devname);
 		goto out;
diff --git a/Monitor.c b/Monitor.c
index 2167523ca3e2..caf6e79f1066 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -1042,7 +1042,7 @@ static dev_t choose_spare(struct state *from, struct state *to,
 			    test_partition_from_id(from->devid[d]))
 				continue;
 
-			if (devid_matches_criteria(from->devid[d], sc) == false)
+			if (devid_matches_criteria(to->metadata, from->devid[d], sc) == false)
 				continue;
 
 			pol = devid_policy(from->devid[d]);
@@ -1190,6 +1190,7 @@ static void try_spare_migration(struct state *statelist)
 				}
 			}
 			domain_free(domlist);
+			dev_policy_free(sc.pols);
 		}
 }
 
diff --git a/mdadm.h b/mdadm.h
index af2bc714bacb..cfa11391415a 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -433,6 +433,7 @@ struct spare_criteria {
 	bool criteria_set;
 	unsigned long long min_size;
 	unsigned int sector_size;
+	struct dev_policy *pols;
 };
 
 typedef enum mdadm_status {
@@ -1734,8 +1735,8 @@ extern int assemble_container_content(struct supertype *st, int mdfd,
 #define	INCR_ALREADY	4
 #define	INCR_YES	8
 
-extern bool devid_matches_criteria(dev_t devid, struct spare_criteria *sc);
-extern bool disk_fd_matches_criteria(int disk_fd, struct spare_criteria *sc);
+extern bool devid_matches_criteria(struct supertype *st, dev_t devid, struct spare_criteria *sc);
+extern bool disk_fd_matches_criteria(struct supertype *st, int disk_fd, struct spare_criteria *sc);
 extern struct mdinfo *container_choose_spares(struct supertype *st,
 					      struct spare_criteria *criteria,
 					      struct domainlist *domlist,
diff --git a/util.c b/util.c
index 041e78cf5426..05ad33436dfe 100644
--- a/util.c
+++ b/util.c
@@ -2056,12 +2056,13 @@ unsigned int __invalid_size_argument_for_IOC = 0;
 
 /**
  * disk_fd_matches_criteria() - check if device matches spare criteria.
+ * @st: supertype, not NULL.
  * @disk_fd: file descriptor of the disk.
  * @sc: criteria to test.
  *
  * Return: true if disk matches criteria, false otherwise.
  */
-bool disk_fd_matches_criteria(int disk_fd, struct spare_criteria *sc)
+bool disk_fd_matches_criteria(struct supertype *st, int disk_fd, struct spare_criteria *sc)
 {
 	unsigned int dev_sector_size = 0;
 	unsigned long long dev_size = 0;
@@ -2076,17 +2077,21 @@ bool disk_fd_matches_criteria(int disk_fd, struct spare_criteria *sc)
 	    sc->sector_size != dev_sector_size)
 		return false;
 
+	if (drive_test_and_add_policies(st, &sc->pols, disk_fd, 0))
+		return false;
+
 	return true;
 }
 
 /**
  * devid_matches_criteria() - check if device referenced by devid matches spare criteria.
+ * @st: supertype, not NULL.
  * @devid: devid of the device to check.
  * @sc: criteria to test.
  *
  * Return: true if disk matches criteria, false otherwise.
  */
-bool devid_matches_criteria(dev_t devid, struct spare_criteria *sc)
+bool devid_matches_criteria(struct supertype *st, dev_t devid, struct spare_criteria *sc)
 {
 	char buf[NAME_MAX];
 	bool ret;
@@ -2102,7 +2107,7 @@ bool devid_matches_criteria(dev_t devid, struct spare_criteria *sc)
 		return false;
 
 	/* Error code inherited */
-	ret = disk_fd_matches_criteria(fd, sc);
+	ret = disk_fd_matches_criteria(st, fd, sc);
 
 	close(fd);
 	return ret;
@@ -2137,7 +2142,7 @@ struct mdinfo *container_choose_spares(struct supertype *st,
 		if (d->disk.state == 0) {
 			dev_t dev = makedev(d->disk.major,d->disk.minor);
 
-			found = devid_matches_criteria(dev, criteria);
+			found = devid_matches_criteria(st, dev, criteria);
 
 			/* check if domain matches */
 			if (found && domlist) {
-- 
2.35.3

[PATCH 11/13] imsm: test_and_add_device_policies() implementation

[Mariusz Tkaczyk]

This patch removes get_disk_controller_domain_imsm() in favour of
test_and_add_device_policies_imsm(). It is used by
create, add and mdmonitor.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 platform-intel.h |   1 -
 super-intel.c    | 123 ++++++++++++++++++++++++++++++++++-------------
 2 files changed, 90 insertions(+), 34 deletions(-)

diff --git a/platform-intel.h b/platform-intel.h
index ce29d3da58e4..3c2bc595f7b5 100644
--- a/platform-intel.h
+++ b/platform-intel.h
@@ -262,7 +262,6 @@ int disk_attached_to_hba(int fd, const char *hba_path);
 int devt_attached_to_hba(dev_t dev, const char *hba_path);
 char *devt_to_devpath(dev_t dev, int dev_level, char *buf);
 int path_attached_to_hba(const char *disk_path, const char *hba_path);
-const char *get_sys_dev_type(enum sys_dev_type);
 const struct orom_entry *get_orom_entry_by_device_id(__u16 dev_id);
 const struct imsm_orom *get_orom_by_device_id(__u16 device_id);
 struct sys_dev *device_by_id(__u16 device_id);
diff --git a/super-intel.c b/super-intel.c
index 90928dce722e..fcbfb85d009f 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -11220,39 +11220,90 @@ abort:
 	return retval;
 }
 
-static char disk_by_path[] = "/dev/disk/by-path/";
-
-static const char *imsm_get_disk_controller_domain(const char *path)
-{
-	char disk_path[PATH_MAX];
-	char *drv=NULL;
-	struct stat st;
-
-	strncpy(disk_path, disk_by_path, PATH_MAX);
-	strncat(disk_path, path, PATH_MAX - strlen(disk_path) - 1);
-	if (stat(disk_path, &st) == 0) {
-		struct sys_dev* hba;
-		char *path;
-
-		path = devt_to_devpath(st.st_rdev, 1, NULL);
-		if (path == NULL)
-			return "unknown";
-		hba = find_disk_attached_hba(-1, path);
-		if (hba && hba->type == SYS_DEV_SAS)
-			drv = "isci";
-		else if (hba && (hba->type == SYS_DEV_SATA || hba->type == SYS_DEV_SATA_VMD))
-			drv = "ahci";
-		else if (hba && hba->type == SYS_DEV_VMD)
-			drv = "vmd";
-		else if (hba && hba->type == SYS_DEV_NVME)
-			drv = "nvme";
-		else
-			drv = "unknown";
-		dprintf("path: %s hba: %s attached: %s\n",
-			path, (hba) ? hba->path : "NULL", drv);
-		free(path);
+/**
+ * test_and_add_drive_controller_policy_imsm() - add disk controller to policies list.
+ * @type: Policy type to search on list.
+ * @pols: List of currently recorded policies.
+ * @disk_fd: File descriptor of the device to check.
+ * @hba: The hba disk is attached, could be NULL if verification is disabled.
+ * @verbose: verbose flag.
+ *
+ * IMSM cares about drive physical placement. If @hba is not set, it adds unknown policy.
+ * If there is no controller policy on pols we are free to add first one. If there is a policy then,
+ * new must be the same - no controller mixing allowed.
+ */
+static mdadm_status_t
+test_and_add_drive_controller_policy_imsm(const char * const type, dev_policy_t **pols, int disk_fd,
+					  struct sys_dev *hba, const int verbose)
+{
+	const char *controller_policy = get_sys_dev_type(SYS_DEV_UNKNOWN);
+	struct dev_policy *pol = pol_find(*pols, (char *)type);
+	char devname[MAX_RAID_SERIAL_LEN];
+
+	if (hba)
+		controller_policy = get_sys_dev_type(hba->type);
+
+	if (!pol) {
+		pol_add(pols, (char *)type, (char *)controller_policy, "imsm");
+		return MDADM_STATUS_SUCCESS;
 	}
-	return drv;
+
+	if (strcmp(pol->value, controller_policy) == 0)
+		return MDADM_STATUS_SUCCESS;
+
+	fd2devname(disk_fd, devname);
+	pr_vrb("Intel(R) raid controller \"%s\" found for %s, but \"%s\" was detected earlier\n",
+	       controller_policy, devname, pol->value);
+	pr_vrb("Disks under different controllers cannot be used, aborting\n");
+
+	return MDADM_STATUS_ERROR;
+}
+
+struct imsm_drive_policy {
+	char *type;
+	mdadm_status_t (*test_and_add_drive_policy)(const char * const type,
+						    struct dev_policy **pols, int disk_fd,
+						    struct sys_dev *hba, const int verbose);
+};
+
+struct imsm_drive_policy imsm_policies[] = {
+	{"controller", test_and_add_drive_controller_policy_imsm},
+};
+
+mdadm_status_t test_and_add_drive_policies_imsm(struct dev_policy **pols, int disk_fd,
+						const int verbose)
+{
+	struct imsm_drive_policy *imsm_pol;
+	struct sys_dev *hba = NULL;
+	char path[PATH_MAX];
+	mdadm_status_t ret;
+	unsigned int i;
+
+	/* If imsm platform verification is disabled, do not search for hba. */
+	if (check_no_platform() != 1) {
+		if (!diskfd_to_devpath(disk_fd, 1, path)) {
+			pr_vrb("IMSM: Failed to retrieve device path by file descriptor.\n");
+			return MDADM_STATUS_ERROR;
+		}
+
+		hba = find_disk_attached_hba(disk_fd, path);
+		if (!hba) {
+			pr_vrb("IMSM: Failed to find hba for %s\n", path);
+			return MDADM_STATUS_ERROR;
+		}
+	}
+
+	for (i = 0; i < ARRAY_SIZE(imsm_policies); i++) {
+		imsm_pol = &imsm_policies[i];
+
+		ret = imsm_pol->test_and_add_drive_policy(imsm_pol->type, pols, disk_fd, hba,
+							  verbose);
+		if (ret != MDADM_STATUS_SUCCESS)
+			/* Inherit error code */
+			return ret;
+	}
+
+	return MDADM_STATUS_SUCCESS;
 }
 
 /**
@@ -11280,6 +11331,7 @@ mdadm_status_t get_spare_criteria_imsm(struct supertype *st, char *mddev_path,
 
 	if (mddev_path) {
 		int fd = open(mddev_path, O_RDONLY);
+		mdadm_status_t rv;
 
 		if (!is_fd_valid(fd))
 			return MDADM_STATUS_ERROR;
@@ -11291,7 +11343,12 @@ mdadm_status_t get_spare_criteria_imsm(struct supertype *st, char *mddev_path,
 			}
 			free_superblock = true;
 		}
+
+		rv = mddev_test_and_add_drive_policies(st, &c->pols, fd, 0);
 		close(fd);
+
+		if (rv != MDADM_STATUS_SUCCESS)
+			goto out;
 	}
 
 	super = st->sb;
@@ -13026,7 +13083,7 @@ struct superswitch super_imsm = {
 	.update_subarray = update_subarray_imsm,
 	.load_container	= load_container_imsm,
 	.default_geometry = default_geometry_imsm,
-	.get_disk_controller_domain = imsm_get_disk_controller_domain,
+	.test_and_add_drive_policies = test_and_add_drive_policies_imsm,
 	.reshape_super  = imsm_reshape_super,
 	.manage_reshape = imsm_manage_reshape,
 	.recover_backup = recover_backup_imsm,
-- 
2.35.3

[PATCH 12/13] mdadm: drop get_disk_controller_domain()

[Mariusz Tkaczyk]

This function is unused now. Drop it.
Controller for IMSM is a device policy and is separated from user defined
domains.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 mdadm.h  | 15 ---------------
 policy.c | 13 -------------
 2 files changed, 28 deletions(-)

diff --git a/mdadm.h b/mdadm.h
index cfa11391415a..3fedca484bdd 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1286,21 +1286,6 @@ extern struct superswitch {
 	 */
 	struct mdinfo *(*activate_spare)(struct active_array *a,
 					 struct metadata_update **updates);
-	/*
-	 * Return statically allocated string that represents metadata specific
-	 * controller domain of the disk. The domain is used in disk domain
-	 * matching functions. Disks belong to the same domain if the they have
-	 * the same domain from mdadm.conf and belong the same metadata domain.
-	 * Returning NULL or not providing this handler means that metadata
-	 * does not distinguish the differences between disks that belong to
-	 * different controllers. They are in the domain specified by
-	 * configuration file (mdadm.conf).
-	 * In case when the metadata has the notion of domains based on disk
-	 * it shall return NULL for disks that do not belong to the controller
-	 * the supported domains. Such disks will form another domain and won't
-	 * be mixed with supported ones.
-	 */
-	const char *(*get_disk_controller_domain)(const char *path);
 
 	/* for external backup area */
 	int (*recover_backup)(struct supertype *st, struct mdinfo *info);
diff --git a/policy.c b/policy.c
index 4b85f62d9675..404f9b5dd347 100644
--- a/policy.c
+++ b/policy.c
@@ -365,7 +365,6 @@ struct dev_policy *path_policy(char **paths, char *type)
 {
 	struct pol_rule *rules;
 	struct dev_policy *pol = NULL;
-	int i;
 
 	rules = config_rules;
 
@@ -380,18 +379,6 @@ struct dev_policy *path_policy(char **paths, char *type)
 		rules = rules->next;
 	}
 
-	/* Now add any metadata-specific internal knowledge
-	 * about this path
-	 */
-	for (i=0; paths && paths[0] && superlist[i]; i++)
-		if (superlist[i]->get_disk_controller_domain) {
-			const char *d =
-				superlist[i]->get_disk_controller_domain(
-					paths[0]);
-			if (d)
-				pol_new(&pol, pol_domain, d, superlist[i]->name);
-		}
-
 	pol_sort(&pol);
 	pol_dedup(pol);
 	return pol;
-- 
2.35.3

[PATCH 13/13] Revert "policy.c: Avoid to take spare without defined domain by imsm"

[Mariusz Tkaczyk]

This reverts commit 3bf9495270d7 ("policy.c: Avoid to take spare without
defined domain by imsm").

IMSM does not require to be special now because it doesn't create disk
controller domain.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 policy.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/policy.c b/policy.c
index 404f9b5dd347..dfaafdc07cdc 100644
--- a/policy.c
+++ b/policy.c
@@ -759,7 +759,6 @@ int domain_test(struct domainlist *dom, struct dev_policy *pol,
 	 *  1:  has domains, all match
 	 */
 	int found_any = -1;
-	int has_one_domain = 1;
 	struct dev_policy *p;
 
 	pol = pol_find(pol, pol_domain);
@@ -769,9 +768,6 @@ int domain_test(struct domainlist *dom, struct dev_policy *pol,
 			dom = dom->next;
 		if (!dom || strcmp(dom->dom, p->value) != 0)
 			return 0;
-		if (has_one_domain && metadata && strcmp(metadata, "imsm") == 0)
-			found_any = -1;
-		has_one_domain = 0;
 	}
 	return found_any;
 }
-- 
2.35.3

Re: [PATCH 00/13] Custom drives policies verification

[Mariusz Tkaczyk]

On Thu, 29 Feb 2024 12:52:04 +0100
Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com> wrote:

> validate_geometry_imsm() over the years became huge and complicated.
> It is extremely hard to develop or optimize this code now. What the
> most important, it doesn't address all scenarios. For example if
> container contains disks under different controllers (spare container),
> "autolayout" feature allows to create raid array. This code has a lot
> of dependencies and it is almost impossible to add support of this
> scenario without breaking something else.
> 
> There is also get_disk_controller_domain() which in my understating is a
> part of validate_geometry_imsm() functionality moved outside, fit ideally
> to mdmonitor needs.
> 
> Drive encryption determining will be added to IMSM soon. The encryption
> status of the drive will be determined for every drive. There is no
> simple way to add it.
> 
> The solution added in this serie addresses those problems by making one
> easily extendable api to analyze every disk separately, outside
> validate_geometry().
> 
> First five patches are optimizations with no functional changes. New
> functionality replaces get_disk_controller_domain(). It should also
> cover some verifications done in validate_geometry_imsm() and
> add_to_super_imsm() but to lower regression risk these parts are
> not removed yet.
> 
> Mariusz Tkaczyk (13):
>   mdadm: Add functions for spare criteria verification
>   mdadm: drop get_required_spare_criteria()
>   Manage: fix check after dereference issue
>   Manage: implement manage_add_external()
>   mdadm: introduce sysfs_get_container_devnm()
>   mdadm.h: Introduce custom device policies
>   mdadm: test_and_add device policies implementation
>   Create: Use device policies
>   Manage: check device policies in manage_add_external()
>   Monitor, Incremental: use device policies
>   imsm: test_and_add_device_policies() implementation
>   mdadm: drop get_disk_controller_domain()
>   Revert "policy.c: Avoid to take spare without defined domain by imsm"
> 
>  Create.c         |  48 +++++++---
>  Incremental.c    |  77 ++++++++++-----
>  Manage.c         | 195 +++++++++++++++++++++----------------
>  Monitor.c        |  50 ++--------
>  mdadm.h          |  90 +++++++++--------
>  platform-intel.h |   1 -
>  policy.c         | 110 +++++++++++++++++----
>  super-intel.c    | 245 ++++++++++++++++++++++++++++++++---------------
>  sysfs.c          |  23 +++++
>  util.c           | 117 ++++++++++++----------
>  10 files changed, 606 insertions(+), 350 deletions(-)
> 

No comments..

Applied! 

Thanks,
Mariusz