Re: [PATCHv2 2/2] udev: Fix order of execution of the md rules

[NeilBrown <neilb@suse.de>]

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

On Sat,  9 Feb 2013 21:49:47 +0100 Thomas Bächler <thomas@archlinux.org>
wrote:

> Right now, the rules that run blkid on raid arrays are executed after
> the assembly rules. This means incremental assembly will always fail
> when raid arrays are again physical components of raid arrays.

I'm not at all against splitting the udev rules into two files, but your
reasoning seems a bit odd and I want to be sure that I understand.

We run "mdadm -I" based on the contents of ID_FS_TYPE, and ID_FS_TYPE is set
by "blkid", which is called after the "mdadm -I" rules.  So why do they work
at all?

Presumably something else is calling "blkid" ?
That seems to be "persistent-storage.rules"  which calls "blkid" on some
devices, but not on others.  That seems sub-optimal.   It is probably
reasonable that it skips "sr*" (which is explicit) but for us it is
unfortunate that it skips "md*".

I cannot see a good reason for persistent-storage-rules to skip 'md' devices,
so I would suggest that the best way to fix the problem is the fix that
rules file.  Then we could remove the "blkid" call from the md rules which is
nice as it removes duplication.

We could still split the md rules files, but I would rather do that because
it was a good and clean thing to do, not do that because it helps work around
a short-coming in some other rules files.

Thoughts?

Thanks,
NeilBrown



