On Fri, 26 May 2023 22:42:52 +0800
Coly Li <colyli@xxxxxxx> 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@xxxxxxx> 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@xxxxxxx>
> > > Cc: Mariusz Tkaczyk <mariusz.tkaczyk@xxxxxxxxx>
> > > Cc: Jes Sorensen <jes@xxxxxxxxxxxxxxxxxx>
> > > ---
> > > 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
