[Devel] [PATCH hci-8.1 0/3] dm-ploop: fix writes to a read-only device hanging the host

[Devel] [PATCH hci-8.1 0/3] dm-ploop: fix writes to a read-only device hanging the host

[Denis V. Lunev]

A write that reaches a read-only ploop device never completes: the
writer hangs in D state, and after a buffered write every later sync on
the host hangs behind it until a reboot. Such writes come from a device
stacked on top. libploop leaves the partition of "ploop mount -r"
writable, which the ploop side fixes in openvz/ploop.ovz PR 513, and
libvzctl attaches a backup disk to a Container that way and mounts it
read-write (VSTOR-154096).

Two bugs meet there:
- when a cluster allocation or a discard fails, the pios parked on that
  BAT page are never resubmitted (patch 2). Any failing allocation
  leaves them stuck, not only one on a read-only device. Patch 1 moves
  the resubmission loop into a helper for it;
- dm-ploop does not refuse writes to a read-only table, as dm-qcow2
  does since 409d7e697578 (patch 3). The data of such a write reaches
  the image through f_op->write_iter(), while the allocation of a new
  cluster fails with EBADF and lands in the bug of patch 2.

Tested on 6.12.0-211.52.1.22.19.vz10, dm-ploop built out of tree:
- read-only ploop image, its partition mounted read-write, dd of 16M
  with conv=fsync. Stock: the writer and sync stuck in D state until a
  reboot, 12 writes in flight, the image changed. Patches 1-2: the
  fsync fails with EIO, nothing hangs, the image still changes. The
  whole series: the mount fails on its first superblock write, nothing
  hangs, the image is unchanged;
- ploop test suite, make -j48 test on dev-cl04, green for ploop and
  qcow2 with this series together with the VSTOR-154182 one. Its new
  test overwrites a block through a linear device stacked on a
  read-only device: it fails on stock and passes with both series.

Denis V. Lunev (3):
  dm-ploop: factor out ploop_dispatch_md_waiters()
  dm-ploop: fix pios stuck in md->wait_llist after a failed BAT update
  dm-ploop: fail writes to a read-only device

 drivers/md/dm-ploop-map.c | 41 ++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)

-- 
2.53.0

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

[Devel] [PATCH hci-8.1 1/3] dm-ploop: factor out ploop_dispatch_md_waiters()

[Denis V. Lunev]

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

Move the loop that hands the pios parked in md->wait_llist back to the
dispatcher out of ploop_advance_local_after_bat_wb() into a helper of
its own, so that the error paths of the BAT updates can use it too.

No functional change.

https://virtuozzo.atlassian.net/browse/VSTOR-154096

Feature: dm-ploop: ploop target driver
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 drivers/md/dm-ploop-map.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
index ae3e287604b7..f0569263622c 100644
--- a/drivers/md/dm-ploop-map.c
+++ b/drivers/md/dm-ploop-map.c
@@ -840,6 +840,23 @@ static void ploop_advance_discarded(struct ploop *ploop, struct md_page *md,
 	ploop_hole_set_bit(d_clu, ploop);
 }
 
