[Devel] [PATCH hci-8.1 v2 0/6] truncate unused tail of images on suspend

[Devel] [PATCH hci-8.1 v2 0/6] truncate unused tail of images on suspend

[Alexander Ivanov]

Preallocated space and clusters at the end of an image freed by discard
are never given back, so the image file never shrinks. Truncate the
unused tail of the top image on suspend in both dm-qcow2 and dm-ploop,
so that userspace doesn't have to do it by hand.

dm-qcow2 (patches 1-2) finds the end of the last cluster with nonzero
refcount by scanning refcount blocks backward and truncates the image
to it on postsuspend.

dm-ploop needs more preparation. On "dmsetup reload" the new ploop
reads metadata in ploop_ctr(), while IO still goes through the old one,
so after resume it may work with outdated BAT and file sizes. Patches
3-4 fix a raw delta handling bug and factor out ploop_read_delta(),
patch 5 reloads all the metadata in ploop_preresume(), and patch 6
truncates the top delta to the end of the last used cluster (BAT area,
mapped cluster or format extension) on postsuspend.

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

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

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 (6):
  dm-qcow2: add qcow2_md_page_read_sync() helper
  dm-qcow2: truncate unused tail of image on suspend
  dm-ploop: set delta is_raw before applying its mappings
  dm-ploop: factor out ploop_read_delta() from ploop_add_delta()
  dm-ploop: reload metadata on resume
  dm-ploop: truncate unused tail of top delta on suspend

 drivers/md/dm-ploop-bat.c    | 229 +++++++++++++++++++++++++++++++++--
 drivers/md/dm-ploop-map.c    |  26 +++-
 drivers/md/dm-ploop-target.c | 100 +++++++++++----
 drivers/md/dm-ploop.h        |   8 +-
 drivers/md/dm-qcow2-map.c    |  53 ++++++++
 drivers/md/dm-qcow2-target.c |  48 +++++++-
 drivers/md/dm-qcow2.h        |   3 +
 7 files changed, 421 insertions(+), 46 deletions(-)

-- 
2.53.0

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

[Devel] [PATCH hci-8.1 v2 1/6] dm-qcow2: add qcow2_md_page_read_sync() helper

[Alexander Ivanov]

Add a helper, which returns an up to date md page from md cache,
or allocates a new one, inserts it and reads it synchronously.
On read failure the page is erased from cache and freed.

Use it in qcow2_parse_header() instead of open-coded allocation
and read of page 0. It will also be used by the next patch.

https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-qcow2: block device over QCOW2 files driver

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 drivers/md/dm-qcow2-target.c | 36 +++++++++++++++++++++++++++++++-----
 drivers/md/dm-qcow2.h        |  2 ++
 2 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
index be09615eb8e2..3e47e66b4007 100644
--- a/drivers/md/dm-qcow2-target.c
+++ b/drivers/md/dm-qcow2-target.c
@@ -418,6 +418,36 @@ int qcow2_alloc_and_insert_md_page(struct qcow2 *qcow2, u64 index, struct md_pag
 }
 ALLOW_ERROR_INJECTION(qcow2_alloc_and_insert_md_page, ERRNO);
 
