| Message ID | 20230525170843.4616-1-colyli@suse.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2] Incremental: remove obsoleted calls to udisks | expand |
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
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.
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
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; }
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(-)