[Devel] [PATCH hci-8.1 v4 00/11] dm-ploop: give back unused tail of top delta on suspend

[Devel] [PATCH hci-8.1 v4 00/11] dm-ploop: give back unused tail of top delta on suspend

[Alexander Ivanov]

Preallocated space and clusters at the end of the top delta freed by
discard are never given back, so the image file never shrinks. The
last patch truncates the top delta to the end of its last used cluster
on suspend.

The next resumed ploop must see the new file size, so the previous
patch reloads all the metadata (header, BAT, file sizes) on resume.
This also fixes "dmsetup reload", after which the new ploop worked with
metadata read in ploop_ctr() while the old one kept changing images.

Patches 1-5 fix header validation and a lost wakeup in the allocator,
which the reload would otherwise hit. Patches 6-9 prepare the reload
and fix races on md pages and inflight_md that the new waits on
suspend expose.

The corresponding ploop userspace changes:
https://bitbucket.org/openvz/ploop.ovz/pull-requests/486

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

v4:
- Patch 1: was sent separately as "[PATCH hci-8.1] dm-ploop: fix u32
  overflow of the delta BAT byte size", unchanged.
- Patch 2 (was 1): drop the comment above set_current_state(), add
  Reviewed-by.
- Patches 3, 4: new. Reject a top delta header whose BAT does not
  cover the target, and reject empty deltas, instead of skipping the
  mappings of an empty delta in patch 5.
- Patch 5 (was 2): say that raw deltas have never worked since the
  driver was added; drop the empty delta handling, see patches 3, 4;
  add Reviewed-by.
- Patch 6 (was 3): fix the stale comment of ploop_add_delta() about
  the order deltas are added in.
- Patches 7, 8: split from patch 10. Patch 7 takes bat_lock around md
  page lookup in ploop_llseek_hole() to protect it from resize, so
  llseek doesn't wait for long control commands.
- Patch 9: new, fixes a use-after-free of the ploop on the last
  inflight_md decrement from BAT write completion.
- Patch 10 (was 4):
  - test MD_* flags with test_bit(), they are bit numbers;
  - keep the in-memory m_DiskInUse only after flip_upper_deltas, take
    it from disk otherwise, so the CBT written on umount isn't lost;
  - don't wait for preallocation on postsuspend in standby, and make
    the wait killable; skip the reload and prealloc on resume in
    standby or while preallocation is pending;
  - track standby per ploop, as ploop_ctr() of a new table clears the
    queue bit before the old table is suspended;
  - drop ctl_rwsem and the suspended check from ploop_llseek_hole():
    dm core doesn't call it on a suspended device;
  - describe the new preresume failures in the commit message.
- Patch 11 (was 5): don't truncate in standby, if the wait for
  preallocation was killed, or if the file size differs from
  top->file_size, i.e. another ploop writes to the top delta.
  Shorten the comments of ploop_calc_used_end(), which repeated the
  commit message.

v3:
- Split from the common series with dm-qcow2: dm-qcow2 patches are
  sent as a separate series. Patches 3-6 of v2 are patches 2-5 now.
- Patch 1: new patch.
- Patch 2: also skip applying the mappings of an empty delta, as BAT
  iterators can't handle nr_be == 0.
- Patch 4: wait for inflight_md on postsuspend, as data bios are
  completed before BAT writeback is finished, and use wake_up() for
  inflight_md_wq; reload metadata under memalloc_noio_save(), as the
  device is suspended; restore m_Sig, m_Type and m_Sectors on reload
  failure.
- Patch 5: don't truncate anything if m_FormatExtensionOffset is not
  cluster-aligned or out of range; truncate under memalloc_noio_save().

v2:
- Add dm-ploop support (patches 3-6), as requested in v1 review.
- dm-qcow2: move get_md_page_sync() to a separate patch as
  qcow2_md_page_read_sync() and use it in qcow2_parse_header() as well.
- dm-qcow2: start the refcount scan below file_preallocated_area_start
  instead of the end of the file, and truncate to the used end directly
  instead of min(file_preallocated_area_start, used end).

Alexander Ivanov (11):
  dm-ploop: fix u32 overflow of the delta BAT byte size
  dm-ploop: fix lost wakeup in ploop_allocator()
  dm-ploop: reject a delta header whose BAT does not cover the target
  dm-ploop: reject empty deltas
  dm-ploop: set delta is_raw before applying its mappings
  dm-ploop: factor out ploop_read_delta() from ploop_add_delta()
  dm-ploop: take bat_lock around md page lookup in ploop_llseek_hole()
  dm-ploop: use wake_up() for inflight_md_wq and dispatcher_wq_prealloc
  dm-ploop: fix use-after-free on the last inflight_md decrement
  dm-ploop: reload metadata on resume
  dm-ploop: truncate unused tail of top delta on suspend

 drivers/md/dm-ploop-bat.c    | 280 ++++++++++++++++++++++++++++++++---
 drivers/md/dm-ploop-cmd.c    |  26 ++--
 drivers/md/dm-ploop-map.c    |  79 ++++++----
 drivers/md/dm-ploop-target.c | 152 +++++++++++++++----
 drivers/md/dm-ploop.h        |  22 ++-
 5 files changed, 469 insertions(+), 90 deletions(-)

-- 
2.53.0

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

[Devel] [PATCH hci-8.1 v4 01/11] dm-ploop: fix u32 overflow of the delta BAT byte size

[Alexander Ivanov]

The byte size of header and BAT, (PLOOP_MAP_OFFSET + m_Size) *
sizeof(map_index_t), is computed in u32 in several places. It wraps for
m_Size >= 2^30 - PLOOP_MAP_OFFSET, which a delta with small clusters
may legally have.

Compute the size in u64 and use ploop_nr_bat_clusters() for the number
of BAT clusters everywhere.

Reject a header or a resize whose hb_nr exceeds PLOOP_MAX_HB_NR,
which also keeps DIV_ROUND_UP(hb_nr, 8) and the bitmap size in bits
from wrapping.

https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-ploop: ploop target driver
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 drivers/md/dm-ploop-bat.c | 23 ++++++++++++-----------
 drivers/md/dm-ploop-cmd.c | 19 +++++++++----------
 drivers/md/dm-ploop.h     |  6 ++++--
 3 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/drivers/md/dm-ploop-bat.c b/drivers/md/dm-ploop-bat.c
index a7849de8aaa6..5ddb8ffdff07 100644
--- a/drivers/md/dm-ploop-bat.c
+++ b/drivers/md/dm-ploop-bat.c
@@ -189,7 +189,6 @@ int ploop_setup_metadata(struct ploop *ploop, struct page *page)
 	struct ploop_pvd_header *m_hdr = NULL;
 	u32 bat_clusters, offset_clusters;
 	struct dm_target *ti = ploop->ti;
-	unsigned long size;
 	int ret;
 
 	m_hdr = kmap(page);
@@ -213,8 +212,11 @@ int ploop_setup_metadata(struct ploop *ploop, struct page *page)
 	ploop->nr_bat_entries = le32_to_cpu(m_hdr->m_Size);
 
 	/* Header and BAT-occupied clusters at start of file */
-	size = (PLOOP_MAP_OFFSET + ploop->nr_bat_entries) * sizeof(map_index_t);
-	bat_clusters = DIV_ROUND_UP(size, CLU_SIZE(ploop));
+	bat_clusters = ploop_nr_bat_clusters(ploop, ploop->nr_bat_entries);
+	if ((u64)bat_clusters + ploop->nr_bat_entries > PLOOP_MAX_HB_NR) {
+		PL_ERR("Too large BAT");
+		goto out;
+	}
 
 	/* Clusters from start of file to first data block */
 	offset_clusters = SEC_TO_CLU(ploop, le32_to_cpu(m_hdr->m_FirstBlockOffset));
