* [PATCH RESEND 0/2] LVM2: fix lvmetad udev rules for CHANGE events @ 2018-04-16 18:53 ` Martin Wilck 0 siblings, 0 replies; 10+ messages in thread From: Martin Wilck @ 2018-04-16 18:53 UTC (permalink / raw) To: Zdenek Kabelac, Peter Rajnoha Cc: dm-devel, Martin Wilck, Alasdair G Kergon, lvm-devel (This has been sent to lvm-devel on Dec 21, 2017, and apparently got lost. Resending). The current logic in 69-dm-lvm-metad.rules is broken for the default "enable-udev-systemd-background-jobs" case. Detailed information about the problem can be found in the commit message of the 2nd patch in the set. That patch also contains the tiny actual change of this patch set: if systemd background jobs are active, the variables SYSTEMD_ALIAS and SYSTEMD_WANTS are also set for CHANGE events, not only for ADD. The reason that the patch set is quite large nonetheless is that I wanted the comments in the rules file to match the actual behavior. Substitution of multi-line comments is very hard, if not impossible, with the string substitution approach in the current Makefile. That necessitates the first patch, which introduces no functional change. Martin Wilck (2): lvm2: 69-dm-lvm-metad.rules: explicit pvscan rule lvm2: 69-dm-lvm-metad.rules: set systemd vars on "change" udev/69-dm-lvm-metad.rules.in | 53 +++++++++++++++++++++++++++++++++++++++---- udev/Makefile.in | 9 +++++--- 2 files changed, 54 insertions(+), 8 deletions(-) -- 2.16.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH RESEND 0/2] LVM2: fix lvmetad udev rules for CHANGE events @ 2018-04-16 18:53 ` Martin Wilck 0 siblings, 0 replies; 10+ messages in thread From: Martin Wilck @ 2018-04-16 18:53 UTC (permalink / raw) To: lvm-devel (This has been sent to lvm-devel on Dec 21, 2017, and apparently got lost. Resending). The current logic in 69-dm-lvm-metad.rules is broken for the default "enable-udev-systemd-background-jobs" case. Detailed information about the problem can be found in the commit message of the 2nd patch in the set. That patch also contains the tiny actual change of this patch set: if systemd background jobs are active, the variables SYSTEMD_ALIAS and SYSTEMD_WANTS are also set for CHANGE events, not only for ADD. The reason that the patch set is quite large nonetheless is that I wanted the comments in the rules file to match the actual behavior. Substitution of multi-line comments is very hard, if not impossible, with the string substitution approach in the current Makefile. That necessitates the first patch, which introduces no functional change. Martin Wilck (2): lvm2: 69-dm-lvm-metad.rules: explicit pvscan rule lvm2: 69-dm-lvm-metad.rules: set systemd vars on "change" udev/69-dm-lvm-metad.rules.in | 53 +++++++++++++++++++++++++++++++++++++++---- udev/Makefile.in | 9 +++++--- 2 files changed, 54 insertions(+), 8 deletions(-) -- 2.16.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH RESEND 1/2] lvm2: 69-dm-lvm-metad.rules: explicit pvscan rule 2018-04-16 18:53 ` Martin Wilck @ 2018-04-16 18:53 ` Martin Wilck -1 siblings, 0 replies; 10+ messages in thread From: Martin Wilck @ 2018-04-16 18:53 UTC (permalink / raw) To: Zdenek Kabelac, Peter Rajnoha Cc: dm-devel, Martin Wilck, Alasdair G Kergon, lvm-devel Make the distinction between the cases with and without systemd background jobs explicit in 69-dm-lvm-metad.rules rather than substituting the rule from the Makefile. At this stage, this improves only readibility, at the cost of one GOTO statement. The next patch will add more differences between the two cases (mostly comments), which are practically impossible to generate with the current string subsitution approach. This patch introduces no functional change to the udev rules. Signed-off-by: Martin Wilck <mwilck@suse.com> --- udev/69-dm-lvm-metad.rules.in | 19 ++++++++++++++++++- udev/Makefile.in | 7 ++++--- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/udev/69-dm-lvm-metad.rules.in b/udev/69-dm-lvm-metad.rules.in index bd75fc8..38687f4 100644 --- a/udev/69-dm-lvm-metad.rules.in +++ b/udev/69-dm-lvm-metad.rules.in @@ -88,6 +88,23 @@ LABEL="lvm_scan" # loop | | X | X* | | # other | X | | X | | X ENV{SYSTEMD_READY}="1" -(PVSCAN_RULE) + +# The method for invoking pvscan is selected at build time with the option +# --(enable|disable)-udev-systemd-background-jobs to "configure". +# On modern distributions with recent systemd, it's "systemd_background"; +# on others, "direct_pvscan". +GOTO="(PVSCAN_RULE)" + +LABEL="systemd_background" + +ACTION!="remove", ENV{LVM_PV_GONE}=="1", RUN+="(BINDIR)/systemd-run (LVM_EXEC)/lvm pvscan --cache $major:$minor", GOTO="lvm_end" +ENV{SYSTEMD_ALIAS}="/dev/block/$major:$minor" +ENV{ID_MODEL}="LVM PV $env{ID_FS_UUID_ENC} on /dev/$name" +ENV{SYSTEMD_WANTS}+="lvm2-pvscan@$major:$minor.service" +GOTO="lvm_end" + +LABEL="direct_pvscan" + +RUN+="(LVM_EXEC)/lvm pvscan --background --cache --activate ay --major $major --minor $minor", ENV{LVM_SCANNED}="1" LABEL="lvm_end" diff --git a/udev/Makefile.in b/udev/Makefile.in index c498aa8..9b2e2c3 100644 --- a/udev/Makefile.in +++ b/udev/Makefile.in @@ -25,6 +25,7 @@ endif DM_DIR=$(shell $(GREP) "\#define DM_DIR" $(top_srcdir)/libdm/misc/dm-ioctl.h | $(AWK) '{print $$3}') +BINDIR=@bindir@ ifeq ("@UDEV_RULE_EXEC_DETECTION@", "yes") SBIN=\$$env{DM_SBIN_PATH} DM_EXEC_RULE=ENV{DM_SBIN_PATH}=\"\/sbin\"\\nTEST!=\"\$$env{DM_SBIN_PATH}\/dmsetup\", ENV{DM_SBIN_PATH}=\"\/usr\/sbin\" @@ -46,13 +47,13 @@ BLKID_RULE=IMPORT{program}=\"${SBIN}\/blkid -o udev -p \$$tempnode\" endif ifeq ("@UDEV_SYSTEMD_BACKGROUND_JOBS@", "yes") -PVSCAN_RULE=ACTION\!=\"remove\", ENV{LVM_PV_GONE}==\"1\", RUN\+=\"@bindir@/systemd-run $(LVM_EXEC)\/lvm pvscan --cache \$$major\:\$$minor\", GOTO=\"lvm_end\"\nENV{SYSTEMD_ALIAS}=\"\/dev\/block\/\$$major:\$$minor\"\nENV{ID_MODEL}=\"LVM PV \$$env{ID_FS_UUID_ENC} on \/dev\/\$$name\"\nENV{SYSTEMD_WANTS}\+=\"lvm2-pvscan@\$$major:\$$minor.service\" +PVSCAN_RULE=systemd_background else -PVSCAN_RULE=RUN\+\=\"$(LVM_EXEC)/lvm pvscan --background --cache --activate ay --major \$$major --minor \$$minor\", ENV{LVM_SCANNED}=\"1\" +PVSCAN_RULE=direct_pvscan endif %.rules: $(srcdir)/%.rules.in - $(SED) -e "s+(DM_DIR)+$(DM_DIR)+;s+(BLKID_RULE)+$(BLKID_RULE)+;s+(PVSCAN_RULE)+$(PVSCAN_RULE)+;s+(DM_EXEC_RULE)+$(DM_EXEC_RULE)+;s+(DM_EXEC)+$(DM_EXEC)+;s+(LVM_EXEC_RULE)+$(LVM_EXEC_RULE)+;s+(LVM_EXEC)+$(LVM_EXEC)+;" $< >$@ + $(SED) -e "s+(DM_DIR)+$(DM_DIR)+;s+(BINDIR)+$(BINDIR)+;s+(BLKID_RULE)+$(BLKID_RULE)+;s+(PVSCAN_RULE)+$(PVSCAN_RULE)+;s+(DM_EXEC_RULE)+$(DM_EXEC_RULE)+;s+(DM_EXEC)+$(DM_EXEC)+;s+(LVM_EXEC_RULE)+$(LVM_EXEC_RULE)+;s+(LVM_EXEC)+$(LVM_EXEC)+;" $< >$@ %_install: %.rules $(INSTALL_DATA) -D $< $(udevdir)/$(<F) -- 2.16.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH RESEND 1/2] lvm2: 69-dm-lvm-metad.rules: explicit pvscan rule @ 2018-04-16 18:53 ` Martin Wilck 0 siblings, 0 replies; 10+ messages in thread From: Martin Wilck @ 2018-04-16 18:53 UTC (permalink / raw) To: lvm-devel Make the distinction between the cases with and without systemd background jobs explicit in 69-dm-lvm-metad.rules rather than substituting the rule from the Makefile. At this stage, this improves only readibility, at the cost of one GOTO statement. The next patch will add more differences between the two cases (mostly comments), which are practically impossible to generate with the current string subsitution approach. This patch introduces no functional change to the udev rules. Signed-off-by: Martin Wilck <mwilck@suse.com> --- udev/69-dm-lvm-metad.rules.in | 19 ++++++++++++++++++- udev/Makefile.in | 7 ++++--- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/udev/69-dm-lvm-metad.rules.in b/udev/69-dm-lvm-metad.rules.in index bd75fc8..38687f4 100644 --- a/udev/69-dm-lvm-metad.rules.in +++ b/udev/69-dm-lvm-metad.rules.in @@ -88,6 +88,23 @@ LABEL="lvm_scan" # loop | | X | X* | | # other | X | | X | | X ENV{SYSTEMD_READY}="1" -(PVSCAN_RULE) + +# The method for invoking pvscan is selected at build time with the option +# --(enable|disable)-udev-systemd-background-jobs to "configure". +# On modern distributions with recent systemd, it's "systemd_background"; +# on others, "direct_pvscan". +GOTO="(PVSCAN_RULE)" + +LABEL="systemd_background" + +ACTION!="remove", ENV{LVM_PV_GONE}=="1", RUN+="(BINDIR)/systemd-run (LVM_EXEC)/lvm pvscan --cache $major:$minor", GOTO="lvm_end" +ENV{SYSTEMD_ALIAS}="/dev/block/$major:$minor" +ENV{ID_MODEL}="LVM PV $env{ID_FS_UUID_ENC} on /dev/$name" +ENV{SYSTEMD_WANTS}+="lvm2-pvscan@$major:$minor.service" +GOTO="lvm_end" + +LABEL="direct_pvscan" + +RUN+="(LVM_EXEC)/lvm pvscan --background --cache --activate ay --major $major --minor $minor", ENV{LVM_SCANNED}="1" LABEL="lvm_end" diff --git a/udev/Makefile.in b/udev/Makefile.in index c498aa8..9b2e2c3 100644 --- a/udev/Makefile.in +++ b/udev/Makefile.in @@ -25,6 +25,7 @@ endif DM_DIR=$(shell $(GREP) "\#define DM_DIR" $(top_srcdir)/libdm/misc/dm-ioctl.h | $(AWK) '{print $$3}') +BINDIR=@bindir@ ifeq ("@UDEV_RULE_EXEC_DETECTION@", "yes") SBIN=\$$env{DM_SBIN_PATH} DM_EXEC_RULE=ENV{DM_SBIN_PATH}=\"\/sbin\"\\nTEST!=\"\$$env{DM_SBIN_PATH}\/dmsetup\", ENV{DM_SBIN_PATH}=\"\/usr\/sbin\" @@ -46,13 +47,13 @@ BLKID_RULE=IMPORT{program}=\"${SBIN}\/blkid -o udev -p \$$tempnode\" endif ifeq ("@UDEV_SYSTEMD_BACKGROUND_JOBS@", "yes") -PVSCAN_RULE=ACTION\!=\"remove\", ENV{LVM_PV_GONE}==\"1\", RUN\+=\"@bindir@/systemd-run $(LVM_EXEC)\/lvm pvscan --cache \$$major\:\$$minor\", GOTO=\"lvm_end\"\nENV{SYSTEMD_ALIAS}=\"\/dev\/block\/\$$major:\$$minor\"\nENV{ID_MODEL}=\"LVM PV \$$env{ID_FS_UUID_ENC} on \/dev\/\$$name\"\nENV{SYSTEMD_WANTS}\+=\"lvm2-pvscan@\$$major:\$$minor.service\" +PVSCAN_RULE=systemd_background else -PVSCAN_RULE=RUN\+\=\"$(LVM_EXEC)/lvm pvscan --background --cache --activate ay --major \$$major --minor \$$minor\", ENV{LVM_SCANNED}=\"1\" +PVSCAN_RULE=direct_pvscan endif %.rules: $(srcdir)/%.rules.in - $(SED) -e "s+(DM_DIR)+$(DM_DIR)+;s+(BLKID_RULE)+$(BLKID_RULE)+;s+(PVSCAN_RULE)+$(PVSCAN_RULE)+;s+(DM_EXEC_RULE)+$(DM_EXEC_RULE)+;s+(DM_EXEC)+$(DM_EXEC)+;s+(LVM_EXEC_RULE)+$(LVM_EXEC_RULE)+;s+(LVM_EXEC)+$(LVM_EXEC)+;" $< >$@ + $(SED) -e "s+(DM_DIR)+$(DM_DIR)+;s+(BINDIR)+$(BINDIR)+;s+(BLKID_RULE)+$(BLKID_RULE)+;s+(PVSCAN_RULE)+$(PVSCAN_RULE)+;s+(DM_EXEC_RULE)+$(DM_EXEC_RULE)+;s+(DM_EXEC)+$(DM_EXEC)+;s+(LVM_EXEC_RULE)+$(LVM_EXEC_RULE)+;s+(LVM_EXEC)+$(LVM_EXEC)+;" $< >$@ %_install: %.rules $(INSTALL_DATA) -D $< $(udevdir)/$(<F) -- 2.16.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH RESEND 1/2] lvm2: 69-dm-lvm-metad.rules: explicit pvscan rule 2018-04-16 18:53 ` Martin Wilck @ 2018-04-17 9:46 ` Peter Rajnoha -1 siblings, 0 replies; 10+ messages in thread From: Peter Rajnoha @ 2018-04-17 9:46 UTC (permalink / raw) To: Martin Wilck; +Cc: dm-devel, lvm-devel, Hannes Reinecke, Zdenek Kabelac On 04/16/2018 08:53 PM, Martin Wilck wrote: > Make the distinction between the cases with and without systemd > background jobs explicit in 69-dm-lvm-metad.rules rather than > substituting the rule from the Makefile. At this stage, > this improves only readibility, at the cost of one GOTO statement. > > The next patch will add more differences between the two cases (mostly > comments), which are practically impossible to generate with the current > string subsitution approach. > > This patch introduces no functional change to the udev rules. > > Signed-off-by: Martin Wilck <mwilck@suse.com> > --- > udev/69-dm-lvm-metad.rules.in | 19 ++++++++++++++++++- > udev/Makefile.in | 7 ++++--- > 2 files changed, 22 insertions(+), 4 deletions(-) > OK, I agree, this makes it a bit more readable, applied: https://sourceware.org/git/?p=lvm2.git;a=commit;h=99bfbbf229acf4548f1ffc06625f464dc0ae4ca4 Thanks -- Peter -- lvm-devel mailing list lvm-devel@redhat.com https://www.redhat.com/mailman/listinfo/lvm-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH RESEND 1/2] lvm2: 69-dm-lvm-metad.rules: explicit pvscan rule @ 2018-04-17 9:46 ` Peter Rajnoha 0 siblings, 0 replies; 10+ messages in thread From: Peter Rajnoha @ 2018-04-17 9:46 UTC (permalink / raw) To: lvm-devel On 04/16/2018 08:53 PM, Martin Wilck wrote: > Make the distinction between the cases with and without systemd > background jobs explicit in 69-dm-lvm-metad.rules rather than > substituting the rule from the Makefile. At this stage, > this improves only readibility, at the cost of one GOTO statement. > > The next patch will add more differences between the two cases (mostly > comments), which are practically impossible to generate with the current > string subsitution approach. > > This patch introduces no functional change to the udev rules. > > Signed-off-by: Martin Wilck <mwilck@suse.com> > --- > udev/69-dm-lvm-metad.rules.in | 19 ++++++++++++++++++- > udev/Makefile.in | 7 ++++--- > 2 files changed, 22 insertions(+), 4 deletions(-) > OK, I agree, this makes it a bit more readable, applied: https://sourceware.org/git/?p=lvm2.git;a=commit;h=99bfbbf229acf4548f1ffc06625f464dc0ae4ca4 Thanks -- Peter ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH RESEND 2/2] lvm2: 69-dm-lvm-metad.rules: set systemd vars on "change" 2018-04-16 18:53 ` Martin Wilck @ 2018-04-16 18:53 ` Martin Wilck -1 siblings, 0 replies; 10+ messages in thread From: Martin Wilck @ 2018-04-16 18:53 UTC (permalink / raw) To: Zdenek Kabelac, Peter Rajnoha Cc: dm-devel, Martin Wilck, Alasdair G Kergon, lvm-devel The current logic that avoids setting SYSTEMD_ALIAS and SYSTEMD_WANTS on "change" events is flawed in the default "systemd background job" configuration. For systemd, it's important that device properties don't change spuriously. If an "add" event starts lvm2-pvscan@.service for a device, and a "change" event follows, removing SYSTEMD_ALIAS and SYSTEMD_WANTS from the udev db, information about unit dependencies between the device and the pvscan service can be lost in systemd, in particular if the daemon configuration is reloaded. Steps to reproduce problem: - create a device with an LVM PV - remove device - add device (generates "add" and "change" uevents for the device) (at this point SYSTEMD_ALIAS and SYSTEMD_WANTS are clear in udev db) - systemctl daemon-reload (systemd reloads udev db) - vgchange -a n - remove device => the lvm2-pvscan@.service for the device is still active although the device is gone. - add device again => the PV is not detected, because systemd sees the lvm2-pvscan@.service as active and thus doesn't restart it. The original purpose of this logic was to avoid volumes being scanned over and over again. With systemd background jobs, that isn't necessary, because systemd will not restart the job as long as it's active. Signed-off-by: Martin Wilck <mwilck@suse.com> --- udev/69-dm-lvm-metad.rules.in | 56 +++++++++++++++++++++++++++++++------------ udev/Makefile.in | 4 +++- 2 files changed, 44 insertions(+), 16 deletions(-) diff --git a/udev/69-dm-lvm-metad.rules.in b/udev/69-dm-lvm-metad.rules.in index 38687f4..2ff8ddc 100644 --- a/udev/69-dm-lvm-metad.rules.in +++ b/udev/69-dm-lvm-metad.rules.in @@ -68,25 +68,15 @@ ACTION=="change", ENV{LVM_LOOP_PV_ACTIVATED}!="1", TEST=="loop/backing_file", EN ENV{LVM_LOOP_PV_ACTIVATED}!="1", ENV{SYSTEMD_READY}="0" GOTO="lvm_end" -# If the PV is not a special device listed above, scan only after device addition (ADD event) +# If the PV is not a special device listed above, scan only if necessary. +# For "direct_pvscan" mode (see below), this means run rules only an ADD events. +# For "systemd_background" mode, systemd takes care of this by activating +# the lvm2-pvscan@.service only once. LABEL="next" -ACTION!="add", GOTO="lvm_end" +ACTION!="(PVSCAN_ACTION)", GOTO="lvm_end" LABEL="lvm_scan" -# The table below summarises the situations in which we reach the LABEL="lvm_scan". -# Marked by X, X* means only if the special dev is properly set up. -# The artificial ADD is supported for coldplugging. We avoid running the pvscan -# on artificial CHANGE so there's no unexpected autoactivation when WATCH rule fires. -# N.B. MD and loop never actually reaches lvm_scan on REMOVE as the PV label is gone -# within a CHANGE event (these are caught by the "LVM_PV_GONE" rule at the beginning). -# -# | real ADD | real CHANGE | artificial ADD | artificial CHANGE | REMOVE -# ============================================================================= -# DM | | X | X* | | X -# MD | | X | X* | | -# loop | | X | X* | | -# other | X | | X | | X ENV{SYSTEMD_READY}="1" # The method for invoking pvscan is selected at build time with the option @@ -97,6 +87,27 @@ GOTO="(PVSCAN_RULE)" LABEL="systemd_background" +# The table below summarises the situations in which we reach the LABEL="lvm_scan" +# in the "systemd_background" case. +# Marked by X, X* means only if the special dev is properly set up. +# The artificial ADD is supported for coldplugging. We avoid running the pvscan +# on artificial CHANGE so there's no unexpected autoactivation when WATCH rule fires. +# N.B. MD and loop never actually reaches lvm_scan on REMOVE as the PV label is gone +# within a CHANGE event (these are caught by the "LVM_PV_GONE" rule at the beginning). +# +# In this case, we simply set up the dependency between the device and the pvscan +# job using SYSTEMD_ALIAS (which sets up a simplified device identifier that +# allows using "BindsTo" in the sytemd unit file) and SYSTEMD_WANTS (which tells +# systemd to start the pvscan job once the device is ready). +# We need to set these variables for both "add" and "change" events, otherwise +# systemd may loose information about the device/unit dependencies. +# +# | real ADD | real CHANGE | artificial ADD | artificial CHANGE | REMOVE +# ============================================================================= +# DM | | X | X* | | X +# MD | | X | X* | | +# loop | | X | X* | | +# other | X | X | X | | X ACTION!="remove", ENV{LVM_PV_GONE}=="1", RUN+="(BINDIR)/systemd-run (LVM_EXEC)/lvm pvscan --cache $major:$minor", GOTO="lvm_end" ENV{SYSTEMD_ALIAS}="/dev/block/$major:$minor" ENV{ID_MODEL}="LVM PV $env{ID_FS_UUID_ENC} on /dev/$name" @@ -105,6 +116,21 @@ GOTO="lvm_end" LABEL="direct_pvscan" +# The table below summarises the situations in which we reach the LABEL="lvm_scan" +# for the "direct_pvscan" case. +# Marked by X, X* means only if the special dev is properly set up. +# The artificial ADD is supported for coldplugging. We avoid running the pvscan +# on artificial CHANGE so there's no unexpected autoactivation when WATCH rule fires. +# +# In this case, we need to make sure that pvscan is not invoked spuriously, therefore +# we invoke it only for "add" events for "other" devices. +# +# | real ADD | real CHANGE | artificial ADD | artificial CHANGE | REMOVE +# ============================================================================= +# DM | | X | X* | | X +# MD | | X | X* | | +# loop | | X | X* | | +# other | X | | X | | X RUN+="(LVM_EXEC)/lvm pvscan --background --cache --activate ay --major $major --minor $minor", ENV{LVM_SCANNED}="1" LABEL="lvm_end" diff --git a/udev/Makefile.in b/udev/Makefile.in index 9b2e2c3..6f57d46 100644 --- a/udev/Makefile.in +++ b/udev/Makefile.in @@ -48,12 +48,14 @@ endif ifeq ("@UDEV_SYSTEMD_BACKGROUND_JOBS@", "yes") PVSCAN_RULE=systemd_background +PVSCAN_ACTION=add|change else PVSCAN_RULE=direct_pvscan +PVSCAN_ACTION=add endif %.rules: $(srcdir)/%.rules.in - $(SED) -e "s+(DM_DIR)+$(DM_DIR)+;s+(BINDIR)+$(BINDIR)+;s+(BLKID_RULE)+$(BLKID_RULE)+;s+(PVSCAN_RULE)+$(PVSCAN_RULE)+;s+(DM_EXEC_RULE)+$(DM_EXEC_RULE)+;s+(DM_EXEC)+$(DM_EXEC)+;s+(LVM_EXEC_RULE)+$(LVM_EXEC_RULE)+;s+(LVM_EXEC)+$(LVM_EXEC)+;" $< >$@ + $(SED) -e "s+(DM_DIR)+$(DM_DIR)+;s+(BINDIR)+$(BINDIR)+;s+(BLKID_RULE)+$(BLKID_RULE)+;s+(PVSCAN_RULE)+$(PVSCAN_RULE)+;s+(PVSCAN_ACTION)+$(PVSCAN_ACTION)+;s+(DM_EXEC_RULE)+$(DM_EXEC_RULE)+;s+(DM_EXEC)+$(DM_EXEC)+;s+(LVM_EXEC_RULE)+$(LVM_EXEC_RULE)+;s+(LVM_EXEC)+$(LVM_EXEC)+;" $< >$@ %_install: %.rules $(INSTALL_DATA) -D $< $(udevdir)/$(<F) -- 2.16.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH RESEND 2/2] lvm2: 69-dm-lvm-metad.rules: set systemd vars on "change" @ 2018-04-16 18:53 ` Martin Wilck 0 siblings, 0 replies; 10+ messages in thread From: Martin Wilck @ 2018-04-16 18:53 UTC (permalink / raw) To: lvm-devel The current logic that avoids setting SYSTEMD_ALIAS and SYSTEMD_WANTS on "change" events is flawed in the default "systemd background job" configuration. For systemd, it's important that device properties don't change spuriously. If an "add" event starts lvm2-pvscan at .service for a device, and a "change" event follows, removing SYSTEMD_ALIAS and SYSTEMD_WANTS from the udev db, information about unit dependencies between the device and the pvscan service can be lost in systemd, in particular if the daemon configuration is reloaded. Steps to reproduce problem: - create a device with an LVM PV - remove device - add device (generates "add" and "change" uevents for the device) (at this point SYSTEMD_ALIAS and SYSTEMD_WANTS are clear in udev db) - systemctl daemon-reload (systemd reloads udev db) - vgchange -a n - remove device => the lvm2-pvscan at .service for the device is still active although the device is gone. - add device again => the PV is not detected, because systemd sees the lvm2-pvscan at .service as active and thus doesn't restart it. The original purpose of this logic was to avoid volumes being scanned over and over again. With systemd background jobs, that isn't necessary, because systemd will not restart the job as long as it's active. Signed-off-by: Martin Wilck <mwilck@suse.com> --- udev/69-dm-lvm-metad.rules.in | 56 +++++++++++++++++++++++++++++++------------ udev/Makefile.in | 4 +++- 2 files changed, 44 insertions(+), 16 deletions(-) diff --git a/udev/69-dm-lvm-metad.rules.in b/udev/69-dm-lvm-metad.rules.in index 38687f4..2ff8ddc 100644 --- a/udev/69-dm-lvm-metad.rules.in +++ b/udev/69-dm-lvm-metad.rules.in @@ -68,25 +68,15 @@ ACTION=="change", ENV{LVM_LOOP_PV_ACTIVATED}!="1", TEST=="loop/backing_file", EN ENV{LVM_LOOP_PV_ACTIVATED}!="1", ENV{SYSTEMD_READY}="0" GOTO="lvm_end" -# If the PV is not a special device listed above, scan only after device addition (ADD event) +# If the PV is not a special device listed above, scan only if necessary. +# For "direct_pvscan" mode (see below), this means run rules only an ADD events. +# For "systemd_background" mode, systemd takes care of this by activating +# the lvm2-pvscan at .service only once. LABEL="next" -ACTION!="add", GOTO="lvm_end" +ACTION!="(PVSCAN_ACTION)", GOTO="lvm_end" LABEL="lvm_scan" -# The table below summarises the situations in which we reach the LABEL="lvm_scan". -# Marked by X, X* means only if the special dev is properly set up. -# The artificial ADD is supported for coldplugging. We avoid running the pvscan -# on artificial CHANGE so there's no unexpected autoactivation when WATCH rule fires. -# N.B. MD and loop never actually reaches lvm_scan on REMOVE as the PV label is gone -# within a CHANGE event (these are caught by the "LVM_PV_GONE" rule at the beginning). -# -# | real ADD | real CHANGE | artificial ADD | artificial CHANGE | REMOVE -# ============================================================================= -# DM | | X | X* | | X -# MD | | X | X* | | -# loop | | X | X* | | -# other | X | | X | | X ENV{SYSTEMD_READY}="1" # The method for invoking pvscan is selected at build time with the option @@ -97,6 +87,27 @@ GOTO="(PVSCAN_RULE)" LABEL="systemd_background" +# The table below summarises the situations in which we reach the LABEL="lvm_scan" +# in the "systemd_background" case. +# Marked by X, X* means only if the special dev is properly set up. +# The artificial ADD is supported for coldplugging. We avoid running the pvscan +# on artificial CHANGE so there's no unexpected autoactivation when WATCH rule fires. +# N.B. MD and loop never actually reaches lvm_scan on REMOVE as the PV label is gone +# within a CHANGE event (these are caught by the "LVM_PV_GONE" rule at the beginning). +# +# In this case, we simply set up the dependency between the device and the pvscan +# job using SYSTEMD_ALIAS (which sets up a simplified device identifier that +# allows using "BindsTo" in the sytemd unit file) and SYSTEMD_WANTS (which tells +# systemd to start the pvscan job once the device is ready). +# We need to set these variables for both "add" and "change" events, otherwise +# systemd may loose information about the device/unit dependencies. +# +# | real ADD | real CHANGE | artificial ADD | artificial CHANGE | REMOVE +# ============================================================================= +# DM | | X | X* | | X +# MD | | X | X* | | +# loop | | X | X* | | +# other | X | X | X | | X ACTION!="remove", ENV{LVM_PV_GONE}=="1", RUN+="(BINDIR)/systemd-run (LVM_EXEC)/lvm pvscan --cache $major:$minor", GOTO="lvm_end" ENV{SYSTEMD_ALIAS}="/dev/block/$major:$minor" ENV{ID_MODEL}="LVM PV $env{ID_FS_UUID_ENC} on /dev/$name" @@ -105,6 +116,21 @@ GOTO="lvm_end" LABEL="direct_pvscan" +# The table below summarises the situations in which we reach the LABEL="lvm_scan" +# for the "direct_pvscan" case. +# Marked by X, X* means only if the special dev is properly set up. +# The artificial ADD is supported for coldplugging. We avoid running the pvscan +# on artificial CHANGE so there's no unexpected autoactivation when WATCH rule fires. +# +# In this case, we need to make sure that pvscan is not invoked spuriously, therefore +# we invoke it only for "add" events for "other" devices. +# +# | real ADD | real CHANGE | artificial ADD | artificial CHANGE | REMOVE +# ============================================================================= +# DM | | X | X* | | X +# MD | | X | X* | | +# loop | | X | X* | | +# other | X | | X | | X RUN+="(LVM_EXEC)/lvm pvscan --background --cache --activate ay --major $major --minor $minor", ENV{LVM_SCANNED}="1" LABEL="lvm_end" diff --git a/udev/Makefile.in b/udev/Makefile.in index 9b2e2c3..6f57d46 100644 --- a/udev/Makefile.in +++ b/udev/Makefile.in @@ -48,12 +48,14 @@ endif ifeq ("@UDEV_SYSTEMD_BACKGROUND_JOBS@", "yes") PVSCAN_RULE=systemd_background +PVSCAN_ACTION=add|change else PVSCAN_RULE=direct_pvscan +PVSCAN_ACTION=add endif %.rules: $(srcdir)/%.rules.in - $(SED) -e "s+(DM_DIR)+$(DM_DIR)+;s+(BINDIR)+$(BINDIR)+;s+(BLKID_RULE)+$(BLKID_RULE)+;s+(PVSCAN_RULE)+$(PVSCAN_RULE)+;s+(DM_EXEC_RULE)+$(DM_EXEC_RULE)+;s+(DM_EXEC)+$(DM_EXEC)+;s+(LVM_EXEC_RULE)+$(LVM_EXEC_RULE)+;s+(LVM_EXEC)+$(LVM_EXEC)+;" $< >$@ + $(SED) -e "s+(DM_DIR)+$(DM_DIR)+;s+(BINDIR)+$(BINDIR)+;s+(BLKID_RULE)+$(BLKID_RULE)+;s+(PVSCAN_RULE)+$(PVSCAN_RULE)+;s+(PVSCAN_ACTION)+$(PVSCAN_ACTION)+;s+(DM_EXEC_RULE)+$(DM_EXEC_RULE)+;s+(DM_EXEC)+$(DM_EXEC)+;s+(LVM_EXEC_RULE)+$(LVM_EXEC_RULE)+;s+(LVM_EXEC)+$(LVM_EXEC)+;" $< >$@ %_install: %.rules $(INSTALL_DATA) -D $< $(udevdir)/$(<F) -- 2.16.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH RESEND 2/2] lvm2: 69-dm-lvm-metad.rules: set systemd vars on "change" 2018-04-16 18:53 ` Martin Wilck @ 2018-04-17 9:48 ` Peter Rajnoha -1 siblings, 0 replies; 10+ messages in thread From: Peter Rajnoha @ 2018-04-17 9:48 UTC (permalink / raw) To: Martin Wilck; +Cc: dm-devel, lvm-devel, Hannes Reinecke, Zdenek Kabelac On 04/16/2018 08:53 PM, Martin Wilck wrote: > The current logic that avoids setting SYSTEMD_ALIAS and SYSTEMD_WANTS > on "change" events is flawed in the default "systemd background job" > configuration. For systemd, it's important that device properties don't > change spuriously. > > If an "add" event starts lvm2-pvscan@.service for a device, and a > "change" event follows, removing SYSTEMD_ALIAS and SYSTEMD_WANTS from the > udev db, information about unit dependencies between the device and the > pvscan service can be lost in systemd, in particular if the daemon > configuration is reloaded. > > Steps to reproduce problem: > > - create a device with an LVM PV > - remove device > - add device (generates "add" and "change" uevents for the device) > (at this point SYSTEMD_ALIAS and SYSTEMD_WANTS are clear in udev db) > - systemctl daemon-reload > (systemd reloads udev db) > - vgchange -a n > - remove device > > => the lvm2-pvscan@.service for the device is still active although the > device is gone. > > - add device again > > => the PV is not detected, because systemd sees the lvm2-pvscan@.service > as active and thus doesn't restart it. > > The original purpose of this logic was to avoid volumes being scanned > over and over again. With systemd background jobs, that isn't necessary, > because systemd will not restart the job as long as it's active. > > Signed-off-by: Martin Wilck <mwilck@suse.com> > --- > udev/69-dm-lvm-metad.rules.in | 56 +++++++++++++++++++++++++++++++------------ > udev/Makefile.in | 4 +++- > 2 files changed, 44 insertions(+), 16 deletions(-) > Thanks for the patch! I wasn't aware that systemd is reading those variables again from udev db on reload - nice catch! Applied: https://sourceware.org/git/?p=lvm2.git;a=commit;h=7a7b8a7778aace88c967ee8285485c494ce1f3f8 -- Peter -- lvm-devel mailing list lvm-devel@redhat.com https://www.redhat.com/mailman/listinfo/lvm-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH RESEND 2/2] lvm2: 69-dm-lvm-metad.rules: set systemd vars on "change" @ 2018-04-17 9:48 ` Peter Rajnoha 0 siblings, 0 replies; 10+ messages in thread From: Peter Rajnoha @ 2018-04-17 9:48 UTC (permalink / raw) To: lvm-devel On 04/16/2018 08:53 PM, Martin Wilck wrote: > The current logic that avoids setting SYSTEMD_ALIAS and SYSTEMD_WANTS > on "change" events is flawed in the default "systemd background job" > configuration. For systemd, it's important that device properties don't > change spuriously. > > If an "add" event starts lvm2-pvscan at .service for a device, and a > "change" event follows, removing SYSTEMD_ALIAS and SYSTEMD_WANTS from the > udev db, information about unit dependencies between the device and the > pvscan service can be lost in systemd, in particular if the daemon > configuration is reloaded. > > Steps to reproduce problem: > > - create a device with an LVM PV > - remove device > - add device (generates "add" and "change" uevents for the device) > (at this point SYSTEMD_ALIAS and SYSTEMD_WANTS are clear in udev db) > - systemctl daemon-reload > (systemd reloads udev db) > - vgchange -a n > - remove device > > => the lvm2-pvscan at .service for the device is still active although the > device is gone. > > - add device again > > => the PV is not detected, because systemd sees the lvm2-pvscan at .service > as active and thus doesn't restart it. > > The original purpose of this logic was to avoid volumes being scanned > over and over again. With systemd background jobs, that isn't necessary, > because systemd will not restart the job as long as it's active. > > Signed-off-by: Martin Wilck <mwilck@suse.com> > --- > udev/69-dm-lvm-metad.rules.in | 56 +++++++++++++++++++++++++++++++------------ > udev/Makefile.in | 4 +++- > 2 files changed, 44 insertions(+), 16 deletions(-) > Thanks for the patch! I wasn't aware that systemd is reading those variables again from udev db on reload - nice catch! Applied: https://sourceware.org/git/?p=lvm2.git;a=commit;h=7a7b8a7778aace88c967ee8285485c494ce1f3f8 -- Peter ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-04-17 9:48 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-16 18:53 [PATCH RESEND 0/2] LVM2: fix lvmetad udev rules for CHANGE events Martin Wilck 2018-04-16 18:53 ` Martin Wilck 2018-04-16 18:53 ` [PATCH RESEND 1/2] lvm2: 69-dm-lvm-metad.rules: explicit pvscan rule Martin Wilck 2018-04-16 18:53 ` Martin Wilck 2018-04-17 9:46 ` Peter Rajnoha 2018-04-17 9:46 ` Peter Rajnoha 2018-04-16 18:53 ` [PATCH RESEND 2/2] lvm2: 69-dm-lvm-metad.rules: set systemd vars on "change" Martin Wilck 2018-04-16 18:53 ` Martin Wilck 2018-04-17 9:48 ` Peter Rajnoha 2018-04-17 9:48 ` Peter Rajnoha
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.