[PATCH v5] Incremental: remove obsoleted calls to udisks

[PATCH v5] Incremental: remove obsoleted calls to udisks

[Coly Li]

Utility 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.

In the two modifications where calling force_remove() are removed,
the failure from Manage_subdevs() can be safely ignored, because,
1) udisks doesn't exist, no need to check the return value to umount
   the file system by udisks and remove the component disk again.
2) After the 'I' inremental remove, there is another 'r' hot remove
   following up. The first incremental remove is a best-try effort.

Therefore in this patch, where force_remove() is removed, the return
value of calling Manage_subdevs() is not checked too.

Signed-off-by: Coly Li <colyli@suse.de>
Reviewed-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
Cc: Jes Sorensen <jes@trained-monkey.org>
---
Changelog,
v5: change Mariusz's email address as he suggested
v4: add Reviewed-by from Mariusz.
v3: remove the almost-useless warning message, and make the change
   more simplified.
v2: improve based on code review comments from Mariusz.
v1: initial version.

 Incremental.c | 64 +++++++++++----------------------------------------
 1 file changed, 13 insertions(+), 51 deletions(-)

diff --git a/Incremental.c b/Incremental.c
index f13ce02..05b33c4 100644
--- a/Incremental.c
+++ b/Incremental.c
@@ -1628,54 +1628,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);
-	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);
-		}
+		/*
+		 * Ignore the return value because it's necessary
+		 * to handle failure condition here.
+		 */
+		Manage_subdevs(memb->devnm, subfd, devlist, verbose,
+			       0, UOPT_UNDEFINED, 0);
 		close(subfd);
 	}
 }
@@ -1758,21 +1722,19 @@ int IncrementalRemove(char *devname, char *id_path, int verbose)
 		}
 		free_mdstat(mdstat);
 	} 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.
+		/*
+		 * This 'I' incremental remove is a try-best effort,
+		 * the failure condition can be safely ignored
+		 * because of the following up 'r' remove.
 		 */
-			rv = force_remove(ent->devnm, mdfd, &mdi, verbose);
-			goto end;
-		}
+		Manage_subdevs(ent->devnm, mdfd, &devlist,
+			       verbose, 0, UOPT_UNDEFINED, 0);
 	}
 
 	devlist.disposition = 'r';
 	rv = Manage_subdevs(ent->devnm, mdfd, &devlist,
 			    verbose, 0, UOPT_UNDEFINED, 0);
-end:
+
 	close(mdfd);
 	free_mdstat(ent);
 	return rv;
-- 
2.35.3

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

[Jes Sorensen]

On 8/13/23 12:46, Coly Li wrote:
> Utility 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.
> 
> In the two modifications where calling force_remove() are removed,
> the failure from Manage_subdevs() can be safely ignored, because,
> 1) udisks doesn't exist, no need to check the return value to umount
>    the file system by udisks and remove the component disk again.
> 2) After the 'I' inremental remove, there is another 'r' hot remove
>    following up. The first incremental remove is a best-try effort.
> 
> Therefore in this patch, where force_remove() is removed, the return
> value of calling Manage_subdevs() is not checked too.
> 
> Signed-off-by: Coly Li <colyli@suse.de>
> Reviewed-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
> Cc: Jes Sorensen <jes@trained-monkey.org>
> ---
> Changelog,
> v5: change Mariusz's email address as he suggested
> v4: add Reviewed-by from Mariusz.
> v3: remove the almost-useless warning message, and make the change
>    more simplified.
> v2: improve based on code review comments from Mariusz.
> v1: initial version.
> 
>  Incremental.c | 64 +++++++++++----------------------------------------
>  1 file changed, 13 insertions(+), 51 deletions(-)

Been out of the loop for a while, trying to catch up.

Mariusz, do you consider this one good to go now? You were the one
providing feedback multiple times.

Thanks,
Jes

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

[Mariusz Tkaczyk]

