* [PATCH] Incremental: remove obsoleted calls to udisks
@ 2023-05-05 5:22 Coly Li
2023-05-05 8:49 ` Mariusz Tkaczyk
0 siblings, 1 reply; 3+ messages in thread
From: Coly Li @ 2023-05-05 5:22 UTC (permalink / raw)
To: jes; +Cc: linux-raid, Coly Li, Mariusz Tkaczyk
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);
- 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;
}
}
--
2.35.3
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] Incremental: remove obsoleted calls to udisks
2023-05-05 5:22 [PATCH] Incremental: remove obsoleted calls to udisks Coly Li
@ 2023-05-05 8:49 ` Mariusz Tkaczyk
2023-05-05 16:54 ` Coly Li
0 siblings, 1 reply; 3+ messages in thread
From: Mariusz Tkaczyk @ 2023-05-05 8:49 UTC (permalink / raw)
To: Coly Li; +Cc: jes, linux-raid, Mariusz Tkaczyk
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] Incremental: remove obsoleted calls to udisks
2023-05-05 8:49 ` Mariusz Tkaczyk
@ 2023-05-05 16:54 ` Coly Li
0 siblings, 0 replies; 3+ messages in thread
From: Coly Li @ 2023-05-05 16:54 UTC (permalink / raw)
To: Mariusz Tkaczyk; +Cc: jes, linux-raid, Mariusz Tkaczyk
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.
--
Coly Li
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-05-05 16:54 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-05 5:22 [PATCH] Incremental: remove obsoleted calls to udisks Coly Li
2023-05-05 8:49 ` Mariusz Tkaczyk
2023-05-05 16:54 ` Coly Li
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.