@@ -242,7 +244,7 @@ static int ploop_delta_check_header(struct ploop *ploop,
 				    struct rb_root *md_root,
 				    u32 *delta_nr_be_ret)
 {
-	u32 bytes, delta_nr_be, offset_clusters, bat_clusters;
+	u32 delta_nr_be, offset_clusters, bat_clusters;
 	struct md_page *md0 = ploop_md_first_entry(md_root);
 	struct ploop_pvd_header *d_hdr;
 	int ret = -EPROTO;
@@ -257,8 +259,7 @@ static int ploop_delta_check_header(struct ploop *ploop,
 
 	delta_nr_be = le32_to_cpu(d_hdr->m_Size);
 	offset_clusters = SEC_TO_CLU(ploop, le32_to_cpu(d_hdr->m_FirstBlockOffset));
-	bytes = (PLOOP_MAP_OFFSET + delta_nr_be) * sizeof(map_index_t);
-	bat_clusters = DIV_ROUND_UP(bytes, CLU_SIZE(ploop));
+	bat_clusters = ploop_nr_bat_clusters(ploop, delta_nr_be);
 
 	if (delta_nr_be > ploop->nr_bat_entries ||
 	    bat_clusters != offset_clusters)
@@ -274,13 +275,12 @@ ALLOW_ERROR_INJECTION(ploop_delta_check_header, ERRNO);
 static int ploop_convert_bat_entries(struct ploop *ploop, struct rb_root *md_root,
 			       u32 nr_be, u32 nr_pages, loff_t file_size)
 {
-	u32 i, end, bytes, bat_clusters, page_id, *bat_entries, max_file_clu;
+	u32 i, end, bat_clusters, page_id, *bat_entries, max_file_clu;
 	struct rb_node *node;
 	struct md_page *md;
 	int ret = 0;
 
-	bytes = (PLOOP_MAP_OFFSET + nr_be) * sizeof(map_index_t);
-	bat_clusters = DIV_ROUND_UP(bytes, CLU_SIZE(ploop));
+	bat_clusters = ploop_nr_bat_clusters(ploop, nr_be);
 	max_file_clu = file_size / CLU_SIZE(ploop) - 1;
 
 	page_id = 0;
@@ -311,12 +311,13 @@ int ploop_read_delta_metadata(struct ploop *ploop, struct file *file,
 			      struct rb_root *md_root, u32 *delta_nr_be_ret)
 {
 	struct bio_vec bvec_on_stack, *bvec = &bvec_on_stack;
-	u32 i, size, delta_nr_be, nr_segs;
+	u32 i, delta_nr_be, nr_segs;
 	loff_t pos, file_size;
 	struct iov_iter iter;
 	struct rb_node *node;
 	struct md_page *md;
 	ssize_t len;
+	u64 size;
 	int ret;
 
 	ret = -ENOMEM;
@@ -339,7 +340,7 @@ int ploop_read_delta_metadata(struct ploop *ploop, struct file *file,
 	if (ret)
 		goto out;
 
-	size = (PLOOP_MAP_OFFSET + delta_nr_be) * sizeof(map_index_t);
+	size = ((u64)delta_nr_be + PLOOP_MAP_OFFSET) * sizeof(map_index_t);
 	size = ALIGN(size, PAGE_SIZE); /* file may be open as direct */
 	nr_segs = size / PAGE_SIZE;
 
diff --git a/drivers/md/dm-ploop-cmd.c b/drivers/md/dm-ploop-cmd.c
index 5fa78e6cff9a..ab17520c2d56 100644
--- a/drivers/md/dm-ploop-cmd.c
+++ b/drivers/md/dm-ploop-cmd.c
@@ -386,7 +386,7 @@ ALLOW_ERROR_INJECTION(ploop_grow_relocate_cluster, ERRNO);
 static int ploop_grow_update_header(struct ploop *ploop,
 				    struct ploop_cmd *cmd)
 {
-	unsigned int size, first_block_off;
+	unsigned int first_block_off;
 	struct ploop_pvd_header *hdr;
 	struct ploop_index_wb *piwb;
 	u32 nr_be, offset, clus;
@@ -413,9 +413,7 @@ static int ploop_grow_update_header(struct ploop *ploop,
 	}
 	piwb = md->piwb;
 
-	size = (PLOOP_MAP_OFFSET + cmd->resize.nr_bat_entries);
-	size *= sizeof(map_index_t);
-	clus = DIV_ROUND_UP(size, CLU_SIZE(ploop));
+	clus = ploop_nr_bat_clusters(ploop, cmd->resize.nr_bat_entries);
 	first_block_off = CLU_TO_SEC(ploop, clus);
 
 	hdr = kmap_local_page(piwb->bat_page);
@@ -597,6 +595,11 @@ static int ploop_resize(struct ploop *ploop, sector_t new_sectors)
 	}
 
 	nr_bat_entries = SEC_TO_CLU(ploop, new_sectors);
+	nr_bat_clusters = ploop_nr_bat_clusters(ploop, nr_bat_entries);
+	if ((u64)nr_bat_clusters + nr_bat_entries > PLOOP_MAX_HB_NR) {
+		DMWARN("resize: too large size is requested");
+		return -EINVAL;
+	}
 
 	/* Memory for new md pages */
 	ret = ploop_prealloc_md_pages(&cmd.resize.md_pages_root,
@@ -604,8 +607,6 @@ static int ploop_resize(struct ploop *ploop, sector_t new_sectors)
 	if (ret)
 		return ret;
 
-	size = (PLOOP_MAP_OFFSET + nr_bat_entries) * sizeof(map_index_t);
-	nr_bat_clusters = DIV_ROUND_UP(size, CLU_SIZE(ploop));
 	hb_nr = nr_bat_clusters + nr_bat_entries;
 	size = round_up(DIV_ROUND_UP(hb_nr, 8), sizeof(unsigned long));
 
@@ -1010,11 +1011,9 @@ static int process_flip_upper_deltas(struct ploop *ploop)
 	u8 level = ploop_top_level(ploop) - 1;
 	struct rb_node *node;
 	struct md_page *md;
-	u64 size;
 
-	size = (PLOOP_MAP_OFFSET + READ_ONCE(ploop->nr_bat_entries))
-		* sizeof(map_index_t);
-        bat_clusters = DIV_ROUND_UP(size, CLU_SIZE(ploop));
+	bat_clusters = ploop_nr_bat_clusters(ploop,
+					     READ_ONCE(ploop->nr_bat_entries));
 	hb_nr = ploop->hb_nr;
 
 	/*
diff --git a/drivers/md/dm-ploop.h b/drivers/md/dm-ploop.h
index 23662943d56a..5041fc8f1e13 100644
--- a/drivers/md/dm-ploop.h
+++ b/drivers/md/dm-ploop.h
@@ -12,6 +12,8 @@
 #include <linux/bio.h>
 
 #define PLOOP_MAP_OFFSET 16
+/* Keeps DIV_ROUND_UP(hb_nr, 8) and clu + PLOOP_MAP_OFFSET within u32 */
+#define PLOOP_MAX_HB_NR round_down(U32_MAX - PLOOP_MAP_OFFSET, 8)
 typedef u32 map_index_t;
 #define BAT_ENTRIES_PER_PAGE (PAGE_SIZE / sizeof(map_index_t))
 
@@ -410,9 +412,9 @@ static inline unsigned int ploop_nr_pages_in_cluster(struct ploop *ploop)
 static inline unsigned int ploop_nr_bat_clusters(struct ploop *ploop,
 						 u32 nr_bat_entries)
 {
-	unsigned long size, bat_clusters;
+	u64 size, bat_clusters;
 
-	size = (PLOOP_MAP_OFFSET + nr_bat_entries) * sizeof(map_index_t);
+	size = ((u64)nr_bat_entries + PLOOP_MAP_OFFSET) * sizeof(map_index_t);
 	bat_clusters = DIV_ROUND_UP(size, CLU_SIZE(ploop));
 
 	return bat_clusters;
-- 
2.53.0

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

[Devel] [PATCH hci-8.1 v4 02/11] dm-ploop: fix lost wakeup in ploop_allocator()

[Alexander Ivanov]

ploop_allocator() checks prealloc_size and kthread_should_stop() before
setting TASK_INTERRUPTIBLE. If ploop_req_prealloc() sets prealloc_size
and calls wake_up_process() in between, the wakeup finds the task
running and is lost, so the allocator goes to sleep with a pending
preallocation request. Later requests for the same size return early
without waking it up, so the request may stay pending forever and
waiters on dispatcher_wq_prealloc hang. The same applies to
kthread_stop().

Set the task state before checking the conditions, as the canonical
kthread loop does. set_current_state() implies a full barrier, which
pairs with the one in try_to_wake_up().

https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-ploop: ploop target driver

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
---
 drivers/md/dm-ploop-map.c | 44 ++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
index 48466435dd19..92ceadf41b42 100644
--- a/drivers/md/dm-ploop-map.c
+++ b/drivers/md/dm-ploop-map.c
@@ -2347,31 +2347,33 @@ int ploop_allocator(void *data)
 	struct file *file;
 
 	for (;;) {
-		/* 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);
-			file = ploop_top_delta(ploop)->file;
-			current->flags |= PLOOP_PFLAGS;
-			ret = ploop_preallocate_cluster(ploop, file);
-			if (ret) {
-				/*
-				 * threads requesting prealloc must check
-				 * if size actually changed
-				 */
-				/* ENOSPC is only for fallocate */
-				pr_err_ratelimited(
-					PL_FMT("failed to allocate space: %d\n"),
-					       ploop_device_name(ploop), ret);
-			}
-			wake_up_interruptible(&ploop->dispatcher_wq_prealloc);
-			current_restore_flags(old_flags, PLOOP_PFLAGS);
-		}
+		set_current_state(TASK_INTERRUPTIBLE);
 		if (kthread_should_stop()) {
 			__set_current_state(TASK_RUNNING);
 			break;
 		}
-		set_current_state(TASK_INTERRUPTIBLE);
-		schedule();
+		/* we do not care about exact number here - risk one extra schedule wrt taking the lock */
+		if (!READ_ONCE(ploop->prealloc_size)) {
+			schedule();
+			continue;
+		}
+		__set_current_state(TASK_RUNNING);
+
+		file = ploop_top_delta(ploop)->file;
+		current->flags |= PLOOP_PFLAGS;
+		ret = ploop_preallocate_cluster(ploop, file);
+		if (ret) {
+			/*
+			 * threads requesting prealloc must check
+			 * if size actually changed
+			 */
+			/* ENOSPC is only for fallocate */
+			pr_err_ratelimited(
+				PL_FMT("failed to allocate space: %d\n"),
+				       ploop_device_name(ploop), ret);
+		}
+		wake_up_interruptible(&ploop->dispatcher_wq_prealloc);
+		current_restore_flags(old_flags, PLOOP_PFLAGS);
 	}
 
 	return 0;
-- 
2.53.0

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

[Devel] [PATCH hci-8.1 v4 03/11] dm-ploop: reject a delta header whose BAT does not cover the target

[Alexander Ivanov]

ploop_setup_metadata() takes the number of BAT entries from m_Size of
the top delta header, but checks only m_SizeInSectors_v2 against the
target length. A header with a small m_Size (including zero) and a large
enough m_SizeInSectors_v2 passes all checks. Requests past the last BAT
entry then hit WARN_ONCE() in ploop_rq_valid(), which panics the host
with panic_on_warn.

Reject the header if the BAT does not cover the target, taking the
skip_off shift of request positions into account.

https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-ploop: ploop target driver

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 drivers/md/dm-ploop-bat.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/md/dm-ploop-bat.c b/drivers/md/dm-ploop-bat.c
index 5ddb8ffdff07..7b12b0f9fe81 100644
--- a/drivers/md/dm-ploop-bat.c
+++ b/drivers/md/dm-ploop-bat.c
@@ -10,6 +10,7 @@
 #include <linux/uio.h>
 #include <linux/error-injection.h>
 #include <linux/mm.h>
+#include <linux/overflow.h>
 #include "dm-ploop.h"
 
 struct md_page *ploop_md_page_find(struct ploop *ploop, u32 id)
@@ -189,6 +190,7 @@ int ploop_setup_metadata(struct ploop *ploop, struct page *page)
 	struct ploop_pvd_header *m_hdr = NULL;
 	u32 bat_clusters, offset_clusters;
 	struct dm_target *ti = ploop->ti;
+	u64 end;
 	int ret;
 
 	m_hdr = kmap(page);
@@ -229,6 +231,11 @@ int ploop_setup_metadata(struct ploop *ploop, struct page *page)
 		PL_ERR("Too short BAT");
 		goto out;
 	}
+	if (check_add_overflow((u64)ti->len, ploop->skip_off, &end) ||
+	    SEC_TO_CLU(ploop, end - 1) >= ploop->nr_bat_entries) {
+		PL_ERR("BAT does not cover the target");
+		goto out;
+	}
 	kunmap(page);
 	m_hdr = NULL;
 
-- 
2.53.0

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

[Devel] [PATCH hci-8.1 v4 04/11] dm-ploop: reject empty deltas

[Alexander Ivanov]

ploop_delta_check_header() accepts a header with m_Size == 0, and
ploop_add_delta() accepts a raw delta of zero size. The BAT iterators
can't handle nr_be == 0: ploop_bat_clu_to_page_nr(nr_be - 1) wraps to
page 0, so md pages past the first one are iterated whole and the
clu == nr_be - 1 stop condition never matches. A non-raw empty delta
has a single md page, so ploop_apply_delta_mappings() and
notify_delta_merged() (and ploop_check_delta_before_flip() with
PLOOP_DEBUG) walk past it and dereference a NULL d_md on a device with
more than one md page. A raw delta is not handled as raw yet, as
ploop_add_delta() sets is_raw only after applying its mappings; once it
is, an empty raw delta would get all the clusters mapped to it.

Reject empty deltas in ploop_delta_check_header(), which covers every
caller reading a delta header, and in ploop_add_delta() for raw deltas.

https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-ploop: ploop target driver

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 drivers/md/dm-ploop-bat.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-ploop-bat.c b/drivers/md/dm-ploop-bat.c
index 7b12b0f9fe81..4fc99400a0a4 100644
--- a/drivers/md/dm-ploop-bat.c
+++ b/drivers/md/dm-ploop-bat.c
@@ -268,7 +268,7 @@ static int ploop_delta_check_header(struct ploop *ploop,
 	offset_clusters = SEC_TO_CLU(ploop, le32_to_cpu(d_hdr->m_FirstBlockOffset));
 	bat_clusters = ploop_nr_bat_clusters(ploop, delta_nr_be);
 
-	if (delta_nr_be > ploop->nr_bat_entries ||
+	if (!delta_nr_be || delta_nr_be > ploop->nr_bat_entries ||
 	    bat_clusters != offset_clusters)
 		goto out;
 
@@ -503,6 +503,9 @@ int ploop_add_delta(struct ploop *ploop, u32 level, struct file *file, bool is_r
 			goto out;
 	} else {
 		nr_be = POS_TO_CLU(ploop, file_size);
+		ret = -EPROTO;
+		if (!nr_be)
+			goto out;
 	}
 
 	ret = -EBADSLT; /* Lower delta can't be bigger then upper */
-- 
2.53.0

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

[Devel] [PATCH hci-8.1 v4 05/11] dm-ploop: set delta is_raw before applying its mappings

[Alexander Ivanov]

ploop_apply_delta_mappings() takes is_raw from deltas[level], but
ploop_add_delta() sets deltas[level].is_raw only after the mappings
are applied. So a raw delta is handled as a non-raw one: its md_root
is empty, ploop_md_first_entry() returns NULL and d_md->kmpage
dereferences it.

This has been the case since the driver was added in commit
6ddad5a4d226 ("dm-ploop: Add ploop target driver"), i.e. raw deltas
have never worked here; this is not a regression.

Set is_raw before applying the mappings.

https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-ploop: ploop target driver

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
---
 drivers/md/dm-ploop-bat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/dm-ploop-bat.c b/drivers/md/dm-ploop-bat.c
index 4fc99400a0a4..3427d1a90d81 100644
--- a/drivers/md/dm-ploop-bat.c
+++ b/drivers/md/dm-ploop-bat.c
@@ -512,6 +512,7 @@ int ploop_add_delta(struct ploop *ploop, u32 level, struct file *file, bool is_r
 	if (level != ploop_top_level(ploop) && nr_be > deltas[level + 1].nr_be)
 		goto out;
 
+	deltas[level].is_raw = is_raw;
 	ploop_apply_delta_mappings(ploop, deltas, level, &md_root, nr_be);
 
 	deltas[level].file = file;
@@ -532,7 +533,6 @@ int ploop_add_delta(struct ploop *ploop, u32 level, struct file *file, bool is_r
 	deltas[level].file_size = file_size;
 	deltas[level].file_preallocated_area_start = file_size;
 	deltas[level].nr_be = nr_be;
-	deltas[level].is_raw = is_raw;
 	ret = 0;
 out:
 	ploop_free_md_pages_tree(&md_root);
-- 
2.53.0

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

[Devel] [PATCH hci-8.1 v4 06/11] dm-ploop: factor out ploop_read_delta() from ploop_add_delta()

[Alexander Ivanov]

Move reading of delta BAT and applying its mappings to
ploop->bat_entries into a separate function ploop_read_delta(). It
will be used to reload metadata of already added deltas.

ploop_read_delta() takes is_raw from deltas[level], so ploop_add_delta()
sets it before the call.

Replace the stale comment of ploop_add_delta(): it claimed deltas are
added from oldest to newest, while ploop_add_deltas_stack() adds them
from top to base, which ploop_read_delta() relies on.

No functional changes.

https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-ploop: ploop target driver

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
---
 drivers/md/dm-ploop-bat.c | 43 +++++++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 8 deletions(-)

diff --git a/drivers/md/dm-ploop-bat.c b/drivers/md/dm-ploop-bat.c
index 3427d1a90d81..5a89771a2e02 100644
--- a/drivers/md/dm-ploop-bat.c
+++ b/drivers/md/dm-ploop-bat.c
@@ -481,23 +481,27 @@ int ploop_check_delta_length(struct ploop *ploop, struct file *file, loff_t *fil
 ALLOW_ERROR_INJECTION(ploop_check_delta_length, ERRNO);
 
 /*
- * @fd refers to a new delta, which is placed right before top_delta.
- * So, userspace has to populate deltas stack from oldest to newest.
+ * Read BAT of delta @level and apply its mappings to ploop->bat_entries.
+ * deltas[@level].is_raw and nr_be of upper deltas must be already set.
+ *
+ * Deltas must be read from top to base: mappings of upper deltas take
+ * precedence (already populated BAT entries are not overwritten), and
+ * size of the delta is checked against nr_be of the upper one.
  */
-int ploop_add_delta(struct ploop *ploop, u32 level, struct file *file, bool is_raw)
+static int ploop_read_delta(struct ploop *ploop, u32 level, struct file *file,
+			    loff_t *file_size_ret, u32 *nr_be_ret)
 {
 	struct ploop_delta *deltas = ploop->deltas;
 	struct rb_root md_root = RB_ROOT;
-	struct file *cloned_file = NULL;
 	loff_t file_size;
 	u32 nr_be;
-	int ret, i;
+	int ret;
 
 	ret = ploop_check_delta_length(ploop, file, &file_size);
 	if (ret)
 		goto out;
 
-	if (!is_raw) {
+	if (!deltas[level].is_raw) {
 		ret = ploop_read_delta_metadata(ploop, file, &md_root, &nr_be);
 		if (ret)
 			goto out;
@@ -512,9 +516,33 @@ int ploop_add_delta(struct ploop *ploop, u32 level, struct file *file, bool is_r
 	if (level != ploop_top_level(ploop) && nr_be > deltas[level + 1].nr_be)
 		goto out;
 
-	deltas[level].is_raw = is_raw;
 	ploop_apply_delta_mappings(ploop, deltas, level, &md_root, nr_be);
 
+	*file_size_ret = file_size;
+	*nr_be_ret = nr_be;
+	ret = 0;
+out:
+	ploop_free_md_pages_tree(&md_root);
+	return ret;
+}
+
+/*
+ * Add @file as delta @level. Deltas must be added from top to base,
+ * see ploop_read_delta().
+ */
+int ploop_add_delta(struct ploop *ploop, u32 level, struct file *file, bool is_raw)
+{
+	struct ploop_delta *deltas = ploop->deltas;
+	struct file *cloned_file = NULL;
+	loff_t file_size;
+	u32 nr_be;
+	int ret, i;
+
+	deltas[level].is_raw = is_raw;
+	ret = ploop_read_delta(ploop, level, file, &file_size, &nr_be);
+	if (ret)
+		goto out;
+
 	deltas[level].file = file;
 	deltas[level].mtfile = kcalloc(ploop->nkt_runners, sizeof(*file),
 					GFP_KERNEL);
@@ -535,7 +563,6 @@ int ploop_add_delta(struct ploop *ploop, u32 level, struct file *file, bool is_r
 	deltas[level].nr_be = nr_be;
 	ret = 0;
 out:
-	ploop_free_md_pages_tree(&md_root);
 	return ret;
 }
 ALLOW_ERROR_INJECTION(ploop_add_delta, ERRNO);
-- 
2.53.0

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

[Devel] [PATCH hci-8.1 v4 07/11] dm-ploop: take bat_lock around md page lookup in ploop_llseek_hole()

[Alexander Ivanov]

ploop_llseek_hole() walks the md pages tree without any lock, while
resize inserts new md pages into it in ploop_add_md_pages(). A lookup
racing with rb-tree rebalancing may follow a stale pointer or miss the
page and return -EINVAL.

https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-ploop: ploop target driver

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 drivers/md/dm-ploop-map.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
index 92ceadf41b42..691284a239df 100644
--- a/drivers/md/dm-ploop-map.c
+++ b/drivers/md/dm-ploop-map.c
@@ -2772,7 +2772,11 @@ loff_t ploop_llseek_hole(struct dm_target *ti, loff_t offset, int whence)
 	while (clu < READ_ONCE(ploop->nr_bat_entries)) {
 		if (id != ploop_bat_clu_to_page_nr(clu)) {
 			id = ploop_bat_clu_to_page_nr(clu);
+
+			spin_lock_irq(&ploop->bat_lock);
 			md = ploop_md_page_find(ploop, id);
+			spin_unlock_irq(&ploop->bat_lock);
+
 			if (!md)
 				return -EINVAL;
 		}
-- 
2.53.0

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

[Devel] [PATCH hci-8.1 v4 08/11] dm-ploop: use wake_up() for inflight_md_wq and dispatcher_wq_prealloc

[Alexander Ivanov]

wake_up_interruptible() wakes only TASK_INTERRUPTIBLE waiters. All the
current waiters on inflight_md_wq and dispatcher_wq_prealloc sleep
interruptibly, but one of the next patches adds uninterruptible ones on
postsuspend, which would never be woken. Use wake_up(), which wakes
both kinds.

https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-ploop: ploop target driver

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 drivers/md/dm-ploop-map.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
index 691284a239df..c313f303fff9 100644
--- a/drivers/md/dm-ploop-map.c
+++ b/drivers/md/dm-ploop-map.c
@@ -926,7 +926,7 @@ static void ploop_put_piwb(struct ploop_index_wb *piwb)
 		 */
 
 		if (atomic_dec_and_test(&piwb->ploop->inflight_md))
-			wake_up_interruptible(&piwb->ploop->inflight_md_wq);
+			wake_up(&piwb->ploop->inflight_md_wq);
 		ploop_free_piwb(piwb);
 	}
 }
@@ -1106,7 +1106,7 @@ void ploop_break_bat_update(struct ploop *ploop, struct md_page *md,
 	spin_unlock_irqrestore(&ploop->bat_lock, flags);
 
 	if (atomic_dec_and_test(&ploop->inflight_md))
-		wake_up_interruptible(&ploop->inflight_md_wq);
+		wake_up(&ploop->inflight_md_wq);
 	ploop_free_piwb(piwb);
 }
 
@@ -2372,7 +2372,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);
 	}
 
-- 
2.53.0

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

[Devel] [PATCH hci-8.1 v4 09/11] dm-ploop: fix use-after-free on the last inflight_md decrement

[Alexander Ivanov]

The last piwb reference is often dropped from the BAT write completion
in aio context. ploop_bat_write_finish() completes the data pios there,
and once the last original request is done, suspend can finish and a
table reload or remove can free the ploop in ploop_destroy(). The
completing CPU still decrements inflight_md, calls wake_up() on
inflight_md_wq and frees the piwb into ploop->pio_pool, so it can lock
a freed waitqueue or free into a destroyed mempool.

Nothing on the suspend or destroy path waits for inflight_md, and the
one waiter that exists, ploop_suspend_submitting_pios(), checks the
counter without a lock. It can see zero and return before wake_up() and
ploop_free_piwb() are done with the ploop, so the message can finish
and the table can be destroyed under the waker.

Free the piwb first, then decrement inflight_md and wake the waiters
under inflight_md_wq.lock as the last access to the ploop. Have the
waiters check the counter under the same lock, so they cannot see zero
until the waker has released it, and wait for it in ploop_destroy()
before freeing anything.

https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-ploop: ploop target driver

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 drivers/md/dm-ploop-cmd.c    |  6 ++++--
 drivers/md/dm-ploop-map.c    | 22 ++++++++++++++++++----
 drivers/md/dm-ploop-target.c |  7 +++++++
 3 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/drivers/md/dm-ploop-cmd.c b/drivers/md/dm-ploop-cmd.c
index ab17520c2d56..977b6920a5b4 100644
--- a/drivers/md/dm-ploop-cmd.c
+++ b/drivers/md/dm-ploop-cmd.c
@@ -154,8 +154,10 @@ static int ploop_suspend_submitting_pios(struct ploop *ploop)
 	if (ret)
 		goto out;
 
-	ret = wait_event_interruptible(ploop->inflight_md_wq,
-				       !atomic_read(&ploop->inflight_md));
+	spin_lock_irq(&ploop->inflight_md_wq.lock);
+	ret = wait_event_interruptible_locked_irq(ploop->inflight_md_wq,
+					!atomic_read(&ploop->inflight_md));
+	spin_unlock_irq(&ploop->inflight_md_wq.lock);
 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 c313f303fff9..f673e94d0e38 100644
--- a/drivers/md/dm-ploop-map.c
+++ b/drivers/md/dm-ploop-map.c
@@ -909,9 +909,25 @@ static void ploop_free_piwb(struct ploop_index_wb *piwb)
 
 static void ploop_bat_write_finish(struct pio *pio, void *piwb_ptr,
 				     blk_status_t bi_status);
+/*
+ * Waiters check inflight_md under inflight_md_wq.lock, so they cannot see
+ * zero while we still hold it. Once it is zero, the ploop may be freed, so
+ * this must be the last access to it.
+ */
+static void ploop_dec_inflight_md(struct ploop *ploop)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&ploop->inflight_md_wq.lock, flags);
+	if (atomic_dec_and_test(&ploop->inflight_md))
+		wake_up_locked(&ploop->inflight_md_wq);
+	spin_unlock_irqrestore(&ploop->inflight_md_wq.lock, flags);
+}
+
 static void ploop_put_piwb(struct ploop_index_wb *piwb)
 {
 	if (atomic_dec_and_test(&piwb->count)) {
+		struct ploop *ploop = piwb->ploop;
 
 		ploop_bat_write_finish(piwb->pio, piwb, piwb->bi_status);
 
@@ -925,9 +941,8 @@ static void ploop_put_piwb(struct ploop_index_wb *piwb)
 		 * zero keeps it as is
 		 */
 
-		if (atomic_dec_and_test(&piwb->ploop->inflight_md))
-			wake_up(&piwb->ploop->inflight_md_wq);
 		ploop_free_piwb(piwb);
+		ploop_dec_inflight_md(ploop);
 	}
 }
 
@@ -1105,9 +1120,8 @@ void ploop_break_bat_update(struct ploop *ploop, struct md_page *md,
 	spin_unlock(&md->md_lock);
 	spin_unlock_irqrestore(&ploop->bat_lock, flags);
 
-	if (atomic_dec_and_test(&ploop->inflight_md))
-		wake_up(&ploop->inflight_md_wq);
 	ploop_free_piwb(piwb);
+	ploop_dec_inflight_md(ploop);
 }
 
 static void ploop_bat_page_zero_cluster(struct ploop *ploop,
diff --git a/drivers/md/dm-ploop-target.c b/drivers/md/dm-ploop-target.c
index 3b589579741c..11d25277d60c 100644
--- a/drivers/md/dm-ploop-target.c
+++ b/drivers/md/dm-ploop-target.c
@@ -192,6 +192,13 @@ static void ploop_destroy(struct ploop *ploop)
 		if (ploop->kt_allocator)
 			kthread_stop(ploop->kt_allocator->task);
 
+		/* BAT writes may still be completing from aio context */
+		spin_lock_irq(&ploop->inflight_md_wq.lock);
+		wait_event_lock_irq(ploop->inflight_md_wq,
+				    !atomic_read(&ploop->inflight_md),
+				    ploop->inflight_md_wq.lock);
+		spin_unlock_irq(&ploop->inflight_md_wq.lock);
+
 		WARN_ON(!llist_empty(&ploop->pios[PLOOP_LIST_PREPARE]));
 		WARN_ON(!llist_empty(&ploop->llresubmit_pios));
 		WARN_ON(!llist_empty(&ploop->enospc_pios));
-- 
2.53.0

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

[Devel] [PATCH hci-8.1 v4 10/11] dm-ploop: reload metadata on resume

[Alexander Ivanov]

On "dmsetup reload" the new ploop reads image metadata in ploop_ctr(),
but IO keeps going through the old ploop until suspend, and userspace
may modify images of a suspended device (merge zeroes BAT entries of
the base delta, snapshot creation trims the top delta). So after resume
the new ploop may work with a stale BAT and file sizes, e.g. allocate
clusters already in use.

Reload all metadata in ploop_preresume(): the top delta header, BAT of
every delta and delta file sizes, and rebuild the BAT tree and
holes_bitmap. On error the old metadata is kept and resume fails. DM
has already swapped the new table in by then, so the device stays
suspended and the next resume retries. Userspace must check the result
and must not resume the previous instance on the same images.

Skip the in-use signature check on reload, it is done in ploop_ctr().
Take the signature from disk: libploop marks the image closed on the
suspended device before umount, and an in-memory IN_USE written over
it makes umount trim the image and lose CBT. After flip_upper_deltas
keep the in-memory signature, as userspace relies on it being written
to the new top delta, otherwise the next load fails with -ENOLCK after
online merge.

If metadata is dirty after suspend, keep it, as md pages may still be
referenced. Data bios complete before BAT writeback, so wait for
inflight_md in postsuspend.

Request preallocation in ploop_preresume() after the reload instead of
in ploop_ctr(), so the new ploop doesn't truncate the top delta back to
a stale size before resume. Wait for pending preallocation in
postsuspend, killably, as the allocator may hang on a dead file. Don't
wait in standby, so that dm_replace() of libploop can suspend the
device to recover it. Skip the reload and prealloc on resume while
preallocation is pending, as the allocator changes the top delta size
under bat_lock only. Skip the reload in standby too, as the images may
be inaccessible.

Track standby per ploop instead of the queue bit. The queue is shared
by all tables, and ploop_ctr() of a new table clears the bit before the
old table is suspended, so the old ploop would wait for its stuck
allocator in postsuspend.

ploop_llseek_hole() needs no locking against the reload:
dm_llseek_hole() calls it under md->io_barrier and returns -EAGAIN on
a suspended device.

https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-ploop: ploop target driver

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 drivers/md/dm-ploop-bat.c    | 146 ++++++++++++++++++++++++++++++++++-
 drivers/md/dm-ploop-cmd.c    |   1 +
 drivers/md/dm-ploop-map.c    |   9 ++-
 drivers/md/dm-ploop-target.c |  87 +++++++++++++++------
 drivers/md/dm-ploop.h        |  13 +++-
 5 files changed, 227 insertions(+), 29 deletions(-)

diff --git a/drivers/md/dm-ploop-bat.c b/drivers/md/dm-ploop-bat.c
index 5a89771a2e02..c060afdd8d3e 100644
--- a/drivers/md/dm-ploop-bat.c
+++ b/drivers/md/dm-ploop-bat.c
@@ -185,7 +185,8 @@ static int ploop_setup_holes_bitmap(struct ploop *ploop, u32 bat_clusters)
 }
 ALLOW_ERROR_INJECTION(ploop_setup_holes_bitmap, ERRNO);
 
-int ploop_setup_metadata(struct ploop *ploop, struct page *page)
+int ploop_setup_metadata(struct ploop *ploop, struct page *page,
+			 bool check_in_use)
 {
 	struct ploop_pvd_header *m_hdr = NULL;
 	u32 bat_clusters, offset_clusters;
@@ -200,7 +201,8 @@ int ploop_setup_metadata(struct ploop *ploop, struct page *page)
 		goto out;
 
 	ret = -ENOLCK;
-	if (m_hdr->m_DiskInUse != cpu_to_le32(SIGNATURE_DISK_IN_USE) &&
+	if (check_in_use &&
+	    m_hdr->m_DiskInUse != cpu_to_le32(SIGNATURE_DISK_IN_USE) &&
 	    !ploop_is_ro(ploop) && !ignore_signature_disk_in_use)
 		goto out;
 
@@ -567,6 +569,146 @@ int ploop_add_delta(struct ploop *ploop, u32 level, struct file *file, bool is_r
 }
 ALLOW_ERROR_INJECTION(ploop_add_delta, ERRNO);
 
+static bool ploop_md_pages_are_clean(struct ploop *ploop)
+{
+	struct rb_node *node;
+	struct md_page *md;
+
+	if (!llist_empty(&ploop->wb_batch_llist) ||
+	    atomic_read(&ploop->inflight_md))
+		return false;
+
+	ploop_for_each_md_page(ploop, md, node) {
+		if (test_bit(MD_DIRTY, &md->status) ||
+		    test_bit(MD_WRITEBACK, &md->status) ||
+		    test_bit(MD_UPDATING, &md->status) ||
+		    md->piwb || !llist_empty(&md->wait_llist))
+			return false;
+	}
+
+	return true;
+}
+
+/*
+ * Build in-memory metadata from scratch: re-read header of top delta,
+ * BAT of every delta and sizes of delta files. This is called on resume
+ * of suspended device, since after "dmsetup reload" the metadata of the
+ * new ploop may be outdated: the images could be changed by the old
+ * ploop after the new one was constructed. On error the old metadata
+ * is kept.
+ */
+int ploop_reload_metadata(struct ploop *ploop)
+{
+	struct rb_root old_bat_entries = ploop->bat_entries;
+	void *old_holes_bitmap = ploop->holes_bitmap;
+	u32 old_nr_bat_entries = ploop->nr_bat_entries;
+	u32 old_hb_nr = ploop->hb_nr;
+	u32 old_m_Type = ploop->m_Type;
+	u32 old_m_Sectors = ploop->m_Sectors;
+	struct ploop_pvd_header *hdr, *old_hdr;
+	u8 old_m_Sig[sizeof(ploop->m_Sig)];
+	struct ploop_delta *delta, *old_deltas;
+	struct page *page;
+	loff_t file_size;
+	u32 nr_be;
+	int level, ret;
+
+	/*
+	 * Metadata of the device that has already been running is up to
+	 * date, so we only need to reload clean metadata. Dirty metadata
+	 * must not appear after suspend, as all IO and BAT writeback are
+	 * completed (see ploop_postsuspend()), but we can't free md pages,
+	 * which are still in use, in any case.
+	 */
+	if (WARN_ON_ONCE(!ploop_md_pages_are_clean(ploop)))
+		return 0;
+
+	memcpy(old_m_Sig, ploop->m_Sig, sizeof(old_m_Sig));
+
+	old_deltas = kmemdup(ploop->deltas,
+			     sizeof(*old_deltas) * ploop->nr_deltas,
+			     GFP_KERNEL);
+	page = alloc_page(GFP_KERNEL);
+	ret = -ENOMEM;
+	if (!old_deltas || !page)
+		goto out;
+
+	ret = ploop_rw_page_sync(READ, ploop_top_delta(ploop)->file, 0, page);
+	if (ret)
+		goto out;
+
+	ploop->bat_entries = RB_ROOT;
+	ploop->holes_bitmap = NULL;
+
+	/* Image is already checked for being in use in ploop_ctr() */
+	ret = ploop_setup_metadata(ploop, page, false);
+	if (ret)
+		goto restore;
+
+	ret = ploop_prealloc_md_pages(&ploop->bat_entries, 0,
+				      ploop->nr_bat_entries);
+	if (ret)
+		goto restore;
+
+	/*
+	 * Same order as in ploop_add_deltas_stack(): from top to base.
+	 * ploop_read_delta() relies on it, see the comment there. nr_be of
+	 * the delta is updated right away, as the lower delta is checked
+	 * against it on the next iteration.
+	 */
+	for (level = ploop_top_level(ploop); level >= 0; level--) {
+		delta = &ploop->deltas[level];
+
+		ret = ploop_read_delta(ploop, level, delta->file,
+				       &file_size, &nr_be);
+		if (ret)
+			goto restore;
+
+		delta->file_size = file_size;
+		delta->file_preallocated_area_start = file_size;
+		delta->nr_be = nr_be;
+	}
+
+	/*
+	 * Header of top delta is written with BAT page 0. Userspace doesn't
+	 * mark the delta, which becomes top after flip_upper_deltas, as in
+	 * use, and relies that the in-use signature of the old top delta
+	 * will be written to it. So keep the signature we had. In other
+	 * cases take it from disk: userspace may have changed it on the
+	 * suspended device (e.g. marked the image closed on umount).
+	 */
+	if (ploop->top_flipped) {
+		old_hdr = ploop_md_first_entry(&old_bat_entries)->kmpage;
+		hdr = ploop_md_first_entry(&ploop->bat_entries)->kmpage;
+		hdr->m_DiskInUse = old_hdr->m_DiskInUse;
+		ploop->top_flipped = false;
+	}
+
+	ploop_free_md_pages_tree(&old_bat_entries);
+	kvfree(old_holes_bitmap);
+	goto out;
+restore:
+	ploop_free_md_pages_tree(&ploop->bat_entries);
+	kvfree(ploop->holes_bitmap);
+	ploop->bat_entries = old_bat_entries;
+	ploop->holes_bitmap = old_holes_bitmap;
+	ploop->nr_bat_entries = old_nr_bat_entries;
+	ploop->hb_nr = old_hb_nr;
+	memcpy(ploop->m_Sig, old_m_Sig, sizeof(ploop->m_Sig));
+	ploop->m_Type = old_m_Type;
+	ploop->m_Sectors = old_m_Sectors;
+	memcpy(ploop->deltas, old_deltas,
+	       sizeof(*old_deltas) * ploop->nr_deltas);
+out:
+	if (page)
+		put_page(page);
+	kfree(old_deltas);
+	if (ret)
+		PL_ERR("Failed to reload metadata: %d", ret);
+	return ret;
+}
+ALLOW_ERROR_INJECTION(ploop_reload_metadata, ERRNO);
+
 void ploop_put_delta(struct ploop *ploop, struct ploop_delta *delta)
 {
 	int i;
diff --git a/drivers/md/dm-ploop-cmd.c b/drivers/md/dm-ploop-cmd.c
index 977b6920a5b4..1590af23360f 100644
--- a/drivers/md/dm-ploop-cmd.c
+++ b/drivers/md/dm-ploop-cmd.c
@@ -1049,6 +1049,7 @@ static int process_flip_upper_deltas(struct ploop *ploop)
 
 	/* FIXME */
 	swap(ploop->deltas[level], ploop->deltas[level+1]);
+	ploop->top_flipped = true;
 	return 0;
 }
 
diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
index f673e94d0e38..80540186b5a8 100644
--- a/drivers/md/dm-ploop-map.c
+++ b/drivers/md/dm-ploop-map.c
@@ -1247,7 +1247,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;
@@ -1259,6 +1259,12 @@ static int ploop_no_pending_prealloc(struct ploop *ploop)
 	return ret;
 }
 
+void ploop_wait_prealloc(struct ploop *ploop)
+{
+	wait_event_killable(ploop->dispatcher_wq_prealloc,
+			    ploop_no_pending_prealloc(ploop));
+}
+
 static int ploop_allocate_cluster(struct ploop *ploop, u32 *dst_clu, struct file *file)
 {
 	struct ploop_delta *top = ploop_top_delta(ploop);
@@ -1446,6 +1452,7 @@ static void ploop_check_standby_mode(struct ploop *ploop, long res)
 	spin_lock_irqsave(&q->queue_lock, flags);
 	prev = test_and_set_bit(QUEUE_FLAG_STANDBY, &q->queue_flags);
 	spin_unlock_irqrestore(&q->queue_lock, flags);
+	WRITE_ONCE(ploop->standby, true);
 
 	if (!prev)
 		PL_INFO("was switched into the standby mode");
diff --git a/drivers/md/dm-ploop-target.c b/drivers/md/dm-ploop-target.c
index 11d25277d60c..208480d049fb 100644
--- a/drivers/md/dm-ploop-target.c
+++ b/drivers/md/dm-ploop-target.c
@@ -21,6 +21,7 @@
 #include <linux/uio.h>
 #include <linux/timer.h>
 #include <linux/error-injection.h>
+#include <linux/sched/mm.h>
 #include "dm-ploop.h"
 
 #define DM_MSG_PREFIX "ploop"
@@ -295,7 +296,7 @@ static int ploop_check_top_delta(struct ploop *ploop, struct file *file)
 	/* flush dirty pages before changing direct IO */
 	vfs_fsync(file, 1);
 
-	ret = ploop_setup_metadata(ploop, page);
+	ret = ploop_setup_metadata(ploop, page, true);
 	if (ret)
 		goto out;
 
@@ -426,8 +427,7 @@ static struct ploop_worker *ploop_worker_create(struct ploop *ploop,
  * suspended and resumed. This behavior is intentional to allow for atomic
  * updates to the mapping table without disrupting ongoing I/O operations.
  *
- * But dm-ploop is not fully compatible with this design. Ploop does not do a
- * "metadata" reload on resume. So there can be situations like:
+ * So there can be situations like:
  *
  *  1. dmsetup reload is called, and new ploop is created with ploop_ctr for
  *  the same delta image file as an old ploop and ploop_add_deltas_stack reads
@@ -438,23 +438,19 @@ static struct ploop_worker *ploop_worker_create(struct ploop *ploop,
  *  the new one, new ploop has outdated metadata and thus may corrupt the
  *  images by relying on it.
  *
- * For instance we thought to truncate the preallocations to the delta image
- * file on ploop_dtr, but that can't be done as ploop_dtr is called on the old
- * ploop after the new one is resumed, so we can't possibly modify the image
- * from the old ploop context at this point, as new ploop may be already doing
- * its own IO or preallocation.
+ * To handle this, ploop_preresume reloads all the metadata from the images
+ * (see ploop_reload_metadata), so that the new ploop "sees" all previous
+ * updates from the old one. For the same reason the new ploop must not
+ * modify the images before resume, so preallocation is started only after
+ * the metadata reload, and the old ploop waits for its preallocation to
+ * complete on postsuspend.
  *
- * The better way to do this truncation is on ploop_*suspend, and then on resume
- * to reload the metadata in the new ploop so that the new ploop "sees" all
- * previous updates from the old one.
- *
- * For now we can leave with it, as we chose not to do this truncation on
- * destroy and we rely that userspace always does dmsetup reload on already
- * suspended device.
- *
- * FIXME: We must implement metadata reload on resume. After that we can
- * implement preallocation truncation on suspend (see dm-qcow2 as it already
- * implements that in qcow2_parse_metadata and qcow2_truncate_preallocations).
+ * Still, we can't truncate the preallocations to the delta image file on
+ * ploop_dtr, as ploop_dtr is called on the old ploop after the new one is
+ * resumed, so we can't possibly modify the image from the old ploop context
+ * at this point, as new ploop may be already doing its own IO or
+ * preallocation. This truncation should be done on ploop_*suspend (see
+ * dm-qcow2 as it already implements that in qcow2_truncate_preallocations).
  */
 static int ploop_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 {
@@ -644,9 +640,6 @@ static int ploop_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 	ti->num_discard_bios = 1;
 	ti->discards_supported = true;
 
-	if (!ploop_is_ro(ploop) && ploop->nr_deltas > 0)
-		ploop_should_prealloc(ploop);
-
 	return 0;
 
 err:
@@ -726,6 +719,11 @@ static void ploop_set_suspended(struct dm_target *ti, bool suspended)
 	up_write(&ploop->ctl_rwsem);
 }
 
+static bool ploop_is_standby(struct ploop *ploop)
+{
+	return READ_ONCE(ploop->standby);
+}
+
 static void ploop_presuspend(struct dm_target *ti)
 {
 	struct ploop *ploop = ti->private;
@@ -750,19 +748,58 @@ static void ploop_presuspend_undo(struct dm_target *ti)
 }
 static void ploop_postsuspend(struct dm_target *ti)
 {
+	struct ploop *ploop = ti->private;
+
+	/*
+	 * Let the next resumed ploop (maybe of reloaded table)
+	 * see final size of the top delta. Don't wait in standby
+	 * mode: the allocator may be stuck on the dead file, while
+	 * dm_replace() of libploop must be able to suspend the device
+	 * to recover it. The wait is killable for the same reason.
+	 * If the wait is skipped or killed, a ploop of a reloaded
+	 * table may see a stale size, and a late truncate of our
+	 * allocator may shrink the file under it.
+	 */
+	if (!ploop_is_standby(ploop))
+		ploop_wait_prealloc(ploop);
+	/*
+	 * Data bios are completed before BAT writeback is finished
+	 * (see ploop_put_piwb()), so wait for it explicitly to make
+	 * md pages clean for ploop_reload_metadata().
+	 */
+	wait_event(ploop->inflight_md_wq, !atomic_read(&ploop->inflight_md));
 	ploop_set_suspended(ti, true);
 }
 static int ploop_preresume(struct dm_target *ti)
 {
 	struct ploop *ploop = ti->private;
+	unsigned int noio_flags;
+	bool reload = false;
 	int ret = 0;
 
-	down_read(&ploop->ctl_rwsem);
-	if (ploop->noresume)
+	down_write(&ploop->ctl_rwsem);
+	if (ploop->noresume) {
 		ret = -EAGAIN;
-	up_read(&ploop->ctl_rwsem);
+	} else if (!ploop_is_standby(ploop) &&
+		   ploop_no_pending_prealloc(ploop)) {
+		reload = true;
+		/*
+		 * The device is suspended, so reclaim must not wait
+		 * for IO to it. Otherwise we deadlock with ourselves.
+		 */
+		noio_flags = memalloc_noio_save();
+		ret = ploop_reload_metadata(ploop);
+		memalloc_noio_restore(noio_flags);
+	}
+	up_write(&ploop->ctl_rwsem);
 
 	if (ret == 0) {
+		/*
+		 * Preallocate only after the metadata is reloaded,
+		 * as prealloc relies on the actual top delta size.
+		 */
+		if (reload && !ploop_is_ro(ploop))
+			ploop_should_prealloc(ploop);
 		/*
 		 * We are singleton target. There will be
 		 * no more reasons to break resume.
diff --git a/drivers/md/dm-ploop.h b/drivers/md/dm-ploop.h
index 5041fc8f1e13..5750eafc8a1b 100644
--- a/drivers/md/dm-ploop.h
+++ b/drivers/md/dm-ploop.h
@@ -245,6 +245,13 @@ struct ploop {
 
 	/* Resume is prohibited */
 	bool noresume;
+	/* Top delta was swapped by flip_upper_deltas */
+	bool top_flipped;
+	/*
+	 * This ploop switched the queue into standby. Unlike the queue
+	 * bit, it is not cleared by ploop_ctr() of a new table.
+	 */
+	bool standby;
 	/* Device is suspended */
 	bool suspended;
 	/* Device wants suspend */
@@ -633,7 +640,9 @@ extern struct pio *ploop_alloc_pio_with_pages(struct ploop *ploop);
 extern void ploop_free_pio_with_pages(struct ploop *ploop, struct pio *pio);
 extern void ploop_pio_prepare_offsets(struct ploop *ploop, struct pio *pio, u32 clu);
 
-extern int ploop_setup_metadata(struct ploop *ploop, struct page *page);
+extern int ploop_setup_metadata(struct ploop *ploop, struct page *page,
+				bool check_in_use);
+extern int ploop_reload_metadata(struct ploop *ploop);
 extern int ploop_read_delta_metadata(struct ploop *ploop, struct file *file,
 				     struct rb_root *md_root, u32 *delta_nr_be);
 extern void ploop_index_wb_init(struct ploop_index_wb *piwb,
@@ -651,6 +660,8 @@ 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 void ploop_wait_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 v4 11/11] dm-ploop: truncate unused tail of top delta on suspend

[Alexander Ivanov]

Preallocated space and clusters at the end of the top delta freed by
discard are never given back, so the file size never shrinks. Now that
ploop reloads metadata on resume, we can truncate the top delta on
suspend: the next resumed ploop (maybe of reloaded table) picks up the
new file size.

Add ploop_calc_used_end(), which returns the end of the last cluster
used by the delta: either BAT area, the last mapped cluster or format
extension. Metadata is read from disk rather than taken from memory:
the next table load checks BAT entries against the file size (-EPROTO
otherwise), while in-memory BAT may differ from on-disk one after
failed BAT writeback. Skip it if md pages are not clean, as on-disk BAT
may miss allocated clusters then.

Userspace writes format extension (CBT) behind the last data cluster
of suspended device on umount, resumes it, and then removes the device,
which suspends it again. So keep the area up to the end of format
extension header, which is the last cluster of the extension, as
userspace does. Name the corresponding header field properly instead
of m_Reserved. The field was never interpreted before, so if it is not
cluster-aligned or out of range, don't truncate anything: unaligned
truncation would make the next resume fail with -EPROTO.

Truncate the top delta to the used end in ploop_postsuspend() after
pending preallocation is completed. Do it under ctl_rwsem, so that
ploop messages (e.g. resize) don't change the image in parallel. Skip
read-only devices and raw top delta. On error just leave the file as is.

Skip devices in standby mode as well: the top delta file is dead then,
and the truncation may hang with md->suspend_lock held, so dm_replace()
of libploop could neither replace nor remove the device.

Also skip it if the file size differs from top->file_size. Then another
ploop on the same images writes to the top delta (e.g. a live one left
by libploop error path), and on-disk BAT may miss clusters it has just
allocated, so the truncation would cut its data.

https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-ploop: ploop target driver

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 drivers/md/dm-ploop-bat.c    | 56 +++++++++++++++++++++++++++++++
 drivers/md/dm-ploop-target.c | 64 +++++++++++++++++++++++++++++++++---
 drivers/md/dm-ploop.h        |  3 +-
 3 files changed, 118 insertions(+), 5 deletions(-)

diff --git a/drivers/md/dm-ploop-bat.c b/drivers/md/dm-ploop-bat.c
index c060afdd8d3e..7bab746a3707 100644
--- a/drivers/md/dm-ploop-bat.c
+++ b/drivers/md/dm-ploop-bat.c
@@ -709,6 +709,62 @@ int ploop_reload_metadata(struct ploop *ploop)
 }
 ALLOW_ERROR_INJECTION(ploop_reload_metadata, ERRNO);
 
+/*
+ * Returns the end of the last cluster of delta @file used by BAT area,
+ * mapped clusters or format extension, according to on-disk metadata.
+ * Must be called on suspended device.
+ */
+loff_t ploop_calc_used_end(struct ploop *ploop, struct file *file)
+{
+	u32 i, end, nr_be, dst_clu, used_clus, *bat_entries;
+	struct rb_root md_root = RB_ROOT;
+	struct ploop_pvd_header *hdr;
+	struct rb_node *node;
+	struct md_page *md;
+	u64 ext_off, ext_clu;
+	loff_t used_end;
+	int ret;
+
+	/* BAT on disk may miss allocated clusters until writeback is done */
+	if (WARN_ON_ONCE(!ploop_md_pages_are_clean(ploop)))
+		return -EBUSY;
+
+	ret = ploop_read_delta_metadata(ploop, file, &md_root, &nr_be);
+	if (ret)
+		return ret;
+
+	used_clus = ploop_nr_bat_clusters(ploop, nr_be);
+	rb_root_for_each_md_page(&md_root, md, node) {
+		bat_entries = md->kmpage;
+		init_be_iter(nr_be, md->id, &i, &end);
+		for (; i <= end; i++) {
+			dst_clu = READ_ONCE(bat_entries[i]);
+			if (dst_clu != BAT_ENTRY_NONE && dst_clu >= used_clus)
+				used_clus = dst_clu + 1;
+		}
+	}
+
+	/* The header is the last cluster, see delta_save_optional_header() */
+	hdr = ploop_md_first_entry(&md_root)->kmpage;
+	ext_off = le64_to_cpu(hdr->m_FormatExtensionOffset);
+	if (ext_off) {
+		ext_clu = SEC_TO_CLU(ploop, ext_off);
+		if (CLU_TO_SEC(ploop, ext_clu) != ext_off ||
+		    ext_clu >= BAT_ENTRY_NONE) {
+			used_end = -EPROTO;
+			goto out;
+		}
+		if (ext_clu >= used_clus)
+			used_clus = ext_clu + 1;
+	}
+
+	used_end = CLU_TO_POS(ploop, used_clus);
+out:
+	ploop_free_md_pages_tree(&md_root);
+	return used_end;
+}
+ALLOW_ERROR_INJECTION(ploop_calc_used_end, ERRNO);
+
 void ploop_put_delta(struct ploop *ploop, struct ploop_delta *delta)
 {
 	int i;
diff --git a/drivers/md/dm-ploop-target.c b/drivers/md/dm-ploop-target.c
index 208480d049fb..9c655bf9399f 100644
--- a/drivers/md/dm-ploop-target.c
+++ b/drivers/md/dm-ploop-target.c
@@ -449,8 +449,9 @@ static struct ploop_worker *ploop_worker_create(struct ploop *ploop,
  * ploop_dtr, as ploop_dtr is called on the old ploop after the new one is
  * resumed, so we can't possibly modify the image from the old ploop context
  * at this point, as new ploop may be already doing its own IO or
- * preallocation. This truncation should be done on ploop_*suspend (see
- * dm-qcow2 as it already implements that in qcow2_truncate_preallocations).
+ * preallocation. So this truncation is done on ploop_postsuspend (see
+ * ploop_truncate_unused_tail), and the new ploop picks up the new size
+ * of the top delta on metadata reload.
  */
 static int ploop_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 {
@@ -746,10 +747,59 @@ static void ploop_presuspend_undo(struct dm_target *ti)
 {
 	ploop_set_wants_suspend(ti, false);
 }
+
+/*
+ * Truncate preallocations and the tail of the top delta, which became
+ * unused after discard. The next resumed ploop (maybe of reloaded
+ * table) re-reads the size of the top delta in ploop_preresume().
+ */
+static void ploop_truncate_unused_tail(struct ploop *ploop)
+{
+	struct ploop_delta *top = ploop_top_delta(ploop);
+	unsigned long flags;
+	loff_t end;
+	int ret;
+
+	lockdep_assert_held_write(&ploop->ctl_rwsem);
+
+	if (ploop_is_ro(ploop) || top->is_raw)
+		return;
+	if (ploop_is_standby(ploop))
+		return;
+	if (!ploop_no_pending_prealloc(ploop))
+		return;
+	if (i_size_read(file_inode(top->file)) != top->file_size) {
+		PL_ERR("Top delta is changed by someone else, don't truncate it");
+		return;
+	}
+
+	end = ploop_calc_used_end(ploop, top->file);
+	if (end < 0) {
+		PL_ERR("Can't find used end of top delta: %lld", end);
+		return;
+	}
+	if (end >= top->file_size)
+		return;
+
+	ret = do_ftruncate(top->file, end, 0);
+	if (ret) {
+		PL_ERR("Can't truncate unused tail of top delta: %d", ret);
+		return;
+	}
+
+	spin_lock_irqsave(&ploop->bat_lock, flags);
+	top->file_size = end;
+	if (top->file_preallocated_area_start > end)
+		top->file_preallocated_area_start = end;
+	spin_unlock_irqrestore(&ploop->bat_lock, flags);
+}
+
 static void ploop_postsuspend(struct dm_target *ti)
 {
 	struct ploop *ploop = ti->private;
+	unsigned int noio_flags;
 
+	down_write(&ploop->ctl_rwsem);
 	/*
 	 * Let the next resumed ploop (maybe of reloaded table)
 	 * see final size of the top delta. Don't wait in standby
@@ -765,10 +815,16 @@ static void ploop_postsuspend(struct dm_target *ti)
 	/*
 	 * Data bios are completed before BAT writeback is finished
 	 * (see ploop_put_piwb()), so wait for it explicitly to make
-	 * md pages clean for ploop_reload_metadata().
+	 * md pages clean for ploop_reload_metadata() and
+	 * ploop_calc_used_end().
 	 */
 	wait_event(ploop->inflight_md_wq, !atomic_read(&ploop->inflight_md));
-	ploop_set_suspended(ti, true);
+	/* The device is suspended, see ploop_preresume() */
+	noio_flags = memalloc_noio_save();
+	ploop_truncate_unused_tail(ploop);
+	memalloc_noio_restore(noio_flags);
+	ploop->suspended = true;
+	up_write(&ploop->ctl_rwsem);
 }
 static int ploop_preresume(struct dm_target *ti)
 {
diff --git a/drivers/md/dm-ploop.h b/drivers/md/dm-ploop.h
index 5750eafc8a1b..bdc8033c9cc5 100644
--- a/drivers/md/dm-ploop.h
+++ b/drivers/md/dm-ploop.h
@@ -37,7 +37,7 @@ struct ploop_pvd_header {
 	__u32 m_DiskInUse;	/* Disk in use */
 	__u32 m_FirstBlockOffset; /* First data block offset (in sectors) */
 	__u32 m_Flags;		/* Misc flags */
-	__u8  m_Reserved[8];	/* Reserved */
+	__u64 m_FormatExtensionOffset; /* Format extension offset (in sectors) */
 };
 #pragma pack(pop)
 
@@ -643,6 +643,7 @@ extern void ploop_pio_prepare_offsets(struct ploop *ploop, struct pio *pio, u32
 extern int ploop_setup_metadata(struct ploop *ploop, struct page *page,
 				bool check_in_use);
 extern int ploop_reload_metadata(struct ploop *ploop);
+extern loff_t ploop_calc_used_end(struct ploop *ploop, struct file *file);
 extern int ploop_read_delta_metadata(struct ploop *ploop, struct file *file,
 				     struct rb_root *md_root, u32 *delta_nr_be);
 extern void ploop_index_wb_init(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 v4 02/11] dm-ploop: fix lost wakeup in ploop_allocator()

[no-reply]

> ploop_allocator() checks prealloc_size and kthread_should_stop() before
> setting TASK_INTERRUPTIBLE. If ploop_req_prealloc() sets prealloc_size
> and calls wake_up_process() in between, the wakeup finds the task
> running and is lost, so the allocator goes to sleep with a pending
> preallocation request. Later requests for the same size return early
> without waking it up, so the request may stay pending forever and
> waiters on dispatcher_wq_prealloc hang. The same applies to
> kthread_stop().
> 
> Set the task state before checking the conditions, as the canonical
> kthread loop does. set_current_state() implies a full barrier, which
> pairs with the one in try_to_wake_up().
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-148171
> Feature: dm-ploop: ploop target driver
> 
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

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

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261012132646.41494-1-alexander.ivanov@virtuozzo.com?part=2


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

Re: [Devel] [PATCH hci-8.1 v4 03/11] dm-ploop: reject a delta header whose BAT does not cover the target

[no-reply]

> ploop_setup_metadata() takes the number of BAT entries from m_Size of
> the top delta header, but checks only m_SizeInSectors_v2 against the
> target length. A header with a small m_Size (including zero) and a large
> enough m_SizeInSectors_v2 passes all checks. Requests past the last BAT
> entry then hit WARN_ONCE() in ploop_rq_valid(), which panics the host
> with panic_on_warn.
> 
> Reject the header if the BAT does not cover the target, taking the
> skip_off shift of request positions into account.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-148171
> Feature: dm-ploop: ploop target driver
> 
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

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

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261012132646.41494-1-alexander.ivanov@virtuozzo.com?part=3


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

Re: [Devel] [PATCH hci-8.1 v4 04/11] dm-ploop: reject empty deltas

[no-reply]

> ploop_delta_check_header() accepts a header with m_Size == 0, and
> ploop_add_delta() accepts a raw delta of zero size. The BAT iterators
> can't handle nr_be == 0: ploop_bat_clu_to_page_nr(nr_be - 1) wraps to
> page 0, so md pages past the first one are iterated whole and the
> clu == nr_be - 1 stop condition never matches. A non-raw empty delta
> has a single md page, so ploop_apply_delta_mappings() and
> notify_delta_merged() (and ploop_check_delta_before_flip() with
> PLOOP_DEBUG) walk past it and dereference a NULL d_md on a device with
> more than one md page. A raw delta is not handled as raw yet, as
> ploop_add_delta() sets is_raw only after applying its mappings; once it
> is, an empty raw delta would get all the clusters mapped to it.
> 
> Reject empty deltas in ploop_delta_check_header(), which covers every
> caller reading a delta header, and in ploop_add_delta() for raw deltas.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-148171
> Feature: dm-ploop: ploop target driver
> 
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

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

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261012132646.41494-1-alexander.ivanov@virtuozzo.com?part=4


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

Re: [Devel] [PATCH hci-8.1 v4 01/11] dm-ploop: fix u32 overflow of the delta BAT byte size

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dm-ploop: ploop_bat_clu_to_page_nr() still wraps size_t multiply before widening
--

commit 490ad8e2663849858d62f84d5062973178fffee9
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

dm-ploop: fix u32 overflow of the delta BAT byte size

This patch computes the byte size of the header and BAT in u64 and uses
ploop_nr_bat_clusters() everywhere for the BAT cluster count. It also
rejects a header or a resize whose hb_nr exceeds PLOOP_MAX_HB_NR.

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

> diff --git a/drivers/md/dm-ploop.h b/drivers/md/dm-ploop.h
> --- a/drivers/md/dm-ploop.h
> +++ b/drivers/md/dm-ploop.h

[ ... ]

> @@ -410,9 +412,9 @@ static inline unsigned int ploop_nr_pages_in_cluster(struct ploop *ploop)
>  static inline unsigned int ploop_nr_bat_clusters(struct ploop *ploop,
>  						 u32 nr_bat_entries)
>  {
> -	unsigned long size, bat_clusters;
> +	u64 size, bat_clusters;
>  
> -	size = (PLOOP_MAP_OFFSET + nr_bat_entries) * sizeof(map_index_t);
> +	size = ((u64)nr_bat_entries + PLOOP_MAP_OFFSET) * sizeof(map_index_t);
>  	bat_clusters = DIV_ROUND_UP(size, CLU_SIZE(ploop));
>  
>  	return bat_clusters;

[Severity: Medium]
Does the sibling helper ploop_bat_clu_to_page_nr() in dm-ploop.h still
wrap the same multiplication?

static inline u32 ploop_bat_clu_to_page_nr(u32 clu)
{
	u64 byte;

	byte = (clu + PLOOP_MAP_OFFSET) * sizeof(map_index_t);
	return byte >> PAGE_SHIFT;
}

The multiplication is evaluated in size_t before the result is widened to
u64. Where size_t is 32 bits, this wraps for clu >= 2^30 - PLOOP_MAP_OFFSET.
That is the same range the commit message says is being fixed ("It wraps
for m_Size >= 2^30 - PLOOP_MAP_OFFSET"), and the message also says the size
is now computed in u64 "everywhere".

The new PLOOP_MAX_HB_NR limit only caps nr_bat_entries plus bat_clusters
near 2^32. A header or a resize with nr_bat_entries >= 2^30 - 16 therefore
still passes the new checks in ploop_setup_metadata() and ploop_resize().

Can a wrapped page number then reach these callers?

ploop_resize()->ploop_prealloc_md_pages()
    new_nr_pages = ploop_bat_clu_to_page_nr(new_nr_bat_entries - 1) + 1;

This could allocate too few md pages.

init_be_iter()
    u32 rem, last_page = ploop_bat_clu_to_page_nr(nr_be - 1);

This could give a wrong iteration end for the BAT entries.

ploop_map_bat_entry()
    id = ploop_bat_clu_to_page_nr(clu);
    md = ploop_md_page_find(ploop, id);
    BUG_ON(!md);

A page id that wraps would then either miss the lookup and hit BUG_ON(!md),
or index past the pages that were allocated.

On 64-bit, size_t is 64 bits and clu + 16 cannot wrap in u32 because of the
PLOOP_MAX_HB_NR cap, so this looks specific to 32-bit builds. The DM_PLOOP
entry in drivers/md/Kconfig depends only on BLK_DEV_DM, so nothing prevents
such a build.

I have not confirmed whether other 32-bit limits make these sizes unusable
first. For example, 2^30 entries would need about 4 GiB of md pages, which
would probably fail to allocate.

Would it make sense to widen the multiplication in ploop_bat_clu_to_page_nr()
the same way, with ((u64)clu + PLOOP_MAP_OFFSET) * sizeof(map_index_t)?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261012132646.41494-1-alexander.ivanov@virtuozzo.com?part=1

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

Re: [Devel] [PATCH hci-8.1 v4 06/11] dm-ploop: factor out ploop_read_delta() from ploop_add_delta()

[no-reply]

> Move reading of delta BAT and applying its mappings to
> ploop->bat_entries into a separate function ploop_read_delta(). It
> will be used to reload metadata of already added deltas.
> 
> ploop_read_delta() takes is_raw from deltas[level], so ploop_add_delta()
> sets it before the call.
> 
> Replace the stale comment of ploop_add_delta(): it claimed deltas are
> added from oldest to newest, while ploop_add_deltas_stack() adds them
> from top to base, which ploop_read_delta() relies on.
> 
> No functional changes.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-148171
> Feature: dm-ploop: ploop target driver
> 
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

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

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261012132646.41494-1-alexander.ivanov@virtuozzo.com?part=6


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

Re: [Devel] [PATCH hci-8.1 v4 05/11] dm-ploop: set delta is_raw before applying its mappings

[no-reply]

> ploop_apply_delta_mappings() takes is_raw from deltas[level], but
> ploop_add_delta() sets deltas[level].is_raw only after the mappings
> are applied. So a raw delta is handled as a non-raw one: its md_root
> is empty, ploop_md_first_entry() returns NULL and d_md->kmpage
> dereferences it.
> 
> This has been the case since the driver was added in commit
> 6ddad5a4d226 ("dm-ploop: Add ploop target driver"), i.e. raw deltas
> have never worked here; this is not a regression.
> 
> Set is_raw before applying the mappings.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-148171
> Feature: dm-ploop: ploop target driver
> 
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

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

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261012132646.41494-1-alexander.ivanov@virtuozzo.com?part=5


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

Re: [Devel] [PATCH hci-8.1 v4 08/11] dm-ploop: use wake_up() for inflight_md_wq and dispatcher_wq_prealloc

[no-reply]

> wake_up_interruptible() wakes only TASK_INTERRUPTIBLE waiters. All the
> current waiters on inflight_md_wq and dispatcher_wq_prealloc sleep
> interruptibly, but one of the next patches adds uninterruptible ones on
> postsuspend, which would never be woken. Use wake_up(), which wakes
> both kinds.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-148171
> Feature: dm-ploop: ploop target driver
> 
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

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

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261012132646.41494-1-alexander.ivanov@virtuozzo.com?part=8


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

Re: [Devel] [PATCH hci-8.1 v4 07/11] dm-ploop: take bat_lock around md page lookup in ploop_llseek_hole()

[no-reply]

> ploop_llseek_hole() walks the md pages tree without any lock, while
> resize inserts new md pages into it in ploop_add_md_pages(). A lookup
> racing with rb-tree rebalancing may follow a stale pointer or miss the
> page and return -EINVAL.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-148171
> Feature: dm-ploop: ploop target driver
> 
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

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

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261012132646.41494-1-alexander.ivanov@virtuozzo.com?part=7


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

Re: [Devel] [PATCH hci-8.1 v4 09/11] dm-ploop: fix use-after-free on the last inflight_md decrement

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dm-ploop: ploop_destroy() self-deadlocks using wait_event_lock_irq() on wq.lock
--

commit e7aad5678d9b3ec2b2234a7e4beccd4508f457af
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

dm-ploop: fix use-after-free on the last inflight_md decrement

This patch moves the inflight_md decrement and wakeup to the end of the
piwb completion path, so that it is the last access to the ploop. The
decrement and wakeup are done under inflight_md_wq.lock, the waiters check
the counter under the same lock, and ploop_destroy() now waits for
inflight_md to reach zero before freeing anything.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-148171

> diff --git a/drivers/md/dm-ploop-target.c b/drivers/md/dm-ploop-target.c
> --- a/drivers/md/dm-ploop-target.c
> +++ b/drivers/md/dm-ploop-target.c

[ ... ]

> @@ -192,6 +192,13 @@ static void ploop_destroy(struct ploop *ploop)
>  		if (ploop->kt_allocator)
>  			kthread_stop(ploop->kt_allocator->task);
>  
> +		/* BAT writes may still be completing from aio context */
> +		spin_lock_irq(&ploop->inflight_md_wq.lock);
> +		wait_event_lock_irq(ploop->inflight_md_wq,
> +				    !atomic_read(&ploop->inflight_md),
> +				    ploop->inflight_md_wq.lock);
> +		spin_unlock_irq(&ploop->inflight_md_wq.lock);
> +

[Severity: High]
Can this deadlock on the waitqueue lock when inflight_md is non-zero?

ploop_destroy() takes inflight_md_wq.lock and then passes that same lock
as the lock argument to wait_event_lock_irq().  If the condition is
already true, the macro exits through its fast path and nothing goes
wrong.  If inflight_md is still non-zero, the call chain is:

ploop_destroy()
  spin_lock_irq(&ploop->inflight_md_wq.lock)
  wait_event_lock_irq()
    __wait_event_lock_irq()
      ___wait_event()
        prepare_to_wait_event()
          spin_lock_irqsave(&wq_head->lock, flags)

prepare_to_wait_event() acquires wq_head->lock, which is the lock the
caller already holds.  The spinlock is not recursive, so the task would
spin on itself with interrupts disabled.  The spin_unlock_irq() inside the
macro, before schedule(), is never reached.

This is the case the patch is meant to handle, namely BAT writes still
completing from aio context when ploop_destroy() runs.

wait_event_lock_irq() looks only usable when the lock argument is a
different lock from the waitqueue's own lock.  ploop_suspend_submitting_pios()
uses wait_event_interruptible_locked_irq() for this situation.

Would it work to drop the outer spin_lock_irq()/spin_unlock_irq() and use a
plain wait_event() here, since the waker already takes the waitqueue lock
in wake_up_locked()?  Alternatively, a *_locked_irq style wait could be
used, as in ploop_suspend_submitting_pios().

>  		WARN_ON(!llist_empty(&ploop->pios[PLOOP_LIST_PREPARE]));
>  		WARN_ON(!llist_empty(&ploop->llresubmit_pios));
>  		WARN_ON(!llist_empty(&ploop->enospc_pios));

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261012132646.41494-1-alexander.ivanov@virtuozzo.com?part=9

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

Re: [Devel] [PATCH hci-8.1 v4 10/11] dm-ploop: reload metadata on resume

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dm-ploop: ploop_postsuspend() waits uninterruptibly for BAT writeback, even in standby
--

Subject: Re: [PATCH] dm-ploop: reload metadata on resume

commit bd5e28d3cdaaa9ef0b4e2b1af46f0379281a0d21
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

dm-ploop: reload metadata on resume

This patch reloads the top delta header, the BAT of every delta and the
delta file sizes in ploop_preresume(), and rebuilds the BAT tree and
holes_bitmap. It also moves preallocation from ploop_ctr() to
ploop_preresume(), tracks standby per ploop, and adds waits in
ploop_postsuspend() for pending preallocation and for inflight BAT
writeback.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-148171

> diff --git a/drivers/md/dm-ploop-target.c b/drivers/md/dm-ploop-target.c
> --- a/drivers/md/dm-ploop-target.c
> +++ b/drivers/md/dm-ploop-target.c

[ ... ]

>  static void ploop_postsuspend(struct dm_target *ti)
>  {
> +	struct ploop *ploop = ti->private;
> +
> +	/*
> +	 * Let the next resumed ploop (maybe of reloaded table)
> +	 * see final size of the top delta. Don't wait in standby
> +	 * mode: the allocator may be stuck on the dead file, while
> +	 * dm_replace() of libploop must be able to suspend the device
> +	 * to recover it. The wait is killable for the same reason.
> +	 * [ ... ]
> +	 */
> +	if (!ploop_is_standby(ploop))
> +		ploop_wait_prealloc(ploop);
> +	/*
> +	 * Data bios are completed before BAT writeback is finished
> +	 * (see ploop_put_piwb()), so wait for it explicitly to make
> +	 * md pages clean for ploop_reload_metadata().
> +	 */
> +	wait_event(ploop->inflight_md_wq, !atomic_read(&ploop->inflight_md));
>  	ploop_set_suspended(ti, true);
>  }

[Severity: Medium]
Can this wait_event() hang suspend when the backing file is dead?

The preallocation wait just above is skipped in standby and is killable,
because the allocator may be stuck on the dead file and dm_replace() of
libploop has to be able to suspend the device to recover it.

The inflight_md wait has no such escape. It is uninterruptible, and it is
also reached when ploop_is_standby() is true.

inflight_md is decremented only when the BAT write kiocb completes:

ploop_bat_write_complete()
  ploop_put_piwb()
    ploop_dec_inflight_md()
      atomic_dec_and_test(&ploop->inflight_md)
      wake_up_locked(&ploop->inflight_md_wq)

Standby is set only in ploop_check_standby_mode(), after some IO has
already completed with -EBUSY, -ENOTCONN or -EIO:

ploop_check_standby_mode()
    ...
    WRITE_ONCE(ploop->standby, true);

That does not cancel or complete other BAT writes already in flight. If one
of them is stuck on the same dead file, inflight_md stays above zero and the
suspending task sits in D state.

ploop_postsuspend() returns void, so the wait cannot be turned into an
error. Whether a BAT write to a dead file hangs or fails depends on the
backing filesystem, so I could not confirm this from the code alone.

The follow-up patch "dm-ploop: truncate unused tail of top delta on
suspend" keeps this as a plain wait_event(), and moves it to run under
ctl_rwsem held for write. Other paths that take ctl_rwsem would then also
block behind it.

Would it make sense to make this wait killable, or to skip or bound it in
standby, as is done for the preallocation wait?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261012132646.41494-1-alexander.ivanov@virtuozzo.com?part=10

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

Re: [Devel] [PATCH hci-8.1 v4 11/11] dm-ploop: truncate unused tail of top delta on suspend

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dm-ploop: ctl_rwsem held across unbounded waits in ploop_postsuspend()
--

commit d2ae283df5af32ea17061c2b914cc8df355ea0dd
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

dm-ploop: truncate unused tail of top delta on suspend

This patch adds ploop_calc_used_end() and uses it from ploop_postsuspend()
to truncate the unused tail of the top delta file. The truncation runs
under ctl_rwsem, which is now taken at the start of ploop_postsuspend().

Link: https://virtuozzo.atlassian.net/browse/VSTOR-148171

> diff --git a/drivers/md/dm-ploop-target.c b/drivers/md/dm-ploop-target.c
> --- a/drivers/md/dm-ploop-target.c
> +++ b/drivers/md/dm-ploop-target.c

[ ... ]

>  static void ploop_postsuspend(struct dm_target *ti)
>  {
>  	struct ploop *ploop = ti->private;
> +	unsigned int noio_flags;
>  
> +	down_write(&ploop->ctl_rwsem);
>  	/*
>  	 * Let the next resumed ploop (maybe of reloaded table)
>  	 * see final size of the top delta. Don't wait in standby

[ ... ]

> +	if (!ploop_is_standby(ploop))
> +		ploop_wait_prealloc(ploop);
>  	wait_event(ploop->inflight_md_wq, !atomic_read(&ploop->inflight_md));
> -	ploop_set_suspended(ti, true);
> +	/* The device is suspended, see ploop_preresume() */
> +	noio_flags = memalloc_noio_save();
> +	ploop_truncate_unused_tail(ploop);
> +	memalloc_noio_restore(noio_flags);
> +	ploop->suspended = true;
> +	up_write(&ploop->ctl_rwsem);
>  }

[Severity: Medium]
Can ctl_rwsem being held across these waits cause a hang?

ploop_postsuspend() now takes down_write(&ploop->ctl_rwsem) before
ploop_wait_prealloc() and before the wait_event() on inflight_md_wq.
Previously those waits ran without the lock, and ploop_set_suspended()
took it only briefly.

ploop_wait_prealloc() is only killable, and the wait_event() on
inflight_md_wq is not interruptible at all. ploop_calc_used_end() does
a vfs_iter_read() of the BAT, and do_ftruncate() is also IO-bound. If
the top delta stalls before ploop->standby has been set, none of these
waits may finish while the write lock is held.

In that state, ploop_status() for STATUSTYPE_TABLE blocks in
down_read(&ploop->ctl_rwsem). ploop_message() blocks in
down_read()/down_write() as well. Only get_event does not take the lock.
Recovery messages such as set_noresume and flip_upper_deltas are also
blocked.

The commit message says the standby skip exists so that dm_replace() of
libploop can still replace or remove a device stuck on a dead file. Does
holding the write lock across the earlier waits undermine that goal?

From the code, ctl_rwsem only needs to cover ploop_truncate_unused_tail()
and the update of ploop->suspended. Could the lock be taken after the two
waits instead?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261012132646.41494-1-alexander.ivanov@virtuozzo.com?part=11

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

Re: [Devel] [PATCH hci-8.1 v4 01/11] dm-ploop: fix u32 overflow of the delta BAT byte size

[Alexander Ivanov]

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

Re: [Devel] [PATCH hci-8.1 v4 09/11] dm-ploop: fix use-after-free on the last inflight_md decrement

[Alexander Ivanov]

Fixed, ploop_destroy() now uses plain wait_event() followed by 
lock/unlock of inflight_md_wq.lock to wait out the waker.
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v4 01/11] dm-ploop: fix u32 overflow of the delta BAT byte size

[Alexander Ivanov]

> Fixed. 

Replied to the wrong comment.

We don't have any problem on 64-bit arhcs.

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

Re: [Devel] [PATCH hci-8.1 v4 10/11] dm-ploop: reload metadata on resume

[Alexander Ivanov]

Inflight BAT writes are tied to bios that dm_wait_for_completion() 
already waited for; message-originated ones complete under ctl_rwsem; 
only the post-endio tail of ploop_put_piwb() can remain, which does no IO.
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v4 11/11] dm-ploop: truncate unused tail of top delta on suspend

[Alexander Ivanov]

The waits are not new: they already ran with md->suspend_lock held, so
a stalled delta blocked dm_replace() before this patch.
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel