[Devel] [PATCH hci-8.1 0/4] dm-ploop: fix the pio submission stop around cmds

[Devel] [PATCH hci-8.1 0/4] dm-ploop: fix the pio submission stop around cmds

[Denis V. Lunev]

A cmd stops pio submission and waits for the inflight pios before it
rewrites the BAT levels or deltas[], and expects nothing to touch them
until it resumes. Konstantin found three ways this does not hold while
reviewing the VSTOR-151979 series (VSTOR-152985, VSTOR-152984,
VSTOR-152981):

- a submitter racing the stop either runs without the cmd waiting for
  it, or is parked after resume and never completes (patch 1);

- the ploop_allocator() thread is not stopped at all. It truncates the
  top delta while merge or flip moves it in deltas[], cutting the live
  image to the preallocation size (patch 3). The thread can also miss a
  wakeup, which stalls the device by itself and would hang the new wait,
  so patch 2 fixes that first;

- a grow failing to copy a cluster resumes submission with that
  cluster's BAT page still busy, and writes to it hang (patch 4).

Patch 3 is the one losing data: a snapshot delete under write load is
enough.

Tested on dev-cl04, stock 6.12.0-211.52.1.22.11.vz10, dm-ploop built
out of tree:
- each bug reproduced on the stock module, the races with a test-only
  kprobe module widening the window: a pio running past the suspend of
  a cmd; a pio never completing after resume; the top image truncated
  from 512M to 128M by a merge and to 256M by a flip; after a grow
  failing with ENOSPC on a full host fs, a writer stuck in D state, an
  unsuspendable device and a later cmd failing with ETIMEDOUT. None of
  them reproduce with the series;
- fio 4k random reads and writes, O_DIRECT, 1 to 48 jobs, on a fully
  allocated image over brd: IOPS and system CPU stay within the
  run-to-run spread of the stock module. This setup tops out at about
  420K IOPS elsewhere in dm-ploop and would not show a lock taken on
  submission either, so patch 1 keeps that path lockless;
- the ploop pytest suite, ploop and qcow2 formats, passes. A test for
  patch 4 goes to the ploop tests separately.

Denis V. Lunev (4):
  dm-ploop: fix pio submission racing with the suspend and resume of a
    cmd
  dm-ploop: fix a lost wakeup of the ploop_allocator() thread
  dm-ploop: wait for the ploop_allocator() thread before changing
    deltas[]
  dm-ploop: fix a failed cluster copy on grow leaving the BAT page busy

 drivers/md/dm-ploop-cmd.c    | 18 ++++++++++++++++--
 drivers/md/dm-ploop-map.c    | 22 +++++++++++++++-------
 drivers/md/dm-ploop-target.c |  4 ++++
 drivers/md/dm-ploop.h        |  1 +
 4 files changed, 36 insertions(+), 9 deletions(-)


base-commit: 09eb9a82fe872048d082391415d1cdeda2ac6879
-- 
2.53.0

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

[Devel] [PATCH hci-8.1 1/4] dm-ploop: fix pio submission racing with the suspend and resume of a cmd

[Denis V. Lunev]

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

ploop_submit_embedded_pio() tests stop_submitting_pios and then takes a
ref on the current inflight_bios_ref index without deferred_lock. A cmd
sets the flag and flips the index under that lock and then waits only
for the refs of the old index, so:

- a pio that passed the check before the flag was set takes its ref on
  the new index or on the already killed old one. The cmd does not
  wait for it, and it runs while a merge, resize or delta index update
  rewrites the BAT levels and deltas[];

- a pio that saw the flag set is added to llsuspended_pios after resume
  has cleared the flag and drained the list. Nothing drains it until
  the next cmd, the request never completes and a dm suspend of the
  device hangs.

The submission path dropped deferred_lock on purpose when the lists
became lockless, so keep it lockless. Test the flag and take the ref in
one RCU read-side section, and wait for a grace period in the cmd
between setting the flag and flipping the index: a submitter that missed
the flag then holds its ref on the index the cmd waits for. Only a
submitter that saw the flag takes deferred_lock and tests it again
before parking the pio, and resume clears the flag under that lock
before it drains the list.