+/*
+ * Returns up to date md page from cache, or allocates
+ * a new one and reads it synchronously.
+ */
+int qcow2_md_page_read_sync(struct qcow2 *qcow2, u64 id, struct md_page **md)
+{
+	int ret;
+
+	spin_lock_irq(&qcow2->md_pages_lock);
+	*md = qcow2_md_page_find_or_postpone(qcow2, id, NULL);
+	spin_unlock_irq(&qcow2->md_pages_lock);
+	if (*md)
+		return 0;
+
+	ret = qcow2_alloc_and_insert_md_page(qcow2, id, md);
+	if (ret)
+		return ret;
+
+	ret = qcow2_rw_page_sync(READ, qcow2, id, (*md)->page);
+	spin_lock_irq(&qcow2->md_pages_lock);
+	if (ret)
+		qcow2_md_page_erase(qcow2, *md);
+	else
+		(*md)->status |= MD_UPTODATE;
+	spin_unlock_irq(&qcow2->md_pages_lock);
+	if (ret)
+		qcow2_free_md_page(*md);
+	return ret;
+}
+
 static void inflight_ref_exit0(struct percpu_ref *ref)
 {
 	struct qcow2_target *tgt = container_of(ref, struct qcow2_target,
@@ -851,13 +881,9 @@ static int qcow2_parse_header(struct dm_target *ti, struct qcow2 *qcow2,
 	}
 	qcow2->file_preallocated_area_start = qcow2->file_size;
 
-	ret = qcow2_alloc_and_insert_md_page(qcow2, 0, &md);
-	if (ret)
-		return ret;
-	ret = qcow2_rw_page_sync(READ, qcow2, md->id, md->page);
+	ret = qcow2_md_page_read_sync(qcow2, 0, &md);
 	if (ret)
 		return ret;
-	md->status |= MD_UPTODATE;
 
 	raw_hdr = kmap(md->page);
 	min_len = to_bytes(ti->len);
diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h
index d9b8c38e093f..69d3ae6fca8a 100644
--- a/drivers/md/dm-qcow2.h
+++ b/drivers/md/dm-qcow2.h
@@ -386,6 +386,8 @@ void do_qcow2_work(struct work_struct *ws);
 void do_qcow2_fsync_work(struct work_struct *ws);
 int qcow2_alloc_and_insert_md_page(struct qcow2 *qcow2, u64 index,
 					struct md_page **md);
+int qcow2_md_page_read_sync(struct qcow2 *qcow2, u64 id,
+			    struct md_page **md);
 struct md_page *qcow2_md_page_find_or_postpone(struct qcow2 *qcow2,
 					       u64 id, struct qio **qio);
 struct md_page *qcow2_md_page_renumber(struct qcow2 *qcow2, u64 id,
-- 
2.53.0

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

[Devel] [PATCH hci-8.1 v2 2/6] dm-qcow2: truncate unused tail of image on suspend

[Alexander Ivanov]

On postsuspend we truncate the image file only down to
file_preallocated_area_start, i.e. we drop preallocations only.
Clusters at the end of the file freed by discard (refcount dropped
to zero) are never given back, so the file size never shrinks.

Add qcow2_calc_used_end(), which scans refcount blocks backward from
file_preallocated_area_start and returns the end of the last cluster
with non-zero refcount. Refcount ranges with no allocated refblock
are skipped at once. Metadata pages are taken from md cache or read
synchronously. This is called on suspended device after md writeback,
so R1 and R2 can't change under us.

Truncate the file to the used end. Skip the scan if md writeback
failed, as refcounts on disk are stale then, and fall back to the old
behaviour on scan error.

https://virtuozzo.atlassian.net/browse/VSTOR-148171
Feature: dm-qcow2: block device over QCOW2 files driver

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 drivers/md/dm-qcow2-map.c    | 53 ++++++++++++++++++++++++++++++++++++
 drivers/md/dm-qcow2-target.c | 12 +++++++-
 drivers/md/dm-qcow2.h        |  1 +
 3 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
index 78ee7cf5e881..7f2ffe7b2405 100644
--- a/drivers/md/dm-qcow2-map.c
+++ b/drivers/md/dm-qcow2-map.c
@@ -2208,6 +2208,59 @@ int qcow2_truncate_safe(struct file *file, loff_t new_len)
 	return do_ftruncate(file, new_len, 0);
 }
 
+/*
+ * Returns the end of the last cluster, which is used according
+ * to refcount table. Clusters freed by discard may leave the tail
+ * of file unused. Everything at or above file_preallocated_area_start
+ * is unused by construction, so the scan starts below it. This must be
+ * called on suspended device after md writeback, so nobody changes
+ * R1 and R2.
+ */
+loff_t qcow2_calc_used_end(struct qcow2 *qcow2)
+{
+	u32 clu_size = qcow2->clu_size;
+	struct qcow2_map_item r1, r2;
+	struct md_page *r1_md = NULL;
+	loff_t pos;
+	s32 i;
+	int ret;
+
+	pos = round_down(qcow2->file_preallocated_area_start - 1, clu_size);
+	while (pos >= 0) {
+		if (calc_refcounters_map(qcow2, pos, &r1, &r2) < 0)
+			return -EIO;
+
+		if (!r1_md || r1_md->id != r1.page_id) {
+			ret = qcow2_md_page_read_sync(qcow2, r1.page_id, &r1_md);
+			if (ret)
+				return ret;
+		}
+		r1.md = r1_md;
+
+		ret = calc_r2_page_id(qcow2, &r1, &r2);
+		if (ret == -ENOENT) {
+			/* No R2 cluster, so all clusters it covers are unused */
+			pos = (loff_t)r1.index * qcow2->refblock_entries * clu_size;
+			pos -= clu_size;
+			continue;
+		}
+
+		if (ret < 0)
+			return ret;
+
+		ret = qcow2_md_page_read_sync(qcow2, r2.page_id, &r2.md);
+		if (ret)
+			return ret;
+
+		for (i = r2.index_in_page; i >= 0; i--, pos -= clu_size) {
+			if (get_r2_entry(qcow2, r2.md, i))
+				return pos + clu_size;
+		}
+	}
+
+	return -EIO;
+}
+
 static int truncate_prealloc_safe(struct qcow2 *qcow2, loff_t len, const char *func)
 {
 	loff_t prealloc_len, max_prealloc_len = qcow2->reftable_max_file_size;
diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
index 3e47e66b4007..3cbec3b9b407 100644
--- a/drivers/md/dm-qcow2-target.c
+++ b/drivers/md/dm-qcow2-target.c
@@ -1042,11 +1042,21 @@ static void qcow2_truncate_preallocations(struct dm_target *ti)
 {
 	struct qcow2_target *tgt = to_qcow2_target(ti);
 	struct qcow2 *qcow2 = top_qcow2_protected(ti);
-	loff_t end = qcow2->file_preallocated_area_start;
+	loff_t used_end, end = qcow2->file_preallocated_area_start;
 	int ret;
 
 	if (!(dm_table_get_mode(ti->table) & FMODE_WRITE))
 		return;
+
+	/* Refcounts on disk are stale after failed md writeback */
+	if (!tgt->md_writeback_error) {
+		used_end = qcow2_calc_used_end(qcow2);
+		if (used_end < 0)
+			QC_ERR(ti, "dm-qcow2: Can't find used end: %lld", used_end);
+		else
+			end = used_end;
+	}
+
 	if (end == qcow2->file_size)
 		return;
 
diff --git a/drivers/md/dm-qcow2.h b/drivers/md/dm-qcow2.h
index 69d3ae6fca8a..73d3cf175bbf 100644
--- a/drivers/md/dm-qcow2.h
+++ b/drivers/md/dm-qcow2.h
@@ -411,6 +411,7 @@ void qcow2_ref_dec(struct qcow2_target *tgt, u8 ref_index);
 int qcow2_inflight_ref_switch(struct qcow2_target *tgt);
 void qcow2_flush_deferred_activity(struct qcow2_target *tgt, struct qcow2 *qcow2);
 int qcow2_truncate_safe(struct file *file, loff_t new_len);
+loff_t qcow2_calc_used_end(struct qcow2 *qcow2);
 
 void qcow2_merge_backward_work(struct work_struct *work);
 void qcow2_merge_backward_cancel(struct qcow2_target *tgt);
-- 
2.53.0

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

[Devel] [PATCH hci-8.1 v2 3/6] 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.

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>
---
 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 a7849de8aaa6..76fcfa64f875 100644
--- a/drivers/md/dm-ploop-bat.c
+++ b/drivers/md/dm-ploop-bat.c
@@ -501,6 +501,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;
@@ -521,7 +522,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 v2 4/6] 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.

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>
---
 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 76fcfa64f875..93119e97ad0c 100644
--- a/drivers/md/dm-ploop-bat.c
+++ b/drivers/md/dm-ploop-bat.c
@@ -473,23 +473,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;
@@ -501,9 +505,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;
+}
+
+/*
+ * @fd refers to a new delta, which is placed right before top_delta.
+ * So, userspace has to populate deltas stack from oldest to newest.
+ */
+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);
@@ -524,7 +552,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 v2 5/6] dm-ploop: reload metadata on resume

[Alexander Ivanov]

