[Devel] [PATCH hci-8.1 0/2] dm-ploop, dm-qcow2: fix merge status shared by all devices

[Devel] [PATCH hci-8.1 0/2] dm-ploop, dm-qcow2: fix merge status shared by all devices

[Denis V. Lunev]

dm-ploop and dm-qcow2 collect the status of their merge service I/O in
a function-local static, so every device on the host shares one copy.

In dm-ploop it is never reset: after a single failed fake merge pio,
every later online merge on any ploop device stops after its first pio
and returns that old error until the module is reloaded, which on a
working node means a reboot.

dm-qcow2 resets it when each run starts, which makes it worse: a merge
or L1 COW break starting on one device wipes the error another device
has just recorded. That merge then reports success although a write
failed, userspace completes it and drops the top image, and the data
that never reached the lower image is gone.

The status goes on the stack. The pios/qios carry a pointer to it, so
both loops must not return while one is outstanding: every exit
already waits for the counter to drain, and a BUG_ON after the wait
stops the kernel rather than let a late pio/qio write into a dead
stack frame if that ever changes.

Tested on dev-cl04, 6.12.0-211.52.1.22.9.vz10 with the modules built
out of tree, using test-only module knobs to fail one service pio/qio:
- dm-ploop: after one injected failure, a merge on another device and a
  second merge on the same device both failed with EIO; with the fix
  both succeed;
- dm-qcow2: a backward merge with an injected qio error, held for 5 s
  while a merge on a second device started, returned success; with the
  fix it fails with "Can't break L1 COW";
- the ploop pytest suite, ploop and qcow2 formats, passes.

Denis V. Lunev (2):
  dm-ploop: fix a failed merge failing every later merge on the host
  dm-qcow2: fix a merge reporting success after a failed write

 drivers/md/dm-ploop-cmd.c | 4 +++-
 drivers/md/dm-qcow2-cmd.c | 6 +++---
 2 files changed, 6 insertions(+), 4 deletions(-)


base-commit: 02401c5e378f282fbf11bdc65996253ff65dad9b
-- 
2.53.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH hci-8.1 1/2] dm-ploop: fix a failed merge failing every later merge on the host

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

ploop_process_merge_latest_snapshot() collects the status of its fake
merge pios in a function-local static. It is never reset and it is
shared by every ploop device, so once a single pio fails, every later
merge on any device stops after its first pio and returns that old
error, until the module is reloaded. A failure on one device also
aborts a merge running on another.

Keep the status on the stack. The pios carry a pointer to it, so the
function must not return while one of them is outstanding: every exit
already waits for service_pios to drain, and a BUG_ON after the wait
stops the kernel rather than let a late pio write into a dead stack
frame if that ever changes.

Fixes: 6ddad5a4d226 ("dm-ploop: Add ploop target driver")

Signed-off-by: Denis V. Lunev <den@openvz.org>

Feature: dm-ploop: ploop target driver
---
 drivers/md/dm-ploop-cmd.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-ploop-cmd.c b/drivers/md/dm-ploop-cmd.c
index 5fa78e6cff9a..9084ca37f45b 100644
--- a/drivers/md/dm-ploop-cmd.c
+++ b/drivers/md/dm-ploop-cmd.c
@@ -671,7 +671,7 @@ static void service_pio_endio(struct pio *pio, void *data, blk_status_t status)
 
 static int ploop_process_merge_latest_snapshot(struct ploop *ploop)
 {
-	static blk_status_t service_status;
+	blk_status_t service_status = BLK_STS_OK;
 	struct bio_vec bvec = {0};
 	struct pio *pio;
 	int ret = 0;
@@ -711,6 +711,8 @@ static int ploop_process_merge_latest_snapshot(struct ploop *ploop)
 	}
 
 	wait_event(ploop->service_wq, !atomic_read(&ploop->service_pios));
+	/* The pios point at service_status on our stack */
+	BUG_ON(atomic_read(&ploop->service_pios));
 	if (!ret) {
 		spin_lock_irq(&ploop->err_status_lock);
 		ret = blk_status_to_errno(service_status);
-- 
2.53.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH hci-8.1 2/2] dm-qcow2: fix a merge reporting success after a failed write

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

qcow2_service_iter() collects the status of its service qios in a
function-local static shared by every dm-qcow2 device, and resets it
to OK when it starts. A merge or L1 COW break starting on one device
while another one runs wipes the error the other one recorded. That
one then finishes and reports success although a qio failed, so
userspace may complete the merge and drop the top image, losing data
that never reached the lower image.