Fixes: b90109c12b1c ("dm-ploop: convert the rest of the lists to use llist variant")
Reported-by: Konstantin Khorenko <khorenko@virtuozzo.com>
https://virtuozzo.atlassian.net/browse/VSTOR-152985
Feature: dm-ploop: ploop target driver
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 drivers/md/dm-ploop-cmd.c |  7 +++++--
 drivers/md/dm-ploop-map.c | 16 ++++++++++++----
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/md/dm-ploop-cmd.c b/drivers/md/dm-ploop-cmd.c
index 645c5136e7ba..d85f4d803b61 100644
--- a/drivers/md/dm-ploop-cmd.c
+++ b/drivers/md/dm-ploop-cmd.c
@@ -132,7 +132,7 @@ static void ploop_resume_submitting_pios(struct ploop *ploop)
 	ploop_enable_writeback_delay(ploop);
 	spin_lock_irq(&ploop->deferred_lock);
 	WARN_ON_ONCE(!ploop->stop_submitting_pios);
-	ploop->stop_submitting_pios = false;
+	WRITE_ONCE(ploop->stop_submitting_pios, false);
 	spin_unlock_irq(&ploop->deferred_lock);
 
 	suspended_pending = llist_del_all(&ploop->llsuspended_pios);
@@ -147,9 +147,12 @@ static int ploop_suspend_submitting_pios(struct ploop *ploop)
 	ploop_disable_writeback_delay(ploop);
 	spin_lock_irq(&ploop->deferred_lock);
 	WARN_ON_ONCE(ploop->stop_submitting_pios);
-	ploop->stop_submitting_pios = true;
+	WRITE_ONCE(ploop->stop_submitting_pios, true);
 	spin_unlock_irq(&ploop->deferred_lock);
 
+	/* A submitter that missed the stop gets its ref on the old index */
+	synchronize_rcu();
+
 	ret = ploop_inflight_bios_ref_switch(ploop, true);
 	if (ret)
 		goto out;
diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
index 016be97caaa8..f0832fcefeb8 100644
--- a/drivers/md/dm-ploop-map.c
+++ b/drivers/md/dm-ploop-map.c
@@ -2528,6 +2528,7 @@ static void ploop_submit_embedded_pio(struct ploop *ploop, struct pio *pio, gfp_
 	struct ploop_rq *prq = pio->endio_cb_data;
 	struct request *rq = prq->rq;
 	LLIST_HEAD(deferred_pios);
+	unsigned long irqflags;
 	int ret = 0;
 	struct pio *spio;
 
@@ -2538,12 +2539,19 @@ static void ploop_submit_embedded_pio(struct ploop *ploop, struct pio *pio, gfp_
 		pio->queue_list_id = PLOOP_LIST_FLUSH;
 	}
 
-	if (unlikely(ploop->stop_submitting_pios)) {
-		llist_add((struct llist_node *)(&pio->list), &ploop->llsuspended_pios);
-		return;
+	rcu_read_lock();
+	if (unlikely(READ_ONCE(ploop->stop_submitting_pios))) {
+		spin_lock_irqsave(&ploop->deferred_lock, irqflags);
+		if (ploop->stop_submitting_pios) {
+			llist_add((struct llist_node *)(&pio->list), &ploop->llsuspended_pios);
+			spin_unlock_irqrestore(&ploop->deferred_lock, irqflags);
+			rcu_read_unlock();
+			return;
+		}
+		spin_unlock_irqrestore(&ploop->deferred_lock, irqflags);
 	}
-
 	ploop_inc_nr_inflight(ploop, pio);
+	rcu_read_unlock();
 
 	if ((pio->bi_op & REQ_OP_MASK) == REQ_OP_FLUSH || pio->bi_op & REQ_PREFLUSH) {
 		ret = ploop_prepare_flush(ploop, pio);
-- 
2.53.0

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

[Devel] [PATCH hci-8.1 2/4] dm-ploop: fix a lost wakeup of the ploop_allocator() thread

[Denis V. Lunev]

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

ploop_allocator() tests prealloc_size and only then sets itself
TASK_INTERRUPTIBLE before schedule(). A request that lands between the
two finds the thread still running, so wake_up_process() does nothing,
and the thread goes to sleep with the request pending. A pio whose
cluster is past the end of the top delta then waits in
ploop_allocate_cluster() until some later request wakes the thread. The
dispatcher waits for every runner to go idle before it hands out more
work, so the whole device stalls, and an idle device has no later
request.

Set the task state before testing prealloc_size.

Fixes: cf489cf22cd5 ("dm-ploop: make filespace preallocations async")
Reported-by: Konstantin Khorenko <khorenko@virtuozzo.com>
https://virtuozzo.atlassian.net/browse/VSTOR-152984
Feature: dm-ploop: ploop target driver
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 drivers/md/dm-ploop-map.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
index f0832fcefeb8..e64fd8965d46 100644
--- a/drivers/md/dm-ploop-map.c
+++ b/drivers/md/dm-ploop-map.c
@@ -2325,6 +2325,7 @@ int ploop_allocator(void *data)
 	struct file *file;
 
 	for (;;) {
+		set_current_state(TASK_INTERRUPTIBLE);
 		/* we do not care about exact number here - risk one extra schedule wrt taking the lock */
 		if (ploop->prealloc_size) {
 			__set_current_state(TASK_RUNNING);
@@ -2348,7 +2349,6 @@ int ploop_allocator(void *data)
 			__set_current_state(TASK_RUNNING);
 			break;
 		}
-		set_current_state(TASK_INTERRUPTIBLE);
 		schedule();
 	}
 
-- 
2.53.0

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

[Devel] [PATCH hci-8.1 3/4] dm-ploop: wait for the ploop_allocator() thread before changing deltas[]

[Denis V. Lunev]

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

An allocating pio requests the preallocation of the top delta ahead of
itself and does not wait when its cluster already fits, so the
allocator can still be working when submission is suspended or the
device is dm suspended. It looks up the top delta without a lock and
keeps the pointer and the file across its truncate or fallocate, while
a cmd rewrites deltas[] under neither of its locks:

- merge and notify_merged_backward move the top delta one slot down and
  zero the old slot. The allocator then reads a zero file_size from it
  and truncates the live top image down to the preallocation size, or
  reads a NULL file and oopses;

- flip_upper_deltas swaps the two upper slots of a dm suspended device.
  The allocator computes the new length from the size of the delta now
  on top and truncates the old top one, now holding data below it, to
  that length.

Wait for the allocator to finish in ploop_suspend_submitting_pios()
after the inflight pios are gone, and in ploop_postsuspend() before
the device is marked suspended, which is what the flip checks. No pio
can request a new preallocation at either point. Wake the waiters with
wake_up(), as the one in ploop_postsuspend() sleeps uninterruptibly.

Fixes: cf489cf22cd5 ("dm-ploop: make filespace preallocations async")
Reported-by: Konstantin Khorenko <khorenko@virtuozzo.com>
https://virtuozzo.atlassian.net/browse/VSTOR-152984
Feature: dm-ploop: ploop target driver
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 drivers/md/dm-ploop-cmd.c    | 5 +++++
 drivers/md/dm-ploop-map.c    | 4 ++--
 drivers/md/dm-ploop-target.c | 4 ++++
 drivers/md/dm-ploop.h        | 1 +
 4 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-ploop-cmd.c b/drivers/md/dm-ploop-cmd.c
index d85f4d803b61..72a9662d2bdb 100644
--- a/drivers/md/dm-ploop-cmd.c
+++ b/drivers/md/dm-ploop-cmd.c
@@ -159,6 +159,11 @@ static int ploop_suspend_submitting_pios(struct ploop *ploop)
 
 	ret = wait_event_interruptible(ploop->inflight_md_wq,
 				       !atomic_read(&ploop->inflight_md));
+	if (ret)
+		goto out;
+
+	ret = wait_event_interruptible(ploop->dispatcher_wq_prealloc,
+				       ploop_no_pending_prealloc(ploop));
 out:
 	if (ret)
 		ploop_resume_submitting_pios(ploop);
diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
index e64fd8965d46..ae3e287604b7 100644
--- a/drivers/md/dm-ploop-map.c
+++ b/drivers/md/dm-ploop-map.c
@@ -1233,7 +1233,7 @@ void ploop_should_prealloc(struct ploop *ploop)
 	spin_unlock_irqrestore(&ploop->bat_lock, flags);
 }
 
-static int ploop_no_pending_prealloc(struct ploop *ploop)
+int ploop_no_pending_prealloc(struct ploop *ploop)
 {
 	int ret;
 	unsigned long flags;
@@ -2342,7 +2342,7 @@ int ploop_allocator(void *data)
 					PL_FMT("failed to allocate space: %d\n"),
 					       ploop_device_name(ploop), ret);
 			}
-			wake_up_interruptible(&ploop->dispatcher_wq_prealloc);
+			wake_up(&ploop->dispatcher_wq_prealloc);
 			current_restore_flags(old_flags, PLOOP_PFLAGS);
 		}
 		if (kthread_should_stop()) {
diff --git a/drivers/md/dm-ploop-target.c b/drivers/md/dm-ploop-target.c
index 3b589579741c..d8279c79ef56 100644
--- a/drivers/md/dm-ploop-target.c
+++ b/drivers/md/dm-ploop-target.c
@@ -743,6 +743,10 @@ static void ploop_presuspend_undo(struct dm_target *ti)
 }
 static void ploop_postsuspend(struct dm_target *ti)
 {
+	struct ploop *ploop = ti->private;
+
+	wait_event(ploop->dispatcher_wq_prealloc,
+		   ploop_no_pending_prealloc(ploop));
 	ploop_set_suspended(ti, true);
 }
 static int ploop_preresume(struct dm_target *ti)