On "dmsetup reload" the new ploop reads metadata of the images in
ploop_ctr(), but IO continues through the old ploop until the device
is suspended. So the old ploop may change BAT and sizes of the delta
files, and after resume the new ploop works with outdated metadata,
e.g. allocates clusters already used by the old one or relies on a
wrong file size. Also userspace may modify images of suspended device
(e.g. merge zeroes BAT entries of the base delta, snapshot creation
trims the tail of the top delta).

Reload all the metadata in ploop_preresume(): re-read header of the top
delta, BAT of every delta and sizes of delta files, and rebuild BAT
tree and holes_bitmap from scratch. On error the old metadata is kept
and resume fails.

Skip the in-use signature check on reload, as the image is already
checked in ploop_ctr(), and userspace doesn't mark the delta, which
becomes top after flip_upper_deltas, as in use. Instead it relies that
the in-use signature of the old top delta from in-memory header will
be written to the new top delta with BAT page 0. So keep the in-use
signature of the old in-memory header in the reloaded one, otherwise
the next table load fails with -ENOLCK after online merge.

Metadata of the device which has already been running can't be dirty
after suspend, since all IO is completed. If it is dirty anyway, keep
it as is, as md pages may still be referenced.

The new ploop must not modify images before resume either, so request
preallocation in ploop_preresume() after the reload instead of
ploop_ctr(). Otherwise it may truncate the top delta back to the stale
file size. And wait for pending preallocation on postsuspend, so that
the reloaded ploop sees the final size of the top delta.

Reload frees the old md pages tree, while ploop_llseek_hole() walks it
locklessly: dm_llseek_hole() checks for suspend only once on entry and
isn't synchronized with it. So take ctl_rwsem for read in
ploop_llseek_hole() (reload is done under it for write) and return
-EAGAIN if the device is suspended. This also protects the walk from
resize, which changes the tree under ctl_rwsem too.

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    | 130 ++++++++++++++++++++++++++++++++++-
 drivers/md/dm-ploop-map.c    |  26 ++++++-
 drivers/md/dm-ploop-target.c |  55 ++++++++-------
 drivers/md/dm-ploop.h        |   5 +-
 4 files changed, 186 insertions(+), 30 deletions(-)

diff --git a/drivers/md/dm-ploop-bat.c b/drivers/md/dm-ploop-bat.c
index 93119e97ad0c..fa87dd248f5b 100644
--- a/drivers/md/dm-ploop-bat.c
+++ b/drivers/md/dm-ploop-bat.c
@@ -184,7 +184,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;
@@ -199,7 +200,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;
 
@@ -556,6 +558,130 @@ 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 (md->status & (MD_DIRTY | MD_WRITEBACK | MD_UPDATING) ||
+		    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;
+	struct ploop_pvd_header *hdr, *old_hdr;
+	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 is completed, 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;
+
+	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.
+	 */
+	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_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->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-map.c b/drivers/md/dm-ploop-map.c
index 48466435dd19..6cf4912bd3b8 100644
--- a/drivers/md/dm-ploop-map.c
+++ b/drivers/md/dm-ploop-map.c
@@ -1245,6 +1245,12 @@ static int ploop_no_pending_prealloc(struct ploop *ploop)
 	return ret;
 }
 
+void ploop_wait_prealloc(struct ploop *ploop)
+{
+	wait_event(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);
@@ -2363,7 +2369,7 @@ int ploop_allocator(void *data)
 					PL_FMT("failed to allocate space: %d\n"),
 					       ploop_device_name(ploop), ret);
 			}
-			wake_up_interruptible(&ploop->dispatcher_wq_prealloc);
+			wake_up(&ploop->dispatcher_wq_prealloc);
 			current_restore_flags(old_flags, PLOOP_PFLAGS);
 		}
 		if (kthread_should_stop()) {
@@ -2764,6 +2770,16 @@ loff_t ploop_llseek_hole(struct dm_target *ti, loff_t offset, int whence)
 	struct md_page *md;
 	loff_t result;
 
+	/*
+	 * Protect md pages tree from being freed or replaced under us
+	 * by metadata reload in ploop_preresume() or by resize.
+	 */
+	down_read(&ploop->ctl_rwsem);
+	if (ploop->suspended) {
+		result = -EAGAIN;
+		goto unlock;
+	}
+
 	clu = SEC_TO_CLU(ploop, to_sector(offset) + ploop->skip_off);
 	id = U32_MAX;
 
@@ -2771,8 +2787,10 @@ loff_t ploop_llseek_hole(struct dm_target *ti, loff_t offset, int whence)
 		if (id != ploop_bat_clu_to_page_nr(clu)) {
 			id = ploop_bat_clu_to_page_nr(clu);
 			md = ploop_md_page_find(ploop, id);
-			if (!md)
-				return -EINVAL;
+			if (!md) {
+				result = -EINVAL;
+				goto unlock;
+			}
 		}
 
 		bat = md->kmpage;
@@ -2787,5 +2805,7 @@ loff_t ploop_llseek_hole(struct dm_target *ti, loff_t offset, int whence)
 	}
 
 	result = to_bytes(CLU_TO_SEC(ploop, clu) - ploop->skip_off);
+unlock:
+	up_read(&ploop->ctl_rwsem);
 	return result;
 }
diff --git a/drivers/md/dm-ploop-target.c b/drivers/md/dm-ploop-target.c
index 3b589579741c..c9183fa9d3ca 100644
--- a/drivers/md/dm-ploop-target.c
+++ b/drivers/md/dm-ploop-target.c
@@ -288,7 +288,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;
 
@@ -419,8 +419,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
@@ -431,23 +430,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)
 {
@@ -637,9 +632,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:
@@ -743,6 +735,13 @@ 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.
+	 */
+	ploop_wait_prealloc(ploop);
 	ploop_set_suspended(ti, true);
 }
 static int ploop_preresume(struct dm_target *ti)
@@ -750,12 +749,20 @@ static int ploop_preresume(struct dm_target *ti)
 	struct ploop *ploop = ti->private;
 	int ret = 0;
 
-	down_read(&ploop->ctl_rwsem);
+	down_write(&ploop->ctl_rwsem);
 	if (ploop->noresume)
 		ret = -EAGAIN;
