[PATCH 0/2] udev rules behaviour

[PATCH 0/2] udev rules behaviour

[Michal Soltys]

Current udev rules installed cause autoassembly of everything that is possible
during coldplug - due to mdadm -I calls for each matching device.

This causes two immediate problems:

- mdadm.conf kind-of rendered pointless, as the assembly of any array will be
  attempted either way

- 65-md-inc*.rules (e.g. present in dracut and different distributions)
  offering more fine grained controls (e.g. incremental assembly limited to
  certain uuids) are also shadowed by mdadm's default rules

If this is not expected behaviour, following patch removes -I calls.
Second patch adds ddf (and any future) containers to -If calls.


Alternatively, we could detect presence of 65-md-inc* and mdadm.conf and
attempt assemlby only if none of those are present ? Not perfect, but
a bit more flexible.


Michal Soltys (2):
  udev rules: don't incrementally autoassemble everything
  shorten remove rules

 udev-md-raid.rules |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

-- 
1.7.5.3

[PATCH 1/2] udev rules: don't incrementally autoassemble everything

[Michal Soltys]

All native and imsm arrays were autoassembled unconditionally (excluding
forgotten ddf containers).

This caused few issues:

- mdadm.conf configuration irrelevant - container or not - everything
  was assembled as soon as any component device was found during the boot

- problems with any 65-md-inc* rules often present, that also allow
  more fine-grained control - e.g. dracut's ones allow incremental assembly
  limited to certain uuids

Signed-off-by: Michal Soltys <soltys@ziu.info>
---
 udev-md-raid.rules |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/udev-md-raid.rules b/udev-md-raid.rules
index c2105bc..02172de 100644
--- a/udev-md-raid.rules
+++ b/udev-md-raid.rules
@@ -5,10 +5,8 @@ SUBSYSTEM!="block", GOTO="md_end"
 # handle potential components of arrays
 ENV{ID_FS_TYPE}=="linux_raid_member", ENV{ID_PATH}!="", ACTION=="remove", RUN+="/sbin/mdadm -If $name --path $env{ID_PATH}"
 ENV{ID_FS_TYPE}=="linux_raid_member", ENV{ID_PATH}=="", ACTION=="remove", RUN+="/sbin/mdadm -If $name"
-ENV{ID_FS_TYPE}=="linux_raid_member", ACTION=="add", RUN+="/sbin/mdadm --incremental $env{DEVNAME}"
 ENV{ID_FS_TYPE}=="isw_raid_member", ENV{ID_PATH}!="", ACTION=="remove", RUN+="/sbin/mdadm -If $name --path $env{ID_PATH}"
 ENV{ID_FS_TYPE}=="isw_raid_member", ENV{ID_PATH}=="", ACTION=="remove", RUN+="/sbin/mdadm -If $name"
-ENV{ID_FS_TYPE}=="isw_raid_member", ACTION=="add", RUN+="/sbin/mdadm --incremental $env{DEVNAME}"
 
 # handle md arrays
 ACTION!="add|change", GOTO="md_end"
-- 
1.7.5.3

[PATCH 2/2] shorten remove rules

[Michal Soltys]

This implicitly adds ddf containers to remove rules.

Signed-off-by: Michal Soltys <soltys@ziu.info>
---
 udev-md-raid.rules |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/udev-md-raid.rules b/udev-md-raid.rules
index 02172de..56201f2 100644
--- a/udev-md-raid.rules
+++ b/udev-md-raid.rules
@@ -3,10 +3,8 @@
 SUBSYSTEM!="block", GOTO="md_end"
 
 # handle potential components of arrays