diff --git a/drivers/md/dm-ploop.h b/drivers/md/dm-ploop.h
index 7d0d94e7b266..5ae02ef5ffc2 100644
--- a/drivers/md/dm-ploop.h
+++ b/drivers/md/dm-ploop.h
@@ -652,6 +652,7 @@ extern void ploop_disable_writeback_delay(struct ploop *ploop);
 extern void ploop_enable_writeback_delay(struct ploop *ploop);
 
 extern void ploop_should_prealloc(struct ploop *ploop);
+extern int ploop_no_pending_prealloc(struct ploop *ploop);
 extern void ploop_resubmit_enospc_pios(struct ploop *ploop);
 
 #endif /* __DM_PLOOP_H */
-- 
2.53.0

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

[Devel] [PATCH hci-8.1 4/4] dm-ploop: fix a failed cluster copy on grow leaving the BAT page busy

[Denis V. Lunev]

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

ploop_prepare_reloc_index_wb() marks the BAT page of a relocated
cluster MD_UPDATING and MD_DIRTY and allocates the cluster to copy it
to. When ploop_write_cluster_sync() then fails, an I/O error or ENOSPC
on the host file system, ploop_grow_relocate_cluster() only frees the
piwb. The resize fails and submission resumes with both bits set:

- every allocating write, discard or COW on a cluster of that page is
  parked in md->wait_llist, which only a writeback completion of the
  page drains, and none comes. These requests never complete, a dm
  suspend of the device hangs and later ploop cmds time out in the
  inflight ref switch;

- the allocated cluster stays taken in holes_bitmap until the image is
  reopened.

Clear the bits as the error path of ploop_prepare_reloc_index_wb()
does and give the cluster back. Submission is stopped for the whole
resize, so no pio waits on the page and none has to be resubmitted.

Fixes: 018063db0eeb ("dm-ploop: fix resize and grow to use the new way of updating md pages")
Reported-by: Konstantin Khorenko <khorenko@virtuozzo.com>
https://virtuozzo.atlassian.net/browse/VSTOR-152981
Feature: dm-ploop: ploop target driver
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 drivers/md/dm-ploop-cmd.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/md/dm-ploop-cmd.c b/drivers/md/dm-ploop-cmd.c
index 72a9662d2bdb..54458316673e 100644
--- a/drivers/md/dm-ploop-cmd.c
+++ b/drivers/md/dm-ploop-cmd.c
@@ -351,6 +351,12 @@ static int ploop_grow_relocate_cluster(struct ploop *ploop,
 		PL_ERR("reloc: failed write: %d", ret);
 		/* piwb can not change here we are the only user of md page */
 		ploop_break_bat_update(ploop, md, piwb);
+		spin_lock_irq(&ploop->bat_lock);
+		if (add_for_wb)
+			clear_bit(MD_DIRTY, &md->status);
+		clear_bit(MD_UPDATING, &md->status);
+		ploop_hole_set_bit(new_dst, ploop);
+		spin_unlock_irq(&ploop->bat_lock);
 		goto out;
 	}
 
-- 
2.53.0

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

Re: [Devel] [PATCH hci-8.1 2/4] dm-ploop: fix a lost wakeup of the ploop_allocator() thread

[no-reply]

> From: Denis V. Lunev <den@openvz.org>
> 
> ploop_allocator() tests prealloc_size and only then sets itself
> TASK_INTERRUPTIBLE before schedule(). A request that lands between the
> two finds the thread still running, so wake_up_process() does nothing,
> and the thread goes to sleep with the request pending. A pio whose
> cluster is past the end of the top delta then waits in
> ploop_allocate_cluster() until some later request wakes the thread. The
> dispatcher waits for every runner to go idle before it hands out more
> work, so the whole device stalls, and an idle device has no later
> request.
> 
> Set the task state before testing prealloc_size.
> 
> Fixes: cf489cf22cd5 ("dm-ploop: make filespace preallocations async")
> Reported-by: Konstantin Khorenko <khorenko@virtuozzo.com>
> https://virtuozzo.atlassian.net/browse/VSTOR-152984
> Feature: dm-ploop: ploop target driver
> Signed-off-by: Denis V. Lunev <den@openvz.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261103083642.130885-1-den@openvz.org?part=2


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

