[Devel] [PATCH hci-8.1 v3 0/5] dm-ploop: truncate unused tail of top delta on suspend

[Devel] [PATCH hci-8.1 v3 0/5] 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 delta file never shrinks. This
series makes dm-ploop truncate the unused tail of the top delta on
suspend, so that userspace doesn't have to do it by hand.

This needs some 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.

Patch 1 fixes a lost wakeup of the allocator thread, which may leave
a preallocation request pending forever, so waiting for it on suspend
would hang. Patch 2 fixes raw and empty delta handling, patch 3
factors out ploop_read_delta(), patch 4 reloads all the metadata in
ploop_preresume(), and patch 5 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

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 (5):
  dm-ploop: fix lost wakeup in ploop_allocator()
  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    | 260 +++++++++++++++++++++++++++++++++--
 drivers/md/dm-ploop-map.c    |  77 +++++++----
 drivers/md/dm-ploop-target.c | 122 ++++++++++++----
 drivers/md/dm-ploop.h        |   8 +-
 4 files changed, 403 insertions(+), 64 deletions(-)

-- 
2.53.0

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

[Devel] [PATCH hci-8.1 v3 1/5] 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>
---
 drivers/md/dm-ploop-map.c | 49 ++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 21 deletions(-)

diff --git a/drivers/md/dm-ploop-map.c b/drivers/md/dm-ploop-map.c
index 48466435dd19..a3a5393e533d 100644
--- a/drivers/md/dm-ploop-map.c
+++ b/drivers/md/dm-ploop-map.c
@@ -2347,31 +2347,38 @@ 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 the state before checking the conditions, otherwise
+		 * wake_up_process() from ploop_req_prealloc() or
+		 * kthread_stop() may come in between and get lost.
+		 */
+		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 v3 2/5] 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.

Also skip applying the mappings of empty delta (e.g. raw delta of zero
size): with nr_be == 0 the BAT iterators never reach the last entry
(nr_be - 1 wraps around), so all the clusters would be mapped to the
delta.

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 | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-ploop-bat.c b/drivers/md/dm-ploop-bat.c
index a7849de8aaa6..2cb06193eaf9 100644
--- a/drivers/md/dm-ploop-bat.c
+++ b/drivers/md/dm-ploop-bat.c
@@ -501,7 +501,10 @@ 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;
 
-	ploop_apply_delta_mappings(ploop, deltas, level, &md_root, nr_be);
+	deltas[level].is_raw = is_raw;
+	/* Empty delta has no mappings, and BAT iterators can't handle it */
+	if (nr_be)
+		ploop_apply_delta_mappings(ploop, deltas, level, &md_root, nr_be);
 
 	deltas[level].file = file;
 	deltas[level].mtfile = kcalloc(ploop->nkt_runners, sizeof(*file),
@@ -521,7 +524,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 v3 3/5] 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 2cb06193eaf9..57a45119f0c4 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,11 +505,35 @@ 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;
 	/* Empty delta has no mappings, and BAT iterators can't handle it */
 	if (nr_be)
 		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);
@@ -526,7 +554,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 v3 4/5] 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. Note that data bios
are completed before BAT writeback is finished, so wait for inflight_md
on postsuspend too. Use wake_up() for inflight_md_wq, as the waiter is
uninterruptible.

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    | 139 ++++++++++++++++++++++++++++++++++-
 drivers/md/dm-ploop-map.c    |  30 ++++++--
 drivers/md/dm-ploop-target.c |  72 +++++++++++-------
 drivers/md/dm-ploop.h        |   5 +-
 4 files changed, 213 insertions(+), 33 deletions(-)

diff --git a/drivers/md/dm-ploop-bat.c b/drivers/md/dm-ploop-bat.c
index 57a45119f0c4..2e7949a1b154 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;
 
@@ -558,6 +560,139 @@ 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;
+	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.
+	 */
+	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->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-map.c b/drivers/md/dm-ploop-map.c
index a3a5393e533d..9a892ad71863 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);
 }
 
@@ -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);
@@ -2377,7 +2383,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);
 	}
 
@@ -2771,6 +2777,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;
 