-ENV{ID_FS_TYPE}=="linux_raid_member", ENV{ID_PATH}!="", ACTION=="remove", RUN+="/sbin/mdadm -If $name --path $env{ID_PATH}"
-ENV{ID_FS_TYPE}=="linux_raid_member", ENV{ID_PATH}=="", ACTION=="remove", RUN+="/sbin/mdadm -If $name"
-ENV{ID_FS_TYPE}=="isw_raid_member", ENV{ID_PATH}!="", ACTION=="remove", RUN+="/sbin/mdadm -If $name --path $env{ID_PATH}"
-ENV{ID_FS_TYPE}=="isw_raid_member", ENV{ID_PATH}=="", ACTION=="remove", RUN+="/sbin/mdadm -If $name"
+ENV{ID_FS_TYPE}=="*_raid_member", ENV{ID_PATH}!="", ACTION=="remove", RUN+="/sbin/mdadm -If $name --path $env{ID_PATH}"
+ENV{ID_FS_TYPE}=="*_raid_member", ENV{ID_PATH}=="", ACTION=="remove", RUN+="/sbin/mdadm -If $name"
 
 # handle md arrays
 ACTION!="add|change", GOTO="md_end"
-- 
1.7.5.3

v2 - keep incremental, check types

[Michal Soltys]

Similar to the earlier patch, without removing incremental assembly - add ddf,
use $tempnode, comments, check for exact types.


Michal Soltys (1):
  udev rules: add ddf, shorten checks, use $tempnode

 udev-md-raid.rules |   20 +++++++++++++-------
 1 files changed, 13 insertions(+), 7 deletions(-)

-- 
1.7.5.3

[PATCH] udev rules: add ddf, shorten checks, use $tempnode

[Michal Soltys]

This patch adjusts few minor details:

- add ddf to incremental assembly/removal
- shorten the rules, with initial test for supported types
- when accessing the device, use $tempnode
- few more comments

Signed-off-by: Michal Soltys <soltys@ziu.info>
---
 udev-md-raid.rules |   20 +++++++++++++-------
 1 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/udev-md-raid.rules b/udev-md-raid.rules
index c2105bc..f564f70 100644
--- a/udev-md-raid.rules
+++ b/udev-md-raid.rules
@@ -2,13 +2,19 @@
 
 SUBSYSTEM!="block", GOTO="md_end"
 
-# handle potential components of arrays
-ENV{ID_FS_TYPE}=="linux_raid_member", ENV{ID_PATH}!="", ACTION=="remove", RUN+="/sbin/mdadm -If $name --path $env{ID_PATH}"
-ENV{ID_FS_TYPE}=="linux_raid_member", ENV{ID_PATH}=="", ACTION=="remove", RUN+="/sbin/mdadm -If $name"
-ENV{ID_FS_TYPE}=="linux_raid_member", ACTION=="add", RUN+="/sbin/mdadm --incremental $env{DEVNAME}"
-ENV{ID_FS_TYPE}=="isw_raid_member", ENV{ID_PATH}!="", ACTION=="remove", RUN+="/sbin/mdadm -If $name --path $env{ID_PATH}"
-ENV{ID_FS_TYPE}=="isw_raid_member", ENV{ID_PATH}=="", ACTION=="remove", RUN+="/sbin/mdadm -If $name"
-ENV{ID_FS_TYPE}=="isw_raid_member", ACTION=="add", RUN+="/sbin/mdadm --incremental $env{DEVNAME}"
+# handle potential components of arrays (the ones supported by md)
+ENV{ID_FS_TYPE}=="ddf_raid_member|isw_raid_member|linux_raid_member", GOTO="md_inc"
+GOTO="md_inc_skip"
+
+LABEL="md_inc"
+
+# remember you can limit what gets auto/incrementally assembled by
+# mdadm.conf(5)'s 'AUTO' and selectively whitelist using 'ARRAY'
+ACTION=="add", RUN+="/sbin/mdadm --incremental $tempnode"
+ACTION=="remove", ENV{ID_PATH}=="?*", RUN+="/sbin/mdadm -If $name --path $env{ID_PATH}"
+ACTION=="remove", ENV{ID_PATH}!="?*", RUN+="/sbin/mdadm -If $name"
+
+LABEL="md_inc_skip"
 
 # handle md arrays
 ACTION!="add|change", GOTO="md_end"
