[PATCH v2] Incremental: remove obsoleted calls to udisks

[PATCH v2] Incremental: remove obsoleted calls to udisks

[Coly Li]

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. Considering force_remove() is removed with udisks util, it is
fair to remove Manage_stop() inside force_remove() as well.

After force_remove() is not called anymore, if Manage_subdevs() returns
failure due to a busy array, nothing else to do. If the failure is from
a broken array and verbose information is wanted, a warning message will
be printed by pr_err().

Signed-off-by: Coly Li <colyli@suse.de>
Cc: Mariusz Tkaczyk <mariusz.tkaczyk@intel.com>
Cc: Jes Sorensen <jes@trained-monkey.org>
---
Changelog,
v2: improve based on code review comments from Mariusz.
v1: initial version.

 Incremental.c | 88 ++++++++++++++++++++++++---------------------------
 1 file changed, 42 insertions(+), 46 deletions(-)

diff --git a/Incremental.c b/Incremental.c
index f13ce02..d390a08 100644
--- a/Incremental.c
+++ b/Incremental.c
@@ -1628,56 +1628,38 @@ 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);
-	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;
+	char buf[32];
 	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 (subfd < 0)
+		return;
+
+	rv = Manage_subdevs(memb->devnm, subfd, devlist, verbose,
+			    0, UOPT_UNDEFINED, 0);
+	if (rv) {
+		/*
+		 * If the array is busy or no verbose info
+		 * desired, nonthing else to do.
+		 */
+		if ((rv & 2) || verbose <= 0)
+			goto close;
+
+		/* Otherwise if failed due to a broken array, warn */
+		if (sysfs_init(&mmdi, -1, memb->devnm) == 0 &&
+		    sysfs_get_str(&mmdi, NULL, "array_state",
+				  buf, sizeof(buf)) > 0 &&
+		    strncmp(buf, "broken", 6) == 0) {
+			pr_err("Fail to remove %s from broken array.\n",
+			       memb->devnm);
 		}
-		close(subfd);
 	}
+close:
+	close(subfd);
 }
 
 /*
@@ -1760,11 +1742,22 @@ int IncrementalRemove(char *devname, char *id_path, int verbose)
 	} else {
 		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 (rv) {
+			/*
+			 * If the array is busy or no verbose info
+			 * desired, nothing else to do.
+			 */
+			if ((rv & 2) || verbose <= 0)
+				goto end;
+
+			/* Otherwise if failed due to a broken array, warn */
+			if (sysfs_get_str(&mdi, NULL, "array_state",
+					  buf, sizeof(buf)) > 0 &&
+			    strncmp(buf, "broken", 6) == 0) {
+				pr_err("Fail to remove %s from broken array.\n",
+				       ent->devnm);
+			}
+
 			goto end;
 		}
 	}
@@ -1775,5 +1768,8 @@ int IncrementalRemove(char *devname, char *id_path, int verbose)
 end:
 	close(mdfd);
 	free_mdstat(ent);
+	/* Only return 0 or 1 */
+	if (rv != 0)
+		rv = 1;
 	return rv;
 }
-- 
2.35.3

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

[Mariusz Tkaczyk]

On Fri, 26 May 2023 01:08:43 +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. Considering force_remove() is removed with udisks util, it is
> fair to remove Manage_stop() inside force_remove() as well.
> 
> After force_remove() is not called anymore, if Manage_subdevs() returns
> failure due to a busy array, nothing else to do. If the failure is from
> a broken array and verbose information is wanted, a warning message will
> be printed by pr_err().
> 
> Signed-off-by: Coly Li <colyli@suse.de>
> Cc: Mariusz Tkaczyk <mariusz.tkaczyk@intel.com>
> Cc: Jes Sorensen <jes@trained-monkey.org>
> ---
> Changelog,
> v2: improve based on code review comments from Mariusz.
> v1: initial version.
> 
>  Incremental.c | 88 ++++++++++++++++++++++++---------------------------
>  1 file changed, 42 insertions(+), 46 deletions(-)
> 
> diff --git a/Incremental.c b/Incremental.c
> index f13ce02..d390a08 100644
> --- a/Incremental.c
> +++ b/Incremental.c
> @@ -1628,56 +1628,38 @@ 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);
> -	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;
> +	char buf[32];

Another place where we hard-coding array size. We already
addressed it (patch is waiting for internal regression), so please left it as is
for now. Just to let everyone know.