Re: [Devel] [PATCH hci-8.1 1/4] dm-ploop: fix pio submission racing with the suspend and resume of a cmd

[no-reply]

> From: Denis V. Lunev <den@openvz.org>
> 
> ploop_submit_embedded_pio() tests stop_submitting_pios and then takes a
> ref on the current inflight_bios_ref index without deferred_lock. A cmd
> sets the flag and flips the index under that lock and then waits only
> for the refs of the old index, so:
> 
> - a pio that passed the check before the flag was set takes its ref on
>   the new index or on the already killed old one. The cmd does not
>   wait for it, and it runs while a merge, resize or delta index update
>   rewrites the BAT levels and deltas[];
> 
> - a pio that saw the flag set is added to llsuspended_pios after resume
>   has cleared the flag and drained the list. Nothing drains it until
>   the next cmd, the request never completes and a dm suspend of the
> [ ... ]
> Fixes: b90109c12b1c ("dm-ploop: convert the rest of the lists to use llist variant")
> Reported-by: Konstantin Khorenko <khorenko@virtuozzo.com>
> https://virtuozzo.atlassian.net/browse/VSTOR-152985
> Feature: dm-ploop: ploop target driver
> Signed-off-by: Denis V. Lunev <den@openvz.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261103083642.130885-1-den@openvz.org?part=1


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

Re: [Devel] [PATCH hci-8.1 4/4] dm-ploop: fix a failed cluster copy on grow leaving the BAT page busy

[no-reply]

> From: Denis V. Lunev <den@openvz.org>
> 
> ploop_prepare_reloc_index_wb() marks the BAT page of a relocated
> cluster MD_UPDATING and MD_DIRTY and allocates the cluster to copy it
> to. When ploop_write_cluster_sync() then fails, an I/O error or ENOSPC
> on the host file system, ploop_grow_relocate_cluster() only frees the
> piwb. The resize fails and submission resumes with both bits set:
> 
> - every allocating write, discard or COW on a cluster of that page is
>   parked in md->wait_llist, which only a writeback completion of the
>   page drains, and none comes. These requests never complete, a dm
>   suspend of the device hangs and later ploop cmds time out in the
>   inflight ref switch;
> 
> - the allocated cluster stays taken in holes_bitmap until the image is
>   reopened.
> 
> Clear the bits as the error path of ploop_prepare_reloc_index_wb()
> does and give the cluster back. Submission is stopped for the whole
> resize, so no pio waits on the page and none has to be resubmitted.
> 
> Fixes: 018063db0eeb ("dm-ploop: fix resize and grow to use the new way of updating md pages")
> Reported-by: Konstantin Khorenko <khorenko@virtuozzo.com>
> https://virtuozzo.atlassian.net/browse/VSTOR-152981
> Feature: dm-ploop: ploop target driver
> Signed-off-by: Denis V. Lunev <den@openvz.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261103083642.130885-1-den@openvz.org?part=4


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

Re: [Devel] [PATCH hci-8.1 3/4] dm-ploop: wait for the ploop_allocator() thread before changing deltas[]

[no-reply]

> From: Denis V. Lunev <den@openvz.org>
> 
> An allocating pio requests the preallocation of the top delta ahead of
> itself and does not wait when its cluster already fits, so the
> allocator can still be working when submission is suspended or the
> device is dm suspended. It looks up the top delta without a lock and
> keeps the pointer and the file across its truncate or fallocate, while
> a cmd rewrites deltas[] under neither of its locks:
> 
> - merge and notify_merged_backward move the top delta one slot down and
>   zero the old slot. The allocator then reads a zero file_size from it
>   and truncates the live top image down to the preallocation size, or
>   reads a NULL file and oopses;
> 
> - flip_upper_deltas swaps the two upper slots of a dm suspended device.
>   The allocator computes the new length from the size of the delta now
>   on top and truncates the old top one, now holding data below it, to
>   that length.
> 
> Wait for the allocator to finish in ploop_suspend_submitting_pios()
> after the inflight pios are gone, and in ploop_postsuspend() before
> the device is marked suspended, which is what the flip checks. No pio
> can request a new preallocation at either point. Wake the waiters with
> wake_up(), as the one in ploop_postsuspend() sleeps uninterruptibly.
> 
> Fixes: cf489cf22cd5 ("dm-ploop: make filespace preallocations async")
> Reported-by: Konstantin Khorenko <khorenko@virtuozzo.com>
> https://virtuozzo.atlassian.net/browse/VSTOR-152984
> Feature: dm-ploop: ploop target driver
> Signed-off-by: Denis V. Lunev <den@openvz.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261103083642.130885-1-den@openvz.org?part=3


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