-- 
1.7.5.3

Re: [PATCH 0/2] udev rules behaviour

[NeilBrown]

On Sun,  4 Sep 2011 14:42:42 +0200 Michal Soltys <soltys@ziu.info> wrote:

> Current udev rules installed cause autoassembly of everything that is possible
> during coldplug - due to mdadm -I calls for each matching device.
> 
> This causes two immediate problems:
> 
> - mdadm.conf kind-of rendered pointless, as the assembly of any array will be
>   attempted either way
> 
> - 65-md-inc*.rules (e.g. present in dracut and different distributions)
>   offering more fine grained controls (e.g. incremental assembly limited to
>   certain uuids) are also shadowed by mdadm's default rules
> 
> If this is not expected behaviour, following patch removes -I calls.
> Second patch adds ddf (and any future) containers to -If calls.
> 
> 
> Alternatively, we could detect presence of 65-md-inc* and mdadm.conf and
> attempt assemlby only if none of those are present ? Not perfect, but
> a bit more flexible.
> 

hi Michal,
 thanks for raising this.

I find it unfortunate that someone would ship an md-specific rules file
without even discussing it with me or on this list.  That can easily lead to
inconsistent or suboptimal behaviour.

It isn't entirely true that calling "mdadm -I" on all devices makes
mdadm.conf pointless.
If you put "auto -all" in mdadm.conf it will disable all auto-assembly and
will only assemble arrays that are explicitly listed in mdadm.conf.

So I'm not really sure what that problem is here.

Can you give me an example of a situation that cannot be handled with the
mdadm udev rules as they are?

I like your "shorten remove rules" patch and will apply that.

Thanks,
NeilBrown

v2 once more, for current head

[Michal Soltys]

As the earlier v2 was after 2/2 was commited, this one adds missing
stuff on top.

Michal Soltys (1):
  udev rules: use $tempnode, check for supported types, comments

 udev-md-raid.rules |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

-- 
1.7.5.3

[PATCH] udev rules: use $tempnode, check for supported types, comments

[Michal Soltys]

Few things adjusted in addition to
0f82fe603a42f37f1e2a6f826b4164811bf2d188:

- keep strict tests for supported types
- when accessing the device, use $tempnode
- few more comments

Signed-off-by: Michal Soltys <soltys@ziu.info>
---
 udev-md-raid.rules |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/udev-md-raid.rules b/udev-md-raid.rules
index e251ac5..f564f70 100644
--- a/udev-md-raid.rules
+++ b/udev-md-raid.rules
@@ -2,10 +2,19 @@
 
 SUBSYSTEM!="block", GOTO="md_end"
 
-# handle potential components of arrays
-ENV{ID_FS_TYPE}=="*_raid_member", ENV{ID_PATH}!="", ACTION=="remove", RUN+="/sbin/mdadm -If $name --path $env{ID_PATH}"
-ENV{ID_FS_TYPE}=="*_raid_member", ENV{ID_PATH}=="", ACTION=="remove", RUN+="/sbin/mdadm -If $name"
-ENV{ID_FS_TYPE}=="*_raid_member", ACTION=="add", RUN+="/sbin/mdadm --incremental $env{DEVNAME}"
+# handle potential components of arrays (the ones supported by md)
+ENV{ID_FS_TYPE}=="ddf_raid_member|isw_raid_member|linux_raid_member", GOTO="md_inc"
+GOTO="md_inc_skip"
+
+LABEL="md_inc"
+
+# remember you can limit what gets auto/incrementally assembled by
+# mdadm.conf(5)'s 'AUTO' and selectively whitelist using 'ARRAY'
+ACTION=="add", RUN+="/sbin/mdadm --incremental $tempnode"
+ACTION=="remove", ENV{ID_PATH}=="?*", RUN+="/sbin/mdadm -If $name --path $env{ID_PATH}"
+ACTION=="remove", ENV{ID_PATH}!="?*", RUN+="/sbin/mdadm -If $name"
+
+LABEL="md_inc_skip"
 
 # handle md arrays
 ACTION!="add|change", GOTO="md_end"
