Hi, This is a rather complex series with changes that aren’t exactly local to the mirror job, so maybe it’s too complex for 6.1.
However, it is a bug fix, and not an insignificant one, though probably not a regression of any kind. Bug report: https://gitlab.com/qemu-project/qemu/-/issues/462 (I didn’t put any “Fixes:” or “Resolves:” into the commit messages, because there is no single patch here that fixes the bug.) The root of the problem is that if you cancel a mirror job during its READY phase, any kind of I/O error (with the error action 'stop') is likely to not be handled gracefully, which means that perhaps the job will just loop forever without pausing, doing nothing but emitting errors. There is no way to stop the job, or cancel it with force=true, and so you also cannot quit qemu normally, because, well, cancelling the job doesn’t do anything. So you have to kill qemu to stop the mess. If you’re lucky, the error is transient. Then qemu will just kill itself with a failed assertion, because it’ll try a READY -> READY transition, which isn’t allowed. There are a couple of problems contributing to it all: (1) The READY -> READY transition comes from the fact that we will enter the READY state whenever source and target are synced, and whenever s->synced is false. I/O errors reset s->synced. I believe they shouldn’t. (Patch 1) (2) Quitting qemu doesn’t force-cancel jobs. I don’t understand why. If for all jobs but mirror we want them to be cancelled and not properly completed, why do we want mirror to get a consistent result? (Which is what cancel with force=false gives you.) I believe we actually don’t care, and so on many occasions where we invoke job_cancel_sync() and job_cancel_sync_all(), we want to force-cancel the job(s) in question. (Patch 2) (3) Cancelling mirror post-READY with force=false is actually not really cancelling the job. It’s a different completion mode. The job should run like any normal job, it shouldn’t be special-cased. However, we have a couple of places that special-case cancelled job because they believe that such jobs are on their way to definite termination. For example, we don’t allow pausing cancelled jobs. We definitely do want to allow pausing a mirror post-READY job that is being non-force-cancelled. The job may still take an arbitrary amount of time, so it absolutely should be pausable. (Patches 3, 4) (4) Mirror only checks whether it’s been force-cancelled at the bottom of its main loop, after several `continue`s. Therefore, if flushing fails (and it then `continue`s), that check will be skipped. If flushing fails continuously, the job cannot be force-cancelled. (Patch 5) Max Reitz (6): mirror: Keep s->synced on error job: @force parameter for job_cancel_sync{,_all}() jobs: Give Job.force_cancel more meaning job: Add job_cancel_requested() mirror: Check job_is_cancelled() earlier iotests: Add mirror-ready-cancel-error test include/qemu/job.h | 29 +++- block/backup.c | 3 +- block/mirror.c | 35 +++-- block/replication.c | 4 +- blockdev.c | 4 +- job.c | 46 ++++-- qemu-nbd.c | 2 +- softmmu/runstate.c | 2 +- storage-daemon/qemu-storage-daemon.c | 2 +- tests/unit/test-block-iothread.c | 2 +- tests/unit/test-blockjob.c | 2 +- tests/qemu-iotests/109.out | 60 +++----- .../tests/mirror-ready-cancel-error | 143 ++++++++++++++++++ .../tests/mirror-ready-cancel-error.out | 5 + tests/qemu-iotests/tests/qsd-jobs.out | 2 +- 15 files changed, 262 insertions(+), 79 deletions(-) create mode 100755 tests/qemu-iotests/tests/mirror-ready-cancel-error create mode 100644 tests/qemu-iotests/tests/mirror-ready-cancel-error.out -- 2.31.1
