All of lore.kernel.org
 help / color / mirror / Atom feed
From: Olaf Hering <olaf@aepfle.de>
To: xen-devel@lists.xenproject.org
Cc: Olaf Hering <olaf@aepfle.de>, Ian Jackson <iwj@xenproject.org>,
	Wei Liu <wl@xen.org>
Subject: [PATCH v20210616 01/36] hotplug/Linux: fix starting of xenstored with restarting systemd
Date: Wed, 16 Jun 2021 14:50:54 +0200	[thread overview]
Message-ID: <20210616125129.26563-2-olaf@aepfle.de> (raw)
In-Reply-To: <20210616125129.26563-1-olaf@aepfle.de>

A hard to trigger race with another unrelated systemd service and
xenstored.service unveiled a bug in the way how xenstored is launched
with systemd.

launch-xenstore may start either a daemon or a domain. In case a domain
is used, systemd-notify was called. If another service triggered a
restart of systemd while xenstored.service was executed, systemd may
temporary lose track of services with Type=notify. As a result,
xenstored.service would be marked as failed and units that depend on it
will not be started. This breaks the enire Xen toolstack.

The chain of events is basically: xenstored.service sends the
notification to systemd, this is a one-way event. Then systemd may be
restarted by the other unit. During this time, xenstored.service is done
and exits. Once systemd is done with its restart, it collects the pending
notifications and childs. If it does not find the unit which sent the
notification it will declare it as failed.

A workaround for this scenario is to leave the child processes running
for a short time after sending the "READY=1" notification. If systemd
happens to restart it will still find the unit it launched.

Adjust the callers of launch-xenstore to specifiy the init system:
Do not fork xenstored with systemd, preserve pid. This wil also avoid
the need for a sleep because the process which sent the "READY=1" (the
previously forked child) is still alive.

Remove the --pid-file in the systemd case because the pid of the child
is known, and the file had probably little effect anyway due to lack of
PidFile= and Type=forking in the unit file.

Be verbose about xenstored startup only with sysv to avoid interleaved
output in systemd journal. Do the same also for domain case, even if is
not strictly needed because init-xenstore-domain has no output.

The fix for upstream systemd which is supposed to fix it:
575b300b795b6 ("pid1: rework how we dispatch SIGCHLD and other signals")

Signed-off-by: Olaf Hering <olaf@aepfle.de>

--
v04:
- do mkdir unconditionally because init-xenstore-domain writes the domid to
  xenstored.pid
v03:
- remove run_xenstored function, follow style of shell built-in test function
v02:
- preserve Type=notify
---
 tools/hotplug/Linux/init.d/xencommons.in      |  2 +-
 tools/hotplug/Linux/launch-xenstore.in        | 40 ++++++++++++++-----
 .../Linux/systemd/xenstored.service.in        |  2 +-
 3 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/tools/hotplug/Linux/init.d/xencommons.in b/tools/hotplug/Linux/init.d/xencommons.in
index 7fd6903b98..dcb0ce4b73 100644
--- a/tools/hotplug/Linux/init.d/xencommons.in
+++ b/tools/hotplug/Linux/init.d/xencommons.in
@@ -60,7 +60,7 @@ do_start () {
 	mkdir -m700 -p ${XEN_LOCK_DIR}
 	mkdir -p ${XEN_LOG_DIR}
 
-	@XEN_SCRIPT_DIR@/launch-xenstore || exit 1
+	@XEN_SCRIPT_DIR@/launch-xenstore 'sysv' || exit 1
 
 	echo Setting domain 0 name, domid and JSON config...
 	${LIBEXEC_BIN}/xen-init-dom0 ${XEN_DOM0_UUID}
diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in
index 019f9d6f4d..d40c66482a 100644
--- a/tools/hotplug/Linux/launch-xenstore.in
+++ b/tools/hotplug/Linux/launch-xenstore.in
@@ -15,6 +15,17 @@
 # License along with this library; If not, see <http://www.gnu.org/licenses/>.
 #
 
+initd=$1
+
+case "$initd" in
+	sysv) nonl='-n' ;;
+	systemd) nonl= ;;
+	*)
+	echo "first argument must be 'sysv' or 'systemd'"
+	exit 1
+	;;
+esac
+
 XENSTORED=@XENSTORED@
 
 . @XEN_SCRIPT_DIR@/hotplugpath.sh
@@ -44,14 +55,16 @@ timeout_xenstore () {
 	return 0
 }
 
-test_xenstore && exit 0
+mkdir -p @XEN_RUN_DIR@
+
+if test "$initd" = 'sysv' ; then
+	test_xenstore && exit 0
+fi
 
 test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
 
 [ "$XENSTORETYPE" = "" ] && XENSTORETYPE=daemon
 
-/bin/mkdir -p @XEN_RUN_DIR@
-
 [ "$XENSTORETYPE" = "daemon" ] && {
 	[ -z "$XENSTORED_TRACE" ] || XENSTORED_ARGS="$XENSTORED_ARGS -T @XEN_LOG_DIR@/xenstored-trace.log"
 	[ -z "$XENSTORED" ] && XENSTORED=@XENSTORED@
@@ -59,13 +72,15 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF
 		echo "No xenstored found"
 		exit 1
 	}
+	[ "$initd" = 'sysv' ] && {
+		echo $nonl Starting $XENSTORED...
+		$XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS
+		timeout_xenstore $XENSTORED || exit 1
+		exit 0
+	}
 