+static void ploop_dispatch_md_waiters(struct ploop *ploop,
+				      struct llist_node *waiters)
+{
+	struct llist_node *pos, *t;
+	struct pio *pio;
+	LIST_HEAD(list);
+
+	llist_for_each_safe(pos, t, waiters) {
+		pio = list_entry((struct list_head *)pos, typeof(*pio), list);
+		INIT_LIST_HEAD(&pio->list);
+		list_add(&pio->list, &list);
+	}
+
+	if (!list_empty(&list))
+		ploop_dispatch_pios(ploop, NULL, &list);
+}
+
 /*
  * Apply to the local BAT copy the changes made by the pios attached
  * to @piwb, whatever type @piwb was prepared with: allocations and
@@ -856,11 +873,10 @@ static void ploop_advance_local_after_bat_wb(struct ploop *ploop,
 	struct md_page *md = piwb->md;
 	map_index_t *dst_clu;
 	unsigned long flags;
-	LIST_HEAD(list);
 	struct llist_node *wait_llist_pending;
 	struct ploop_cow *cow;
 	struct pio *pio;
-	struct llist_node *pos, *t;
+	struct llist_node *pos;
 	BUG_ON(!md);
 
 	spin_lock_irqsave(&ploop->bat_lock, flags);
@@ -893,17 +909,7 @@ static void ploop_advance_local_after_bat_wb(struct ploop *ploop,
 	spin_unlock_irqrestore(&ploop->bat_lock, flags);
 	kunmap_local(dst_clu);
 
-	if (wait_llist_pending) {
-		llist_for_each_safe(pos, t, wait_llist_pending) {
-			pio = list_entry((struct list_head *)pos, typeof(*pio), list);
-			INIT_LIST_HEAD(&pio->list);
-			list_add(&pio->list, &list);
-		}
-	}
-
-	if (!list_empty(&list))
-		ploop_dispatch_pios(ploop, NULL, &list);
-
+	ploop_dispatch_md_waiters(ploop, wait_llist_pending);
 }
 
 static void ploop_free_piwb(struct ploop_index_wb *piwb)
-- 
2.53.0

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

Re: [Devel] [PATCH hci-8.1 1/3] dm-ploop: factor out ploop_dispatch_md_waiters()

[no-reply]

> From: Denis V. Lunev <den@openvz.org>
> 
> Move the loop that hands the pios parked in md->wait_llist back to the
> dispatcher out of ploop_advance_local_after_bat_wb() into a helper of
> its own, so that the error paths of the BAT updates can use it too.
> 
> No functional change.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-154096
> 
> 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/20261109190604.1850504-1-den@openvz.org?part=1


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

[Devel] [PATCH hci-8.1 2/3] dm-ploop: fix pios stuck in md->wait_llist after a failed BAT update

[Denis V. Lunev]

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

ploop_locate_new_cluster_and_attach_pio() and
ploop_process_one_discard_pio() set MD_UPDATING on the BAT page while
they prepare its update. A pio that needs the page meanwhile is parked
in md->wait_llist by ploop_delay_if_md_busy(), and only
ploop_advance_local_after_bat_wb(), at the end of a writeback of the
page, gives the parked pios back.

When the update fails, the error path breaks it, clears MD_DIRTY and
MD_UPDATING and leaves the parked pios where they are. No writeback of
the page is queued, so they are never resubmitted: the writers hang in
D state, and after a buffered write every later sync on the host hangs
behind them until a reboot. An allocation fails this way when
ploop_prepare_bat_update() runs out of memory, when no free cluster is
left, or when the punch, zero or preallocation of the new cluster in
the top delta fails, e.g. on a write to a read-only device that needs
a new cluster.

Take the parked pios together with clearing MD_UPDATING, under
bat_lock, which every caller of ploop_delay_if_md_busy() holds, and
resubmit them with ploop_dispatch_md_waiters(), as the writeback
completion does. The error paths of ploop_prepare_reloc_index_wb() and
ploop_grow_relocate_cluster() clear MD_UPDATING without a drain too,
but they run with pio submission stopped, so nothing is parked there.

https://virtuozzo.atlassian.net/browse/VSTOR-154096

Fixes: ddf38fa44df4 ("dm-ploop: fix and rework md updates")
Reported-by: Denis Genov <denis.genov@virtuozzo.com>
Feature: dm-ploop: ploop target driver
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 drivers/md/dm-ploop-map.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
index f0569263622c..b49281691a01 100644
--- a/drivers/md/dm-ploop-map.c
+++ b/drivers/md/dm-ploop-map.c
@@ -1750,6 +1750,7 @@ static bool ploop_locate_new_cluster_and_attach_pio(struct ploop *ploop,
 						    struct pio *pio)
 {
 	bool bat_update_prepared = false;
+	struct llist_node *waiters;
 	struct ploop_index_wb *piwb;
 	bool attached = false;
 	u32 page_id;
@@ -1818,7 +1819,9 @@ static bool ploop_locate_new_cluster_and_attach_pio(struct ploop *ploop,
 	if (add_to_wblist)
 		clear_bit(MD_DIRTY, &md->status);
 	clear_bit(MD_UPDATING, &md->status);
+	waiters = llist_del_all(&md->wait_llist);
 	spin_unlock_irqrestore(&ploop->bat_lock, flags);
+	ploop_dispatch_md_waiters(ploop, waiters);
 	return false;
 }
 
@@ -2026,6 +2029,7 @@ static void ploop_process_one_discard_pio(struct ploop *ploop, struct pio *pio)
 {
 	bool bat_update_prepared = false;
 	u32 page_id, clu = pio->clu;
+	struct llist_node *waiters;
 	struct ploop_index_wb *piwb;
 	struct md_page *md;
 	map_index_t *to;
@@ -2092,7 +2096,9 @@ static void ploop_process_one_discard_pio(struct ploop *ploop, struct pio *pio)
 	if (add_to_wblist)
 		clear_bit(MD_DIRTY, &md->status);
 	clear_bit(MD_UPDATING, &md->status);
+	waiters = llist_del_all(&md->wait_llist);
 	spin_unlock_irqrestore(&ploop->bat_lock, flags);
+	ploop_dispatch_md_waiters(ploop, waiters);
 }
 
 static inline int ploop_runners_have_pending(struct ploop *ploop)
-- 
2.53.0

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

Re: [Devel] [PATCH hci-8.1 2/3] dm-ploop: fix pios stuck in md->wait_llist after a failed BAT update

[no-reply]

> From: Denis V. Lunev <den@openvz.org>
> 
> ploop_locate_new_cluster_and_attach_pio() and
> ploop_process_one_discard_pio() set MD_UPDATING on the BAT page while
> they prepare its update. A pio that needs the page meanwhile is parked
> in md->wait_llist by ploop_delay_if_md_busy(), and only
> ploop_advance_local_after_bat_wb(), at the end of a writeback of the
> page, gives the parked pios back.
> 
> When the update fails, the error path breaks it, clears MD_DIRTY and
> MD_UPDATING and leaves the parked pios where they are. No writeback of
> the page is queued, so they are never resubmitted: the writers hang in
> D state, and after a buffered write every later sync on the host hangs
> behind them until a reboot. An allocation fails this way when
> ploop_prepare_bat_update() runs out of memory, when no free cluster is
> [ ... ]
> 
> Fixes: ddf38fa44df4 ("dm-ploop: fix and rework md updates")
> Reported-by: Denis Genov <denis.genov@virtuozzo.com>
> 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/20261109190604.1850504-1-den@openvz.org?part=2


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

[Devel] [PATCH hci-8.1 3/3] dm-ploop: fail writes to a read-only device

[Denis V. Lunev]

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

bio_check_ro() only warns about a write to a read-only disk and lets it
through, and ploop_clone_and_map() passes it on into the mapping code
as on a writable device. Such a write comes from a device stacked on
top, e.g. the partition device that partprobe creates over a ploop
device; a userspace write to the ploop device itself gets EPERM.

Userspace opens the deltas of a read-only table read-only, but the data
of a pio goes to the file through f_op->write_iter() directly, which
does not check FMODE_WRITE, so it reaches the image. Allocating a cluster
punches or zeroes it with vfs_fallocate(), which does, and fails with
EBADF, so part of such a write fails with an I/O error and the rest
changes the read-only image.

Kill write requests of a read-only table in ploop_clone_and_map(), as
dm-qcow2 does since commit 409d7e697578
("drivers/md/dm-qcow2: fail writes to a read-only table"). The write
fails at once with an I/O error and the image stays untouched. Flushes
still pass.

https://virtuozzo.atlassian.net/browse/VSTOR-154096

Reported-by: Denis Genov <denis.genov@virtuozzo.com>
Feature: dm-ploop: ploop target driver
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 drivers/md/dm-ploop-map.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
index b49281691a01..f4b6606127b4 100644
--- a/drivers/md/dm-ploop-map.c
+++ b/drivers/md/dm-ploop-map.c
@@ -2653,6 +2653,9 @@ int ploop_clone_and_map(struct dm_target *ti, struct request *rq,
 			return DM_MAPIO_KILL;
 	}
 
+	if (op_is_write(req_op(rq)) && ploop_is_ro(ploop))
+		return DM_MAPIO_KILL;
+
 	if (blk_rq_bytes(rq) && ploop_rq_valid(ploop, rq) < 0)
 		return DM_MAPIO_KILL;
 
-- 
2.53.0

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

Re: [Devel] [PATCH hci-8.1 3/3] dm-ploop: fail writes to a read-only device

[no-reply]

> From: Denis V. Lunev <den@openvz.org>
> 
> bio_check_ro() only warns about a write to a read-only disk and lets it
> through, and ploop_clone_and_map() passes it on into the mapping code
> as on a writable device. Such a write comes from a device stacked on
> top, e.g. the partition device that partprobe creates over a ploop
> device; a userspace write to the ploop device itself gets EPERM.
> 
> Userspace opens the deltas of a read-only table read-only, but the data
> of a pio goes to the file through f_op->write_iter() directly, which
> does not check FMODE_WRITE, so it reaches the image. Allocating a cluster
> punches or zeroes it with vfs_fallocate(), which does, and fails with
> EBADF, so part of such a write fails with an I/O error and the rest
> changes the read-only image.
> 
> Kill write requests of a read-only table in ploop_clone_and_map(), as
> dm-qcow2 does since commit 409d7e697578
> ("drivers/md/dm-qcow2: fail writes to a read-only table"). The write
> fails at once with an I/O error and the image stays untouched. Flushes
> still pass.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-154096
> 
> Reported-by: Denis Genov <denis.genov@virtuozzo.com>
> 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/20261109190604.1850504-1-den@openvz.org?part=3


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