@@ -2778,8 +2794,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;
@@ -2794,5 +2812,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..1fbcb48cff7a 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"
@@ -288,7 +289,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 +420,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 +431,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 +633,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,19 +736,48 @@ 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);
+	/*
+	 * 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;
 	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 {
+		/*
+		 * 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 (!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 v3 5/5] 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.

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    | 74 ++++++++++++++++++++++++++++++++++++
 drivers/md/dm-ploop-target.c | 56 +++++++++++++++++++++++++--
 drivers/md/dm-ploop.h        |  3 +-
 3 files changed, 128 insertions(+), 5 deletions(-)

diff --git a/drivers/md/dm-ploop-bat.c b/drivers/md/dm-ploop-bat.c
index 2e7949a1b154..5b5b5b1c4356 100644
--- a/drivers/md/dm-ploop-bat.c
+++ b/drivers/md/dm-ploop-bat.c
@@ -693,6 +693,80 @@ 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;
+	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;
+		}
+	}
+
+	/*
+	 * Format extension (CBT) is written by userspace on umount, see
+	 * delta_save_optional_header() in ploop library: dirty bitmap
+	 * blocks are appended at EOF one by one, then the header cluster
+	 * is appended behind them and m_FormatExtensionOffset points to
+	 * it. So the header is the last cluster of the extension, and
+	 * userspace treats L1 pointers beyond it as corruption (see
+	 * check_ext_blocks_from_raw()).
+	 */
+	hdr = ploop_md_first_entry(&md_root)->kmpage;
+	ext_off = le64_to_cpu(hdr->m_FormatExtensionOffset);
+	if (ext_off) {
+		/*
+		 * Userspace places extension at cluster boundary. Don't
+		 * truncate anything if the offset is bogus, as we don't
+		 * know where the extension ends then.
+		 */
+		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 1fbcb48cff7a..e82d5b6c9095 100644
--- a/drivers/md/dm-ploop-target.c
+++ b/drivers/md/dm-ploop-target.c
@@ -442,8 +442,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)
 {
@@ -734,10 +735,51 @@ 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;
+	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.
@@ -746,10 +788,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 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 v3 1/5] 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/20261008140333.614909-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 v3 3/5] 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/20261008140333.614909-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 v3 2/5] 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: if (nr_be) guard in ploop_add_delta() skips the top delta header copy
--

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

dm-ploop: set delta is_raw before applying its mappings

This commit sets deltas[level].is_raw before ploop_apply_delta_mappings()
runs, so that raw deltas are no longer handled as non-raw ones. It also
skips applying mappings when the delta is empty (nr_be == 0).

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

[ ... ]

> @@ -501,7 +501,10 @@ 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;
>  
> -	ploop_apply_delta_mappings(ploop, deltas, level, &md_root, nr_be);
> +	deltas[level].is_raw = is_raw;
> +	/* Empty delta has no mappings, and BAT iterators can't handle it */
> +	if (nr_be)
> +		ploop_apply_delta_mappings(ploop, deltas, level, &md_root, nr_be);

[Severity: Low]
Can this unconditional if (nr_be) guard skip the top-level header copy?
The commit message only justifies the guard for an empty raw delta.

In ploop_apply_delta_mappings(), the on-disk header is copied into md0
only here:

    if (is_top_level && md->id == 0 && !is_raw) {
            /* bat_entries before PLOOP_MAP_OFFSET is hdr */
            memcpy(bat_entries, d_bat_entries,
                   sizeof(struct ploop_pvd_header));
    }

A non-raw top delta with m_Size == 0 appears to reach this path.
ploop_add_deltas_stack() calls ploop_check_top_delta() and then
ploop_add_delta().  ploop_check_top_delta() does not reject m_Size == 0,
and ploop_read_delta_metadata() can then return nr_be == 0.

Before the patch, ploop->bat_entries held md page 0 in this case, so the
loop body ran once and the memcpy was executed.  With the guard, the whole
function is skipped.

ploop_check_top_delta() only allocates md pages through
ploop_prealloc_md_pages().  Those pages are filled with BAT_ENTRY_NONE and
do not get the header.

Would md0 then keep the BAT_ENTRY_NONE fill instead of the header?
ploop_resize() reads hdr->m_SizeInSectors_v2 from md0, and writeback or
grow writes it back.

This looks limited to crafted or corrupt images, since a device with
nr_bat_entries == 0 cannot serve normal I/O.

Would it be better to apply the guard only for the raw case, or to still
copy the header for a non-raw top delta?  The later ploop_read_delta() and
ploop_reload_metadata() changes in the series keep the same if (nr_be)
guard, so it does not look like this is fixed later.