-	echo -n Starting $XENSTORED...
-	$XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS
-
-	systemd-notify --booted 2>/dev/null || timeout_xenstore $XENSTORED || exit 1
-
-	exit 0
+	exec $XENSTORED -N $XENSTORED_ARGS
+	exit 1
 }
 
 [ "$XENSTORETYPE" = "domain" ] && {
@@ -75,9 +90,12 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF
 	XENSTORE_DOMAIN_ARGS="$XENSTORE_DOMAIN_ARGS --memory $XENSTORE_DOMAIN_SIZE"
 	[ -z "$XENSTORE_MAX_DOMAIN_SIZE" ] || XENSTORE_DOMAIN_ARGS="$XENSTORE_DOMAIN_ARGS --maxmem $XENSTORE_MAX_DOMAIN_SIZE"
 
-	echo -n Starting $XENSTORE_DOMAIN_KERNEL...
+	echo $nonl Starting $XENSTORE_DOMAIN_KERNEL...
 	${LIBEXEC_BIN}/init-xenstore-domain $XENSTORE_DOMAIN_ARGS || exit 1
-	systemd-notify --ready 2>/dev/null
+	[ "$initd" = 'systemd' ] && {
+		systemd-notify --ready
+		sleep 9
+	}
 
 	exit 0
 }
diff --git a/tools/hotplug/Linux/systemd/xenstored.service.in b/tools/hotplug/Linux/systemd/xenstored.service.in
index 80c1d408a5..c226eb3635 100644
--- a/tools/hotplug/Linux/systemd/xenstored.service.in
+++ b/tools/hotplug/Linux/systemd/xenstored.service.in
@@ -11,7 +11,7 @@ Type=notify
 NotifyAccess=all
 RemainAfterExit=true
 ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities
-ExecStart=@XEN_SCRIPT_DIR@/launch-xenstore
+ExecStart=@XEN_SCRIPT_DIR@/launch-xenstore 'systemd'
 
 [Install]
 WantedBy=multi-user.target


  reply	other threads:[~2021-06-16 12:51 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-16 12:50 [PATCH v20210616 00/36] leftover from 2020 Olaf Hering
2021-06-16 12:50 ` Olaf Hering [this message]
2021-06-16 12:50 ` [PATCH v20210616 02/36] tools: add API to work with sevaral bits at once Olaf Hering
2021-06-16 12:50 ` [PATCH v20210616 03/36] xl: fix description of migrate --debug Olaf Hering
2021-06-16 12:50 ` [PATCH v20210616 04/36] tools: create libxensaverestore Olaf Hering
2021-06-16 15:17   ` Olaf Hering
2021-06-16 12:50 ` [PATCH v20210616 05/36] MAINTAINERS: add myself as saverestore maintainer Olaf Hering
2021-06-16 12:50 ` [PATCH v20210616 06/36] tools: add readv_exact to libxenctrl Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 07/36] tools: add sr_is_known_page_type to libsaverestore Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 08/36] tools: use sr_is_known_page_type Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 09/36] tools: unify type checking for data pfns in migration stream Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 10/36] tools: show migration transfer rate in send_dirty_pages Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 11/36] tools: prepare to allocate saverestore arrays once Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 12/36] tools: save: move mfns array Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 13/36] tools: save: move types array Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 14/36] tools: save: move errors array Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 15/36] tools: save: move iov array Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 16/36] tools: save: move rec_pfns array Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 17/36] tools: save: move guest_data array Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 18/36] tools: save: move local_pages array Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 19/36] tools: restore: move types array Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 20/36] tools: restore: move mfns array Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 21/36] tools: restore: move map_errs array Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 22/36] tools: restore: move mfns array in populate_pfns Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 23/36] tools: restore: move pfns " Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 24/36] tools: restore: split record processing Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 25/36] tools: restore: split handle_page_data Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 26/36] tools: restore: write data directly into guest Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 27/36] tools: recognize LIBXL_API_VERSION for 4.16 Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 28/36] tools: adjust libxl_domain_suspend to receive a struct props Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 29/36] tools: change struct precopy_stats to precopy_stats_t Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 30/36] tools: add callback to libxl for precopy_policy and precopy_stats_t Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 31/36] tools: add --max_iters to libxl_domain_suspend Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 32/36] tools: add --min_remaining " Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 33/36] tools: add --abort_if_busy " Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 34/36] tools: add API for expandable bitmaps Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 35/36] tools: use sr_bitmap for populated_pfns Olaf Hering
2021-06-16 12:51 ` [PATCH v20210616 36/36] tools: use superpages during restore of HVM guest Olaf Hering
2021-06-16 14:50 ` [PATCH v20210616 00/36] leftover from 2020 Andrew Cooper
2021-06-16 15:02   ` Olaf Hering
2021-06-17 11:02     ` Julien Grall
2021-06-17 17:40       ` Olaf Hering
2021-06-16 15:38   ` Olaf Hering
2021-06-17 11:24     ` Andrew Cooper
2021-06-17 14:55       ` Olaf Hering
2021-06-17 15:05         ` Andrew Cooper

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210616125129.26563-2-olaf@aepfle.de \
    --to=olaf@aepfle.de \
    --cc=iwj@xenproject.org \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    --subject='Re: [PATCH v20210616 01/36] hotplug/Linux: fix starting of xenstored with restarting systemd' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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.