Re: [PATCH] Incremental: remove obsoleted calls to udisks

[Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>]

On Fri,  5 May 2023 13:22:31 +0800
Coly Li <colyli@suse.de> wrote:

> Utilility udisks is removed from udev upstream, calling this obsoleted
> command in run_udisks() doesn't make any sense now.
> 
> This patch removes the calls chain of udisks, which includes routines
> run_udisk(), force_remove(), and 2 locations where force_remove() are
> called.
> 
> In remove_from_member_array() and IncrementalRemove(), if return value
> of calling Manage_subdevs() is not 0, don't call force_remove() and only
> print error message when parameter 'verbose' is true.
> 
> Signed-off-by: Coly Li <colyli@suse.de>
> Cc: Mariusz Tkaczyk <mariusz.tkaczyk@intel.com>
> Cc: Jes Sorensen <jes@trained-monkey.org>
> ---
>  Incremental.c | 50 +++++++-------------------------------------------
>  1 file changed, 7 insertions(+), 43 deletions(-)
> 
> diff --git a/Incremental.c b/Incremental.c
> index 49a71f7..e1a953a 100644
> --- a/Incremental.c
> +++ b/Incremental.c
> @@ -1630,54 +1630,18 @@ release:
>  	return rv;
>  }
>  
> -static void run_udisks(char *arg1, char *arg2)
> -{
> -	int pid = fork();
> -	int status;
> -	if (pid == 0) {
> -		manage_fork_fds(1);
> -		execl("/usr/bin/udisks", "udisks", arg1, arg2, NULL);
> -		execl("/bin/udisks", "udisks", arg1, arg2, NULL);
> -		exit(1);
> -	}
> -	while (pid > 0 && wait(&status) != pid)
> -		;
> -}
> -
> -static int force_remove(char *devnm, int fd, struct mdinfo *mdi, int verbose)
> -{
> -	int rv;
> -	int devid = devnm2devid(devnm);
> -
> -	run_udisks("--unmount", map_dev(major(devid), minor(devid), 0));
> -	rv = Manage_stop(devnm, fd, verbose, 1);

Hi Coly,
Please see that you removed Manage_stop(). Now mdadm won't try to
stop failed arrays. It is good change but please describe it.

> -	if (rv) {
> -		/* At least we can try to trigger a 'remove' */
> -		sysfs_uevent(mdi, "remove");
> -		if (verbose)
> -			pr_err("Fail to stop %s too.\n", devnm);
> -	}
> -	return rv;
> -}
> -
>  static void remove_from_member_array(struct mdstat_ent *memb,
>  				    struct mddev_dev *devlist, int verbose)
>  {
>  	int rv;
> -	struct mdinfo mmdi;
>  	int subfd = open_dev(memb->devnm);
>  
>  	if (subfd >= 0) {
>  		rv = Manage_subdevs(memb->devnm, subfd, devlist, verbose,
>  				    0, UOPT_UNDEFINED, 0);
> -		if (rv & 2) {
> -			if (sysfs_init(&mmdi, -1, memb->devnm))
> -				pr_err("unable to initialize sysfs for:
> %s\n",
> -				       memb->devnm);
> -			else
> -				force_remove(memb->devnm, subfd, &mmdi,
> -					     verbose);
> -		}
> +		if ((rv & 2) && verbose)

There is a rule (at least we at Intel tried to follow) to use indirect
comparisons only for pointers. I know that we don't have log level in mdadm,
we need to compare with values directly:
if ((rv & 2) && verbose > 0)

It is not mandatory, just to let you know.
> +			pr_err("Fail to remove %s from array.\n",
> memb->devnm);

Could we make this error less "dangerous"? I mean that someone may think that
something is wrong here but it is not - the message is expected in case when
raid becomes failed. Note that for raid5 disk is removed from array but EBUSY
is returned anyway. Maybe we should check for MD_BROKEN in array_state to
differentiate and make errors more detailed?

Same applies to the native case below.

>  		close(subfd);
>  	}
>  }
> @@ -1763,10 +1727,10 @@ int IncrementalRemove(char *devname, char *id_path,
> int verbose) rv |= Manage_subdevs(ent->devnm, mdfd, &devlist,
>  				    verbose, 0, UOPT_UNDEFINED, 0);
>  		if (rv & 2) {
> -		/* Failed due to EBUSY, try to stop the array.
> -		 * Give udisks a chance to unmount it first.
> -		 */
> -			rv = force_remove(ent->devnm, mdfd, &mdi, verbose);
> +			if (verbose)
> +				pr_err("Fail to remove %s from array.\n",
> ent->devnm);
> +			/* Only return 0 or 1 */
> +			rv = !!rv;
we are in if (rv & 2) so I think that we can simply set rv = 1, am I right?

>  			goto end;
>  		}
>  	}

Thanks,
Mariusz