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