-	up_read(&ploop->ctl_rwsem);
+	else
+		ret = ploop_reload_metadata(ploop);
+	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 (!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 23662943d56a..1cf0d5e0cea8 100644
--- a/drivers/md/dm-ploop.h
+++ b/drivers/md/dm-ploop.h
@@ -631,7 +631,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,
@@ -649,6 +651,7 @@ extern void ploop_disable_writeback_delay(struct ploop *ploop);
 extern void ploop_enable_writeback_delay(struct ploop *ploop);
 
 extern void ploop_should_prealloc(struct ploop *ploop);
+extern void ploop_wait_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 v2 6/6] 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.

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.

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 | 49 +++++++++++++++++++++++++++++--
 drivers/md/dm-ploop.h        |  3 +-
 3 files changed, 104 insertions(+), 4 deletions(-)

diff --git a/drivers/md/dm-ploop-bat.c b/drivers/md/dm-ploop-bat.c
index fa87dd248f5b..0a40702fb7d3 100644
--- a/drivers/md/dm-ploop-bat.c
+++ b/drivers/md/dm-ploop-bat.c
@@ -682,6 +682,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, which is used
+ * according to its header and BAT: either BAT area, the last mapped
+ * cluster or format extension. Clusters freed by discard may leave the
+ * tail of file unused. Metadata is read from disk, as the next table
+ * load checks BAT entries against the file size, while in-memory BAT
+ * may differ from it after failed BAT writeback. Also userspace may
+ * write format extension (e.g. CBT) behind the last data cluster of
+ * suspended device. This 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;
+	loff_t used_end, ext_end;
+	struct rb_node *node;
+	struct md_page *md;
+	u64 ext_off;
+	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;
+		}
+	}
+
+	used_end = CLU_TO_POS(ploop, used_clus);
+
+	/* Format extension header is the last cluster of extension */
+	hdr = ploop_md_first_entry(&md_root)->kmpage;
+	ext_off = le64_to_cpu(hdr->m_FormatExtensionOffset);
+	if (ext_off) {
+		ext_end = to_bytes(ext_off) + CLU_SIZE(ploop);
+		if (ext_end > used_end)
+			used_end = ext_end;
+	}
+
+	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 c9183fa9d3ca..88cc91a3d03a 100644
--- a/drivers/md/dm-ploop-target.c
+++ b/drivers/md/dm-ploop-target.c
@@ -441,8 +441,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)
 {
@@ -733,16 +734,58 @@ 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;
+
+	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;
 
+	down_write(&ploop->ctl_rwsem);
 	/*
 	 * Let the next resumed ploop (maybe of reloaded table)
 	 * see final size of the top delta.
 	 */
 	ploop_wait_prealloc(ploop);
-	ploop_set_suspended(ti, true);
+	ploop_truncate_unused_tail(ploop);
+	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 1cf0d5e0cea8..e46d6efdce59 100644
--- a/drivers/md/dm-ploop.h
+++ b/drivers/md/dm-ploop.h
@@ -35,7 +35,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)
 
@@ -634,6 +634,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 v2 1/6] dm-qcow2: add qcow2_md_page_read_sync() helper

[no-reply]

> Add a helper, which returns an up to date md page from md cache,
> or allocates a new one, inserts it and reads it synchronously.
> On read failure the page is erased from cache and freed.
> 
> Use it in qcow2_parse_header() instead of open-coded allocation
> and read of page 0. It will also be used by the next patch.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-148171
> Feature: dm-qcow2: block device over QCOW2 files 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/20261008073837.510678-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 v2 2/6] dm-qcow2: truncate unused tail of image on suspend

[no-reply]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dm-qcow2: qcow2_calc_used_end() can return an end beyond file_size, so suspend grows the file
- [Low] dm-qcow2: duplicated prefix in used-end error message in qcow2_truncate_preallocations()
--

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

dm-qcow2: truncate unused tail of image on suspend

This patch adds qcow2_calc_used_end(), which scans refcount blocks
backward from file_preallocated_area_start and returns the end of the
last cluster with a non-zero refcount. On postsuspend the image file is
then truncated to that end instead of only to
file_preallocated_area_start. The scan is skipped if md writeback
failed, and the old behaviour is used if the scan fails.

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

> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
> --- a/drivers/md/dm-qcow2-map.c
> +++ b/drivers/md/dm-qcow2-map.c

[ ... ]

> +loff_t qcow2_calc_used_end(struct qcow2 *qcow2)
> +{
> +	u32 clu_size = qcow2->clu_size;
> +	struct qcow2_map_item r1, r2;
> +	struct md_page *r1_md = NULL;
> +	loff_t pos;
> +	s32 i;
> +	int ret;
> +
> +	pos = round_down(qcow2->file_preallocated_area_start - 1, clu_size);

[ ... ]

> +		for (i = r2.index_in_page; i >= 0; i--, pos -= clu_size) {
> +			if (get_r2_entry(qcow2, r2.md, i))
> +				return pos + clu_size;
> +		}

[Severity: Medium]
Can the value returned here exceed qcow2->file_size?

file_preallocated_area_start is only page aligned after
qcow2_parse_metadata(), not aligned to clu_size.  When clu_size is larger
than PAGE_SIZE and the file ends in the middle of a cluster, the first
cluster examined starts below file_preallocated_area_start but extends
past it.

If that cluster has a non-zero refcount, the function returns
pos + clu_size, which is greater than file_size.

> diff --git a/drivers/md/dm-qcow2-target.c b/drivers/md/dm-qcow2-target.c
> --- a/drivers/md/dm-qcow2-target.c
> +++ b/drivers/md/dm-qcow2-target.c
> @@ -1042,11 +1042,21 @@ static void qcow2_truncate_preallocations(struct dm_target *ti)

[ ... ]

> +	/* Refcounts on disk are stale after failed md writeback */
> +	if (!tgt->md_writeback_error) {
> +		used_end = qcow2_calc_used_end(qcow2);
> +		if (used_end < 0)
> +			QC_ERR(ti, "dm-qcow2: Can't find used end: %lld", used_end);
> +		else
> +			end = used_end;
> +	}
> +
>  	if (end == qcow2->file_size)
>  		return;
>  
>  	ret = qcow2_truncate_safe(qcow2->file, end);

[Severity: Medium]
Is the check against file_size sufficient here?

Only end == qcow2->file_size is tested, so with used_end greater than
file_size the code goes on to call qcow2_truncate_safe(), and ftruncate
extends the file.  qcow2->file_size and file_preallocated_area_start are
then set to the larger value, although this patch is meant only to shrink
the image.

The growth is at most one cluster, but would it make sense to clamp with
min(used_end, qcow2->file_size), or to use end >= qcow2->file_size for the
early return?

[Severity: Low]
This isn't a bug, but QC_ERR already adds the "dm-qcow2: " prefix through
QCOW2_FMT, so this message prints the prefix twice:

    dm-qcow2: <dev>: dm-qcow2: Can't find used end: -5

The raw negative errno is also printed with %lld.  Neighbouring messages in
the same function repeat the prefix as well.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261008073837.510678-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 v2 4/6] 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.
> 
> 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/20261008073837.510678-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 v2 3/6] dm-ploop: set delta is_raw before applying its mappings

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] dm-ploop: zero-length raw base delta underflows nr_be - 1 in init_be_iter()
--

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

dm-ploop: set delta is_raw before applying its mappings

This patch moves the assignment of deltas[level].is_raw in
ploop_add_delta() so that it happens before
ploop_apply_delta_mappings() runs. Previously a raw delta was handled
as a non-raw one, which dereferenced a NULL d_md.

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

> diff --git a/drivers/md/dm-ploop-bat.c b/drivers/md/dm-ploop-bat.c
> index a7849de8aaa6..76fcfa64f875 100644
> --- a/drivers/md/dm-ploop-bat.c
> +++ b/drivers/md/dm-ploop-bat.c

[ ... ]

> @@ -501,6 +501,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);

[Severity: Low]
With this change the raw branch of ploop_apply_delta_mappings() becomes
reachable, and it looks like it can misbehave for a zero-length raw base
delta.

For a raw delta, nr_be comes straight from the file size:

ploop_add_delta()
    nr_be = POS_TO_CLU(ploop, file_size);

ploop_check_delta_length() only rejects sizes that are not cluster
aligned, so a size of 0 passes. The check against deltas[level + 1].nr_be
cannot fail for 0 either.

Then, with nr_be == 0, init_be_iter() computes:

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

nr_be - 1 is 0xFFFFFFFF, and the u32 wrap in ploop_bat_clu_to_page_nr()
gives last_page = 0. Page 0 then gets start=16 and end=15 and is
skipped, while every other page iterates in full.

In ploop_apply_delta_mappings() the stop condition can never trigger:

    if (clu == nr_be - 1)
        stop = true;
    ...
    dst_clu = clu;

so can identity mappings be written at the raw level for every
unpopulated slot on pages above 0, past EOF of the raw file, including
tail slots of the last page beyond nr_bat_entries?

Should ploop_add_delta() reject nr_be == 0 for a raw delta (or should
init_be_iter() handle it)?

The BAT arrays do not appear to be overrun since the loop stays within
each md page. Reads of those clusters would hit EOF on the raw file and
likely return zeros, like a hole. I did not trace the write/COW path for
these mappings. Reaching this requires a table load with an empty
"raw@fd" base, so it needs CAP_SYS_ADMIN and is an unusual
misconfiguration. I did not find a check for this in later patches of
the series.

>  
>  	deltas[level].file = file;

[ ... ]

> @@ -521,7 +522,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);

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261008073837.510678-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 v2 6/6] dm-ploop: truncate unused tail of top delta on suspend

[no-reply]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] dm-ploop: unvalidated m_FormatExtensionOffset can truncate delta to unaligned size
- [Medium] dm-ploop: GFP_KERNEL BAT read in postsuspend() can deadlock via reclaim IO
- [Low] dm-ploop: WARN_ON_ONCE in ploop_calc_used_end() can fire on a benign inflight_md race
- [Low] dm-ploop: tail truncation assumes CBT header is the last extension cluster
--

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

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

This patch adds ploop_calc_used_end(), which reads the on-disk BAT and the
format extension offset to find the end of the used part of the top delta.
ploop_postsuspend() now truncates the top delta to that end under
ctl_rwsem. The header field m_Reserved is renamed to m_FormatExtensionOffset.

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

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

[ ... ]

