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@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.
>>
>> 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@xxxxxxx>
>> Cc: Mariusz Tkaczyk <mariusz.tkaczyk@xxxxxxxxx>
>> Cc: Jes Sorensen <jes@xxxxxxxxxxxxxxxxxx>
>> ---
>> 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
