From mboxrd@z Thu Jan 1 00:00:00 1970 From: Zhilong Liu Subject: Re: [mdadm PATCH] mdopen: call "modprobe md_mod" if it might be needed. Date: Thu, 12 Oct 2017 11:55:35 +0800 Message-ID: <7631f4ae-bcc5-b36d-bb63-678515109f05@suse.com> References: <87y3p372b0.fsf@notabene.neil.brown.name> <87bmleyd0y.fsf@notabene.neil.brown.name> <375cf2fb-bd4b-abef-5012-1361c224d254@suse.com> <87h8v5nsa6.fsf@notabene.neil.brown.name> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <87h8v5nsa6.fsf@notabene.neil.brown.name> Sender: linux-raid-owner@vger.kernel.org To: NeilBrown , "jes.sorensen@gmail.com" Cc: Linux Raid List-Id: linux-raid.ids 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 >>>>> --- >>>>> 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. 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 +#define NEW_ARRAY_FILE "/sys/module/md_mod/parameters/new_array" + 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