> +loff_t ploop_calc_used_end(struct ploop *ploop, struct file *file)
> +{

[ ... ]

> +	/* BAT on disk may miss allocated clusters until writeback is done */
> +	if (WARN_ON_ONCE(!ploop_md_pages_are_clean(ploop)))
> +		return -EBUSY;

[Severity: Low]
Can this WARN_ON_ONCE() fire on a benign race? The commit message
describes the not-clean case as something to skip, but here it raises a
warning backtrace (and would panic with panic_on_warn).

In ploop_put_piwb(), the data pios are ended first, and inflight_md is
decremented only afterwards:

    ploop_bat_write_finish(piwb->pio, piwb, piwb->bi_status);
    ...
    if (atomic_dec_and_test(&piwb->ploop->inflight_md))
            wake_up_interruptible(&piwb->ploop->inflight_md_wq);

dm suspend can see those bios complete and reach ploop_postsuspend()
while inflight_md is still 1. ploop_postsuspend() only calls
ploop_wait_prealloc(), and does not wait on inflight_md_wq. So
ploop_md_pages_are_clean() can return false with no I/O error involved.

ploop_truncate_unused_tail() then also prints "Can't find used end of top
delta: -16" and skips the truncation. Should this be a plain condition,
or should ploop_postsuspend() wait on inflight_md_wq first? The same
pattern is already in ploop_reload_metadata(), but this adds a second
site.

[ ... ]

> +	ret = ploop_read_delta_metadata(ploop, file, &md_root, &nr_be);
> +	if (ret)
> +		return ret;

[Severity: Medium]
Can this full BAT read deadlock when called from ploop_postsuspend()?

ploop_read_delta_metadata() allocates with GFP_KERNEL, both through
ploop_prealloc_md_pages() (alloc_page()) and in:

    bvec = kvmalloc(sizeof(*bvec) * nr_segs, GFP_KERNEL);

dm_suspend() calls dm_table_postsuspend_targets() after IO to the device
has been stopped, with suspend_lock held and no memalloc_noio scope.
Direct reclaim here could write back dirty pages of a filesystem on the
ploop device, or swap out to it. Those bios are deferred because the
device is suspended, so the reclaiming task waits for a resume. The
resume needs md->suspend_lock, which the suspending task still holds.

The old ploop_postsuspend() did no allocations. Would wrapping the call
in memalloc_noio_save()/memalloc_noio_restore() be appropriate? The same
hazard already exists in ploop_reload_metadata() from ploop_preresume(),
but this patch adds the postsuspend site.

[ ... ]

> +	/* Format extension header is the last cluster of extension */
> +	hdr = ploop_md_first_entry(&md_root)->kmpage;
> +	ext_off = le64_to_cpu(hdr->m_FormatExtensionOffset);
> +	if (ext_off) {
> +		ext_end = to_bytes(ext_off) + CLU_SIZE(ploop);

[Severity: Medium]
Can an unvalidated m_FormatExtensionOffset lead to an unaligned
truncation? This field used to be m_Reserved[8] and was never
interpreted. ploop_delta_check_header() and ploop_setup_metadata() do not
look at it.

If ext_off is sector-aligned but not cluster-aligned, ext_end is not
cluster-aligned. If it is also above the BAT-derived used_end and below
top->file_size, ploop_truncate_unused_tail() passes it to do_ftruncate().

The next ploop_preresume() -> ploop_reload_metadata() ->
ploop_read_delta() -> ploop_check_delta_length() then hits:

    if (loff & (CLU_SIZE(ploop) - 1))
            return -EPROTO;

and the device cannot be resumed.

Also, to_bytes() is an unsigned long shift by 9, so a large ext_off such
as 2^55 or more wraps, and the wrapped value can land in the same range.

Should ext_off be required to be cluster-aligned and checked for
overflow, with truncation skipped when it is out of range?

> +		if (ext_end > used_end)
> +			used_end = ext_end;
> +	}

[Severity: Low]
Is it guaranteed that the format extension header is the last cluster of
the extension? The code assumes the extension ends one cluster after its
offset:

    ext_end = to_bytes(ext_off) + CLU_SIZE(ploop);

If a CBT extension ever has data after its header cluster, anything beyond
ext_end that lies past the last BAT-referenced cluster is cut off by
do_ftruncate() in ploop_truncate_unused_tail(). That loss is irreversible.

The only support for this layout is the commit message ("as userspace
does") and the code comment. Could the layout be documented in the code or
in a header comment?

[ ... ]

> 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

[ ... ]

> +	end = ploop_calc_used_end(ploop, top->file);
> +	if (end < 0) {
> +		PL_ERR("Can't find used end of top delta: %lld", end);
> +		return;
> +	}

[Severity: Low]
This isn't a bug, but when ploop_calc_used_end() returns -EBUSY for the
not-clean skip case, this path reports it as an error on top of the
WARN_ON_ONCE() in ploop_calc_used_end(). Should the skip case be silent?

[ ... ]

> +	ret = do_ftruncate(top->file, end, 0);

[ ... ]

> +	ploop_wait_prealloc(ploop);
> +	ploop_truncate_unused_tail(ploop);
> +	ploop->suspended = true;
> +	up_write(&ploop->ctl_rwsem);

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261008073837.510678-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 v2 5/6] dm-ploop: reload metadata on resume

[no-reply]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] dm-ploop: lost wakeup in ploop_allocator() can hang ploop_postsuspend() in wait_event()
- [Medium] dm-ploop: ploop_reload_metadata() uses GFP_KERNEL allocs with no NOIO scope
- [Medium] dm-ploop: postsuspend wait_event on prealloc can hang on allocator lost wakeup
- [Low] dm-ploop: ploop_reload_metadata() restore path leaves m_Sig/m_Type/m_Sectors modified
--

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

dm-ploop: reload metadata on resume

This patch makes ploop_preresume() re-read the top delta header, the BAT
of every delta and the delta file sizes, and rebuild the BAT tree and
holes_bitmap. It moves preallocation from ploop_ctr() to ploop_preresume(),
makes ploop_postsuspend() wait for pending preallocation, and takes
ctl_rwsem in ploop_llseek_hole().

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

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

[ ... ]

> +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;

[ ... ]

> +	old_deltas = kmemdup(ploop->deltas,
> +			     sizeof(*old_deltas) * ploop->nr_deltas,
> +			     GFP_KERNEL);
> +	page = alloc_page(GFP_KERNEL);

[Severity: Medium]
Can these GFP_KERNEL allocations deadlock under memory pressure?

ploop_preresume() runs in the DM ioctl task while the device is still
suspended, and no memalloc_noio scope is set on that path.  Direct reclaim
with __GFP_IO and __GFP_FS could write back dirty pages of a filesystem
mounted on this ploop device.  Those bios are held until resume completes,
but the resume is the task waiting in the allocation.

The same applies to the other allocations reached from here:

ploop_reload_metadata()
  ploop_setup_metadata()
    ploop_setup_holes_bitmap()
      ploop->holes_bitmap = kvmalloc(size, GFP_KERNEL);

and to ploop_prealloc_md_pages().  The ploop kthreads avoid this by running
with PF_MEMALLOC_NOIO (PLOOP_PFLAGS in dm-ploop-map.c), but the ioctl task
has no such protection.

Would it make sense to wrap ploop_reload_metadata() in
memalloc_noio_save()/memalloc_noio_restore(), or to use GFP_NOIO?

[ ... ]

> +	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;

[ ... ]

> +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;

[Severity: Low]
The commit message says "On error the old metadata is kept and resume
fails."  Is that fully true here?

ploop_setup_metadata() overwrites these fields before its later checks
('custom FirstBlockOffset', 'Too short BAT', ploop_setup_holes_bitmap()
returning -ENOMEM):

	memcpy(ploop->m_Sig, m_hdr->m_Sig, sizeof(ploop->m_Sig));
	ploop->m_Type = le32_to_cpu(m_hdr->m_Type);
	ploop->m_Sectors = le32_to_cpu(m_hdr->m_Sectors);

The restore label puts back bat_entries, holes_bitmap, nr_bat_entries,
hb_nr and deltas[], but not m_Sig, m_Type or m_Sectors.  The same applies
when ploop_prealloc_md_pages() or ploop_read_delta() fails.

