Re: [mdadm PATCH] mdopen: call "modprobe md_mod" if it might be needed.

[NeilBrown <neilb@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 6829 bytes --]

On Thu, Oct 12 2017, Zhilong Liu wrote:

> On 10/12/2017 08:06 AM, NeilBrown wrote:
>> On Wed, Oct 11 2017, Zhilong Liu wrote:
>>
>>> On 10/11/2017 04:16 AM, NeilBrown wrote:
>>>> On Tue, Oct 10 2017, Zhilong Liu wrote:
>>>>
>>>>> On 09/25/2017 01:52 PM, NeilBrown wrote:
>>>>>> Creating an array by opening a block-device with major number of 9
>>>>>> will transparently load the md module if needed.
>>>>>> Creating an array by opening
>>>>>>       /sys/module/md_mod/parameters/new_array
>>>>>> and writing to it won't, it will just fail if md_mod isn't loaded.
>>>>>>
>>>>>> So when opening that file fails with ENOENT, run "modprobe md_mod" and
>>>>>> try again.
>>>>>>
>>>>>> This fixes a bug whereby if you have "CREATE names=yes" in mdadm.conf,
>>>>>> and the md modules isn't loaded, then creating or assembling an
>>>>>> array will not honor the "names=yes" configuration.
>>>>>>
>>>>>> Signed-off-by: NeilBrown <neilb@suse.com>
>>>>>> ---
>>>>>>     mdopen.c | 4 ++++
>>>>>>     1 file changed, 4 insertions(+)
>>>>>>
>>>>>> diff --git a/mdopen.c b/mdopen.c
>>>>>> index 3c0052f2db23..dcdc6f23e6c1 100644
>>>>>> --- a/mdopen.c
>>>>>> +++ b/mdopen.c
>>>>>> @@ -312,6 +312,10 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
>>>>>>     		if (block_udev)
>>>>>>     			udev_block(devnm);
>>>>>>     		fd = open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
>>>>>> +		if (fd < 0 && errno == ENOENT) {
>>>>>> +			system("modprobe md_mod");
>>> [nip]
>>>> Hmmm.. that's annoying.  I wonder why "system" is marked
>>>> "warn_unused_result".
>>> in /usr/include/stdlib.h:
>>> ...
>>> 712 /* Execute the given line as a shell command.
>>> 713
>>> 714    This function is a cancellation point and therefore not marked with
>>> 715    __THROW.  */
>>> 716 extern int system (const char *__command) __wur;
>>> ...
>>>
>>> the "warn_unused_result" is from the __wur parameter, re-compile mdadm
>>> after delete the '__wur',
>>> it works.
>>>
>>>> In this case I really don't care - I'm not convinced an extra error
>>>> message will really help.
>>>> Maybe
>>>>      if (system("modprobe md_mod") == 0)
>>>>           fd = open("/sys/......", O_WRONLY);
>>> Agree.
>>>> We do what a better error message, then it should be based on 'fd < 0'.
>>>> e.g.
>>>>     if (fd < 0 || n != strlen(devnm))
>>>>         pr_err("Fail create array using /sys/module/md_mod/parameters/new_array\n");
>>> you mean something like this?
>>>
>>> diff --git a/mdopen.c b/mdopen.c
>>> index dcdc6f2..9de347e 100644
>>> --- a/mdopen.c
>>> +++ b/mdopen.c
>>> @@ -313,14 +313,17 @@ int create_mddev(char *dev, char *name, int autof,
>>> int trustworthy,
>>>                           udev_block(devnm);
>>>                   fd = open("/sys/module/md_mod/parameters/new_array",
>>> O_WRONLY);
>>>                   if (fd < 0 && errno == ENOENT) {
>>> -                       system("modprobe md_mod");
>>> -                       fd =
>>> open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
>>> +                       if (system("modprobe md_mod") == 0)
>>> +                               fd =
>>> open("/sys/module/md_mod/parameters/new_array",
>>> +                                         O_WRONLY);
>>>                   }
>>>                   if (fd >= 0) {
>>>                           n = write(fd, devnm, strlen(devnm));
>>>                           close(fd);
>>>                   }
>>> -               if (n < 0) {
>>> +               if (fd < 0 || n != strlen(devnm)) {
>>> +                       pr_err("Fail create array using "
>>> + "/sys/module/md_mod/parameters/new_array\n");
>>>                           devnm[0] = 0;
>>>                           udev_unblock();
>>>                   }
>>>
>> Yes - exactly like that except that I wouldn't wrap the long string.
>> Lines longer than 80 chars are good to avoid, but breaking string
>> literals is worse than having long lines.  e.g. it makes searching for
>> the string hard.
>
> Thanks for your detail explanation. I draft it like this, and
> already checked it via to ./linux/scripts/checkpatch.pl.

I wouldn't worry too much about checkpatch.pl.
It is worth looking at what it reports, but if you don't agree or the
maintainer doesn't agree, then feel free to ignore it.


> Is this changing ok to you?
>
> diff --git a/mdopen.c b/mdopen.c
> index dcdc6f2..da8d9d1 100644
> --- a/mdopen.c
> +++ b/mdopen.c
> @@ -26,6 +26,8 @@
>   #include "md_p.h"
>   #include <ctype.h>
>
> +#define NEW_ARRAY_FILE "/sys/module/md_mod/parameters/new_array"
> +

Splitting this out into a separate string does make sense.
However I would use
  static const char new_array_file[] = ....;
rather than #define.

I hadn't noticed that there are two places where we write to new_array.
Maybe that should be split out into a function.
Both should use modprobe if the open fails.

Thanks,
NeilBrown


>   void make_parts(char *dev, int cnt)
>   {
>       /* make 'cnt' partition devices for 'dev'
> @@ -311,16 +313,17 @@ int create_mddev(char *dev, char *name, int autof, 
> int trustworthy,
>           sprintf(devnm, "md_%s", cname);
>           if (block_udev)
>               udev_block(devnm);
> -        fd = open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
> +        fd = open(NEW_ARRAY_FILE, O_WRONLY);
>           if (fd < 0 && errno == ENOENT) {
> -            system("modprobe md_mod");
> -            fd = open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
> +            if (system("modprobe md_mod") == 0)
> +                fd = open(NEW_ARRAY_FILE, O_WRONLY);
>           }
>           if (fd >= 0) {
>               n = write(fd, devnm, strlen(devnm));
>               close(fd);
>           }
> -        if (n < 0) {
> +        if (fd < 0 || n != strlen(devnm)) {
> +            pr_err("Fail create array using %s\n", NEW_ARRAY_FILE);
>               devnm[0] = 0;
>               udev_unblock();
>           }
> @@ -331,12 +334,13 @@ int create_mddev(char *dev, char *name, int autof, 
> int trustworthy,
>           sprintf(devnm, "md%d", num);
>           if (block_udev)
>               udev_block(devnm);
> -        fd = open("/sys/module/md_mod/parameters/new_array", O_WRONLY);
> +        fd = open(NEW_ARRAY_FILE, O_WRONLY);
>           if (fd >= 0) {
>               n = write(fd, devnm, strlen(devnm));
>               close(fd);
>           }
> -        if (n < 0) {
> +        if (fd < 0 || n != strlen(devnm)) {
> +            pr_err("Fail create array using %s\n", NEW_ARRAY_FILE);
>               devnm[0] = 0;
>               udev_unblock();
>           }
> -- 
> 2.6.6
>
>
> Thanks,
> -Zhilong
>
>> Thanks,
>> NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]