> 
> Instead of simply reversing the order, split the rules up into two files,
> one dealing with array properties and one dealing with assembly.
> ---
>  Makefile                    |  5 +++--
>  udev-md-raid-arrays.rules   | 35 ++++++++++++++++++++++++++++++++
>  udev-md-raid-assembly.rules | 19 ++++++++++++++++++
>  udev-md-raid.rules          | 49 ---------------------------------------------
>  4 files changed, 57 insertions(+), 51 deletions(-)
>  create mode 100644 udev-md-raid-arrays.rules
>  create mode 100644 udev-md-raid-assembly.rules
>  delete mode 100644 udev-md-raid.rules
> 
> diff --git a/Makefile b/Makefile
> index b6edb23..ad0819d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -262,8 +262,9 @@ install-man: mdadm.8 md.4 mdadm.conf.5 mdmon.8
>  	$(INSTALL) -D -m 644 md.4 $(DESTDIR)$(MAN4DIR)/md.4
>  	$(INSTALL) -D -m 644 mdadm.conf.5 $(DESTDIR)$(MAN5DIR)/mdadm.conf.5
>  
> -install-udev: udev-md-raid.rules
> -	$(INSTALL) -D -m 644 udev-md-raid.rules $(DESTDIR)$(UDEVDIR)/rules.d/64-md-raid.rules
> +install-udev: udev-md-raid-arrays.rules udev-md-raid-assembly.rules
> +	$(INSTALL) -D -m 644 udev-md-raid-arrays.rules $(DESTDIR)$(UDEVDIR)/rules.d/63-md-raid-arrays.rules
> +	$(INSTALL) -D -m 644 udev-md-raid-assembly.rules $(DESTDIR)$(UDEVDIR)/rules.d/64-md-raid-assembly.rules
>  
>  install-systemd: systemd/mdmon@.service
>  	$(INSTALL) -D -m 644 systemd/mdmon@.service $(DESTDIR)$(SYSTEMD_DIR)/mdmon@.service
> diff --git a/udev-md-raid-arrays.rules b/udev-md-raid-arrays.rules
> new file mode 100644
> index 0000000..0540ed8
> --- /dev/null
> +++ b/udev-md-raid-arrays.rules
> @@ -0,0 +1,35 @@
> +# do not edit this file, it will be overwritten on update
> +
> +SUBSYSTEM!="block", GOTO="md_end"
> +
> +# handle md arrays
> +ACTION!="add|change", GOTO="md_end"
> +KERNEL!="md*", GOTO="md_end"
> +
> +# partitions have no md/{array_state,metadata_version}, but should not
> +# for that reason be ignored.
> +ENV{DEVTYPE}=="partition", GOTO="md_ignore_state"
> +
> +# container devices have a metadata version of e.g. 'external:ddf' and
> +# never leave state 'inactive'
> +ATTR{md/metadata_version}=="external:[A-Za-z]*", ATTR{md/array_state}=="inactive", GOTO="md_ignore_state"
> +TEST!="md/array_state", ENV{SYSTEMD_READY}="0", GOTO="md_end"
> +ATTR{md/array_state}=="|clear|inactive", ENV{SYSTEMD_READY}="0", GOTO="md_end"
> +LABEL="md_ignore_state"
> +
> +IMPORT{program}="/sbin/mdadm --detail --export $devnode"
> +ENV{DEVTYPE}=="disk", ENV{MD_NAME}=="?*", SYMLINK+="disk/by-id/md-name-$env{MD_NAME}", OPTIONS+="string_escape=replace"
> +ENV{DEVTYPE}=="disk", ENV{MD_UUID}=="?*", SYMLINK+="disk/by-id/md-uuid-$env{MD_UUID}"
> +ENV{DEVTYPE}=="disk", ENV{MD_DEVNAME}=="?*", SYMLINK+="md/$env{MD_DEVNAME}"
> +ENV{DEVTYPE}=="partition", ENV{MD_NAME}=="?*", SYMLINK+="disk/by-id/md-name-$env{MD_NAME}-part%n", OPTIONS+="string_escape=replace"
> +ENV{DEVTYPE}=="partition", ENV{MD_UUID}=="?*", SYMLINK+="disk/by-id/md-uuid-$env{MD_UUID}-part%n"
> +ENV{DEVTYPE}=="partition", ENV{MD_DEVNAME}=="*[^0-9]", SYMLINK+="md/$env{MD_DEVNAME}%n"
> +ENV{DEVTYPE}=="partition", ENV{MD_DEVNAME}=="*[0-9]", SYMLINK+="md/$env{MD_DEVNAME}p%n"
> +
> +IMPORT{builtin}="blkid"
> +OPTIONS+="link_priority=100"
> +OPTIONS+="watch"
> +ENV{ID_FS_USAGE}=="filesystem|other|crypto", ENV{ID_FS_UUID_ENC}=="?*", SYMLINK+="disk/by-uuid/$env{ID_FS_UUID_ENC}"
> +ENV{ID_FS_USAGE}=="filesystem|other", ENV{ID_FS_LABEL_ENC}=="?*", SYMLINK+="disk/by-label/$env{ID_FS_LABEL_ENC}"
> +
> +LABEL="md_end"
> diff --git a/udev-md-raid-assembly.rules b/udev-md-raid-assembly.rules
> new file mode 100644
> index 0000000..b653265
> --- /dev/null
> +++ b/udev-md-raid-assembly.rules
> @@ -0,0 +1,19 @@
> +# do not edit this file, it will be overwritten on update
> +
> +# assemble md arrays
> +
> +SUBSYSTEM!="block", GOTO="md_inc_end"
> +
> +# 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_end"
> +
> +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 $devnode --offroot"
> +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_end"
> diff --git a/udev-md-raid.rules b/udev-md-raid.rules
> deleted file mode 100644
> index ea97261..0000000
> --- a/udev-md-raid.rules
> +++ /dev/null
> @@ -1,49 +0,0 @@
> -# do not edit this file, it will be overwritten on update
> -
> -SUBSYSTEM!="block", GOTO="md_end"
> -
> -# 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 $devnode --offroot"
> -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"
> -KERNEL!="md*", GOTO="md_end"
> -
> -# partitions have no md/{array_state,metadata_version}, but should not
> -# for that reason be ignored.
> -ENV{DEVTYPE}=="partition", GOTO="md_ignore_state"
> -
> -# container devices have a metadata version of e.g. 'external:ddf' and
> -# never leave state 'inactive'
> -ATTR{md/metadata_version}=="external:[A-Za-z]*", ATTR{md/array_state}=="inactive", GOTO="md_ignore_state"
> -TEST!="md/array_state", ENV{SYSTEMD_READY}="0", GOTO="md_end"
> -ATTR{md/array_state}=="|clear|inactive", ENV{SYSTEMD_READY}="0", GOTO="md_end"
> -LABEL="md_ignore_state"
> -
> -IMPORT{program}="/sbin/mdadm --detail --export $devnode"
> -ENV{DEVTYPE}=="disk", ENV{MD_NAME}=="?*", SYMLINK+="disk/by-id/md-name-$env{MD_NAME}", OPTIONS+="string_escape=replace"
> -ENV{DEVTYPE}=="disk", ENV{MD_UUID}=="?*", SYMLINK+="disk/by-id/md-uuid-$env{MD_UUID}"
> -ENV{DEVTYPE}=="disk", ENV{MD_DEVNAME}=="?*", SYMLINK+="md/$env{MD_DEVNAME}"
> -ENV{DEVTYPE}=="partition", ENV{MD_NAME}=="?*", SYMLINK+="disk/by-id/md-name-$env{MD_NAME}-part%n", OPTIONS+="string_escape=replace"
> -ENV{DEVTYPE}=="partition", ENV{MD_UUID}=="?*", SYMLINK+="disk/by-id/md-uuid-$env{MD_UUID}-part%n"
> -ENV{DEVTYPE}=="partition", ENV{MD_DEVNAME}=="*[^0-9]", SYMLINK+="md/$env{MD_DEVNAME}%n"
> -ENV{DEVTYPE}=="partition", ENV{MD_DEVNAME}=="*[0-9]", SYMLINK+="md/$env{MD_DEVNAME}p%n"
> -
> -IMPORT{builtin}="blkid"
> -OPTIONS+="link_priority=100"
> -OPTIONS+="watch"
> -ENV{ID_FS_USAGE}=="filesystem|other|crypto", ENV{ID_FS_UUID_ENC}=="?*", SYMLINK+="disk/by-uuid/$env{ID_FS_UUID_ENC}"
> -ENV{ID_FS_USAGE}=="filesystem|other", ENV{ID_FS_LABEL_ENC}=="?*", SYMLINK+="disk/by-label/$env{ID_FS_LABEL_ENC}"
> -
> -LABEL="md_end"


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