| Message ID | 20240324211301.7200-8-mwilck@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | multipath udev rules changes for dm rules V3 | expand |
On Sun, Mar 24, 2024 at 10:12:57PM +0100, Martin Wilck wrote: > With the late changes to the device mapper rules, DM_SUSPENDED > is not exported any more. Use .DM_SUSPENDED instead. > > Note that although 11-dm-mpath.rules is not a part of lvm2, it > can be considered as part of the device-mapper layer (everything > before 13-dm-disk.rules can), and is thus allowed to use > .DM_SUSPENDED. In practice .DM_SUSPENDED is equivalent to > DM_UDEV_DISABLE_OTHER_RULES_FLAG for multipath devices, but > using .DM_SUSPENDED here makes the intention more obvious. > > Signed-off-by: Martin Wilck <mwilck@suse.com> > --- > multipath/11-dm-mpath.rules.in | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/multipath/11-dm-mpath.rules.in b/multipath/11-dm-mpath.rules.in > index 8f22954..c4b5685 100644 > --- a/multipath/11-dm-mpath.rules.in > +++ b/multipath/11-dm-mpath.rules.in > @@ -4,8 +4,11 @@ ENV{DM_UUID}!="mpath-?*", GOTO="mpath_end" > > IMPORT{db}="MPATH_DEVICE_READY" > > +# device-mapper rules v2 compatibility > +ENV{.DM_SUSPENDED}!="?*", ENV{.DM_SUSPENDED}="$env{DM_SUSPENDED}" > + > # Coldplug event while device is suspended (e.g. during a reload) > -ACTION=="add", ENV{DM_ACTIVATION}=="1", ENV{DM_SUSPENDED}=="1", \ > +ACTION=="add", ENV{DM_ACTIVATION}=="1", ENV{.DM_SUSPENDED}=="1", \ > PROGRAM="/bin/logger -t 11-dm-mpath.rules -p daemon.warning \"Coldplug event for suspended device\"", \ > ENV{DM_COLDPLUG_SUSPENDED}="1", GOTO="scan_import" > > @@ -17,7 +20,7 @@ ENV{DM_UDEV_RULES_VSN}=="3", ENV{.DM_SUSPENDED}!="1", GOTO="scan_import" > # from DB in 10-dm.rules. If the device is not suspended, clear the flag. > # This is safe for multipath where DM_UDEV_DISABLE_OTHER_RULES_FLAG is basically > # equivalent to DM_SUSPENDED==1 || DISK_RO==1 > -ENV{DM_UDEV_RULES_VSN}!="3", ENV{DM_SUSPENDED}!="1", ENV{DISK_RO}!="1", \ > +ENV{DM_UDEV_RULES_VSN}!="3", ENV{.DM_SUSPENDED}!="1", ENV{DISK_RO}!="1", \ > ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="", GOTO="scan_import" > LABEL="mpath_coldplug_end" > > @@ -68,7 +71,7 @@ LABEL="mpath_action" > # Activation might have been partially skipped. Activate the device now, > # i.e. disable the MPATH_UNCHANGED logic and set DM_ACTIVATION=1. > IMPORT{db}="DM_COLDPLUG_SUSPENDED" > -ENV{DM_COLDPLUG_SUSPENDED}=="1", ENV{DM_SUSPENDED}!="1", \ > +ENV{DM_COLDPLUG_SUSPENDED}=="1", ENV{.DM_SUSPENDED}!="1", \ When I reviewed patch 0001, I wondered if it was o.k. to not force DM_ACTIVATION to "1" if a device had previously been suspended when MPATH_DEVICE_READY switched to "1". I thought it might it might be o.k. because there would be another event when the device resumed, which would have DM_ACTIVATION set to "1". But if that's the case then we shouldn't need to worry about a device being suspended when a coldplug happens. We will get an event when the device resumes, and there's not much we can do until it gets resumed at any rate. So either patch 0001 is wrong or we can remove this DM_COLDPLUG_SUSPENDED forced activation code or I'm missing some important difference between the two. -Ben > ENV{DM_ACTIVATION}="1", ENV{MPATH_UNCHANGED}="0", \ > PROGRAM="/bin/logger -t 11-dm-mpath.rules -p daemon.notice \"Forcing activation of previously suspended device\"", \ > GOTO="force_activation" > @@ -109,7 +112,7 @@ LABEL="mpath_is_ready" > # upper layers do a rescan. If the device is currently suspended, > # we have to postpone the activation until the next event. > ENV{.MPATH_DEVICE_READY_OLD}!="0", GOTO="dont_activate" > -ENV{DM_SUSPENDED}=="1", GOTO="dont_activate" > +ENV{.DM_SUSPENDED}=="1", GOTO="dont_activate" > > ENV{DM_ACTIVATION}="1", ENV{MPATH_UNCHANGED}="0" > LABEL="dont_activate" > @@ -143,7 +146,7 @@ IMPORT{db}="ID_PART_GPT_AUTO_ROOT" > LABEL="import_end" > > # Reset previous DM_COLDPLUG_SUSPENDED if activation happens now > -ENV{DM_SUSPENDED}!="1", ENV{DM_ACTIVATION}=="1", ENV{DM_COLDPLUG_SUSPENDED}="" > +ENV{.DM_SUSPENDED}!="1", ENV{DM_ACTIVATION}=="1", ENV{DM_COLDPLUG_SUSPENDED}="" > > # Multipath maps should take precedence over their members. > ENV{DM_UDEV_LOW_PRIORITY_FLAG}!="1", OPTIONS+="link_priority=50" > -- > 2.43.2
On Tue, 2024-03-26 at 19:36 -0400, Benjamin Marzinski wrote: > On Sun, Mar 24, 2024 at 10:12:57PM +0100, Martin Wilck wrote: > > With the late changes to the device mapper rules, DM_SUSPENDED > > is not exported any more. Use .DM_SUSPENDED instead. > > > > Note that although 11-dm-mpath.rules is not a part of lvm2, it > > can be considered as part of the device-mapper layer (everything > > before 13-dm-disk.rules can), and is thus allowed to use > > .DM_SUSPENDED. In practice .DM_SUSPENDED is equivalent to > > DM_UDEV_DISABLE_OTHER_RULES_FLAG for multipath devices, but > > using .DM_SUSPENDED here makes the intention more obvious. > > > > Signed-off-by: Martin Wilck <mwilck@suse.com> > > --- > > multipath/11-dm-mpath.rules.in | 13 ++++++++----- > > 1 file changed, 8 insertions(+), 5 deletions(-) > > > > diff --git a/multipath/11-dm-mpath.rules.in b/multipath/11-dm- > > mpath.rules.in > > index 8f22954..c4b5685 100644 > > --- a/multipath/11-dm-mpath.rules.in > > +++ b/multipath/11-dm-mpath.rules.in > > @@ -4,8 +4,11 @@ ENV{DM_UUID}!="mpath-?*", GOTO="mpath_end" > > > > IMPORT{db}="MPATH_DEVICE_READY" > > > > +# device-mapper rules v2 compatibility > > +ENV{.DM_SUSPENDED}!="?*", ENV{.DM_SUSPENDED}="$env{DM_SUSPENDED}" > > + > > # Coldplug event while device is suspended (e.g. during a reload) > > -ACTION=="add", ENV{DM_ACTIVATION}=="1", ENV{DM_SUSPENDED}=="1", \ > > +ACTION=="add", ENV{DM_ACTIVATION}=="1", ENV{.DM_SUSPENDED}=="1", \ > > PROGRAM="/bin/logger -t 11-dm-mpath.rules -p > > daemon.warning \"Coldplug event for suspended device\"", \ > > ENV{DM_COLDPLUG_SUSPENDED}="1", GOTO="scan_import" > > > > @@ -17,7 +20,7 @@ ENV{DM_UDEV_RULES_VSN}=="3", > > ENV{.DM_SUSPENDED}!="1", GOTO="scan_import" > > # from DB in 10-dm.rules. If the device is not suspended, clear > > the flag. > > # This is safe for multipath where > > DM_UDEV_DISABLE_OTHER_RULES_FLAG is basically > > # equivalent to DM_SUSPENDED==1 || DISK_RO==1 > > -ENV{DM_UDEV_RULES_VSN}!="3", ENV{DM_SUSPENDED}!="1", > > ENV{DISK_RO}!="1", \ > > +ENV{DM_UDEV_RULES_VSN}!="3", ENV{.DM_SUSPENDED}!="1", > > ENV{DISK_RO}!="1", \ > > ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="", > > GOTO="scan_import" > > LABEL="mpath_coldplug_end" > > > > @@ -68,7 +71,7 @@ LABEL="mpath_action" > > # Activation might have been partially skipped. Activate the > > device now, > > # i.e. disable the MPATH_UNCHANGED logic and set DM_ACTIVATION=1. > > IMPORT{db}="DM_COLDPLUG_SUSPENDED" > > -ENV{DM_COLDPLUG_SUSPENDED}=="1", ENV{DM_SUSPENDED}!="1", \ > > +ENV{DM_COLDPLUG_SUSPENDED}=="1", ENV{.DM_SUSPENDED}!="1", \ > > When I reviewed patch 0001, I wondered if it was o.k. to not force > DM_ACTIVATION to "1" if a device had previously been suspended when > MPATH_DEVICE_READY switched to "1". I thought it might it might be > o.k. > because there would be another event when the device resumed, which > would have DM_ACTIVATION set to "1". But if that's the case then we > shouldn't need to worry about a device being suspended when a > coldplug > happens. We will get an event when the device resumes, and there's > not > much we can do until it gets resumed at any rate. So either patch > 0001 > is wrong or we can remove this DM_COLDPLUG_SUSPENDED forced > activation > code or I'm missing some important difference between the two. I tried to explain this in 02bbc17 ("11-dm-mpath.rules: handle reloads during coldplug events"). But I just needed to recall the logic again myself, it's subtle. We will indeed get another change event when the device is resumed. But because of our MPATH_UNCHANGED logic, we may see that the the properties of the device haven't changed, and unset DM_ACTIVATION. Thus follow-up rules won't get executed, neither in coldplug event itself (device suspended) nor in the follow-up change event (device unchanged). I hope this makes sense. The problem that you pointed out for 0001 for this series is similar, but it handles a different case where the device switched from 0 to one usable path while the device was suspended. Patch 0001 of this series is really broken, and it breaks a patch that I'd just created a month ago, which is truly embarrassing. udev rules are confusing :-/ Regards Martin
diff --git a/multipath/11-dm-mpath.rules.in b/multipath/11-dm-mpath.rules.in index 8f22954..c4b5685 100644 --- a/multipath/11-dm-mpath.rules.in +++ b/multipath/11-dm-mpath.rules.in @@ -4,8 +4,11 @@ ENV{DM_UUID}!="mpath-?*", GOTO="mpath_end" IMPORT{db}="MPATH_DEVICE_READY" +# device-mapper rules v2 compatibility +ENV{.DM_SUSPENDED}!="?*", ENV{.DM_SUSPENDED}="$env{DM_SUSPENDED}" + # Coldplug event while device is suspended (e.g. during a reload) -ACTION=="add", ENV{DM_ACTIVATION}=="1", ENV{DM_SUSPENDED}=="1", \ +ACTION=="add", ENV{DM_ACTIVATION}=="1", ENV{.DM_SUSPENDED}=="1", \ PROGRAM="/bin/logger -t 11-dm-mpath.rules -p daemon.warning \"Coldplug event for suspended device\"", \ ENV{DM_COLDPLUG_SUSPENDED}="1", GOTO="scan_import" @@ -17,7 +20,7 @@ ENV{DM_UDEV_RULES_VSN}=="3", ENV{.DM_SUSPENDED}!="1", GOTO="scan_import" # from DB in 10-dm.rules. If the device is not suspended, clear the flag. # This is safe for multipath where DM_UDEV_DISABLE_OTHER_RULES_FLAG is basically # equivalent to DM_SUSPENDED==1 || DISK_RO==1 -ENV{DM_UDEV_RULES_VSN}!="3", ENV{DM_SUSPENDED}!="1", ENV{DISK_RO}!="1", \ +ENV{DM_UDEV_RULES_VSN}!="3", ENV{.DM_SUSPENDED}!="1", ENV{DISK_RO}!="1", \ ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="", GOTO="scan_import" LABEL="mpath_coldplug_end" @@ -68,7 +71,7 @@ LABEL="mpath_action" # Activation might have been partially skipped. Activate the device now, # i.e. disable the MPATH_UNCHANGED logic and set DM_ACTIVATION=1. IMPORT{db}="DM_COLDPLUG_SUSPENDED" -ENV{DM_COLDPLUG_SUSPENDED}=="1", ENV{DM_SUSPENDED}!="1", \ +ENV{DM_COLDPLUG_SUSPENDED}=="1", ENV{.DM_SUSPENDED}!="1", \ ENV{DM_ACTIVATION}="1", ENV{MPATH_UNCHANGED}="0", \ PROGRAM="/bin/logger -t 11-dm-mpath.rules -p daemon.notice \"Forcing activation of previously suspended device\"", \ GOTO="force_activation" @@ -109,7 +112,7 @@ LABEL="mpath_is_ready" # upper layers do a rescan. If the device is currently suspended, # we have to postpone the activation until the next event. ENV{.MPATH_DEVICE_READY_OLD}!="0", GOTO="dont_activate" -ENV{DM_SUSPENDED}=="1", GOTO="dont_activate" +ENV{.DM_SUSPENDED}=="1", GOTO="dont_activate" ENV{DM_ACTIVATION}="1", ENV{MPATH_UNCHANGED}="0" LABEL="dont_activate" @@ -143,7 +146,7 @@ IMPORT{db}="ID_PART_GPT_AUTO_ROOT" LABEL="import_end" # Reset previous DM_COLDPLUG_SUSPENDED if activation happens now -ENV{DM_SUSPENDED}!="1", ENV{DM_ACTIVATION}=="1", ENV{DM_COLDPLUG_SUSPENDED}="" +ENV{.DM_SUSPENDED}!="1", ENV{DM_ACTIVATION}=="1", ENV{DM_COLDPLUG_SUSPENDED}="" # Multipath maps should take precedence over their members. ENV{DM_UDEV_LOW_PRIORITY_FLAG}!="1", OPTIONS+="link_priority=50"
With the late changes to the device mapper rules, DM_SUSPENDED is not exported any more. Use .DM_SUSPENDED instead. Note that although 11-dm-mpath.rules is not a part of lvm2, it can be considered as part of the device-mapper layer (everything before 13-dm-disk.rules can), and is thus allowed to use .DM_SUSPENDED. In practice .DM_SUSPENDED is equivalent to DM_UDEV_DISABLE_OTHER_RULES_FLAG for multipath devices, but using .DM_SUSPENDED here makes the intention more obvious. Signed-off-by: Martin Wilck <mwilck@suse.com> --- multipath/11-dm-mpath.rules.in | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)