Skip to contentSkip to conversation

Parallel make bug in Makefile of mdadm-3.3.1 (corrupted udev rules and/or systemd services)

PATCH In review Samuli Suominen 2 participants 5 messages Jul 17, 2014 – Jul 18, 2014

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Parallel make bug in Makefile of mdadm-3.3.1 (corrupted udev rules and/or systemd services)

If you run with:

$ export MAKEFLAGS="-j9"
$ make install install-systemd

As in, combine "install" and "install-systemd" in the same command using parallel make,
the content of .service files could end up in .rules, or otherway around because GNU
make fill run them in parallel and the commands might ran at the same time, and both
use same variables $file and same temporary files .install.tmp

This was reported at,
https://bugs.gentoo.org/show_bug.cgi?id=517218

The immediate workaround is,

$ export MAKEFLAGS="-j9"
$ make install
$ make install-systemd

And the attach patch makes it full proof so they can be ran at the same line.

Thanks,
Samuli

Makefile +15 -15
diff -ur mdadm-3.3.1.orig/Makefile mdadm-3.3.1/Makefile
@@ -281,26 +281,26 @@
 	$(INSTALL) -D -m 644 mdadm.conf.5 $(DESTDIR)$(MAN5DIR)/mdadm.conf.5
 
 install-udev: udev-md-raid-arrays.rules udev-md-raid-assembly.rules