Keep the status on the stack. The qios carry a pointer to it, so the
function must not return while one of them is outstanding: every exit
already waits for service_qios to drain, and a BUG_ON after the wait
stops the kernel rather than let a late qio write into a dead stack
frame if that ever changes.

Fixes: 222b6f1291c1 ("dm-qcow2: Introduce driver to create block devices over QCOW2 files")

Signed-off-by: Denis V. Lunev <den@openvz.org>

Feature: dm-qcow2: block device over QCOW2 files driver
---
 drivers/md/dm-qcow2-cmd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm-qcow2-cmd.c b/drivers/md/dm-qcow2-cmd.c
index 2865940cff30..c7e77677d8e6 100644
--- a/drivers/md/dm-qcow2-cmd.c
+++ b/drivers/md/dm-qcow2-cmd.c
@@ -65,13 +65,11 @@ static bool qcow2_backward_merge_should_stop(struct qcow2_target *tgt);
 static int qcow2_service_iter(struct qcow2_target *tgt, struct qcow2 *qcow2,
 		  loff_t end, loff_t step, unsigned int bi_op, u8 qio_flags)
 {
-	static blk_status_t service_status;
+	blk_status_t service_status = BLK_STS_OK;
 	struct qio *qio;
 	int ret = 0;
 	loff_t pos;
 
-	WRITE_ONCE(service_status, BLK_STS_OK);
-
 	backward_merge_update_max_progress(tgt, end);
 	for (pos = 0; pos < end; pos += step) {
 		backward_merge_update_progress(tgt, pos);
@@ -106,6 +104,8 @@ static int qcow2_service_iter(struct qcow2_target *tgt, struct qcow2 *qcow2,
 	}
 
 	wait_event(tgt->service_wq, !atomic_read(&tgt->service_qios));
+	/* The qios point at service_status on our stack */
+	BUG_ON(atomic_read(&tgt->service_qios));
 	if (!ret) {
 		smp_rmb(); /* Pairs with smp_wmb() in service_qio_endio() */
 		ret = blk_status_to_errno(READ_ONCE(service_status));
-- 
2.53.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 0/2] dm-ploop, dm-qcow2: fix merge status shared by all devices

[Andrey Zhadchenko]

Patches are great but BUG_ON chunks are completely useless. Let's not 
add junk. Remove them please.

Other than that,
Reviewed-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>

On 10/27/26 18:49, Denis V. Lunev wrote:
> dm-ploop and dm-qcow2 collect the status of their merge service I/O in
> a function-local static, so every device on the host shares one copy.
> 
> In dm-ploop it is never reset: after a single failed fake merge pio,
> every later online merge on any ploop device stops after its first pio
> and returns that old error until the module is reloaded, which on a
> working node means a reboot.
> 
> dm-qcow2 resets it when each run starts, which makes it worse: a merge
> or L1 COW break starting on one device wipes the error another device
> has just recorded. That merge then reports success although a write
> failed, userspace completes it and drops the top image, and the data
> that never reached the lower image is gone.
> 
> The status goes on the stack. The pios/qios carry a pointer to it, so
> both loops must not return while one is outstanding: every exit
> already waits for the counter to drain, and a BUG_ON after the wait
> stops the kernel rather than let a late pio/qio write into a dead
> stack frame if that ever changes.
> 
> Tested on dev-cl04, 6.12.0-211.52.1.22.9.vz10 with the modules built
> out of tree, using test-only module knobs to fail one service pio/qio:
> - dm-ploop: after one injected failure, a merge on another device and a
>    second merge on the same device both failed with EIO; with the fix
>    both succeed;
> - dm-qcow2: a backward merge with an injected qio error, held for 5 s
>    while a merge on a second device started, returned success; with the
>    fix it fails with "Can't break L1 COW";
> - the ploop pytest suite, ploop and qcow2 formats, passes.
> 
> Denis V. Lunev (2):
>    dm-ploop: fix a failed merge failing every later merge on the host
>    dm-qcow2: fix a merge reporting success after a failed write
> 
>   drivers/md/dm-ploop-cmd.c | 4 +++-
>   drivers/md/dm-qcow2-cmd.c | 6 +++---
>   2 files changed, 6 insertions(+), 4 deletions(-)
> 
> 
> base-commit: 02401c5e378f282fbf11bdc65996253ff65dad9b

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel