| Message ID | 20240301224011.11117-6-mwilck@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | device mapper udev rules rework | expand |
On 3/1/24 23:40, Martin Wilck wrote: > DM_SUSPENDED is a device-mapper internal flag, which is not intended to be > used by other rules, and which is determined by 10-dm.rules from sysfs for > every uevent. Rename it to ".DM_SUSPENDED", so that it won't be saved in the > udev database. > > Known consumers of DM_SUSPENDED are 66-kpartx.rules (from multipath-tools) and > 99-systemd.rules (from systemd). These will have to be adapted. > 11-dm-mpath.rules will be changed to use .DM_SUSPENDED. > > Signed-off-by: Martin Wilck <mwilck@suse.com> > --- > udev/10-dm.rules.in | 15 +++++++++------ > udev/12-dm-permissions.rules | 2 +- > udev/13-dm-disk.rules.in | 4 ++-- > 3 files changed, 12 insertions(+), 9 deletions(-) > > diff --git a/udev/10-dm.rules.in b/udev/10-dm.rules.in > index ef36209..d30f663 100644 > --- a/udev/10-dm.rules.in > +++ b/udev/10-dm.rules.in > @@ -11,7 +11,7 @@ > # for use in later rules: > # DM_NAME - actual DM device's name > # DM_UUID - UUID set for DM device (blank if not specified) > -# DM_SUSPENDED - suspended state of DM device (0 or 1) > +# .DM_SUSPENDED - suspended state of DM device (0 or 1) > # DM_UDEV_RULES_VSN - DM udev rules version > # > # These rules cover only basic device-mapper functionality in udev. > @@ -114,15 +114,18 @@ LABEL="dm_no_coldplug" > # The "suspended" item was added even later (kernels >= 2.6.31), > # so we also have to call dmsetup if the kernel version used > # is in between these releases. > -TEST=="dm", ENV{DM_NAME}="$attr{dm/name}", ENV{DM_UUID}="$attr{dm/uuid}", ENV{DM_SUSPENDED}="$attr{dm/suspended}" > +TEST=="dm", ENV{DM_NAME}="$attr{dm/name}", ENV{DM_UUID}="$attr{dm/uuid}", ENV{.DM_SUSPENDED}="$attr{dm/suspended}" > TEST!="dm", IMPORT{program}="(DM_EXEC)/dmsetup info -j %M -m %m -c --nameprefixes --noheadings --rows -o name,uuid,suspended" > -ENV{DM_SUSPENDED}!="?*", IMPORT{program}="(DM_EXEC)/dmsetup info -j %M -m %m -c --nameprefixes --noheadings --rows -o suspended" > > +ENV{.DM_SUSPENDED}=="?*", GOTO="dm_suspended_set" > +TEST=="dm", IMPORT{program}="(DM_EXEC)/dmsetup info -j %M -m %m -c --nameprefixes --noheadings --rows -o suspended" > # dmsetup tool provides suspended state information in textual > # form with values "Suspended"/"Active". We translate it to > # 0/1 respectively to be consistent with sysfs values. > -ENV{DM_SUSPENDED}=="Active", ENV{DM_SUSPENDED}="0" > -ENV{DM_SUSPENDED}=="Suspended", ENV{DM_SUSPENDED}="1" > +ENV{DM_SUSPENDED}=="Active", ENV{.DM_SUSPENDED}="0" > +ENV{DM_SUSPENDED}=="Suspended", ENV{.DM_SUSPENDED}="1" > +ENV{DM_SUSPENDED}="" > +LABEL="dm_suspended_set" > > # This variable provides a reliable way to check that device-mapper > # rules were installed. It means that all needed variables are set > @@ -140,7 +143,7 @@ ENV{DM_UDEV_DISABLE_DM_RULES_FLAG}!="1", ENV{DM_NAME}=="?*", SYMLINK+="(DM_DIR)/ > # Avoid processing and scanning a DM device in the other (foreign) > # rules if it is in suspended state. However, we still keep 'disk' > # and 'DM subsystem' related rules enabled in this case. > -ENV{DM_SUSPENDED}=="1", ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="1" > +ENV{.DM_SUSPENDED}=="1", ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="1" > > GOTO="dm_end" > > diff --git a/udev/12-dm-permissions.rules b/udev/12-dm-permissions.rules > index a9d4c32..6a69d2f 100644 > --- a/udev/12-dm-permissions.rules > +++ b/udev/12-dm-permissions.rules > @@ -14,7 +14,7 @@ > # DM_UDEV_RULES_VSN - DM udev rules version > # DM_NAME - actual DM device's name > # DM_UUID - UUID set for DM device (blank if not specified) > -# DM_SUSPENDED - suspended state of DM device (0 or 1) > +# .DM_SUSPENDED - suspended state of DM device (0 or 1) > # DM_LV_NAME - logical volume name (not set if LVM device not present) > # DM_VG_NAME - volume group name (not set if LVM device not present) > # DM_LV_LAYER - logical volume layer (not set if LVM device not present) > diff --git a/udev/13-dm-disk.rules.in b/udev/13-dm-disk.rules.in > index eaad972..cb2ce2d 100644 > --- a/udev/13-dm-disk.rules.in > +++ b/udev/13-dm-disk.rules.in > @@ -17,9 +17,9 @@ ENV{DM_UDEV_DISABLE_DISK_RULES_FLAG}=="1", GOTO="dm_end" > SYMLINK+="disk/by-id/dm-name-$env{DM_NAME}" > ENV{DM_UUID}=="?*", SYMLINK+="disk/by-id/dm-uuid-$env{DM_UUID}" > > -ENV{DM_SUSPENDED}=="1", ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}=="1", GOTO="dm_import" > +ENV{.DM_SUSPENDED}=="1", ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}=="1", GOTO="dm_import" > ENV{DM_NOSCAN}=="1", ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}=="1", GOTO="dm_import" > -ENV{DM_SUSPENDED}=="1", GOTO="dm_end" > +ENV{.DM_SUSPENDED}=="1", GOTO="dm_end" > ENV{DM_NOSCAN}=="1", GOTO="dm_watch" > > (BLKID_RULE) OK, makes sense. But I haven't looked at what implications this might have for 99-systemd.rules yet, but we surely need to have that covered somehow. Maybe, now, I would probably even remove the mention about DM_SUSPENDED in 12-dm-permissions.rules, it looks superfluous there. We normally set perms based on names, not on DM_SUSPENDED state. I'm not sure why we mentioned it there before. Do mpath rules still need to look at DM_SUSPENDED?
On Mon, 2024-03-04 at 12:00 +0100, Peter Rajnoha wrote: > > OK, makes sense. But I haven't looked at what implications this might > have for 99-systemd.rules yet, but we surely need to have that > covered > somehow. > > Maybe, now, I would probably even remove the mention about > DM_SUSPENDED > in 12-dm-permissions.rules, it looks superfluous there. We normally > set > perms based on names, not on DM_SUSPENDED state. I'm not sure why we > mentioned it there before. > > Do mpath rules still need to look at DM_SUSPENDED? No, and yes :-) If we remove the DISK_RO clause, DM_SUSPENDED and DM_UDEV_DISABLE_OTHER_RULES_FLAG (as input from 10-dm.rules) are equivalent for multipath. We'd be able to modify 11-dm-mpath.rules such that DM_SUSPENDED isn't used any more. The downside is that 11-dm- mpath.rules needs to modify DM_UDEV_DISABLE_OTHER_RULES_FLAG under certain conditions, and that DM_SUSPENDED is shorter and expresses the actual situation more intuitively. Therefore I don't love the idea to replace use of DM_SUSPENDED with "DUDORF" in 11-dm-mpath.rules. My personal take on this is that 11-dm-mpath.rules actually belongs to device-mapper (being executed before 13-dm-disk.rules), even though it's not maintained in the lvm2 repository. As such, it should be allowed to access dm-internal flags like DM_SUSPENDED. Not that's not a problem with this patch; the multipath rules can just access .DM_SUSPENDED instead of DM_SUSPENDED. Regards Martin
On 3/4/24 17:21, Martin Wilck wrote: > On Mon, 2024-03-04 at 12:00 +0100, Peter Rajnoha wrote: >> >> OK, makes sense. But I haven't looked at what implications this might >> have for 99-systemd.rules yet, but we surely need to have that >> covered >> somehow. >> >> Maybe, now, I would probably even remove the mention about >> DM_SUSPENDED >> in 12-dm-permissions.rules, it looks superfluous there. We normally >> set >> perms based on names, not on DM_SUSPENDED state. I'm not sure why we >> mentioned it there before. >> >> Do mpath rules still need to look at DM_SUSPENDED? > > No, and yes :-) > > If we remove the DISK_RO clause, DM_SUSPENDED and > DM_UDEV_DISABLE_OTHER_RULES_FLAG (as input from 10-dm.rules) are > equivalent for multipath. We'd be able to modify 11-dm-mpath.rules such > that DM_SUSPENDED isn't used any more. The downside is that 11-dm- > mpath.rules needs to modify DM_UDEV_DISABLE_OTHER_RULES_FLAG under > certain conditions, and that DM_SUSPENDED is shorter and expresses the > actual situation more intuitively. Therefore I don't love the idea to > replace use of DM_SUSPENDED with "DUDORF" in 11-dm-mpath.rules. > > My personal take on this is that 11-dm-mpath.rules actually belongs to > device-mapper (being executed before 13-dm-disk.rules), even though > it's not maintained in the lvm2 repository. As such, it should be > allowed to access dm-internal flags like DM_SUSPENDED. > Not that's not a problem with this patch; the multipath rules can just > access .DM_SUSPENDED instead of DM_SUSPENDED. Within DM and DM-subsystem rules, it's OK to use DM_SUSPENDED, if needed. We should just hide it from all the "other" rules so they don't need to bother. For them (right now), it's either "usable" or "unusable" device for whatever reason behind and we (DM+DM-subystem) should reimport whatever is needed for the state/set of variables that others may use, to stay sane. Of course, we can do this only for the state that we own. As we discussed before, this can be extended to making a difference among "usable", "temporarily unusable" (so reimport the state/variables needed) and "completely unusable" state for others.
On Tue, 2024-03-05 at 09:19 +0100, Peter Rajnoha wrote: > On 3/4/24 17:21, Martin Wilck wrote: > > > > My personal take on this is that 11-dm-mpath.rules actually belongs > > to > > device-mapper (being executed before 13-dm-disk.rules), even though > > it's not maintained in the lvm2 repository. As such, it should be > > allowed to access dm-internal flags like DM_SUSPENDED. > > Not that's not a problem with this patch; the multipath rules can > > just > > access .DM_SUSPENDED instead of DM_SUSPENDED. > > Within DM and DM-subsystem rules, it's OK to use DM_SUSPENDED, if > needed. I gather that you agree that 11-dm-mpath.rules represents a "DM subsystem" rule set? > > We should just hide it from all the "other" rules so they don't need > to > bother. For them (right now), it's either "usable" or "unusable" > device > for whatever reason behind and we (DM+DM-subystem) should reimport > whatever is needed for the state/set of variables that others may > use, > to stay sane. Of course, we can do this only for the state that we > own. > > As we discussed before, this can be extended to making a difference > among "usable", "temporarily unusable" (so reimport the > state/variables > needed) and "completely unusable" state for others. Yeah, but that's future work, and I doubt that it makes sense to invest much effort into it. I definitely wouldn't want to tie this to the current patch set. As mentioned previously, it might make sense to introduce a flag that expresses something like "you can access this device, but you don't need to" (DISK_RO={0,1} case, for example). But then, we already have DM_ACTIVATION to express the opposite ("you must have a look at this device, its properties have changed"). I wonder if you consider DM_ACTIVATION a dm-internal property? Thanks Martin
On 3/5/24 09:47, Martin Wilck wrote: > On Tue, 2024-03-05 at 09:19 +0100, Peter Rajnoha wrote: >> Within DM and DM-subsystem rules, it's OK to use DM_SUSPENDED, if >> needed. > > I gather that you agree that 11-dm-mpath.rules represents a "DM > subsystem" rule set? > Sure, of course. >> >> We should just hide it from all the "other" rules so they don't need >> to >> bother. For them (right now), it's either "usable" or "unusable" >> device >> for whatever reason behind and we (DM+DM-subystem) should reimport >> whatever is needed for the state/set of variables that others may >> use, >> to stay sane. Of course, we can do this only for the state that we >> own. >> >> As we discussed before, this can be extended to making a difference >> among "usable", "temporarily unusable" (so reimport the >> state/variables >> needed) and "completely unusable" state for others. > > Yeah, but that's future work, and I doubt that it makes sense to invest > much effort into it. I definitely wouldn't want to tie this to the > current patch set. > I agree. > As mentioned previously, it might make sense to introduce a flag that > expresses something like "you can access this device, but you don't > need to" (DISK_RO={0,1} case, for example). But then, we already have > DM_ACTIVATION to express the opposite ("you must have a look at this > device, its properties have changed"). I wonder if you consider > DM_ACTIVATION a dm-internal property? Well, we added DM_ACTIVATION as a helper primarily for DM and DM-subsys rules to have a way to identify when the actual (re)activation happens, or the "add" trigger on coldplug. I think it was 69-dm-lvm.rules (or 69-dm-lvm-metad.rules at that time) where we needed to run pvscan only right after the DM dev is activated and hence avoiding running costly pvscan uselessly where it doesn't matter. If there's anyone else out there with similar use case, I think that checking DM_ACTIVATION might be useful. But it's true it is not advertised and shouted out somehow publicly yet. Usually, all the other rules are interested in rescanning all the other "foreign" state and attributes that is out of DM's hands, which means they know exactly when to do the scan or not or any other actions, it depends on what attributes are they watching for. DM_ACTIVATION is very useful to know when stacking devices on top of DM though, so a time when to activate the layer above. So yes, this variable might be useful for other to look for too.
On Tue, 2024-03-05 at 10:10 +0100, Peter Rajnoha wrote: > On 3/5/24 09:47, Martin Wilck wrote: > > On Tue, 2024-03-05 at 09:19 +0100, Peter Rajnoha wrote: > > > Within DM and DM-subsystem rules, it's OK to use DM_SUSPENDED, if > > > needed. > > > > I gather that you agree that 11-dm-mpath.rules represents a "DM > > subsystem" rule set? > > > > Sure, of course. > > > > > > > We should just hide it from all the "other" rules so they don't > > > need > > > to > > > bother. For them (right now), it's either "usable" or "unusable" > > > device > > > for whatever reason behind and we (DM+DM-subystem) should > > > reimport > > > whatever is needed for the state/set of variables that others may > > > use, > > > to stay sane. Of course, we can do this only for the state that > > > we > > > own. > > > > > > As we discussed before, this can be extended to making a > > > difference > > > among "usable", "temporarily unusable" (so reimport the > > > state/variables > > > needed) and "completely unusable" state for others. > > > > Yeah, but that's future work, and I doubt that it makes sense to > > invest > > much effort into it. I definitely wouldn't want to tie this to the > > current patch set. > > > > I agree. > > > As mentioned previously, it might make sense to introduce a flag > > that > > expresses something like "you can access this device, but you don't > > need to" (DISK_RO={0,1} case, for example). But then, we already > > have > > DM_ACTIVATION to express the opposite ("you must have a look at > > this > > device, its properties have changed"). I wonder if you consider > > DM_ACTIVATION a dm-internal property? > > Well, we added DM_ACTIVATION as a helper primarily for DM and DM- > subsys > rules to have a way to identify when the actual (re)activation > happens, > or the "add" trigger on coldplug. > > I think it was 69-dm-lvm.rules (or 69-dm-lvm-metad.rules at that > time) > where we needed to run pvscan only right after the DM dev is > activated > and hence avoiding running costly pvscan uselessly where it doesn't > matter. > > If there's anyone else out there with similar use case, I think that > checking DM_ACTIVATION might be useful. But it's true it is not > advertised and shouted out somehow publicly yet. > > Usually, all the other rules are interested in rescanning all the > other > "foreign" state and attributes that is out of DM's hands, which means > they know exactly when to do the scan or not or any other actions, it > depends on what attributes are they watching for. > > DM_ACTIVATION is very useful to know when stacking devices on top of > DM > though, so a time when to activate the layer above. So yes, this > variable might be useful for other to look for too. Thanks. I agree, but it's future work and it would mostly be a thing for authors of stacked subsystems to look into. I think are on the same page except perhaps for the DISK_RO part of the set. I will give that some more thought, and submit a non-RFC patch set. Regards Martin
diff --git a/udev/10-dm.rules.in b/udev/10-dm.rules.in index ef36209..d30f663 100644 --- a/udev/10-dm.rules.in +++ b/udev/10-dm.rules.in @@ -11,7 +11,7 @@ # for use in later rules: # DM_NAME - actual DM device's name # DM_UUID - UUID set for DM device (blank if not specified) -# DM_SUSPENDED - suspended state of DM device (0 or 1) +# .DM_SUSPENDED - suspended state of DM device (0 or 1) # DM_UDEV_RULES_VSN - DM udev rules version # # These rules cover only basic device-mapper functionality in udev. @@ -114,15 +114,18 @@ LABEL="dm_no_coldplug" # The "suspended" item was added even later (kernels >= 2.6.31), # so we also have to call dmsetup if the kernel version used # is in between these releases. -TEST=="dm", ENV{DM_NAME}="$attr{dm/name}", ENV{DM_UUID}="$attr{dm/uuid}", ENV{DM_SUSPENDED}="$attr{dm/suspended}" +TEST=="dm", ENV{DM_NAME}="$attr{dm/name}", ENV{DM_UUID}="$attr{dm/uuid}", ENV{.DM_SUSPENDED}="$attr{dm/suspended}" TEST!="dm", IMPORT{program}="(DM_EXEC)/dmsetup info -j %M -m %m -c --nameprefixes --noheadings --rows -o name,uuid,suspended" -ENV{DM_SUSPENDED}!="?*", IMPORT{program}="(DM_EXEC)/dmsetup info -j %M -m %m -c --nameprefixes --noheadings --rows -o suspended" +ENV{.DM_SUSPENDED}=="?*", GOTO="dm_suspended_set" +TEST=="dm", IMPORT{program}="(DM_EXEC)/dmsetup info -j %M -m %m -c --nameprefixes --noheadings --rows -o suspended" # dmsetup tool provides suspended state information in textual # form with values "Suspended"/"Active". We translate it to # 0/1 respectively to be consistent with sysfs values. -ENV{DM_SUSPENDED}=="Active", ENV{DM_SUSPENDED}="0" -ENV{DM_SUSPENDED}=="Suspended", ENV{DM_SUSPENDED}="1" +ENV{DM_SUSPENDED}=="Active", ENV{.DM_SUSPENDED}="0" +ENV{DM_SUSPENDED}=="Suspended", ENV{.DM_SUSPENDED}="1" +ENV{DM_SUSPENDED}="" +LABEL="dm_suspended_set" # This variable provides a reliable way to check that device-mapper # rules were installed. It means that all needed variables are set @@ -140,7 +143,7 @@ ENV{DM_UDEV_DISABLE_DM_RULES_FLAG}!="1", ENV{DM_NAME}=="?*", SYMLINK+="(DM_DIR)/ # Avoid processing and scanning a DM device in the other (foreign) # rules if it is in suspended state. However, we still keep 'disk' # and 'DM subsystem' related rules enabled in this case. -ENV{DM_SUSPENDED}=="1", ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="1" +ENV{.DM_SUSPENDED}=="1", ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="1" GOTO="dm_end" diff --git a/udev/12-dm-permissions.rules b/udev/12-dm-permissions.rules index a9d4c32..6a69d2f 100644 --- a/udev/12-dm-permissions.rules +++ b/udev/12-dm-permissions.rules @@ -14,7 +14,7 @@ # DM_UDEV_RULES_VSN - DM udev rules version # DM_NAME - actual DM device's name # DM_UUID - UUID set for DM device (blank if not specified) -# DM_SUSPENDED - suspended state of DM device (0 or 1) +# .DM_SUSPENDED - suspended state of DM device (0 or 1) # DM_LV_NAME - logical volume name (not set if LVM device not present) # DM_VG_NAME - volume group name (not set if LVM device not present) # DM_LV_LAYER - logical volume layer (not set if LVM device not present) diff --git a/udev/13-dm-disk.rules.in b/udev/13-dm-disk.rules.in index eaad972..cb2ce2d 100644 --- a/udev/13-dm-disk.rules.in +++ b/udev/13-dm-disk.rules.in @@ -17,9 +17,9 @@ ENV{DM_UDEV_DISABLE_DISK_RULES_FLAG}=="1", GOTO="dm_end" SYMLINK+="disk/by-id/dm-name-$env{DM_NAME}" ENV{DM_UUID}=="?*", SYMLINK+="disk/by-id/dm-uuid-$env{DM_UUID}" -ENV{DM_SUSPENDED}=="1", ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}=="1", GOTO="dm_import" +ENV{.DM_SUSPENDED}=="1", ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}=="1", GOTO="dm_import" ENV{DM_NOSCAN}=="1", ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}=="1", GOTO="dm_import" -ENV{DM_SUSPENDED}=="1", GOTO="dm_end" +ENV{.DM_SUSPENDED}=="1", GOTO="dm_end" ENV{DM_NOSCAN}=="1", GOTO="dm_watch" (BLKID_RULE)
DM_SUSPENDED is a device-mapper internal flag, which is not intended to be used by other rules, and which is determined by 10-dm.rules from sysfs for every uevent. Rename it to ".DM_SUSPENDED", so that it won't be saved in the udev database. Known consumers of DM_SUSPENDED are 66-kpartx.rules (from multipath-tools) and 99-systemd.rules (from systemd). These will have to be adapted. 11-dm-mpath.rules will be changed to use .DM_SUSPENDED. Signed-off-by: Martin Wilck <mwilck@suse.com> --- udev/10-dm.rules.in | 15 +++++++++------ udev/12-dm-permissions.rules | 2 +- udev/13-dm-disk.rules.in | 4 ++-- 3 files changed, 12 insertions(+), 9 deletions(-)