>  	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 (subfd < 0)
> +		return;
> +
> +	rv = Manage_subdevs(memb->devnm, subfd, devlist, verbose,
> +			    0, UOPT_UNDEFINED, 0);
> +	if (rv) {
> +		/*
> +		 * If the array is busy or no verbose info
> +		 * desired, nonthing else to do.
> +		 */
> +		if ((rv & 2) || verbose <= 0)
> +			goto close;
> +
> +		/* Otherwise if failed due to a broken array, warn */
> +		if (sysfs_init(&mmdi, -1, memb->devnm) == 0 &&
> +		    sysfs_get_str(&mmdi, NULL, "array_state",
> +				  buf, sizeof(buf)) > 0 &&
> +		    strncmp(buf, "broken", 6) == 0) {
> +			pr_err("Fail to remove %s from broken array.\n",
> +			       memb->devnm);

The codes above and below are almost the same now, can we move them to a
function?
>  		}
> -		close(subfd);
>  	}
> +close:
> +	close(subfd);
>  }
>  
>  /*
> @@ -1760,11 +1742,22 @@ int IncrementalRemove(char *devname, char *id_path,
> int verbose) } else {
>  		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 (rv) {
I would prefer to reverse logic to make one indentation less (if that is
possible):
if (rv != 0)
    goto end;
but it is fine anyway.

> +			/*
> +			 * If the array is busy or no verbose info
> +			 * desired, nothing else to do.
> +			 */
> +			if ((rv & 2) || verbose <= 0)
> +				goto end;
> +
> +			/* Otherwise if failed due to a broken array, warn */
> +			if (sysfs_get_str(&mdi, NULL, "array_state",
> +					  buf, sizeof(buf)) > 0 &&
> +			    strncmp(buf, "broken", 6) == 0) {

Broken is defined in sysfs_array_states[], can we use it?
if (map_name(sysfs_array_states, buf) == ARRAY_BROKEN)
I know that it could looks like a little overhead but compiler should do
the job here.
> +				pr_err("Fail to remove %s from broken
> array.\n",
> +				       ent->devnm);
Not exactly, The broken may be raised even if disk is removed. It is a case for
raid456 and raid1/10 with fail_last_dev=1. I would say just "%s is in broken
state.\n" 
Should be exclude arrays which are already broken (broken was set before we
called mdadm -If)? I don't see printing this message everytime as a problem, but
it is something you should consider.

And I forgot to say it eariler, could you consider adding test/s for both IMSM and native?
It is something that should be tested.
Sorry, scope is growing :(

Thanks,
Mariusz

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

[Coly Li]

On Fri, May 26, 2023 at 09:52:00AM +0200, Mariusz Tkaczyk wrote:
> On Fri, 26 May 2023 01:08:43 +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. Considering force_remove() is removed with udisks util, it is
> > fair to remove Manage_stop() inside force_remove() as well.
> > 
> > After force_remove() is not called anymore, if Manage_subdevs() returns
> > failure due to a busy array, nothing else to do. If the failure is from
> > a broken array and verbose information is wanted, a warning message will
> > be printed by pr_err().
> > 
> > Signed-off-by: Coly Li <colyli@suse.de>
> > Cc: Mariusz Tkaczyk <mariusz.tkaczyk@intel.com>
> > Cc: Jes Sorensen <jes@trained-monkey.org>
> > ---
> > Changelog,
> > v2: improve based on code review comments from Mariusz.
> > v1: initial version.
> > 
> >  Incremental.c | 88 ++++++++++++++++++++++++---------------------------
> >  1 file changed, 42 insertions(+), 46 deletions(-)
> > 
> > diff --git a/Incremental.c b/Incremental.c
> > index f13ce02..d390a08 100644
> > --- a/Incremental.c
> > +++ b/Incremental.c
> > @@ -1628,56 +1628,38 @@ 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);
> > -	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;
> > +	char buf[32];
> 
> Another place where we hard-coding array size. We already
> addressed it (patch is waiting for internal regression), so please left it as is
> for now. Just to let everyone know.
>

Yes, I agree. It should be good to do one thing in each patch. The hard-coding
array size should be addressed in another patch series.

 
> >  	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 (subfd < 0)
> > +		return;
> > +
> > +	rv = Manage_subdevs(memb->devnm, subfd, devlist, verbose,
> > +			    0, UOPT_UNDEFINED, 0);
> > +	if (rv) {
> > +		/*
> > +		 * If the array is busy or no verbose info
> > +		 * desired, nonthing else to do.
> > +		 */
> > +		if ((rv & 2) || verbose <= 0)
> > +			goto close;
> > +
> > +		/* Otherwise if failed due to a broken array, warn */
> > +		if (sysfs_init(&mmdi, -1, memb->devnm) == 0 &&
> > +		    sysfs_get_str(&mmdi, NULL, "array_state",
> > +				  buf, sizeof(buf)) > 0 &&
> > +		    strncmp(buf, "broken", 6) == 0) {
> > +			pr_err("Fail to remove %s from broken array.\n",
> > +			       memb->devnm);
> 
> The codes above and below are almost the same now, can we move them to a
> function?

There is a little difference on calling sysfs_init(), the second location
doesn’t call sysfs_init(). It is possible to use a condition variable to
handle the difference, but that will be another extra complication and
not worthy IMHO.


> >  		}
> > -		close(subfd);
> >  	}
> > +close:
> > +	close(subfd);
> >  }
> >  
> >  /*
> > @@ -1760,11 +1742,22 @@ int IncrementalRemove(char *devname, char *id_path,
> > int verbose) } else {
> >  		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 (rv) {
> I would prefer to reverse logic to make one indentation less (if that is
> possible):
> if (rv != 0)
>     goto end;
> but it is fine anyway.

Indeed I tends to remove all the warning messages, and do nothing else more
if rv != 0 from Manage_subdevs(). How do you think of this idea? Checking the
array state indeed doesn't help too much.

> 
> > +			/*
> > +			 * If the array is busy or no verbose info
> > +			 * desired, nothing else to do.
> > +			 */
> > +			if ((rv & 2) || verbose <= 0)
> > +				goto end;
> > +
> > +			/* Otherwise if failed due to a broken array, warn */
> > +			if (sysfs_get_str(&mdi, NULL, "array_state",
> > +					  buf, sizeof(buf)) > 0 &&
> > +			    strncmp(buf, "broken", 6) == 0) {
> 
> Broken is defined in sysfs_array_states[], can we use it?
> if (map_name(sysfs_array_states, buf) == ARRAY_BROKEN)
> I know that it could looks like a little overhead but compiler should do
> the job here.

Yes, I am aware of this. But the existed coding style in this file is the hard
coded strncmp(). So if we do want to print the warning for a broken array, it
would be better to add the map_name() fixing in another patch. I mean, do one
thing in single patch.


> > +				pr_err("Fail to remove %s from broken
> > array.\n",
> > +				       ent->devnm);
> Not exactly, The broken may be raised even if disk is removed. It is a case for
> raid456 and raid1/10 with fail_last_dev=1. I would say just "%s is in broken
> state.\n" 
> Should be exclude arrays which are already broken (broken was set before we
> called mdadm -If)? I don't see printing this message everytime as a problem, but
> it is something you should consider.
>

Indeed, I still suggest to remove all the pr_err() stuffs, they cannot help too
much, and introduce extra complication.

 
> And I forgot to say it eariler, could you consider adding test/s for both IMSM and native?
> It is something that should be tested.
> Sorry, scope is growing :(

Sure, it's good idea, let's do it later.

Thank you for the code review.

-- 
Coly Li

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

[Mariusz Tkaczyk]

On Fri, 26 May 2023 22:42:52 +0800
Coly Li <colyli@suse.de> wrote:

> On Fri, May 26, 2023 at 09:52:00AM +0200, Mariusz Tkaczyk wrote:
> > On Fri, 26 May 2023 01:08:43 +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. Considering force_remove() is removed with udisks util, it is
> > > fair to remove Manage_stop() inside force_remove() as well.
> > > 
> > > After force_remove() is not called anymore, if Manage_subdevs() returns
> > > failure due to a busy array, nothing else to do. If the failure is from
> > > a broken array and verbose information is wanted, a warning message will
> > > be printed by pr_err().
> > > 
> > > Signed-off-by: Coly Li <colyli@suse.de>
> > > Cc: Mariusz Tkaczyk <mariusz.tkaczyk@intel.com>
> > > Cc: Jes Sorensen <jes@trained-monkey.org>
> > > ---
> > > Changelog,
> > > v2: improve based on code review comments from Mariusz.
> > > v1: initial version.
> > > 
> > >  Incremental.c | 88 ++++++++++++++++++++++++---------------------------
> > >  1 file changed, 42 insertions(+), 46 deletions(-)
> > > 
> > > diff --git a/Incremental.c b/Incremental.c
> > > index f13ce02..d390a08 100644
> > > --- a/Incremental.c
> > > +++ b/Incremental.c
> > > @@ -1628,56 +1628,38 @@ 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);
> > > -	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;
> > > +	char buf[32];  
> > 
> > Another place where we hard-coding array size. We already
> > addressed it (patch is waiting for internal regression), so please left it
> > as is for now. Just to let everyone know.
> >  
> 
> Yes, I agree. It should be good to do one thing in each patch. The hard-coding
> array size should be addressed in another patch series.
> 
>  
> > >  	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 (subfd < 0)
> > > +		return;
> > > +
> > > +	rv = Manage_subdevs(memb->devnm, subfd, devlist, verbose,
> > > +			    0, UOPT_UNDEFINED, 0);
> > > +	if (rv) {
> > > +		/*
> > > +		 * If the array is busy or no verbose info
> > > +		 * desired, nonthing else to do.
> > > +		 */
> > > +		if ((rv & 2) || verbose <= 0)
> > > +			goto close;
> > > +
> > > +		/* Otherwise if failed due to a broken array, warn */
> > > +		if (sysfs_init(&mmdi, -1, memb->devnm) == 0 &&
> > > +		    sysfs_get_str(&mmdi, NULL, "array_state",
> > > +				  buf, sizeof(buf)) > 0 &&
> > > +		    strncmp(buf, "broken", 6) == 0) {
> > > +			pr_err("Fail to remove %s from broken array.\n",
> > > +			       memb->devnm);  
> > 
> > The codes above and below are almost the same now, can we move them to a
> > function?  
> 
> There is a little difference on calling sysfs_init(), the second location
> doesn’t call sysfs_init(). It is possible to use a condition variable to
> handle the difference, but that will be another extra complication and
> not worthy IMHO.
> 

what about initializing the sysfs earlier and passing mdi to function? That
should resolve our problem.(Probably not a case anymore after dropping the
message).
> 
> > >  		}
> > > -		close(subfd);
> > >  	}
> > > +close:
> > > +	close(subfd);
> > >  }
> > >  
> > >  /*
> > > @@ -1760,11 +1742,22 @@ int IncrementalRemove(char *devname, char
> > > *id_path, int verbose) } else {
> > >  		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 (rv) {  
> > I would prefer to reverse logic to make one indentation less (if that is
> > possible):
> > if (rv != 0)
> >     goto end;
> > but it is fine anyway.  
> 
> Indeed I tends to remove all the warning messages, and do nothing else more
> if rv != 0 from Manage_subdevs(). How do you think of this idea? Checking the
> array state indeed doesn't help too much.

LGTM. Incremental remove is designed to be system utility so the warning are
less important. User should use MISC --faulty and --remove.
> 
> >   
> > > +			/*
> > > +			 * If the array is busy or no verbose info
> > > +			 * desired, nothing else to do.
> > > +			 */
> > > +			if ((rv & 2) || verbose <= 0)
> > > +				goto end;
> > > +
> > > +			/* Otherwise if failed due to a broken array,
> > > warn */
> > > +			if (sysfs_get_str(&mdi, NULL, "array_state",
> > > +					  buf, sizeof(buf)) > 0 &&
> > > +			    strncmp(buf, "broken", 6) == 0) {  
> > 
> > Broken is defined in sysfs_array_states[], can we use it?
> > if (map_name(sysfs_array_states, buf) == ARRAY_BROKEN)
> > I know that it could looks like a little overhead but compiler should do
> > the job here.  
> 
> Yes, I am aware of this. But the existed coding style in this file is the hard
> coded strncmp(). So if we do want to print the warning for a broken array, it
> would be better to add the map_name() fixing in another patch. I mean, do one
> thing in single patch.

Not a case if we are going to drop the message, correct? Feel free to handle
the state comparing across code in the future.
> 
> > > +				pr_err("Fail to remove %s from broken
> > > array.\n",
> > > +				       ent->devnm);  
> > Not exactly, The broken may be raised even if disk is removed. It is a case
> > for raid456 and raid1/10 with fail_last_dev=1. I would say just "%s is in
> > broken state.\n" 
> > Should be exclude arrays which are already broken (broken was set before we
> > called mdadm -If)? I don't see printing this message everytime as a
> > problem, but it is something you should consider.
> >  
> 
> Indeed, I still suggest to remove all the pr_err() stuffs, they cannot help
> too much, and introduce extra complication.

Ack.
> 
>  
> > And I forgot to say it eariler, could you consider adding test/s for both
> > IMSM and native? It is something that should be tested.
> > Sorry, scope is growing :(  
> 
> Sure, it's good idea, let's do it later.

Ok, thanks.

Mariusz