| Message ID | 20230813164613.11912-1-colyli@suse.de (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Delegated to: | Jes Sorensen |
| Headers | show |
| Series | [v5] Incremental: remove obsoleted calls to udisks | expand |
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
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
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
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;