m_Sig and m_Sectors are validated, so in practice only m_Type could differ,
and only if userspace rewrote the header.  ploop_delta_check_header()
compares against these fields, so a later delta check could then use a value
that does not match the retained old BAT.

Should these three fields be saved and restored along with the others?

> diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
> --- a/drivers/md/dm-ploop-map.c
> +++ b/drivers/md/dm-ploop-map.c
> @@ -1245,6 +1245,12 @@ static int ploop_no_pending_prealloc(struct ploop *ploop)
>  	return ret;
>  }
>  
> +void ploop_wait_prealloc(struct ploop *ploop)
> +{
> +	wait_event(ploop->dispatcher_wq_prealloc,
> +		   ploop_no_pending_prealloc(ploop));
> +}

[Severity: Medium]
Can this wait hang forever on a lost wakeup in the allocator?

This is an unbounded, uninterruptible wait whose only waker is the
wake_up() in ploop_allocator() after a preallocation completes.  In
ploop_allocator(), prealloc_size is tested locklessly before the task state
is set:

	if (ploop->prealloc_size) {
		...
	}
	if (kthread_should_stop()) {...}
	set_current_state(TASK_INTERRUPTIBLE);
	schedule();

Consider this interleaving:

  allocator: reads prealloc_size == 0, not yet in set_current_state()
  requester: ploop_req_prealloc() sets prealloc_size under bat_lock
  requester: wake_up_process(ploop->kt_allocator->task)  /* TASK_RUNNING */
  allocator: set_current_state(TASK_INTERRUPTIBLE); schedule();

The wakeup is a no-op and the allocator sleeps with prealloc_size != 0.
ploop_req_prealloc() returns early when the requested size is already
covered, so later IO would not re-kick it, and suspend has quiesced IO
anyway.  ploop_no_pending_prealloc() then never becomes true.

The race in the allocator predates this patch.  Before it, a lost wakeup
only delayed preallocation until the next request.  Now it can hang
ploop_postsuspend().  The new ploop_should_prealloc() call in
ploop_preresume() also adds a new place to trigger it.

Could the allocator re-check prealloc_size after set_current_state(), or
could the wait be made bounded or killable?

[ ... ]

> 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.
> +	 */
> +	ploop_wait_prealloc(ploop);
>  	ploop_set_suspended(ti, true);
>  }

[Severity: Medium]
Can ploop_postsuspend() block here indefinitely?

If the allocator loses the wakeup described above, ploop_wait_prealloc()
never returns and dmsetup suspend hangs in D state.  This is a new
dependency of the suspend path on the allocator making progress.

The follow-up patch "dm-ploop: truncate unused tail of top delta on
suspend" does not touch ploop_allocator() or ploop_wait_prealloc().  It
makes ploop_postsuspend() call ploop_wait_prealloc() under
down_write(&ctl_rwsem), so in the final series state the hang would also
block every other ctl_rwsem user (message, status, ploop_llseek_hole()).

Would it be possible to fix the allocator wakeup, or to avoid waiting while
holding ctl_rwsem for write?

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261008073837.510678-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 v2 2/6] dm-qcow2: truncate unused tail of image on suspend

[Alexander Ivanov]

> [Severity: Medium]
> Can the value returned here exceed qcow2->file_size?
>
> file_preallocated_area_start is only page aligned after
> qcow2_parse_metadata(), not aligned to clu_size.  When clu_size is larger
> than PAGE_SIZE and the file ends in the middle of a cluster, the first
> cluster examined starts below file_preallocated_area_start but extends
> past it.
>
> If that cluster has a non-zero refcount, the function returns
> pos + clu_size, which is greater than file_size.

Fixed.

> [Severity: Medium]
> Is the check against file_size sufficient here?
>
> Only end == qcow2->file_size is tested, so with used_end greater than
> file_size the code goes on to call qcow2_truncate_safe(), and ftruncate
> extends the file.  qcow2->file_size and file_preallocated_area_start are
> then set to the larger value, although this patch is meant only to shrink
> the image.
>
> The growth is at most one cluster, but would it make sense to clamp with
> min(used_end, qcow2->file_size), or to use end >= qcow2->file_size for the
> early return?
After the previous fix qcow2_calc_used_end() can't return value greater 
than file_size.

> [Severity: Low]
> This isn't a bug, but QC_ERR already adds the "dm-qcow2: " prefix through
> QCOW2_FMT, so this message prints the prefix twice:
>
>      dm-qcow2: <dev>: dm-qcow2: Can't find used end: -5
>
> The raw negative errno is also printed with %lld.  Neighbouring messages in
> the same function repeat the prefix as well.
Fixed in the old and the new code.


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

Re: [Devel] [PATCH hci-8.1 v2 3/6] dm-ploop: set delta is_raw before applying its mappings

[Alexander Ivanov]

> [Severity: Low]
> With this change the raw branch of ploop_apply_delta_mappings() becomes
> reachable, and it looks like it can misbehave for a zero-length raw base
> delta.
>
> For a raw delta, nr_be comes straight from the file size:
>
> ploop_add_delta()
>      nr_be = POS_TO_CLU(ploop, file_size);
>
> ploop_check_delta_length() only rejects sizes that are not cluster
> aligned, so a size of 0 passes. The check against deltas[level + 1].nr_be
> cannot fail for 0 either.
>
> Then, with nr_be == 0, init_be_iter() computes:
>
>      u32 rem, last_page = ploop_bat_clu_to_page_nr(nr_be - 1);
>
> nr_be - 1 is 0xFFFFFFFF, and the u32 wrap in ploop_bat_clu_to_page_nr()
> gives last_page = 0. Page 0 then gets start=16 and end=15 and is
> skipped, while every other page iterates in full.
>
> In ploop_apply_delta_mappings() the stop condition can never trigger:
>
>      if (clu == nr_be - 1)
>          stop = true;
>      ...
>      dst_clu = clu;
>
> so can identity mappings be written at the raw level for every
> unpopulated slot on pages above 0, past EOF of the raw file, including
> tail slots of the last page beyond nr_bat_entries?
>
> Should ploop_add_delta() reject nr_be == 0 for a raw delta (or should
> init_be_iter() handle it)?
>
> The BAT arrays do not appear to be overrun since the loop stays within
> each md page. Reads of those clusters would hit EOF on the raw file and
> likely return zeros, like a hole. I did not trace the write/COW path for
> these mappings. Reaching this requires a table load with an empty
> "raw@fd" base, so it needs CAP_SYS_ADMIN and is an unusual
> misconfiguration. I did not find a check for this in later patches of
> the series.
Fixed by putting ploop_apply_delta_mappings() call under if (nr_be) 
condition.
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v2 5/6] dm-ploop: reload metadata on resume