>  
>  	deltas[level].file = file;
>  	deltas[level].mtfile = kcalloc(ploop->nkt_runners, sizeof(*file),

[ ... ]

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

[no-reply]

> 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
> [ ... ]
> 
> 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/20261008140333.614909-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 v3 4/5] dm-ploop: reload metadata on resume

[no-reply]

> 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
> [ ... ]
> 
> 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/20261008140333.614909-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 v3 1/5] dm-ploop: fix lost wakeup in ploop_allocator()

[Denis V. Lunev]

On 08.10.2026 16:03, Alexander Ivanov wrote:
> +		/*
> +		 * Set the state before checking the conditions, otherwise
> +		 * wake_up_process() from ploop_req_prealloc() or
> +		 * kthread_stop() may come in between and get lost.
> +		 */
> +		set_current_state(TASK_INTERRUPTIBLE);

This is the canonical kthread loop and the commit message already
explains why. Please drop the comment.

With that:

Reviewed-by: Denis V. Lunev <den@openvz.org>
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v3 2/5] dm-ploop: set delta is_raw before applying its mappings

[Denis V. Lunev]

On 08.10.2026 16:03, Alexander Ivanov wrote:
> 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.

Please say that this has been the case since the driver was added,
i.e. raw@ has never worked here. Otherwise somebody will go looking
for the commit that broke it.

Reviewed-by: Denis V. Lunev <den@openvz.org>
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v3 3/5] dm-ploop: factor out ploop_read_delta() from ploop_add_delta()

[Denis V. Lunev]

On 08.10.2026 16:03, Alexander Ivanov wrote:
> 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.

Reviewed-by: Denis V. Lunev <den@openvz.org>
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

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

[Denis V. Lunev]

On 08.10.2026 16:03, Alexander Ivanov wrote:
> +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_* are bit numbers, not masks. Everybody else does
set_bit(MD_UPDATING, &md->status) and test_bit(). MD_UPDATING is
(1U << 4), so set_bit() sets bit 16, while this expression tests
bits 1, 2 and 4. MD_DIRTY and MD_WRITEBACK are caught only because
their values happen to line up with the bits of other flags.
MD_UPDATING is never caught. Please use test_bit() for each flag.

[...]

> @@ -2771,6 +2777,16 @@ loff_t ploop_llseek_hole(struct dm_target *ti, loff_t offset, int whence)
[...]
> +	down_read(&ploop->ctl_rwsem);
> +	if (ploop->suspended) {
> +		result = -EAGAIN;
> +		goto unlock;
> +	}

ploop_message() holds ctl_rwsem for write for the whole command.
"merge" walks every cluster of the device in
ploop_process_merge_latest_snapshot() and "resize" relocates
clusters. SEEK_DATA/SEEK_HOLE used to return at once, now it sleeps
uninterruptibly until the merge is over. -EAGAIN is already the
"come back later" answer here, so down_read_trylock() returning
-EAGAIN fits better.

[...]

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

This is the uninterruptible wait for the allocator. On a device in
standby (lease stolen or the mount gone) the allocator may be stuck
in fallocate on the dead file forever. Then postsuspend never
returns, under md->suspend_lock, and dm_replace() in libploop, which
is exactly the tool that recovers a standby device, hangs instead.
Before this patch postsuspend never touched the storage. Please skip
the wait when blk_queue_standby() is set. See also my reply to 5/5,
which has the same problem with the truncate.

[...]

> -	down_read(&ploop->ctl_rwsem);
> -	if (ploop->noresume)
> +	down_write(&ploop->ctl_rwsem);
> +	if (ploop->noresume) {
>  		ret = -EAGAIN;
> -	up_read(&ploop->ctl_rwsem);
> +	} else {
[...]
> +		ret = ploop_reload_metadata(ploop);

Until now preresume could fail only because of noresume. Now it fails
on -ENOMEM (a second copy of the whole BAT tree and holes_bitmap is
built under NOIO), on I/O errors and on -EPROTO/-EBADSLT from the
images, and the device stays suspended. The message has to say so,
and userspace has to be ready for it. It is not today:

- create_snapshot_online() ignores the result of
  ploop_resume_device() and reports success with the device left
  suspended. So do ploop_stop(), lib/di.c and the qcow2 path in
  lib/dm.c.

- ploop_tg_deinit() is worse. DM swaps the new table into dev before
  calling preresume, so when dm_resume(dev) fails, dev keeps the new
  ploop and the tracking table is already gone. The error path then
  resumes devtg and dev again, and if the retry succeeds two ploop
  instances are live on the same images. With 5/5 a later suspend or
  removal of devtg truncates the top delta under the live instance.

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

This is needed after flip_upper_deltas, but it is done on every
resume. save_cbt() in libploop writes SIGNATURE_DISK_CLOSED_V21 and
m_FormatExtensionOffset to the suspended device and then resumes it.
If BAT page 0 is written before the device is removed, IN_USE goes
back to disk. The check at umount then sees the image in use and trims
it at the last data cluster, which cuts off the CBT blocks. This needs
a write after the filesystem is unmounted, so it is unlikely. Still,
please restrict the override to the flip case or explain why it is
safe in general.

The patch does four things: the reload, moving prealloc to preresume
with the wait on postsuspend, wake_up_interruptible() -> wake_up() for
inflight_md_wq, and ctl_rwsem in ploop_llseek_hole(). The last two
stand on their own and would be easier to review as separate patches.
That would also cut the commit message down to the reload itself.
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

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

[Denis V. Lunev]

On 08.10.2026 16:03, Alexander Ivanov wrote:
> +	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);