Re: [Devel] [PATCH hci-8.1 0/4] dm-ploop: fix the pio submission stop around cmds

[Andrey Zhadchenko]

Reviewed-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>

On 11/3/26 09:36, Denis V. Lunev wrote:
> A cmd stops pio submission and waits for the inflight pios before it
> rewrites the BAT levels or deltas[], and expects nothing to touch them
> until it resumes. Konstantin found three ways this does not hold while
> reviewing the VSTOR-151979 series (VSTOR-152985, VSTOR-152984,
> VSTOR-152981):
> 
> - a submitter racing the stop either runs without the cmd waiting for
>    it, or is parked after resume and never completes (patch 1);
> 
> - the ploop_allocator() thread is not stopped at all. It truncates the
>    top delta while merge or flip moves it in deltas[], cutting the live
>    image to the preallocation size (patch 3). The thread can also miss a
>    wakeup, which stalls the device by itself and would hang the new wait,
>    so patch 2 fixes that first;
> 
> - a grow failing to copy a cluster resumes submission with that
>    cluster's BAT page still busy, and writes to it hang (patch 4).
> 
> Patch 3 is the one losing data: a snapshot delete under write load is
> enough.
> 
> Tested on dev-cl04, stock 6.12.0-211.52.1.22.11.vz10, dm-ploop built
> out of tree:
> - each bug reproduced on the stock module, the races with a test-only
>    kprobe module widening the window: a pio running past the suspend of
>    a cmd; a pio never completing after resume; the top image truncated
>    from 512M to 128M by a merge and to 256M by a flip; after a grow
>    failing with ENOSPC on a full host fs, a writer stuck in D state, an
>    unsuspendable device and a later cmd failing with ETIMEDOUT. None of
>    them reproduce with the series;
> - fio 4k random reads and writes, O_DIRECT, 1 to 48 jobs, on a fully
>    allocated image over brd: IOPS and system CPU stay within the
>    run-to-run spread of the stock module. This setup tops out at about
>    420K IOPS elsewhere in dm-ploop and would not show a lock taken on
>    submission either, so patch 1 keeps that path lockless;
> - the ploop pytest suite, ploop and qcow2 formats, passes. A test for
>    patch 4 goes to the ploop tests separately.
> 
> Denis V. Lunev (4):
>    dm-ploop: fix pio submission racing with the suspend and resume of a
>      cmd
>    dm-ploop: fix a lost wakeup of the ploop_allocator() thread
>    dm-ploop: wait for the ploop_allocator() thread before changing
>      deltas[]
>    dm-ploop: fix a failed cluster copy on grow leaving the BAT page busy
> 
>   drivers/md/dm-ploop-cmd.c    | 18 ++++++++++++++++--
>   drivers/md/dm-ploop-map.c    | 22 +++++++++++++++-------
>   drivers/md/dm-ploop-target.c |  4 ++++
>   drivers/md/dm-ploop.h        |  1 +
>   4 files changed, 36 insertions(+), 9 deletions(-)
> 
> 
> base-commit: 09eb9a82fe872048d082391415d1cdeda2ac6879

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

Re: [Devel] [PATCH hci-8.1 2/4] dm-ploop: fix a lost wakeup of the ploop_allocator() thread

[Konstantin Khorenko]

I won't apply this version, please send v2 with the changes below.

On Tue, 03 Nov 2026 09:36:40 +0100, Denis V. Lunev wrote:
> ploop_allocate_cluster() until some later request wakes the thread. The

Item 2 of the cover letter reply: drop "until some later request wakes
the thread".

Why: ploop_req_prealloc() returns without a wakeup when the requested
size is already covered by prealloc_size, so later requests in the same
range do not wake the thread either. The stall does not end by itself.

Item 2 of the cover letter reply: add one sentence that the same
reorder fixes a lost kthread_stop() wakeup.

Why: the old loop tested kthread_should_stop() and only then set the
task state, so a kthread_stop() landing in between was lost the same
way and the device removal hung.

With that fixed:

Reviewed-by: Konstantin Khorenko <khorenko@virtuozzo.com>

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

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

Re: [Devel] [PATCH hci-8.1 0/4] dm-ploop: fix the pio submission stop around cmds

[Konstantin Khorenko]

I won't apply this version, please send v2 with the changes below.

On Tue, 03 Nov 2026 09:36:38 +0100, Denis V. Lunev wrote:
> Denis V. Lunev (4):
>   dm-ploop: fix pio submission racing with the suspend and resume of a
>     cmd
>   dm-ploop: fix a lost wakeup of the ploop_allocator() thread
>   dm-ploop: wait for the ploop_allocator() thread before changing
>     deltas[]
>   dm-ploop: fix a failed cluster copy on grow leaving the BAT page busy

What to change for v2:

1. Patch 1, commit message only, small: in the first race add
   notify_merged to "a merge, resize or delta index update". The code
   stays as is.

2. Patch 2, commit message only, small:
   - drop "until some later request wakes the thread", the stall does
     not end by itself;
   - add one sentence that the same reorder also fixes a lost
     kthread_stop() wakeup.
   The code stays as is.

3. Patch 3, code and commit message:
   - code: also wait for ploop_no_pending_prealloc() in
     ploop_flip_upper_deltas() before process_flip_upper_deltas(), keep
     the wait in ploop_postsuspend();
   - message: "merge and notify_merged_backward" becomes "merge and
     notify_merged", both directions move the top delta.

4. Patch 4: no changes, resend it as is.

5. One more old bug found, not part of this series: the forward
   notify_merged stores the new file_size into the wrong deltas[] slot
   (details below). Please file a Jira task for it and fix it with a
   separate patch, not in v2.

6. Cover letter of v2:
   - "cutting the live image to the preallocation size" becomes
     "cutting the live image";
   - reword "Patch 3 is the one losing data".

Details on each item are in the replies to the patches. Below are the
details on items 5 and 6.

Item 5, the old bug. In ploop_delta_clusters_merged() the forward case
stores file_size, file_preallocated_area_start and nr_be into
deltas[level + 1] after notify_delta_merged() has already shifted the
slots down, so the delta that took the clusters is at deltas[level] by
then. With level == top - 2 that overwrites the top delta's file_size,
and the next preallocation can truncate the live top image. It has been
there since the target was added.

> - the ploop_allocator() thread is not stopped at all. It truncates the
>   top delta while merge or flip moves it in deltas[], cutting the live
>   image to the preallocation size (patch 3). The thread can also miss a

Item 6. Only a merge cuts the image to the preallocation size. A flip
cuts the old top image to the size of the other delta plus the
preallocation.

> Patch 3 is the one losing data: a snapshot delete under write load is
> enough.

Item 6. Race A of patch 1 loses data too: a pio can read and write
through a delta that merge has already released. Patch 3 is the easiest
one to hit, not the only one losing data.

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

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

Re: [Devel] [PATCH hci-8.1 1/4] dm-ploop: fix pio submission racing with the suspend and resume of a cmd

[Konstantin Khorenko]

I won't apply this version, please send v2 with the change below.

On Tue, 03 Nov 2026 09:36:39 +0100, Denis V. Lunev wrote:
> - a pio that passed the check before the flag was set takes its ref on
>   the new index or on the already killed old one. The cmd does not
>   wait for it, and it runs while a merge, resize or delta index update
>   rewrites the BAT levels and deltas[];

Item 1 of the cover letter reply: add notify_merged to that list.

Why: ploop_delta_clusters_merged() stops submission the same way and
renumbers deltas[], so it is hit by the same race. The code is fine,
the RCU section and the grace period before the index switch close
both races.

With that fixed:

Reviewed-by: Konstantin Khorenko <khorenko@virtuozzo.com>

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

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

Re: [Devel] [PATCH hci-8.1 3/4] dm-ploop: wait for the ploop_allocator() thread before changing deltas[]

[Konstantin Khorenko]

I won't apply this version, please send v2 with the changes below.

On Tue, 03 Nov 2026 09:36:41 +0100, Denis V. Lunev wrote:
>  static void ploop_postsuspend(struct dm_target *ti)
>  {
> +	struct ploop *ploop = ti->private;
> +
> +	wait_event(ploop->dispatcher_wq_prealloc,
> +		   ploop_no_pending_prealloc(ploop));
>  	ploop_set_suspended(ti, true);
>  }

