| Message ID | 20230505052231.7787-1-colyli@suse.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Incremental: remove obsoleted calls to udisks | expand |
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
On Fri, May 05, 2023 at 10:49:10AM +0200, Mariusz Tkaczyk wrote: > 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. Copied. I will describe in commit log when I remove the call to Manage_stop(). > >> - 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. I see difference ways to check verbose are mixed. But yes, comparing directly with values is better. I will follow your suggestion. >> + 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. I agree. Let me improve this. > >> 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? > OK, I will change it. >> goto end; >> } >> } Thanks for your review.
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); - 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) + pr_err("Fail to remove %s from array.\n", memb->devnm); + 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; goto end; } }
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(-)