Two problems here.

1. A device in standby. ploop switches the queue to standby when the
lease of the delta is stolen (-EBUSY) or the mount is gone. dm_replace()
in libploop recovers such a device: it suspends it and reloads the
table with fresh files. With this patch postsuspend first reads the BAT
from the dead file and then truncates it, and a truncate on a dead file
hangs. That happens with md->suspend_lock held, so the device can be
neither replaced nor removed afterwards. Please do nothing in
postsuspend when blk_queue_standby(ploop_blk_queue(ploop)) is set, and
skip the prealloc wait from 4/5 in that case too.

2. Another live writer. The truncation assumes the ploop being suspended
is the only one writing to the top delta. libploop can break that
on the ploop_tg_deinit() error path (see my reply to 4/5). Then
suspending or removing the stale instance cuts clusters that the live
one has written but whose BAT update is not on disk yet. Before this
patch that was harmless, since postsuspend never wrote. A cheap
guard: do not truncate when i_size_read(file_inode(top->file)) !=
top->file_size. Any other writer grows the file, so the sizes no longer
match.

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

I checked this against delta_save_optional_header(): the bitmap blocks
are appended at EOF and the header block goes after them, so keeping
everything up to the header cluster is right. A bogus offset leads to
"Can't find used end of top delta" on every suspend. That is fine as
long as it stays rare.

The comment above ploop_calc_used_end() and the one inside it repeat the
commit message almost word for word. One line pointing to
delta_save_optional_header() is enough.
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v3 2/5] dm-ploop: set delta is_raw before applying its mappings

[Alexander Ivanov]

> [Severity: Low]
> Can this unconditional if (nr_be) guard skip the top-level header copy?
> The commit message only justifies the guard for an empty raw delta.
>
> In ploop_apply_delta_mappings(), the on-disk header is copied into md0
> only here:
>
>      if (is_top_level && md->id == 0 && !is_raw) {
>              /* bat_entries before PLOOP_MAP_OFFSET is hdr */
>              memcpy(bat_entries, d_bat_entries,
>                     sizeof(struct ploop_pvd_header));
>      }
>
> A non-raw top delta with m_Size == 0 appears to reach this path.
> ploop_add_deltas_stack() calls ploop_check_top_delta() and then
> ploop_add_delta().  ploop_check_top_delta() does not reject m_Size == 0,
> and ploop_read_delta_metadata() can then return nr_be == 0.
>
> Before the patch, ploop->bat_entries held md page 0 in this case, so the
> loop body ran once and the memcpy was executed.  With the guard, the whole
> function is skipped.
>
> ploop_check_top_delta() only allocates md pages through
> ploop_prealloc_md_pages().  Those pages are filled with BAT_ENTRY_NONE and
> do not get the header.
>
> Would md0 then keep the BAT_ENTRY_NONE fill instead of the header?
> ploop_resize() reads hdr->m_SizeInSectors_v2 from md0, and writeback or
> grow writes it back.
>
> This looks limited to crafted or corrupt images, since a device with
> nr_bat_entries == 0 cannot serve normal I/O.
>
> Would it be better to apply the guard only for the raw case, or to still
> copy the header for a non-raw top delta?  The later ploop_read_delta() and
> ploop_reload_metadata() changes in the series keep the same if (nr_be)
> guard, so it does not look like this is fixed later.
Fixed in a new commit: "dm-ploop: reject top delta with empty BAT".
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel