incremental, manage: do not verify if remove is safe

Commit Message

Kinga Tanska Dec. 20, 2022, 3:21 a.m. UTC

Function is_remove_safe() was introduced to verify if removing
member device won't cause failed state of the array. This
verification should be used only with set-faulty command. Add
special mode indicating that Incremental removal was executed.
If this mode is used do not execute is_remove_safe() routine.

Signed-off-by: Kinga Tanska <[e-mail]>
---
 Incremental.c |  2 +-
 Manage.c      | 79 +++++++++++++++++++++++++++++++--------------------
 2 files changed, 49 insertions(+), 32 deletions(-)

Comments

Paul Menzel Dec. 20, 2022, 10:46 a.m. UTC | #1

Dear Kinga,


Thank you for your patch.

Am 20.12.22 um 04:21 schrieb Kinga Tanska:

[…]

> +/**
> + * Manage_subdevs() - Execute operation depending on devmode.
> + *
> + * @devname: name of the device.
> + * @fd: file descriptor.
> + * @devlist: list of sub-devices to manage.
> + * @verbose: verbose level.
> + * @test: test flag.
> + * @update: type of update.
> + * @force: force flag.
> + *
> + * This function executes operation defined by devmode
> + * for each dev from devlist.
> + * Devmode can be:
> + * 'a' - add the device
> + * 'S' - add the device as a spare - don't try re-add
> + * 'j' - add the device as a journal device
> + * 'A' - re-add the device
> + * 'r' - remove the device: HOT_REMOVE_DISK
> + *       device can be 'faulty' or 'detached' in which case all
> + *       matching devices are removed.
> + * 'f' - set the device faulty SET_DISK_FAULTY
> + *       device can be 'detached' in which case any device that
> + *       is inaccessible will be marked faulty.
> + * 'I' - remove device by using incremental fail
> + *       which is executed when device is removed surprisingly.
> + * 'R' - mark this device as wanting replacement.
> + * 'W' - this device is added if necessary and activated as
> + *       a replacement for a previous 'R' device.
> + * -----
> + * 'w' - 'W' will be changed to 'w' when it is paired with
> + *       a 'R' device.  If a 'W' is found while walking the list
> + *       it must be unpaired, and is an error.
> + * 'M' - this is created by a 'missing' target.  It is a slight
> + *       variant on 'A'
> + * 'F' - Another variant of 'A', where the device was faulty
> + *       so must be removed from the array first.
> + * 'c' - confirm the device as found (for clustered environments)
> + *
> + * For 'f' and 'r', the device can also be a kernel-internal
> + * name such as 'sdb'.
> + *
> + * Return: 0 on success, otherwise 1 or 2.
> + */
>   int Manage_subdevs(char *devname, int fd,
>   		   struct mddev_dev *devlist, int verbose, int test,
>   		   char *update, int force)
>   {
> -	/* Do something to each dev.
> -	 * devmode can be
> -	 *  'a' - add the device
> -	 *  'S' - add the device as a spare - don't try re-add
> -	 *  'j' - add the device as a journal device
> -	 *  'A' - re-add the device
> -	 *  'r' - remove the device: HOT_REMOVE_DISK
> -	 *        device can be 'faulty' or 'detached' in which case all
> -	 *	  matching devices are removed.
> -	 *  'f' - set the device faulty SET_DISK_FAULTY
> -	 *        device can be 'detached' in which case any device that
> -	 *	  is inaccessible will be marked faulty.
> -	 *  'R' - mark this device as wanting replacement.
> -	 *  'W' - this device is added if necessary and activated as
> -	 *        a replacement for a previous 'R' device.
> -	 * -----
> -	 *  'w' - 'W' will be changed to 'w' when it is paired with
> -	 *        a 'R' device.  If a 'W' is found while walking the list
> -	 *        it must be unpaired, and is an error.
> -	 *  'M' - this is created by a 'missing' target.  It is a slight
> -	 *        variant on 'A'
> -	 *  'F' - Another variant of 'A', where the device was faulty
> -	 *        so must be removed from the array first.
> -	 *  'c' - confirm the device as found (for clustered environments)
> -	 *
> -	 * For 'f' and 'r', the device can also be a kernel-internal
> -	 * name such as 'sdb'.
> -	 */

Please move the comment in a separate commit.


Kind regards,

Paul

Patch

diff --git a/Incremental.c b/Incremental.c
index 5a5f4c4c..bccfdeb9 100644
--- a/Incremental.c
+++ b/Incremental.c
@@ -1744,7 +1744,7 @@  int IncrementalRemove(char *devname, char *id_path, int verbose)
 
 	memset(&devlist, 0, sizeof(devlist));
 	devlist.devname = devname;
-	devlist.disposition = 'f';
+	devlist.disposition = 'I';
 	/* for a container, we must fail each member array */
 	if (ent->metadata_version &&
 	    strncmp(ent->metadata_version, "external:", 9) == 0) {
diff --git a/Manage.c b/Manage.c
index b1d0e630..9d85237c 100644
--- a/Manage.c
+++ b/Manage.c
@@ -1329,38 +1329,54 @@  bool is_remove_safe(mdu_array_info_t *array, const int fd, char *devname, const
 	return is_enough;
 }
 
+/**
+ * Manage_subdevs() - Execute operation depending on devmode.
+ *
+ * @devname: name of the device.
+ * @fd: file descriptor.
+ * @devlist: list of sub-devices to manage.
+ * @verbose: verbose level.
+ * @test: test flag.
+ * @update: type of update.
+ * @force: force flag.
+ *
+ * This function executes operation defined by devmode
+ * for each dev from devlist.
+ * Devmode can be:
+ * 'a' - add the device
+ * 'S' - add the device as a spare - don't try re-add
+ * 'j' - add the device as a journal device
+ * 'A' - re-add the device
+ * 'r' - remove the device: HOT_REMOVE_DISK
+ *       device can be 'faulty' or 'detached' in which case all
+ *       matching devices are removed.
+ * 'f' - set the device faulty SET_DISK_FAULTY
+ *       device can be 'detached' in which case any device that
+ *       is inaccessible will be marked faulty.
+ * 'I' - remove device by using incremental fail
+ *       which is executed when device is removed surprisingly.
+ * 'R' - mark this device as wanting replacement.
+ * 'W' - this device is added if necessary and activated as
+ *       a replacement for a previous 'R' device.
+ * -----
+ * 'w' - 'W' will be changed to 'w' when it is paired with
+ *       a 'R' device.  If a 'W' is found while walking the list
+ *       it must be unpaired, and is an error.
+ * 'M' - this is created by a 'missing' target.  It is a slight
+ *       variant on 'A'
+ * 'F' - Another variant of 'A', where the device was faulty
+ *       so must be removed from the array first.
+ * 'c' - confirm the device as found (for clustered environments)
+ *
+ * For 'f' and 'r', the device can also be a kernel-internal
+ * name such as 'sdb'.
+ *
+ * Return: 0 on success, otherwise 1 or 2.
+ */
 int Manage_subdevs(char *devname, int fd,
 		   struct mddev_dev *devlist, int verbose, int test,
 		   char *update, int force)
 {
-	/* Do something to each dev.
-	 * devmode can be
-	 *  'a' - add the device
-	 *  'S' - add the device as a spare - don't try re-add
-	 *  'j' - add the device as a journal device
-	 *  'A' - re-add the device
-	 *  'r' - remove the device: HOT_REMOVE_DISK
-	 *        device can be 'faulty' or 'detached' in which case all
-	 *	  matching devices are removed.
-	 *  'f' - set the device faulty SET_DISK_FAULTY
-	 *        device can be 'detached' in which case any device that
-	 *	  is inaccessible will be marked faulty.
-	 *  'R' - mark this device as wanting replacement.
-	 *  'W' - this device is added if necessary and activated as
-	 *        a replacement for a previous 'R' device.
-	 * -----
-	 *  'w' - 'W' will be changed to 'w' when it is paired with
-	 *        a 'R' device.  If a 'W' is found while walking the list
-	 *        it must be unpaired, and is an error.
-	 *  'M' - this is created by a 'missing' target.  It is a slight
-	 *        variant on 'A'
-	 *  'F' - Another variant of 'A', where the device was faulty
-	 *        so must be removed from the array first.
-	 *  'c' - confirm the device as found (for clustered environments)
-	 *
-	 * For 'f' and 'r', the device can also be a kernel-internal
-	 * name such as 'sdb'.
-	 */
 	mdu_array_info_t array;
 	unsigned long long array_size;
 	struct mddev_dev *dv;
@@ -1496,8 +1512,9 @@  int Manage_subdevs(char *devname, int fd,
 			/* Assume this is a kernel-internal name like 'sda1' */
 			int found = 0;
 			char dname[55];
-			if (dv->disposition != 'r' && dv->disposition != 'f') {
-				pr_err("%s only meaningful with -r or -f, not -%c\n",
+			if (dv->disposition != 'r' && dv->disposition != 'f' &&
+			    dv->disposition != 'I') {
+				pr_err("%s only meaningful with -r, -f or -I, not -%c\n",
 					dv->devname, dv->disposition);
 				goto abort;
 			}
@@ -1649,7 +1666,7 @@  int Manage_subdevs(char *devname, int fd,
 					close(sysfd);
 				goto abort;
 			}
-
+		case 'I': /* incremental fail */
 			if ((sysfd >= 0 && write(sysfd, "faulty", 6) != 6) ||
 			    (sysfd < 0 && ioctl(fd, SET_DISK_FAULTY,
 						rdev))) {