Item 3 of the cover letter reply, code: also wait for
ploop_no_pending_prealloc() in ploop_flip_upper_deltas() before
process_flip_upper_deltas(), keep this wait.

Why: a resize on a dm suspended device re-arms the allocator after this
wait. ploop_resize() does not check ->suspended, and the grow allocates
through ploop_prepare_reloc_index_wb() and ploop_allocate_cluster(),
which requests a preallocation and does not wait when the cluster fits.
So suspend, set_noresume 1, resize, flip_upper_deltas still lets the
flip swap the slots under a running allocator, which is the truncation
this patch fixes.

> - merge and notify_merged_backward move the top delta one slot down and
>   zero the old slot. The allocator then reads a zero file_size from it

Item 3 of the cover letter reply, message: "merge and notify_merged".

Why: notify_delta_merged() renumbers the slots above @level for forward
as well. The code already covers it through
ploop_suspend_submitting_pios(), only the message is narrower than the
fix.

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

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

Re: [Devel] [PATCH hci-8.1 4/4] dm-ploop: fix a failed cluster copy on grow leaving the BAT page busy

[Konstantin Khorenko]

No changes needed here, resend it as is with v2 (item 4 of the cover
letter reply).

On Tue, 03 Nov 2026 09:36:42 +0100, Denis V. Lunev wrote:
> Clear the bits as the error path of ploop_prepare_reloc_index_wb()
> does and give the cluster back. Submission is stopped for the whole
> resize, so no pio waits on the page and none has to be resubmitted.

Reviewed-by: Konstantin Khorenko <khorenko@virtuozzo.com>

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

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

[Devel] [PATCH hci-8.1 0/4] dm-ploop: fix the pio submission stop around cmds

[Denis V. Lunev]

A cmd stops pio submission and waits for the inflight pios before it
rewrites the BAT levels or deltas[], and expects nothing to touch them
until it resumes. Konstantin found three ways this does not hold while
reviewing the VSTOR-151979 series (VSTOR-152985, VSTOR-152984,
VSTOR-152981):

- a submitter racing the stop either runs without the cmd waiting for
  it, or is parked after resume and never completes (patch 1);

- the ploop_allocator() thread is not stopped at all. It truncates the
  top delta while merge or flip moves it in deltas[], cutting the live
  image to the preallocation size (patch 3). The thread can also miss a
  wakeup, which stalls the device by itself and would hang the new wait,
  so patch 2 fixes that first;

- a grow failing to copy a cluster resumes submission with that
  cluster's BAT page still busy, and writes to it hang (patch 4).

Patch 3 is the one losing data: a snapshot delete under write load is
enough.

Tested on dev-cl04, stock 6.12.0-211.52.1.22.11.vz10, dm-ploop built
out of tree:
- each bug reproduced on the stock module, the races with a test-only
  kprobe module widening the window: a pio running past the suspend of
  a cmd; a pio never completing after resume; the top image truncated
  from 512M to 128M by a merge and to 256M by a flip; after a grow
  failing with ENOSPC on a full host fs, a writer stuck in D state, an
  unsuspendable device and a later cmd failing with ETIMEDOUT. None of
  them reproduce with the series;
- fio 4k random reads and writes, O_DIRECT, 1 to 48 jobs, on a fully
  allocated image over brd: IOPS and system CPU stay within the
  run-to-run spread of the stock module. This setup tops out at about
  420K IOPS elsewhere in dm-ploop and would not show a lock taken on
  submission either, so patch 1 keeps that path lockless;
- the ploop pytest suite, ploop and qcow2 formats, passes. A test for
  patch 4 goes to the ploop tests separately.

Denis V. Lunev (4):
  dm-ploop: fix pio submission racing with the suspend and resume of a
    cmd
  dm-ploop: fix a lost wakeup of the ploop_allocator() thread
  dm-ploop: wait for the ploop_allocator() thread before changing
    deltas[]
  dm-ploop: fix a failed cluster copy on grow leaving the BAT page busy

 drivers/md/dm-ploop-cmd.c    | 18 ++++++++++++++++--
 drivers/md/dm-ploop-map.c    | 22 +++++++++++++++-------
 drivers/md/dm-ploop-target.c |  4 ++++
 drivers/md/dm-ploop.h        |  1 +
 4 files changed, 36 insertions(+), 9 deletions(-)


base-commit: 09eb9a82fe872048d082391415d1cdeda2ac6879
-- 
2.53.0

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