[PATCH] storage-daemon: Call job_cancel_sync_all() on shutdown

[PATCH] storage-daemon: Call job_cancel_sync_all() on shutdown

[Kevin Wolf]

bdrv_close_all() asserts that no jobs are running any more, so we need
to cancel all jobs first to avoid failing the assertion.

Fixes: b55a3c8860b763b62b2cc2f4a6f55379977bbde5
Reported-by: Nini Gu <ngu@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 storage-daemon/qemu-storage-daemon.c  |  1 +
 tests/qemu-iotests/tests/qsd-jobs     | 66 +++++++++++++++++++++++++++
 tests/qemu-iotests/tests/qsd-jobs.out | 22 +++++++++
 3 files changed, 89 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/qsd-jobs
 create mode 100644 tests/qemu-iotests/tests/qsd-jobs.out

diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
index 23756fc8e5..a39a22386a 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -368,6 +368,7 @@ int main(int argc, char *argv[])
 
     blk_exp_close_all();
     bdrv_drain_all_begin();
+    job_cancel_sync_all();
     bdrv_close_all();
 
     monitor_cleanup();
diff --git a/tests/qemu-iotests/tests/qsd-jobs b/tests/qemu-iotests/tests/qsd-jobs
new file mode 100755
index 0000000000..1a1c534fac
--- /dev/null
+++ b/tests/qemu-iotests/tests/qsd-jobs
@@ -0,0 +1,66 @@
+#!/usr/bin/env bash
+# group: rw auto quick qsd
+#
+# Job tests related specifically to qemu-storage-daemon
+#
+# Copyright (C) 2021 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=kwolf@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+status=1	# failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+cd ..
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto generic
+
+size=128M
+
+TEST_IMG="$TEST_IMG.base" _make_test_img $size
+_make_test_img -b "$TEST_IMG.base" -F $IMGFMT
+
+echo
+echo "=== Job still present at shutdown ==="
+echo
+
+# Just make sure that this doesn't crash
+$QSD --chardev stdio,id=stdio --monitor chardev=stdio \
+    --blockdev node-name=file0,driver=file,filename="$TEST_IMG" \
+    --blockdev node-name=fmt0,driver=qcow2,file=file0 <<EOF | _filter_qmp
+{"execute":"qmp_capabilities"}
+{"execute": "block-commit", "arguments": {"device": "fmt0", "job-id": "job0"}}
+{"execute": "quit"}
+EOF
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
+
diff --git a/tests/qemu-iotests/tests/qsd-jobs.out b/tests/qemu-iotests/tests/qsd-jobs.out
new file mode 100644
index 0000000000..e3a684b34d
--- /dev/null
+++ b/tests/qemu-iotests/tests/qsd-jobs.out
@@ -0,0 +1,22 @@
+QA output created by qsd-jobs
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=134217728
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
+
+=== Job still present at shutdown ===
+
+QMP_VERSION
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "job0"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "job0"}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "job0"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "job0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "standby", "id": "job0"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "job0"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "waiting", "id": "job0"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "pending", "id": "job0"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "job0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "job0"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "job0"}}
+*** done
-- 
2.29.2

Re: [PATCH] storage-daemon: Call job_cancel_sync_all() on shutdown

[Eric Blake]

On 3/9/21 6:18 AM, Kevin Wolf wrote:
> bdrv_close_all() asserts that no jobs are running any more, so we need
> to cancel all jobs first to avoid failing the assertion.
> 
> Fixes: b55a3c8860b763b62b2cc2f4a6f55379977bbde5
> Reported-by: Nini Gu <ngu@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  storage-daemon/qemu-storage-daemon.c  |  1 +
>  tests/qemu-iotests/tests/qsd-jobs     | 66 +++++++++++++++++++++++++++
>  tests/qemu-iotests/tests/qsd-jobs.out | 22 +++++++++
>  3 files changed, 89 insertions(+)
>  create mode 100755 tests/qemu-iotests/tests/qsd-jobs
>  create mode 100644 tests/qemu-iotests/tests/qsd-jobs.out

Reviewed-by: Eric Blake <eblake@redhat.com>

> +++ b/tests/qemu-iotests/tests/qsd-jobs

Yay for nicer iotest names ;)

> +seq="$(basename $0)"
> +echo "QA output created by $seq"

$seq is a bit of a misnomer, now, but such is life.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org