On Fri, 1 Sep 2023 11:47:09 -0400
Jes Sorensen <jes@trained-monkey.org> wrote:

> On 8/13/23 12:46, Coly Li wrote:
> > Utility 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.
> > 
> > In the two modifications where calling force_remove() are removed,
> > the failure from Manage_subdevs() can be safely ignored, because,
> > 1) udisks doesn't exist, no need to check the return value to umount
> >    the file system by udisks and remove the component disk again.
> > 2) After the 'I' inremental remove, there is another 'r' hot remove
> >    following up. The first incremental remove is a best-try effort.
> > 
> > Therefore in this patch, where force_remove() is removed, the return
> > value of calling Manage_subdevs() is not checked too.
> > 
> > Signed-off-by: Coly Li <colyli@suse.de>
> > Reviewed-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
> > Cc: Jes Sorensen <jes@trained-monkey.org>
> > ---
> > Changelog,
> > v5: change Mariusz's email address as he suggested
> > v4: add Reviewed-by from Mariusz.
> > v3: remove the almost-useless warning message, and make the change
> >    more simplified.
> > v2: improve based on code review comments from Mariusz.
> > v1: initial version.
> > 
> >  Incremental.c | 64 +++++++++++----------------------------------------
> >  1 file changed, 13 insertions(+), 51 deletions(-)  
> 
> Been out of the loop for a while, trying to catch up.
> 
> Mariusz, do you consider this one good to go now? You were the one
> providing feedback multiple times.
> 
> Thanks,
> Jes
> 
> 

Hi Jes,

Yes, I see this as a good change. The current behavior is not stable, because
udev is not able to "umount"- if array is not mounted it is stopped, otherwise
not.

With the change, we will not try to stop it at all- fair for me, behavior is
same every time. If we cannot stop array every time we should not try to.

Thanks,
Mariusz

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

[Jes Sorensen]

On 9/4/23 03:48, Mariusz Tkaczyk wrote:
> On Fri, 1 Sep 2023 11:47:09 -0400
> Jes Sorensen <jes@trained-monkey.org> wrote:
> 
>> On 8/13/23 12:46, Coly Li wrote:
>>> Utility 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.
>>>
>>> In the two modifications where calling force_remove() are removed,
>>> the failure from Manage_subdevs() can be safely ignored, because,
>>> 1) udisks doesn't exist, no need to check the return value to umount
>>>    the file system by udisks and remove the component disk again.
>>> 2) After the 'I' inremental remove, there is another 'r' hot remove
>>>    following up. The first incremental remove is a best-try effort.
>>>
>>> Therefore in this patch, where force_remove() is removed, the return
>>> value of calling Manage_subdevs() is not checked too.
>>>
>>> Signed-off-by: Coly Li <colyli@suse.de>
>>> Reviewed-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
>>> Cc: Jes Sorensen <jes@trained-monkey.org>
>>> ---
>>> Changelog,
>>> v5: change Mariusz's email address as he suggested
>>> v4: add Reviewed-by from Mariusz.
>>> v3: remove the almost-useless warning message, and make the change
>>>    more simplified.
>>> v2: improve based on code review comments from Mariusz.
>>> v1: initial version.
>>>
>>>  Incremental.c | 64 +++++++++++----------------------------------------
>>>  1 file changed, 13 insertions(+), 51 deletions(-)  
>>
>> Been out of the loop for a while, trying to catch up.
>>
>> Mariusz, do you consider this one good to go now? You were the one
>> providing feedback multiple times.
>>
>> Thanks,
>> Jes
>>
>>
> 
> Hi Jes,
> 
> Yes, I see this as a good change. The current behavior is not stable, because
> udev is not able to "umount"- if array is not mounted it is stopped, otherwise
> not.
> 
> With the change, we will not try to stop it at all- fair for me, behavior is
> same every time. If we cannot stop array every time we should not try to.

Applied!

Thanks,
Jes