[Alexander Ivanov]

> [Severity: Medium]
> Can these GFP_KERNEL allocations deadlock under memory pressure?
>
> ploop_preresume() runs in the DM ioctl task while the device is still
> suspended, and no memalloc_noio scope is set on that path.  Direct reclaim
> with __GFP_IO and __GFP_FS could write back dirty pages of a filesystem
> mounted on this ploop device.  Those bios are held until resume completes,
> but the resume is the task waiting in the allocation.
>
> The same applies to the other allocations reached from here:
>
> ploop_reload_metadata()
>    ploop_setup_metadata()
>      ploop_setup_holes_bitmap()
>        ploop->holes_bitmap = kvmalloc(size, GFP_KERNEL);
>
> and to ploop_prealloc_md_pages().  The ploop kthreads avoid this by running
> with PF_MEMALLOC_NOIO (PLOOP_PFLAGS in dm-ploop-map.c), but the ioctl task
> has no such protection.
>
> Would it make sense to wrap ploop_reload_metadata() in
> memalloc_noio_save()/memalloc_noio_restore(), or to use GFP_NOIO?
Wrapped ploop_reload_metadata() in ploop_preresume() with 
memalloc_noio_save()/restore() rather than switching to GFP_NOIO, so 
nested allocations (holes_bitmap, md pages, backing fs reads) are 
covered as well. The same applies to ploop_truncate_unused_tail() in 
ploop_postsuspend() in the next patch, fixed there too.

> [Severity: Low]
> The commit message says "On error the old metadata is kept and resume
> fails."  Is that fully true here?
>
> ploop_setup_metadata() overwrites these fields before its later checks
> ('custom FirstBlockOffset', 'Too short BAT', ploop_setup_holes_bitmap()
> returning -ENOMEM):
>
> 	memcpy(ploop->m_Sig, m_hdr->m_Sig, sizeof(ploop->m_Sig));
> 	ploop->m_Type = le32_to_cpu(m_hdr->m_Type);
> 	ploop->m_Sectors = le32_to_cpu(m_hdr->m_Sectors);
>
> The restore label puts back bat_entries, holes_bitmap, nr_bat_entries,
> hb_nr and deltas[], but not m_Sig, m_Type or m_Sectors.  The same applies
> when ploop_prealloc_md_pages() or ploop_read_delta() fails.
>
> m_Sig and m_Sectors are validated, so in practice only m_Type could differ,
> and only if userspace rewrote the header.  ploop_delta_check_header()
> compares against these fields, so a later delta check could then use a value
> that does not match the retained old BAT.
>
> Should these three fields be saved and restored along with the others?
Fixed.

>> +void ploop_wait_prealloc(struct ploop *ploop)
>> +{
>> +	wait_event(ploop->dispatcher_wq_prealloc,
>> +		   ploop_no_pending_prealloc(ploop));
>> +}
> [Severity: Medium]
> Can this wait hang forever on a lost wakeup in the allocator?
>
> This is an unbounded, uninterruptible wait whose only waker is the
> wake_up() in ploop_allocator() after a preallocation completes.  In
> ploop_allocator(), prealloc_size is tested locklessly before the task state
> is set:
>
> 	if (ploop->prealloc_size) {
> 		...
> 	}
> 	if (kthread_should_stop()) {...}
> 	set_current_state(TASK_INTERRUPTIBLE);
> 	schedule();
>
> Consider this interleaving:
>
>    allocator: reads prealloc_size == 0, not yet in set_current_state()
>    requester: ploop_req_prealloc() sets prealloc_size under bat_lock
>    requester: wake_up_process(ploop->kt_allocator->task)  /* TASK_RUNNING */
>    allocator: set_current_state(TASK_INTERRUPTIBLE); schedule();
>
> The wakeup is a no-op and the allocator sleeps with prealloc_size != 0.
> ploop_req_prealloc() returns early when the requested size is already
> covered, so later IO would not re-kick it, and suspend has quiesced IO
> anyway.  ploop_no_pending_prealloc() then never becomes true.
>
> The race in the allocator predates this patch.  Before it, a lost wakeup
> only delayed preallocation until the next request.  Now it can hang
> ploop_postsuspend().  The new ploop_should_prealloc() call in
> ploop_preresume() also adds a new place to trigger it.
>
> Could the allocator re-check prealloc_size after set_current_state(), or
> could the wait be made bounded or killable?
Fixed in a separate patch "dm-ploop: fix lost wakeup in ploop_allocator()".

> [Severity: Medium]
> Can ploop_postsuspend() block here indefinitely?
>
> If the allocator loses the wakeup described above, ploop_wait_prealloc()
> never returns and dmsetup suspend hangs in D state.  This is a new
> dependency of the suspend path on the allocator making progress.
>
> The follow-up patch "dm-ploop: truncate unused tail of top delta on
> suspend" does not touch ploop_allocator() or ploop_wait_prealloc().  It
> makes ploop_postsuspend() call ploop_wait_prealloc() under
> down_write(&ctl_rwsem), so in the final series state the hang would also
> block every other ctl_rwsem user (message, status, ploop_llseek_hole()).
>
> Would it be possible to fix the allocator wakeup, or to avoid waiting while
> holding ctl_rwsem for write?
The lost wakeup in ploop_allocator() is fixed earlier in this series by
"dm-ploop: fix lost wakeup in ploop_allocator()". Also this patch
switches the allocator to wake_up() for dispatcher_wq_prealloc, since
ploop_wait_prealloc() uses uninterruptible wait_event().

ploop_preallocate_cluster() always clears prealloc_size and
prealloc_in_progress, including the error path, and the allocator never
takes ctl_rwsem, so the wait is bounded by the fallocate itself.

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