-- 
1.7.5.3

Re: [PATCH] udev rules: use $tempnode, check for supported types, comments

[NeilBrown]

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

On Thu,  8 Sep 2011 09:25:08 +0200 Michal Soltys <soltys@ziu.info> wrote:

> Few things adjusted in addition to
> 0f82fe603a42f37f1e2a6f826b4164811bf2d188:
> 
> - keep strict tests for supported types
> - when accessing the device, use $tempnode
> - few more comments
> 
> Signed-off-by: Michal Soltys <soltys@ziu.info>
> ---
>  udev-md-raid.rules |   17 +++++++++++++----
>  1 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/udev-md-raid.rules b/udev-md-raid.rules
> index e251ac5..f564f70 100644
> --- a/udev-md-raid.rules
> +++ b/udev-md-raid.rules
> @@ -2,10 +2,19 @@
>  
>  SUBSYSTEM!="block", GOTO="md_end"
>  
> -# handle potential components of arrays
> -ENV{ID_FS_TYPE}=="*_raid_member", ENV{ID_PATH}!="", ACTION=="remove", RUN+="/sbin/mdadm -If $name --path $env{ID_PATH}"
> -ENV{ID_FS_TYPE}=="*_raid_member", ENV{ID_PATH}=="", ACTION=="remove", RUN+="/sbin/mdadm -If $name"
> -ENV{ID_FS_TYPE}=="*_raid_member", ACTION=="add", RUN+="/sbin/mdadm --incremental $env{DEVNAME}"
> +# handle potential components of arrays (the ones supported by md)
> +ENV{ID_FS_TYPE}=="ddf_raid_member|isw_raid_member|linux_raid_member", GOTO="md_inc"
> +GOTO="md_inc_skip"
> +
> +LABEL="md_inc"
> +
> +# remember you can limit what gets auto/incrementally assembled by
> +# mdadm.conf(5)'s 'AUTO' and selectively whitelist using 'ARRAY'
> +ACTION=="add", RUN+="/sbin/mdadm --incremental $tempnode"
> +ACTION=="remove", ENV{ID_PATH}=="?*", RUN+="/sbin/mdadm -If $name --path $env{ID_PATH}"
> +ACTION=="remove", ENV{ID_PATH}!="?*", RUN+="/sbin/mdadm -If $name"
> +
> +LABEL="md_inc_skip"
>  
>  # handle md arrays
>  ACTION!="add|change", GOTO="md_end"

Thanks.
I've applied this... though I must admit that I don't really like all the
GOTOs - but that is reall

y the fault of udev I expect.

What was the rational for changing 
   ==""
to
   !="?*"

??

NeilBrown

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

Re: [PATCH] udev rules: use $tempnode, check for supported types, comments

[Michal Soltys]

W dniu 19.09.2011 05:12, NeilBrown pisze:
> 
> Thanks.
> I've applied this... though I must admit that I don't really like all the
> GOTOs - but that is really the fault of udev I expect.
> 

Yes, well - the alternative is a single bit longer line such as:

ENV{ID_FS_TYPE}!="ddf_raid_member", ENV{ID_FS_TYPE}!="isw_raid_member", ENV{ID_FS_TYPE}!="linux_raid_member", GOTO="md_inc_skip"

I thought the version with one more goto was more readable in the end,
though I also have my doubts about the choice.


> What was the rational for changing
>     ==""
> to
>     !="?*"
> 
> ??
> 

Actually none, just happened during the rewrite (both alternatives 
are functionally identical) - I guess I'm more used to "?*" checks 
for "is / isn't defined", so I didn't pay attention for it to be 
exact like before.