-	@for file in 63-md-raid-arrays.rules 64-md-raid-assembly.rules ; \
-	do sed -e 's,BINDIR,$(BINDIR),g' udev-$${file#??-} > .install.tmp && \
-	   echo $(INSTALL) -D -m 644 udev-$${file#??-} $(DESTDIR)$(UDEVDIR)/rules.d/$$file ; \
-	   $(INSTALL) -D -m 644 .install.tmp $(DESTDIR)$(UDEVDIR)/rules.d/$$file ; \
-	   rm -f .install.tmp; \
+	@for file1 in 63-md-raid-arrays.rules 64-md-raid-assembly.rules ; \
+	do sed -e 's,BINDIR,$(BINDIR),g' udev-$${file1#??-} > .install.tmp.1 && \
+	   echo $(INSTALL) -D -m 644 udev-$${file1#??-} $(DESTDIR)$(UDEVDIR)/rules.d/$$file1 ; \
+	   $(INSTALL) -D -m 644 .install.tmp.1 $(DESTDIR)$(UDEVDIR)/rules.d/$$file1 ; \
+	   rm -f .install.tmp.1; \
 	done
 
 install-systemd: systemd/[email protected]
-	@for file in [email protected] mdmonitor.service [email protected] \
+	@for file2 in [email protected] mdmonitor.service [email protected] \
 		[email protected] ; \
-	do sed -e 's,BINDIR,$(BINDIR),g' systemd/$$file > .install.tmp && \
-	   echo $(INSTALL) -D -m 644 systemd/$$file $(DESTDIR)$(SYSTEMD_DIR)/$$file ; \
-	   $(INSTALL) -D -m 644 .install.tmp $(DESTDIR)$(SYSTEMD_DIR)/$$file ; \
-	   rm -f .install.tmp; \
+	do sed -e 's,BINDIR,$(BINDIR),g' systemd/$$file2 > .install.tmp.2 && \
+	   echo $(INSTALL) -D -m 644 systemd/$$file2 $(DESTDIR)$(SYSTEMD_DIR)/$$file2 ; \
+	   $(INSTALL) -D -m 644 .install.tmp.2 $(DESTDIR)$(SYSTEMD_DIR)/$$file2 ; \
+	   rm -f .install.tmp.2; \
 	done
-	@for file in mdadm.shutdown ; \
-	do sed -e 's,BINDIR,$(BINDIR),g' systemd/$$file > .install.tmp && \
-	   echo $(INSTALL) -D -m 755  systemd/$$file $(DESTDIR)$(SYSTEMD_DIR)-shutdown/$$file ; \
-	   $(INSTALL) -D -m 755  .install.tmp $(DESTDIR)$(SYSTEMD_DIR)-shutdown/$$file ; \
-	   rm -f .install.tmp; \
+	@for file3 in mdadm.shutdown ; \
+	do sed -e 's,BINDIR,$(BINDIR),g' systemd/$$file3 > .install.tmp.3 && \
+	   echo $(INSTALL) -D -m 755  systemd/$$file3 $(DESTDIR)$(SYSTEMD_DIR)-shutdown/$$file3 ; \
+	   $(INSTALL) -D -m 755  .install.tmp.3 $(DESTDIR)$(SYSTEMD_DIR)-shutdown/$$file3 ; \
+	   rm -f .install.tmp.3; \
 	done
 	if [ -f /etc/SuSE-release -o -n "$(SUSE)" ] ;then $(INSTALL) -D -m 755 systemd/SUSE-mdadm_env.sh $(DESTDIR)$(SYSTEMD_DIR)/../scripts/mdadm_env.sh ;fi
 
#1 lore raw
Quoted 10 lines · Samuli Suominen

If you run with:

$ export MAKEFLAGS="-j9"
$ make install install-systemd

As in, combine "install" and "install-systemd" in the same command using parallel make,
the content of .service files could end up in .rules, or otherway around because GNU
make fill

s/fill/will/

Quoted 2 lines

And the attach

s/attach/attached/

Sorry for the embarrasing typing errors in the first post.

#2 lore raw
In reply to Samuli Suominen
Quoted 22 lines · On Thu, 17 Jul 2014 10:30:05 +0300 Samuli Suominen <[email protected]> wrote:

If you run with:

$ export MAKEFLAGS="-j9"
$ make install install-systemd

As in, combine "install" and "install-systemd" in the same command using parallel make,
the content of .service files could end up in .rules, or otherway around because GNU
make fill run them in parallel and the commands might ran at the same time, and both
use same variables $file and same temporary files .install.tmp

This was reported at,
https://bugs.gentoo.org/show_bug.cgi?id=517218

The immediate workaround is,

$ export MAKEFLAGS="-j9"
$ make install
$ make install-systemd
Quoted 5 lines

And the attach patch makes it full proof so they can be ran at the same line.

Thanks,
Samuli

Hi,
thanks for the bug report and the patch!

I don't think it is necessary to change the name for the variable (file -> file1, file2 or file3). Just leave it as 'file'.
However changing the name is important. Could you send a patch which just does that, and I'll apply it.

Also if the patch could be included inline in the mail, rather than as an attachment, that would be nice.

(But if you don't want to bother, I'll make the fix anyway and credit you).

Thanks,
NeilBrown

#3 lore raw
Quoted 38 lines · NeilBrown
Quoted 22 lines · On Thu, 17 Jul 2014 10:30:05 +0300 Samuli Suominen <[email protected]> wrote:

If you run with:

$ export MAKEFLAGS="-j9"
$ make install install-systemd

As in, combine "install" and "install-systemd" in the same command using parallel make,
the content of .service files could end up in .rules, or otherway around because GNU
make fill run them in parallel and the commands might ran at the same time, and both
use same variables $file and same temporary files .install.tmp

This was reported at,
https://bugs.gentoo.org/show_bug.cgi?id=517218

The immediate workaround is,

$ export MAKEFLAGS="-j9"
$ make install
$ make install-systemd
Quoted 5 lines

And the attach patch makes it full proof so they can be ran at the same line.

Thanks,
Samuli

Hi,
thanks for the bug report and the patch!

I don't think it is necessary to change the name for the variable (file -> file1, file2 or file3). Just leave it as 'file'.
However changing the name is important. Could you send a patch which just does that, and I'll apply it.

Quoted 7 lines

Also if the patch could be included inline in the mail, rather than as an attachment, that would be nice.

(But if you don't want to bother, I'll make the fix anyway and credit you).

Thanks,
NeilBrown

Thanks for the review, I wasn't 100% sure which I should be changing, the variable, or the filename,
so I changed both.
So, you are most likely correct.

Here is the patch that only changes the filename,

Makefile +1 -1
@@ -282,25 +282,25 @@
 
 install-udev: udev-md-raid-arrays.rules udev-md-raid-assembly.rules
     @for file in 63-md-raid-arrays.rules 64-md-raid-assembly.rules ; \
-    do sed -e 's,BINDIR,$(BINDIR),g' udev-$${file#??-} > .install.tmp && \
+    do sed -e 's,BINDIR,$(BINDIR),g' udev-$${file#??-} > .install.tmp.1
&& \
        echo $(INSTALL) -D -m 644 udev-$${file#??-}

$(DESTDIR)$(UDEVDIR)/rules.d/$$file ; \
- $(INSTALL) -D -m 644 .install.tmp
$(DESTDIR)$(UDEVDIR)/rules.d/$$file ; \
- rm -f .install.tmp; \
+ $(INSTALL) -D -m 644 .install.tmp.1
$(DESTDIR)$(UDEVDIR)/rules.d/$$file ; \
+ rm -f .install.tmp.1; \
done

install-systemd: systemd/[email protected]
@for file in [email protected] mdmonitor.service

[email protected] \
         [email protected] ; \
-    do sed -e 's,BINDIR,$(BINDIR),g' systemd/$$file > .install.tmp && \
+    do sed -e 's,BINDIR,$(BINDIR),g' systemd/$$file > .install.tmp.2 && \
        echo $(INSTALL) -D -m 644 systemd/$$file

$(DESTDIR)$(SYSTEMD_DIR)/$$file ; \
- $(INSTALL) -D -m 644 .install.tmp
$(DESTDIR)$(SYSTEMD_DIR)/$$file ; \
- rm -f .install.tmp; \
+ $(INSTALL) -D -m 644 .install.tmp.2
$(DESTDIR)$(SYSTEMD_DIR)/$$file ; \

+       rm -f .install.tmp.2; \
     done
     @for file in mdadm.shutdown ; \
-    do sed -e 's,BINDIR,$(BINDIR),g' systemd/$$file > .install.tmp && \
+    do sed -e 's,BINDIR,$(BINDIR),g' systemd/$$file > .install.tmp.3 && \
        echo $(INSTALL) -D -m 755  systemd/$$file

$(DESTDIR)$(SYSTEMD_DIR)-shutdown/$$file ; \
- $(INSTALL) -D -m 755 .install.tmp
$(DESTDIR)$(SYSTEMD_DIR)-shutdown/$$file ; \
- rm -f .install.tmp; \
+ $(INSTALL) -D -m 755 .install.tmp.3
$(DESTDIR)$(SYSTEMD_DIR)-shutdown/$$file ; \

+       rm -f .install.tmp.3; \
     done
     if [ -f /etc/SuSE-release -o -n "$(SUSE)" ] ;then $(INSTALL) -D -m

755 systemd/SUSE-mdadm_env.sh $(DESTDIR)$(SYSTEMD_DIR)/../scripts/mdadm_env.sh ;fi

#4 lore raw

This new patch, that only changes the filename, was tested and verified to fix the problem in the downstream Gentoo bug,
https://bugs.gentoo.org/show_bug.cgi?id=517218#c18
So you were right :)

Quoted 107 lines · Samuli Suominen
Quoted 38 lines · NeilBrown
Quoted 22 lines · On Thu, 17 Jul 2014 10:30:05 +0300 Samuli Suominen <[email protected]> wrote:

If you run with:

$ export MAKEFLAGS="-j9"
$ make install install-systemd

As in, combine "install" and "install-systemd" in the same command using parallel make,
the content of .service files could end up in .rules, or otherway around because GNU
make fill run them in parallel and the commands might ran at the same time, and both
use same variables $file and same temporary files .install.tmp

This was reported at,
https://bugs.gentoo.org/show_bug.cgi?id=517218

The immediate workaround is,

$ export MAKEFLAGS="-j9"
$ make install
$ make install-systemd
Quoted 5 lines

And the attach patch makes it full proof so they can be ran at the same line.

Thanks,
Samuli

Hi,
thanks for the bug report and the patch!

I don't think it is necessary to change the name for the variable (file -> file1, file2 or file3). Just leave it as 'file'.
However changing the name is important. Could you send a patch which just does that, and I'll apply it.

Quoted 7 lines

Also if the patch could be included inline in the mail, rather than as an attachment, that would be nice.

(But if you don't want to bother, I'll make the fix anyway and credit you).

Thanks,
NeilBrown

Thanks for the review, I wasn't 100% sure which I should be changing, the variable, or the filename,
so I changed both.
So, you are most likely correct.

Here is the patch that only changes the filename,

Makefile +6 -6
@@ -282,25 +282,25 @@
 
 install-udev: udev-md-raid-arrays.rules udev-md-raid-assembly.rules
     @for file in 63-md-raid-arrays.rules 64-md-raid-assembly.rules ; \
-    do sed -e 's,BINDIR,$(BINDIR),g' udev-$${file#??-} > .install.tmp && \
+    do sed -e 's,BINDIR,$(BINDIR),g' udev-$${file#??-} > .install.tmp.1
 && \
        echo $(INSTALL) -D -m 644 udev-$${file#??-}
 $(DESTDIR)$(UDEVDIR)/rules.d/$$file ; \
-       $(INSTALL) -D -m 644 .install.tmp
 $(DESTDIR)$(UDEVDIR)/rules.d/$$file ; \
-       rm -f .install.tmp; \
+       $(INSTALL) -D -m 644 .install.tmp.1
 $(DESTDIR)$(UDEVDIR)/rules.d/$$file ; \
+       rm -f .install.tmp.1; \
     done
 
 install-systemd: systemd/[email protected]
     @for file in [email protected] mdmonitor.service
 [email protected] \
         [email protected] ; \
-    do sed -e 's,BINDIR,$(BINDIR),g' systemd/$$file > .install.tmp && \
+    do sed -e 's,BINDIR,$(BINDIR),g' systemd/$$file > .install.tmp.2 && \
        echo $(INSTALL) -D -m 644 systemd/$$file
 $(DESTDIR)$(SYSTEMD_DIR)/$$file ; \
-       $(INSTALL) -D -m 644 .install.tmp
 $(DESTDIR)$(SYSTEMD_DIR)/$$file ; \
-       rm -f .install.tmp; \
+       $(INSTALL) -D -m 644 .install.tmp.2
 $(DESTDIR)$(SYSTEMD_DIR)/$$file ; \
+       rm -f .install.tmp.2; \
     done
     @for file in mdadm.shutdown ; \
Makefile +1 -1
-    do sed -e 's,BINDIR,$(BINDIR),g' systemd/$$file > .install.tmp && \
+    do sed -e 's,BINDIR,$(BINDIR),g' systemd/$$file > .install.tmp.3 && \
        echo $(INSTALL) -D -m 755  systemd/$$file

$(DESTDIR)$(SYSTEMD_DIR)-shutdown/$$file ; \
- $(INSTALL) -D -m 755 .install.tmp
$(DESTDIR)$(SYSTEMD_DIR)-shutdown/$$file ; \

Makefile +1 -1
-       rm -f .install.tmp; \
+       $(INSTALL) -D -m 755  .install.tmp.3

$(DESTDIR)$(SYSTEMD_DIR)-shutdown/$$file ; \

+       rm -f .install.tmp.3; \
     done
     if [ -f /etc/SuSE-release -o -n "$(SUSE)" ] ;then $(INSTALL) -D -m

755 systemd/SUSE-mdadm_env.sh $(DESTDIR)$(SYSTEMD_DIR)/../scripts/mdadm_env.sh ;fi

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html