[Devel] [PATCH hci-8.1 v7 0/8] block/blk-cbt: don't lose writes held across a dm suspend

[Devel] [PATCH hci-8.1 v7 0/8] block/blk-cbt: don't lose writes held across a dm suspend

[Pavel Tikhomirov]

Fix CBT missing writes that request-based dm holds across a suspend,
which made incremental backups taken with ploop-backup silently lose
data.

Claude tested full and incremental ploop-backup under write load (a
random writer, and fio with 4 jobs x iodepth 64): the incremental backup
lost blocks without the fix, and was consistent in 30/30 and 10/10 runs
with it. CBT start/stop and device resize under fio ran without
warnings. With writes throttled by io.max on a loop device, CBT recorded
24 of 64 written blocks without the throttle patch, and all 64 with it.
A RWF_NOWAIT write into a missing bitmap page now fails with EAGAIN
instead of sleeping, and succeeds when retried without RWF_NOWAIT.

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

Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
---
Changes in v7:
- New patch: use list_for_each_entry_safe() in blk_cbt_update_size(),
  which read the next entry through a cbt it had just queued for
  release (sashiko review).
- Take the SRCU read lock in blk_cbt_bio_queue() only after its early
  exits, so that reads and queues without a CBT don't pay for it.
- Clarify the commit message of the unsigned shift patch and the comment
  of cbt_pages_present() (sashiko review).
- Link to v6: http://lore.virtuozzo.com/20261027-ptikhomirov-vstor-149268-cbt-account-at-rq-start-v6-0-f9870cf5c7eb@virtuozzo.com

Changes in v6:
- New patch: load a map slot once with READ_ONCE() in CBT_PAGE(), as a
  racing CBT_PAGE_MISSED store could make it return (struct page *)0x1,
  and install a map page with smp_store_release(), so that a lockless
  reader never sees it before its zeroing (sashiko review).
- Store CBT_PAGE_MISSED with cmpxchg() instead of under cbt->lock, since
  taking a spinlock_t in the flush IPI is not allowed: it may sleep on
  PREEMPT_RT (sashiko review).
- Skip queueing a page allocation that is already queued, instead of
  taking cbt->lock for every write while the page is missing (sashiko
  review).
- Link to v5: http://lore.virtuozzo.com/20261027-ptikhomirov-vstor-149268-cbt-account-at-rq-start-v5-0-88dccc74b48b@virtuozzo.com

Changes in v5:
- New patch: store CBT_PAGE_MISSED under cbt->lock, as the cache flush
  could overwrite a page installed meanwhile; the allocation queued at
  submission in v4 made this much more likely (sashiko review).
- New patch: walk cbt_list under SRCU, so that a sleeping bitmap page
  allocation no longer drops the RCU lock in the middle of the walk, which
  let a concurrent CBT stop or resize free the next cbt under it, a use
  after free (the pre-existing problem noted in v4).
- New patch: don't sleep in CBT for a REQ_NOWAIT write, fail it with
  BLK_STS_AGAIN if a bitmap page is missing (also noted in v4).
- Link to v4: http://lore.virtuozzo.com/20261027-ptikhomirov-vstor-149268-cbt-account-at-rq-start-v4-0-b4d92264f6a4@virtuozzo.com

Changes in v4:
- Only queue the bitmap page allocation at submission instead of
  allocating there: the sleeping allocation dropped the RCU lock in the
  middle of the cbt_list walk, and could put an extent into the cache of
  a cbt that a resize had just replaced (sashiko review).
- Link to v3: http://lore.virtuozzo.com/20261027-ptikhomirov-vstor-149268-cbt-account-at-rq-start-v3-0-e1bc61a829a0@virtuozzo.com

Changes in v3:
- New patch: account a write delayed by blk-throttle in CBT, as the
  throttle dispatch work skipped CBT (sashiko review).
- New patch: shift the CBT block size as unsigned, and use the unsigned
  shift in the new cbt_set_present() too (sashiko review).
- Link to v2: http://lore.virtuozzo.com/20261027-ptikhomirov-vstor-149268-cbt-account-at-rq-start-v2-1-c64ee6178730@virtuozzo.com

Changes in v2:
- No GFP_ATOMIC allocation at dispatch: bitmap pages are now allocated
  at bio submission, and the dispatch hook only sets bits in present pages.
- A page missing at dispatch (a write submitted before BLKCBTSTART) is
  allocated with GFP_NOIO from the kblockd workqueue, and dm-rq returns
  BLK_STS_RESOURCE so that blk-mq dispatches the request again later,
  instead of setting CBT_ERROR when an atomic allocation fails.
- Move the hook from dm_start_request() to dm_mq_queue_rq(), before the
  request is started, so that it can return BLK_STS_RESOURCE.
- Link to v1: http://lore.virtuozzo.com/20261016-ptikhomirov-vstor-149268-cbt-account-at-rq-start-v1-1-1f20b151431d@virtuozzo.com

---
Pavel Tikhomirov (8):
      block/blk-cbt: don't read a released cbt in blk_cbt_update_size()
      block/blk-cbt: shift the block size as unsigned
      block/blk-throttle: account a throttled write in CBT
      block/blk-cbt: read a map slot once, publish a map page with release
      block/blk-cbt: don't overwrite a map page with CBT_PAGE_MISSED
      block/blk-cbt: walk cbt_list under SRCU
      block/blk-cbt: don't sleep in CBT for a REQ_NOWAIT write
      block/blk-cbt: account a write request also when dm starts it

 block/blk-cbt.c        | 308 ++++++++++++++++++++++++++++++++++++++++++-------
 block/blk-core.c       |   5 +-
 block/blk-throttle.c   |   5 +-
 drivers/md/dm-rq.c     |   3 +
 include/linux/blkdev.h |  12 +-
 5 files changed, 288 insertions(+), 45 deletions(-)
---
base-commit: f5d1fdcfffe101fe2bafa7df09d95d2e811c5c5e
change-id: 20261016-ptikhomirov-vstor-149268-cbt-account-at-rq-start-a592cf2010aa

Best regards,
--  
Pavel Tikhomirov <ptikhomirov@virtuozzo.com>

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

[Devel] [PATCH hci-8.1 v7 1/8] block/blk-cbt: don't read a released cbt in blk_cbt_update_size()

[Pavel Tikhomirov]

blk_cbt_update_cbt_size() may replace the cbt in q->cbt_list and queue
the old one for release, and blk_cbt_update_size() then reads the next
entry through the old one. It holds only cbt_mutex, which doesn't delay
the release, so the old cbt may already be freed.

Read the next entry before the call, with list_for_each_entry_safe():
the replacement keeps it, and cbt_mutex excludes other list changes.

https://virtuozzo.atlassian.net/browse/VSTOR-149268
Feature: cbt: changed block tracking (for backup)
Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
---
 block/blk-cbt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/blk-cbt.c b/block/blk-cbt.c
index 90219f58f1ae5..f4ee0a664156e 100644
--- a/block/blk-cbt.c
+++ b/block/blk-cbt.c
@@ -604,14 +604,14 @@ static void blk_cbt_update_cbt_size(struct cbt_info *cbt, loff_t new_sz)
 void blk_cbt_update_size(struct block_device *bdev)
 {
 	struct request_queue *q;
-	struct cbt_info *cbt;
+	struct cbt_info *cbt, *tmp;
 
 	if (!bdev->bd_disk || !bdev_get_queue(bdev))
 		return;
 
 	q = bdev_get_queue(bdev);
 	mutex_lock(&cbt_mutex);
-	list_for_each_entry(cbt, &q->cbt_list, list)
+	list_for_each_entry_safe(cbt, tmp, &q->cbt_list, list)
 		blk_cbt_update_cbt_size(cbt, bdev_nr_bytes(bdev));
 
 	mutex_unlock(&cbt_mutex);

-- 
2.55.0

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

Re: [Devel] [PATCH hci-8.1 v7 1/8] block/blk-cbt: don't read a released cbt in blk_cbt_update_size()

[no-reply]

> blk_cbt_update_cbt_size() may replace the cbt in q->cbt_list and queue
> the old one for release, and blk_cbt_update_size() then reads the next
> entry through the old one. It holds only cbt_mutex, which doesn't delay
> the release, so the old cbt may already be freed.
> 
> Read the next entry before the call, with list_for_each_entry_safe():
> the replacement keeps it, and cbt_mutex excludes other list changes.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-149268
> Feature: cbt: changed block tracking (for backup)
> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>

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

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261028-ptikhomirov-vstor-149268-cbt-account-at-rq-start-v7-0-5c48672191c2@virtuozzo.com?part=1


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

[Devel] [PATCH hci-8.1 v7 2/8] block/blk-cbt: shift the block size as unsigned

[Pavel Tikhomirov]

cbt_ioc_init() accepts any power of two block size, so block_bits can be
31, and then 1 << block_bits overflows int. In blk_cbt_add() and
cbt_ioc_set() the sign extended block size then makes the rounded up
block counts wrong. Shift an unsigned one explicitly everywhere the
block size is computed.

https://virtuozzo.atlassian.net/browse/VSTOR-149268
Feature: cbt: changed block tracking (for backup)
Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
---
 block/blk-cbt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/blk-cbt.c b/block/blk-cbt.c
index f4ee0a664156e..850a78da1dc24 100644
--- a/block/blk-cbt.c
+++ b/block/blk-cbt.c
@@ -229,7 +229,7 @@ static void blk_cbt_add(struct cbt_info *cbt, blkcnt_t start, blkcnt_t len)
 
 	if (unlikely(test_bit(CBT_ERROR, &cbt->flags)))
 		return;
-	end = DIV_ROUND_UP(start + len, 1 << cbt->block_bits);
+	end = DIV_ROUND_UP(start + len, 1UL << cbt->block_bits);
 	start >>= cbt->block_bits;
 	len = end - start;
 	if (unlikely(test_bit(CBT_NOCACHE, &cbt->flags))) {
@@ -575,7 +575,7 @@ static void blk_cbt_update_cbt_size(struct cbt_info *cbt, loff_t new_sz)
 	unsigned bsz;
 	int in_use = 0;
 
-	bsz = 1 << cbt->block_bits;
+	bsz = 1U << cbt->block_bits;
 	if (DIV_ROUND_UP(new_sz, bsz) <= cbt->block_max)
 		return;
 
@@ -964,7 +964,7 @@ static int cbt_ioc_set(struct block_device *bdev, struct blk_user_cbt_info __use
 		struct cbt_extent ex;
 
 		ex.start  = cur_ex->ce_physical >> cbt->block_bits;
-		ex.len  = DIV_ROUND_UP(cur_ex->ce_length, 1 << cbt->block_bits);
+		ex.len  = DIV_ROUND_UP(cur_ex->ce_length, 1UL << cbt->block_bits);
 		if (ex.start > cbt->block_max ||
 		    ex.start + ex.len > cbt->block_max ||
 		    ex.len == 0) {

-- 
2.55.0

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

Re: [Devel] [PATCH hci-8.1 v7 2/8] block/blk-cbt: shift the block size as unsigned

[no-reply]

> cbt_ioc_init() accepts any power of two block size, so block_bits can be
> 31, and then 1 << block_bits overflows int. In blk_cbt_add() and
> cbt_ioc_set() the sign extended block size then makes the rounded up
> block counts wrong. Shift an unsigned one explicitly everywhere the
> block size is computed.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-149268
> Feature: cbt: changed block tracking (for backup)
> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>

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

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261028-ptikhomirov-vstor-149268-cbt-account-at-rq-start-v7-0-5c48672191c2@virtuozzo.com?part=2


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

[Devel] [PATCH hci-8.1 v7 3/8] block/blk-throttle: account a throttled write in CBT

[Pavel Tikhomirov]

submit_bio_noacct() accounts a write in CBT only when blk-throttle does
not hold it: a throttled bio is later issued from the throttle dispatch
work with submit_bio_noacct_nocheck(), which skips CBT. So writes delayed
by an IO limit never reach CBT, and incremental backups miss them.

Account the bio in CBT when the dispatch work issues it. The work runs
in process context with no locks held, so CBT may allocate there as it
does at submission.

https://virtuozzo.atlassian.net/browse/VSTOR-149268
Feature: cbt: changed block tracking (for backup)
Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
---
 block/blk-throttle.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 397b6a410f9e5..e1e008a30bf2c 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -1223,8 +1223,11 @@ static void blk_throtl_dispatch_work_fn(struct work_struct *work)
 
 	if (!bio_list_empty(&bio_list_on_stack)) {
 		blk_start_plug(&plug);
-		while ((bio = bio_list_pop(&bio_list_on_stack)))
+		while ((bio = bio_list_pop(&bio_list_on_stack))) {
+			/* submit_bio_noacct() did not account it in CBT */
+			blk_cbt_bio_queue(q, bio);
 			submit_bio_noacct_nocheck(bio);
+		}
 		blk_finish_plug(&plug);
 	}
 }

-- 
2.55.0

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

Re: [Devel] [PATCH hci-8.1 v7 3/8] block/blk-throttle: account a throttled write in CBT

[no-reply]

> submit_bio_noacct() accounts a write in CBT only when blk-throttle does
> not hold it: a throttled bio is later issued from the throttle dispatch
> work with submit_bio_noacct_nocheck(), which skips CBT. So writes delayed
> by an IO limit never reach CBT, and incremental backups miss them.
> 
> Account the bio in CBT when the dispatch work issues it. The work runs
> in process context with no locks held, so CBT may allocate there as it
> does at submission.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-149268
> Feature: cbt: changed block tracking (for backup)
> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>

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

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261028-ptikhomirov-vstor-149268-cbt-account-at-rq-start-v7-0-5c48672191c2@virtuozzo.com?part=3


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

[Devel] [PATCH hci-8.1 v7 4/8] block/blk-cbt: read a map slot once, publish a map page with release

[Pavel Tikhomirov]

CBT_PAGE() loads a map slot twice. A lockless reader racing with the
cache flush, which marks an empty slot CBT_PAGE_MISSED, can get NULL from
the first load and the marker from the second, and then use
(struct page *)0x1 as a page.

A page is also installed with a plain store under cbt->lock, but readers
don't take that lock: on a weakly ordered CPU a reader may see the
pointer before the page's zeroing, and the late zeroing can then clear a
bit the reader has just set.

Load the slot once with READ_ONCE(), whose address dependency orders the
reader's accesses to the page, and install a page with
smp_store_release().

https://virtuozzo.atlassian.net/browse/VSTOR-149268
Feature: cbt: changed block tracking (for backup)
Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
---
 block/blk-cbt.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/block/blk-cbt.c b/block/blk-cbt.c
index 850a78da1dc24..970a69ea8d58a 100644
--- a/block/blk-cbt.c
+++ b/block/blk-cbt.c
@@ -27,8 +27,11 @@
 #define BITS_PER_PAGE		(1UL << (PAGE_SHIFT + 3))
 
 #define CBT_PAGE_MISSED (struct page *)(0x1)
-#define CBT_PAGE(cbt, idx) (cbt->map[idx] == CBT_PAGE_MISSED ? \
-			    NULL : cbt->map[idx])
+/* Load the slot once: the cache flush may mark it missing meanwhile */
+#define CBT_PAGE(cbt, idx) ({						\
+	struct page *__page = READ_ONCE((cbt)->map[idx]);		\
+	__page == CBT_PAGE_MISSED ? NULL : __page;			\
+})
 
 static __cacheline_aligned_in_smp DEFINE_MUTEX(cbt_mutex);
 
@@ -148,10 +151,12 @@ static int cbt_page_alloc(struct cbt_info  **cbt_pp, unsigned long idx,
 		return -ENOMEM;
 	}
 
-	if (likely(CBT_PAGE(cbt, idx) == NULL))
-		cbt->map[idx] = page;
-	else
+	if (likely(CBT_PAGE(cbt, idx) == NULL)) {
+		/* Release: lockless readers use the page once they see it */
+		smp_store_release(&cbt->map[idx], page);
+	} else {
 		__free_page(page);
+	}
 
 	page = NULL;
 	spin_unlock_irq(&cbt->lock);

-- 
2.55.0

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

Re: [Devel] [PATCH hci-8.1 v7 4/8] block/blk-cbt: read a map slot once, publish a map page with release

[no-reply]

> CBT_PAGE() loads a map slot twice. A lockless reader racing with the
> cache flush, which marks an empty slot CBT_PAGE_MISSED, can get NULL from
> the first load and the marker from the second, and then use
> (struct page *)0x1 as a page.
> 
> A page is also installed with a plain store under cbt->lock, but readers
> don't take that lock: on a weakly ordered CPU a reader may see the
> pointer before the page's zeroing, and the late zeroing can then clear a
> bit the reader has just set.
> 
> Load the slot once with READ_ONCE(), whose address dependency orders the
> reader's accesses to the page, and install a page with
> smp_store_release().
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-149268
> Feature: cbt: changed block tracking (for backup)
> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>

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

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261028-ptikhomirov-vstor-149268-cbt-account-at-rq-start-v7-0-5c48672191c2@virtuozzo.com?part=4


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

[Devel] [PATCH hci-8.1 v7 5/8] block/blk-cbt: don't overwrite a map page with CBT_PAGE_MISSED

[Pavel Tikhomirov]

The cache flush marks a missing map page with CBT_PAGE_MISSED from an
IPI, with a plain store. A page installed in between, by
cbt_page_alloc(), is then overwritten: it leaks, and the bits already
set in it are lost.

Store the marker with cmpxchg() only into an empty slot, and set the
bits in the page if one appeared meanwhile.

https://virtuozzo.atlassian.net/browse/VSTOR-149268
Feature: cbt: changed block tracking (for backup)
Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
---
 block/blk-cbt.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/block/blk-cbt.c b/block/blk-cbt.c
index 970a69ea8d58a..10e03a29114a4 100644
--- a/block/blk-cbt.c
+++ b/block/blk-cbt.c
@@ -199,10 +199,15 @@ static int __blk_cbt_set(struct cbt_info  *cbt, blkcnt_t block,
 			block += len;
 			continue;
 		} else if (pages_missed) {
+			struct page *old;
+
+			/* Don't overwrite a page installed meanwhile */
+			old = cmpxchg(&cbt->map[idx], NULL, CBT_PAGE_MISSED);
+			if (old && old != CBT_PAGE_MISSED)
+				continue;
 			(*pages_missed)++;
 			if (!*idx_first)
 				*idx_first = idx;
-			cbt->map[idx] = CBT_PAGE_MISSED;
 			count -= len;
 			block += len;
 			continue;

-- 
2.55.0

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

Re: [Devel] [PATCH hci-8.1 v7 5/8] block/blk-cbt: don't overwrite a map page with CBT_PAGE_MISSED

[no-reply]

> The cache flush marks a missing map page with CBT_PAGE_MISSED from an
> IPI, with a plain store. A page installed in between, by
> cbt_page_alloc(), is then overwritten: it leaks, and the bits already
> set in it are lost.
> 
> Store the marker with cmpxchg() only into an empty slot, and set the
> bits in the page if one appeared meanwhile.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-149268
> Feature: cbt: changed block tracking (for backup)
> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>

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

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261028-ptikhomirov-vstor-149268-cbt-account-at-rq-start-v7-0-5c48672191c2@virtuozzo.com?part=5


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

[Devel] [PATCH hci-8.1 v7 6/8] block/blk-cbt: walk cbt_list under SRCU

[Pavel Tikhomirov]

blk_cbt_bio_queue() walks q->cbt_list under rcu_read_lock(), but a cbt
may need a bitmap page there: cbt_page_alloc() then drops the RCU lock
around the sleeping allocation, pinning only the current cbt. A CBT stop
or resize meanwhile can unlink it and free the next one, which the walk
then reaches through the stale ->next.

Walk cbt_list and release cbts under SRCU instead, whose readers may
sleep, so the allocation no longer drops the lock. Take it after the
early exits of blk_cbt_bio_queue(), which need no lock.

https://virtuozzo.atlassian.net/browse/VSTOR-149268
Feature: cbt: changed block tracking (for backup)
Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
---
 block/blk-cbt.c | 50 +++++++++++++++++++++++++-------------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/block/blk-cbt.c b/block/blk-cbt.c
index 10e03a29114a4..3ad4b9a25e1f4 100644
--- a/block/blk-cbt.c
+++ b/block/blk-cbt.c
@@ -15,6 +15,7 @@
 #include <linux/cpuset.h>
 #include <linux/list.h>
 #include <linux/spinlock.h>
+#include <linux/srcu.h>
 #include <linux/pagemap.h>
 #include <linux/vmalloc.h>
 #include <linux/log2.h>
@@ -34,6 +35,8 @@
 })
 
 static __cacheline_aligned_in_smp DEFINE_MUTEX(cbt_mutex);
+/* Protects the q->cbt_list walks, which may sleep to allocate */
+DEFINE_STATIC_SRCU(cbt_srcu);
 
 struct cbt_extent{
 	blkcnt_t start;
@@ -105,8 +108,7 @@ static void set_bits(void *bm, int cur, int len, bool is_set)
  * -EBADF if cbt is dead,
  * -ENOMEM if alloc_page failed.
  */
-static int cbt_page_alloc(struct cbt_info  **cbt_pp, unsigned long idx,
-			  int in_rcu)
+static int cbt_page_alloc(struct cbt_info  **cbt_pp, unsigned long idx)
 {
 	struct cbt_info	 *cbt = *cbt_pp;
 	struct page *page;
@@ -118,7 +120,9 @@ static int cbt_page_alloc(struct cbt_info  **cbt_pp, unsigned long idx,
 	} else {
 		struct cbt_info *i, *new = NULL;
 
-		list_for_each_entry_rcu(i, &cbt->queue->cbt_list, list)
+		list_for_each_entry_srcu(i, &cbt->queue->cbt_list, list,
+					 srcu_read_lock_held(&cbt_srcu) ||
+					 lockdep_is_held(&cbt_mutex))
 			if (!memcmp(i->name, cbt->name, sizeof(cbt->name)))
 				new = i;
 
@@ -132,15 +136,11 @@ static int cbt_page_alloc(struct cbt_info  **cbt_pp, unsigned long idx,
 		}
 	}
 	spin_unlock_irq(&cbt->lock);
-	if (in_rcu)
-		rcu_read_unlock();
 	page = alloc_page(GFP_NOIO|__GFP_ZERO);
-	if (in_rcu)
-		rcu_read_lock();
 	spin_lock_irq(&cbt->lock);
 	if (unlikely(!--(cbt->count) && test_bit(CBT_DEAD, &cbt->flags))) {
 		spin_unlock_irq(&cbt->lock);
-		call_rcu(&cbt->rcu, &cbt_release_callback);
+		call_srcu(&cbt_srcu, &cbt->rcu, &cbt_release_callback);
 		if (page)
 			__free_page(page);
 		return -EBADF;
@@ -165,7 +165,7 @@ static int cbt_page_alloc(struct cbt_info  **cbt_pp, unsigned long idx,
 }
 
 static int __blk_cbt_set(struct cbt_info  *cbt, blkcnt_t block,
-			 blkcnt_t count, bool in_rcu, bool set,
+			 blkcnt_t count, bool set,
 			 unsigned long *pages_missed,
 			 unsigned long *idx_first)
 {
@@ -220,7 +220,7 @@ static int __blk_cbt_set(struct cbt_info  *cbt, blkcnt_t block,
 			}
 		}
 
-		ret = cbt_page_alloc(&cbt, idx, in_rcu);
+		ret = cbt_page_alloc(&cbt, idx);
 		if (ret == -EAGAIN) /* new cbt */
 			continue;
 		else if (ret == -EBADF) /* dead cbt */
@@ -243,7 +243,7 @@ static void blk_cbt_add(struct cbt_info *cbt, blkcnt_t start, blkcnt_t len)
 	start >>= cbt->block_bits;
 	len = end - start;
 	if (unlikely(test_bit(CBT_NOCACHE, &cbt->flags))) {
-		__blk_cbt_set(cbt, start, len, 1, 1, NULL, NULL);
+		__blk_cbt_set(cbt, start, len, 1, NULL, NULL);
 		return;
 	}
 	ex = get_cpu_ptr(cbt->cache);
@@ -258,22 +258,22 @@ static void blk_cbt_add(struct cbt_info *cbt, blkcnt_t start, blkcnt_t len)
 	put_cpu_ptr(cbt->cache);
 
 	if (likely(old.len))
-		__blk_cbt_set(cbt, old.start, old.len, 1, 1, NULL, NULL);
+		__blk_cbt_set(cbt, old.start, old.len, 1, NULL, NULL);
 }
 
 void blk_cbt_bio_queue(struct request_queue *q, struct bio *bio)
 {
 	struct cbt_info *cbt;
+	int idx;
 
-	rcu_read_lock();
 	if (list_empty(&q->cbt_list) || bio_data_dir(bio) == READ || !bio->bi_iter.bi_size)
-		goto out_rcu;
+		return;
 
-	list_for_each_entry_rcu(cbt, &q->cbt_list, list)
+	idx = srcu_read_lock(&cbt_srcu);
+	list_for_each_entry_srcu(cbt, &q->cbt_list, list,
+				 srcu_read_lock_held(&cbt_srcu))
 		blk_cbt_add(cbt, bio->bi_iter.bi_sector << 9, bio->bi_iter.bi_size);
-
-out_rcu:
-	rcu_read_unlock();
+	srcu_read_unlock(&cbt_srcu, idx);
 }
 
 static struct cbt_info *do_cbt_alloc(struct request_queue *q, __u8 *name,
@@ -555,7 +555,7 @@ static int blk_cbt_snap_merge_back(struct request_queue *q, __u8 *name)
 
 		if (!page_main) {
 			ret = -ENOMEM;
-			if (cbt_page_alloc(&cbt, i, 0))
+			if (cbt_page_alloc(&cbt, i))
 				goto out;
 
 			page_main = cbt->map[i];
@@ -608,7 +608,7 @@ static void blk_cbt_update_cbt_size(struct cbt_info *cbt, loff_t new_sz)
 	in_use = cbt->count;
 	spin_unlock_irq(&cbt->lock);
 	if (!in_use)
-		call_rcu(&cbt->rcu, &cbt_release_callback);
+		call_srcu(&cbt_srcu, &cbt->rcu, &cbt_release_callback);
 }
 
 void blk_cbt_update_size(struct block_device *bdev)
@@ -696,7 +696,7 @@ static void blk_cbt_del(struct cbt_info *cbt)
 	in_use = cbt->count;
 	spin_unlock_irqrestore(&cbt->lock, flags);
 	if (!in_use)
-		call_rcu(&cbt->rcu, &cbt_release_callback);
+		call_srcu(&cbt_srcu, &cbt->rcu, &cbt_release_callback);
 }
 
 void blk_cbt_release(struct request_queue *q)
@@ -744,7 +744,7 @@ static inline void __cbt_flush_cpu_cache(void *ptr)
 	struct cbt_extent *ex = this_cpu_ptr(cbt->cache);
 
 	if (ex->len) {
-		int ret = __blk_cbt_set(cbt, ex->start, ex->len, 0, 1,
+		int ret = __blk_cbt_set(cbt, ex->start, ex->len, 1,
 					&ctx->pages_missed,
 					&ctx->idx_first);
 		if (!ret) {
@@ -775,7 +775,7 @@ static void cbt_flush_cache(struct cbt_info *cbt)
 			if (cbt->map[i] != CBT_PAGE_MISSED)
 				continue;
 
-			ret = cbt_page_alloc(&cbt, i, 0);
+			ret = cbt_page_alloc(&cbt, i);
 			if (ret == -EAGAIN) /* new cbt */
 				goto try_again;
 			else if (ret) /* dead cbt or alloc_page failed */
@@ -897,7 +897,7 @@ static int cbt_ioc_get(struct block_device *bdev, struct blk_user_cbt_info __use
 		cur_ex->ce_length = ex.len << cbt->block_bits;
 
 		if (ci.ci_flags & CI_FLAG_ONCE)
-			__blk_cbt_set(cbt, ex.start, ex.len, 0, 0, NULL, NULL);
+			__blk_cbt_set(cbt, ex.start, ex.len, 0, NULL, NULL);
 		cur_ex++;
 		ci.ci_mapped_extents++;
 		block = ex.start + ex.len;
@@ -981,7 +981,7 @@ static int cbt_ioc_set(struct block_device *bdev, struct blk_user_cbt_info __use
 			ret = -EINVAL;
 			break;
 		}
-		ret = __blk_cbt_set(cbt, ex.start, ex.len, 0, set, NULL, NULL);
+		ret = __blk_cbt_set(cbt, ex.start, ex.len, set, NULL, NULL);
 		if (ret)
 			break;
 		cur_ex++;

-- 
2.55.0

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

Re: [Devel] [PATCH hci-8.1 v7 6/8] block/blk-cbt: walk cbt_list under SRCU

[no-reply]

> blk_cbt_bio_queue() walks q->cbt_list under rcu_read_lock(), but a cbt
> may need a bitmap page there: cbt_page_alloc() then drops the RCU lock
> around the sleeping allocation, pinning only the current cbt. A CBT stop
> or resize meanwhile can unlink it and free the next one, which the walk
> then reaches through the stale ->next.
> 
> Walk cbt_list and release cbts under SRCU instead, whose readers may
> sleep, so the allocation no longer drops the lock. Take it after the
> early exits of blk_cbt_bio_queue(), which need no lock.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-149268
> Feature: cbt: changed block tracking (for backup)
> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>

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

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261028-ptikhomirov-vstor-149268-cbt-account-at-rq-start-v7-0-5c48672191c2@virtuozzo.com?part=6


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

[Devel] [PATCH hci-8.1 v7 7/8] block/blk-cbt: don't sleep in CBT for a REQ_NOWAIT write

[Pavel Tikhomirov]

Accounting a write in CBT may allocate a bitmap page with GFP_NOIO,
either for the write itself or for the extent it evicts from the per-cpu
cache, so a REQ_NOWAIT bio can sleep in submit_bio_noacct().

For a REQ_NOWAIT write, skip the cache and set the bits directly, which
doesn't allocate when all the pages are there. If one is missing, fail
the bio with BLK_STS_AGAIN, for the submitter to retry it in a context
that may block. The throttle dispatch work may sleep, so it doesn't use
this.

https://virtuozzo.atlassian.net/browse/VSTOR-149268
Feature: cbt: changed block tracking (for backup)
Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
---
 block/blk-cbt.c        | 57 +++++++++++++++++++++++++++++++++++++++++++-------
 block/blk-core.c       |  5 ++++-
 block/blk-throttle.c   |  2 +-
 include/linux/blkdev.h |  7 +++++--
 4 files changed, 59 insertions(+), 12 deletions(-)

diff --git a/block/blk-cbt.c b/block/blk-cbt.c
index 3ad4b9a25e1f4..7b6441fad08f9 100644
--- a/block/blk-cbt.c
+++ b/block/blk-cbt.c
@@ -231,26 +231,55 @@ static int __blk_cbt_set(struct cbt_info  *cbt, blkcnt_t block,
 	return (pages_missed && *pages_missed) ? -EAGAIN : 0;
 }
 
-static void blk_cbt_add(struct cbt_info *cbt, blkcnt_t start, blkcnt_t len)
+/*
+ * Return true if all the map pages for [block, block + count) are there,
+ * or for an empty or out of range request, which needs no page.
+ */
+static bool cbt_pages_present(struct cbt_info *cbt, blkcnt_t block,
+			      blkcnt_t count)
+{
+	unsigned long idx, last;
+
+	if (unlikely(!count || block + count > cbt->block_max))
+		return true;
+
+	last = (block + count - 1) / BITS_PER_PAGE;
+	for (idx = block / BITS_PER_PAGE; idx <= last; idx++)
+		if (!CBT_PAGE(cbt, idx))
+			return false;
+
+	return true;
+}
+
+/* Return true if nowait and a bitmap page is missing */
+static bool blk_cbt_add(struct cbt_info *cbt, blkcnt_t start, blkcnt_t len,
+			bool nowait)
 {
 	struct cbt_extent *ex;
 	struct cbt_extent old;
 	blkcnt_t end;
 
 	if (unlikely(test_bit(CBT_ERROR, &cbt->flags)))
-		return;
+		return false;
 	end = DIV_ROUND_UP(start + len, 1UL << cbt->block_bits);
 	start >>= cbt->block_bits;
 	len = end - start;
+	if (unlikely(nowait)) {
+		/* Don't sleep: set the bits directly, only with all pages there */
+		if (!cbt_pages_present(cbt, start, len))
+			return true;
+		__blk_cbt_set(cbt, start, len, 1, NULL, NULL);
+		return false;
+	}
 	if (unlikely(test_bit(CBT_NOCACHE, &cbt->flags))) {
 		__blk_cbt_set(cbt, start, len, 1, NULL, NULL);
-		return;
+		return false;
 	}
 	ex = get_cpu_ptr(cbt->cache);
 	if (ex->start + ex->len == start) {
 		ex->len += len;
 		put_cpu_ptr(cbt->cache);
-		return;
+		return false;
 	}
 	old = *ex;
 	ex->start = start;
@@ -259,21 +288,33 @@ static void blk_cbt_add(struct cbt_info *cbt, blkcnt_t start, blkcnt_t len)
 
 	if (likely(old.len))
 		__blk_cbt_set(cbt, old.start, old.len, 1, NULL, NULL);
+	return false;
 }
 
-void blk_cbt_bio_queue(struct request_queue *q, struct bio *bio)
+/*
+ * Account a write in CBT. With nowait, don't sleep: return true if a bitmap
+ * page is missing, for the caller to fail the bio with BLK_STS_AGAIN.
+ */
+bool blk_cbt_bio_queue(struct request_queue *q, struct bio *bio, bool nowait)
 {
 	struct cbt_info *cbt;
+	bool wouldblock = false;
 	int idx;
 
 	if (list_empty(&q->cbt_list) || bio_data_dir(bio) == READ || !bio->bi_iter.bi_size)
-		return;
+		return false;
 
 	idx = srcu_read_lock(&cbt_srcu);
 	list_for_each_entry_srcu(cbt, &q->cbt_list, list,
-				 srcu_read_lock_held(&cbt_srcu))
-		blk_cbt_add(cbt, bio->bi_iter.bi_sector << 9, bio->bi_iter.bi_size);
+				 srcu_read_lock_held(&cbt_srcu)) {
+		if (blk_cbt_add(cbt, bio->bi_iter.bi_sector << 9,
+				bio->bi_iter.bi_size, nowait)) {
+			wouldblock = true;
+			break;
+		}
+	}
 	srcu_read_unlock(&cbt_srcu, idx);
+	return wouldblock;
 }
 
 static struct cbt_info *do_cbt_alloc(struct request_queue *q, __u8 *name,
diff --git a/block/blk-core.c b/block/blk-core.c
index bcc3060de670c..6bc78f7c23d38 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -871,7 +871,10 @@ void submit_bio_noacct(struct bio *bio)
 
 	if (blk_throtl_bio(bio))
 		return;
-	blk_cbt_bio_queue(q, bio);
+	if (blk_cbt_bio_queue(q, bio, bio->bi_opf & REQ_NOWAIT)) {
+		bio_wouldblock_error(bio);
+		return;
+	}
 	submit_bio_noacct_nocheck(bio);
 	return;
 
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index e1e008a30bf2c..00f0db0a57f8d 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -1225,7 +1225,7 @@ static void blk_throtl_dispatch_work_fn(struct work_struct *work)
 		blk_start_plug(&plug);
 		while ((bio = bio_list_pop(&bio_list_on_stack))) {
 			/* submit_bio_noacct() did not account it in CBT */
-			blk_cbt_bio_queue(q, bio);
+			blk_cbt_bio_queue(q, bio, false);
 			submit_bio_noacct_nocheck(bio);
 		}
 		blk_finish_plug(&plug);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 1284bb0e4ebc0..885555f480462 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1626,7 +1626,8 @@ static inline bool blk_crypto_register(struct blk_crypto_profile *profile,
 #if defined (CONFIG_BLK_DEV_CBT)
 extern void blk_cbt_update_size(struct block_device *bdev);
 extern void blk_cbt_release(struct request_queue *q);
-extern void blk_cbt_bio_queue(struct request_queue *q, struct bio *bio);
+extern bool blk_cbt_bio_queue(struct request_queue *q, struct bio *bio,
+			      bool nowait);
 extern int blk_cbt_ioctl(struct block_device *bdev, unsigned cmd, char __user *arg);
 #else /* CONFIG_BLK_DEV_CBT */
 static inline void blk_cbt_update_size(struct block_device *bdev)
@@ -1635,8 +1636,10 @@ static inline void blk_cbt_update_size(struct block_device *bdev)
 static inline void blk_cbt_release(struct request_queue *q)
 {
 }
-static inline void blk_cbt_bio_queue(struct request_queue *q, struct bio *bio)
+static inline bool blk_cbt_bio_queue(struct request_queue *q, struct bio *bio,
+				     bool nowait)
 {
+	return false;
 }
 static inline int blk_cbt_ioctl(struct block_device *bdev, unsigned cmd,
 				char __user *arg)

-- 
2.55.0

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

Re: [Devel] [PATCH hci-8.1 v7 7/8] block/blk-cbt: don't sleep in CBT for a REQ_NOWAIT write

[no-reply]

> Accounting a write in CBT may allocate a bitmap page with GFP_NOIO,
> either for the write itself or for the extent it evicts from the per-cpu
> cache, so a REQ_NOWAIT bio can sleep in submit_bio_noacct().
> 
> For a REQ_NOWAIT write, skip the cache and set the bits directly, which
> doesn't allocate when all the pages are there. If one is missing, fail
> the bio with BLK_STS_AGAIN, for the submitter to retry it in a context
> that may block. The throttle dispatch work may sleep, so it doesn't use
> this.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-149268
> Feature: cbt: changed block tracking (for backup)
> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>

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

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261028-ptikhomirov-vstor-149268-cbt-account-at-rq-start-v7-0-5c48672191c2@virtuozzo.com?part=7


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

[Devel] [PATCH hci-8.1 v7 8/8] block/blk-cbt: account a write request also when dm starts it

[Pavel Tikhomirov]

CBT accounts a write at bio submission. dm suspend of request based dm
waits only for started requests, so a request submitted before the
suspend but dispatched after resume is accounted before a backup point
taken under the suspend (BLKCBTSTART, CBT_SNAP_CREATE) while its data
lands after it. push_backup then saves the old data, and no incremental
backup copies the new one.

Account a write again when dm starts it. This runs under the dispatch
rcu_read_lock(), so it can't allocate: it sets the bits directly in the
bitmap pages, and the allocation of missing pages is now queued to the
kblockd workqueue already at submission. If a page is still missing when
dm starts the request (the work did not run yet, or the write was
submitted before BLKCBTSTART), dm returns BLK_STS_RESOURCE, so that
blk-mq dispatches the request again later.

https://virtuozzo.atlassian.net/browse/VSTOR-149268
Feature: cbt: changed block tracking (for backup)
Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
---
 block/blk-cbt.c        | 175 +++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/md/dm-rq.c     |   3 +
 include/linux/blkdev.h |   5 ++
 3 files changed, 183 insertions(+)

diff --git a/block/blk-cbt.c b/block/blk-cbt.c
index 7b6441fad08f9..fb947200a0cdd 100644
--- a/block/blk-cbt.c
+++ b/block/blk-cbt.c
@@ -11,6 +11,7 @@
 #include <linux/string.h>
 #include <linux/bio.h>
 #include <linux/blkdev.h>
+#include <linux/blk-mq.h>
 #include <linux/cpu.h>
 #include <linux/cpuset.h>
 #include <linux/list.h>
@@ -58,6 +59,10 @@ struct cbt_info {
 	struct page **snp_map;
 	blkcnt_t snp_block_max;
 
+	/* Map pages to allocate, see cbt_alloc_work_fn() */
+	unsigned long *missing;
+	struct work_struct alloc_work;
+
 	spinlock_t lock;
 	struct list_head list;
 };
@@ -231,6 +236,85 @@ static int __blk_cbt_set(struct cbt_info  *cbt, blkcnt_t block,
 	return (pages_missed && *pages_missed) ? -EAGAIN : 0;
 }
 
+/* Drop a cbt->count reference, release the cbt if it is dead */
+static void cbt_put(struct cbt_info *cbt)
+{
+	unsigned long flags;
+	bool release;
+
+	spin_lock_irqsave(&cbt->lock, flags);
+	release = !--cbt->count && test_bit(CBT_DEAD, &cbt->flags);
+	spin_unlock_irqrestore(&cbt->lock, flags);
+	if (release)
+		call_srcu(&cbt_srcu, &cbt->rcu, &cbt_release_callback);
+}
+
+/* Allocate the map pages queued in cbt->missing */
+static void cbt_alloc_work_fn(struct work_struct *work)
+{
+	struct cbt_info *cbt = container_of(work, struct cbt_info, alloc_work);
+	unsigned long idx;
+
+	for_each_set_bit(idx, cbt->missing, NR_PAGES(cbt->block_max)) {
+		struct page *page;
+
+		clear_bit(idx, cbt->missing);
+		if (CBT_PAGE(cbt, idx))
+			continue;
+
+		page = alloc_page(GFP_NOIO | __GFP_ZERO);
+		spin_lock_irq(&cbt->lock);
+		if (!CBT_PAGE(cbt, idx) && !test_bit(CBT_DEAD, &cbt->flags)) {
+			if (page) {
+				/* Release: lockless readers use the page */
+				smp_store_release(&cbt->map[idx], page);
+				page = NULL;
+			} else {
+				set_bit(CBT_ERROR, &cbt->flags);
+			}
+		}
+		spin_unlock_irq(&cbt->lock);
+		if (page)
+			__free_page(page);
+	}
+	cbt_put(cbt);
+}
+
+static void cbt_queue_alloc(struct cbt_info *cbt, unsigned long idx)
+{
+	unsigned long flags;
+
+	/* Already queued: the work allocates it */
+	if (test_and_set_bit(idx, cbt->missing))
+		return;
+	spin_lock_irqsave(&cbt->lock, flags);
+	if (test_bit(CBT_DEAD, &cbt->flags)) {
+		spin_unlock_irqrestore(&cbt->lock, flags);
+		return;
+	}
+	cbt->count++;
+	spin_unlock_irqrestore(&cbt->lock, flags);
+
+	/* The queued work drops the reference, or drop it if already queued */
+	if (!kblockd_schedule_work(&cbt->alloc_work))
+		cbt_put(cbt);
+}
+
+/* Queue the allocation of the missing map pages for [block, block + count) */
+static void cbt_queue_alloc_pages(struct cbt_info *cbt, blkcnt_t block,
+				  blkcnt_t count)
+{
+	unsigned long idx, last;
+
+	if (unlikely(!count || block + count > cbt->block_max))
+		return;
+
+	last = (block + count - 1) / BITS_PER_PAGE;
+	for (idx = block / BITS_PER_PAGE; idx <= last; idx++)
+		if (!CBT_PAGE(cbt, idx))
+			cbt_queue_alloc(cbt, idx);
+}
+
 /*
  * Return true if all the map pages for [block, block + count) are there,
  * or for an empty or out of range request, which needs no page.
@@ -275,6 +359,8 @@ static bool blk_cbt_add(struct cbt_info *cbt, blkcnt_t start, blkcnt_t len,
 		__blk_cbt_set(cbt, start, len, 1, NULL, NULL);
 		return false;
 	}
+	/* Queue the allocation early, so blk_cbt_rq_start() finds the pages */
+	cbt_queue_alloc_pages(cbt, start, len);
 	ex = get_cpu_ptr(cbt->cache);
 	if (ex->start + ex->len == start) {
 		ex->len += len;
@@ -317,6 +403,86 @@ bool blk_cbt_bio_queue(struct request_queue *q, struct bio *bio, bool nowait)
 	return wouldblock;
 }
 
+/*
+ * Set the bits of [start, start + len) in the present pages, without
+ * allocating. Return true if a page is missing, its allocation is queued.
+ */
+static bool cbt_set_present(struct cbt_info *cbt, blkcnt_t start, blkcnt_t len)
+{
+	blkcnt_t block, end;
+	unsigned long flags;
+	bool missing = false;
+
+	end = DIV_ROUND_UP(start + len, 1UL << cbt->block_bits);
+	block = start >> cbt->block_bits;
+	if (unlikely(end > cbt->block_max)) {
+		set_bit(CBT_ERROR, &cbt->flags);
+		return false;
+	}
+
+	while (block < end) {
+		unsigned long idx = block / BITS_PER_PAGE;
+		unsigned long off = block & (BITS_PER_PAGE - 1);
+		unsigned long n = min_t(unsigned long, BITS_PER_PAGE - off,
+					end - block);
+		struct page *page = CBT_PAGE(cbt, idx);
+
+		if (page) {
+			local_irq_save(flags);
+			spin_lock_page(page);
+			set_bits(page_address(page), off, n, 1);
+			unlock_page(page);
+			local_irq_restore(flags);
+		} else {
+			cbt_queue_alloc(cbt, idx);
+			missing = true;
+		}
+		block += n;
+	}
+
+	return missing;
+}
+
+/*
+ * Account a write again when request based dm starts it: dm suspend waits
+ * only for started requests, so one accounted at submission before a
+ * backup point taken under suspend may still reach the disk after it.
+ *
+ * Runs under the dispatch rcu_read_lock() (blk_mq_run_dispatch_ops()), so
+ * it must not sleep: if a bitmap page is missing, its allocation is queued
+ * and true is returned, for dm to retry the request later.
+ */
+bool blk_cbt_rq_start(struct request *rq)
+{
+	struct request_queue *q = rq->q;
+	struct cbt_info *cbt;
+	struct bio *bio;
+	bool retry = false;
+	int idx;
+
+	if (likely(list_empty(&q->cbt_list)))
+		return false;
+	if (rq_data_dir(rq) == READ || blk_rq_is_passthrough(rq))
+		return false;
+
+	idx = srcu_read_lock(&cbt_srcu);
+	list_for_each_entry_srcu(cbt, &q->cbt_list, list,
+				 srcu_read_lock_held(&cbt_srcu)) {
+		if (unlikely(test_bit(CBT_ERROR, &cbt->flags)))
+			continue;
+		__rq_for_each_bio(bio, rq) {
+			if (bio->bi_iter.bi_size &&
+			    cbt_set_present(cbt, bio->bi_iter.bi_sector << 9,
+					    bio->bi_iter.bi_size))
+				retry = true;
+		}
+	}
+	srcu_read_unlock(&cbt_srcu, idx);
+
+	return retry;
+}
+EXPORT_SYMBOL_GPL(blk_cbt_rq_start);
+
 static struct cbt_info *do_cbt_alloc(struct request_queue *q, __u8 *name,
 				     loff_t size, loff_t blocksize)
 {
@@ -339,8 +505,16 @@ static struct cbt_info *do_cbt_alloc(struct request_queue *q, __u8 *name,
 		goto err_pcpu;
 
 	memset(cbt->map, 0, NR_PAGES(cbt->block_max) * sizeof(void*));
+
+	cbt->missing = bitmap_zalloc(NR_PAGES(cbt->block_max), GFP_KERNEL);
+	if (!cbt->missing)
+		goto err_map;
+	INIT_WORK(&cbt->alloc_work, cbt_alloc_work_fn);
+
 	cbt->queue = q;
 	return cbt;
+err_map:
+	vfree(cbt->map);
 err_pcpu:
 	free_percpu(cbt->cache);
 err_cbt:
@@ -722,6 +896,7 @@ static void cbt_release_callback(struct rcu_head *head)
 		free_map(cbt->snp_map, nr_pages);
 	}
 
+	bitmap_free(cbt->missing);
 	free_percpu(cbt->cache);
 	kfree(cbt);
 }
diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
index 20f495bf5399d..5a8cd59ec257f 100644
--- a/drivers/md/dm-rq.c
+++ b/drivers/md/dm-rq.c
@@ -522,6 +522,9 @@ static blk_status_t dm_mq_queue_rq(struct blk_mq_hw_ctx *hctx,
 	if (ti->type->busy && ti->type->busy(ti))
 		return BLK_STS_RESOURCE;
 
+	if (blk_cbt_rq_start(rq))
+		return BLK_STS_RESOURCE;
+
 	dm_start_request(md, rq);
 
 	/* Init tio using md established in .init_request */
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 885555f480462..9698a582356a9 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1628,6 +1628,7 @@ extern void blk_cbt_update_size(struct block_device *bdev);
 extern void blk_cbt_release(struct request_queue *q);
 extern bool blk_cbt_bio_queue(struct request_queue *q, struct bio *bio,
 			      bool nowait);
+extern bool blk_cbt_rq_start(struct request *rq);
 extern int blk_cbt_ioctl(struct block_device *bdev, unsigned cmd, char __user *arg);
 #else /* CONFIG_BLK_DEV_CBT */
 static inline void blk_cbt_update_size(struct block_device *bdev)
@@ -1641,6 +1642,10 @@ static inline bool blk_cbt_bio_queue(struct request_queue *q, struct bio *bio,
 {
 	return false;
 }
+static inline bool blk_cbt_rq_start(struct request *rq)
+{
+	return false;
+}
 static inline int blk_cbt_ioctl(struct block_device *bdev, unsigned cmd,
 				char __user *arg)
 {

-- 
2.55.0

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

Re: [Devel] [PATCH hci-8.1 v7 8/8] block/blk-cbt: account a write request also when dm starts it

[no-reply]

> CBT accounts a write at bio submission. dm suspend of request based dm
> waits only for started requests, so a request submitted before the
> suspend but dispatched after resume is accounted before a backup point
> taken under the suspend (BLKCBTSTART, CBT_SNAP_CREATE) while its data
> lands after it. push_backup then saves the old data, and no incremental
> backup copies the new one.
> 
> Account a write again when dm starts it. This runs under the dispatch
> rcu_read_lock(), so it can't allocate: it sets the bits directly in the
> bitmap pages, and the allocation of missing pages is now queued to the
> kblockd workqueue already at submission. If a page is still missing when
> dm starts the request (the work did not run yet, or the write was
> submitted before BLKCBTSTART), dm returns BLK_STS_RESOURCE, so that
> blk-mq dispatches the request again later.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-149268
> Feature: cbt: changed block tracking (for backup)
> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>

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

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261028-ptikhomirov-vstor-149268-cbt-account-at-rq-start-v7-0-5c48672191c2@virtuozzo.com?part=8


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

Re: [Devel] [PATCH hci-8.1 v7 8/8] block/blk-cbt: account a write request also when dm starts it

[Konstantin Khorenko]

Not applying v7 yet: please pick one of the two options for REQ_NOWAIT
below.

On 10/28/26 11:11, Pavel Tikhomirov wrote:
> @@ -275,6 +359,8 @@ static bool blk_cbt_add(struct cbt_info *cbt, blkcnt_t start, blkcnt_t len,
>  		__blk_cbt_set(cbt, start, len, 1, NULL, NULL);
>  		return false;
>  	}
> +	/* Queue the allocation early, so blk_cbt_rq_start() finds the pages */
> +	cbt_queue_alloc_pages(cbt, start, len);

- A REQ_NOWAIT write that finds a bitmap page missing never queues its
  allocation. Either:
  1. send v8 with cbt_queue_alloc_pages() moved above the nowait
     branch, or
  2. I apply v7 with the Fixes: tags added on my side, and you send
     the nowait fix as a separate series.

The nowait branch of blk_cbt_add() returns before this call, so the bio
fails with BLK_STS_AGAIN and nothing allocates the page. A submitter
that retries only with RWF_NOWAIT gets EAGAIN until some blocking write
to the same range allocates it. io_uring is not affected, it retries
from io-wq without REQ_NOWAIT, and ploop devices never get here:
dm-ploop and dm-qcow2 lack DM_TARGET_NOWAIT, so submit_bio_noacct()
rejects such bios before CBT. cbt_queue_alloc_pages() doesn't sleep,
so option 1 is a one-line move.

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

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

[Devel] [PATCH hci-8.1 v8 8/8] block/blk-cbt: account a write request also when dm starts it

[Pavel Tikhomirov]

CBT accounts a write at bio submission. dm suspend of request based dm
waits only for started requests, so a request submitted before the
suspend but dispatched after resume is accounted before a backup point
taken under the suspend (BLKCBTSTART, CBT_SNAP_CREATE) while its data
lands after it. push_backup then saves the old data, and no incremental
backup copies the new one.

Account a write again when dm starts it. This runs under the dispatch
rcu_read_lock(), so it can't allocate: it sets the bits directly in the
bitmap pages, and the allocation of missing pages is now queued to the
kblockd workqueue already at submission, also for a nowait write failed
with BLK_STS_AGAIN, so that its retry finds the page. If a page is still
missing when dm starts the request (the work did not run yet, or the
write was submitted before BLKCBTSTART), dm returns BLK_STS_RESOURCE, so
that blk-mq dispatches the request again later.

https://virtuozzo.atlassian.net/browse/VSTOR-149268
Feature: cbt: changed block tracking (for backup)
Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
---
v8: queue the page allocation above the nowait branch of blk_cbt_add(),
so that a nowait write failed with BLK_STS_AGAIN gets its page for the
retry. Patches 1-7 of v7 are unchanged.
---
 block/blk-cbt.c        | 178 +++++++++++++++++++++++++++++++++++++++++
 drivers/md/dm-rq.c     |   3 +
 include/linux/blkdev.h |   5 ++
 3 files changed, 186 insertions(+)

diff --git a/block/blk-cbt.c b/block/blk-cbt.c
index 7b6441fad08f9..baa56ab1a5389 100644
--- a/block/blk-cbt.c
+++ b/block/blk-cbt.c
@@ -11,6 +11,7 @@
 #include <linux/string.h>
 #include <linux/bio.h>
 #include <linux/blkdev.h>
+#include <linux/blk-mq.h>
 #include <linux/cpu.h>
 #include <linux/cpuset.h>
 #include <linux/list.h>
@@ -58,6 +59,10 @@ struct cbt_info {
 	struct page **snp_map;
 	blkcnt_t snp_block_max;
 
+	/* Map pages to allocate, see cbt_alloc_work_fn() */
+	unsigned long *missing;
+	struct work_struct alloc_work;
+
 	spinlock_t lock;
 	struct list_head list;
 };
@@ -231,6 +236,85 @@ static int __blk_cbt_set(struct cbt_info  *cbt, blkcnt_t block,
 	return (pages_missed && *pages_missed) ? -EAGAIN : 0;
 }
 
+/* Drop a cbt->count reference, release the cbt if it is dead */
+static void cbt_put(struct cbt_info *cbt)
+{
+	unsigned long flags;
+	bool release;
+
+	spin_lock_irqsave(&cbt->lock, flags);
+	release = !--cbt->count && test_bit(CBT_DEAD, &cbt->flags);
+	spin_unlock_irqrestore(&cbt->lock, flags);
+	if (release)
+		call_srcu(&cbt_srcu, &cbt->rcu, &cbt_release_callback);
+}
+
+/* Allocate the map pages queued in cbt->missing */
+static void cbt_alloc_work_fn(struct work_struct *work)
+{
+	struct cbt_info *cbt = container_of(work, struct cbt_info, alloc_work);
+	unsigned long idx;
+
+	for_each_set_bit(idx, cbt->missing, NR_PAGES(cbt->block_max)) {
+		struct page *page;
+
+		clear_bit(idx, cbt->missing);
+		if (CBT_PAGE(cbt, idx))
+			continue;
+
+		page = alloc_page(GFP_NOIO | __GFP_ZERO);
+		spin_lock_irq(&cbt->lock);
+		if (!CBT_PAGE(cbt, idx) && !test_bit(CBT_DEAD, &cbt->flags)) {
+			if (page) {
+				/* Release: lockless readers use the page */
+				smp_store_release(&cbt->map[idx], page);
+				page = NULL;
+			} else {
+				set_bit(CBT_ERROR, &cbt->flags);
+			}
+		}
+		spin_unlock_irq(&cbt->lock);
+		if (page)
+			__free_page(page);
+	}
+	cbt_put(cbt);
+}
+
+static void cbt_queue_alloc(struct cbt_info *cbt, unsigned long idx)
+{
+	unsigned long flags;
+
+	/* Already queued: the work allocates it */
+	if (test_and_set_bit(idx, cbt->missing))
+		return;
+	spin_lock_irqsave(&cbt->lock, flags);
+	if (test_bit(CBT_DEAD, &cbt->flags)) {
+		spin_unlock_irqrestore(&cbt->lock, flags);
+		return;
+	}
+	cbt->count++;
+	spin_unlock_irqrestore(&cbt->lock, flags);
+
+	/* The queued work drops the reference, or drop it if already queued */
+	if (!kblockd_schedule_work(&cbt->alloc_work))
+		cbt_put(cbt);
+}
+
+/* Queue the allocation of the missing map pages for [block, block + count) */
+static void cbt_queue_alloc_pages(struct cbt_info *cbt, blkcnt_t block,
+				  blkcnt_t count)
+{
+	unsigned long idx, last;
+
+	if (unlikely(!count || block + count > cbt->block_max))
+		return;
+
+	last = (block + count - 1) / BITS_PER_PAGE;
+	for (idx = block / BITS_PER_PAGE; idx <= last; idx++)
+		if (!CBT_PAGE(cbt, idx))
+			cbt_queue_alloc(cbt, idx);
+}
+
 /*
  * Return true if all the map pages for [block, block + count) are there,
  * or for an empty or out of range request, which needs no page.
@@ -264,6 +348,11 @@ static bool blk_cbt_add(struct cbt_info *cbt, blkcnt_t start, blkcnt_t len,
 	end = DIV_ROUND_UP(start + len, 1UL << cbt->block_bits);
 	start >>= cbt->block_bits;
 	len = end - start;
+	/*
+	 * Queue the allocation early, so blk_cbt_rq_start() and a retry of a
+	 * nowait write find the pages
+	 */
+	cbt_queue_alloc_pages(cbt, start, len);
 	if (unlikely(nowait)) {
 		/* Don't sleep: set the bits directly, only with all pages there */
 		if (!cbt_pages_present(cbt, start, len))
@@ -317,6 +406,86 @@ bool blk_cbt_bio_queue(struct request_queue *q, struct bio *bio, bool nowait)
 	return wouldblock;
 }
 
+/*
+ * Set the bits of [start, start + len) in the present pages, without
+ * allocating. Return true if a page is missing, its allocation is queued.
+ */
+static bool cbt_set_present(struct cbt_info *cbt, blkcnt_t start, blkcnt_t len)
+{
+	blkcnt_t block, end;
+	unsigned long flags;
+	bool missing = false;
+
+	end = DIV_ROUND_UP(start + len, 1UL << cbt->block_bits);
+	block = start >> cbt->block_bits;
+	if (unlikely(end > cbt->block_max)) {
+		set_bit(CBT_ERROR, &cbt->flags);
+		return false;
+	}
+
+	while (block < end) {
+		unsigned long idx = block / BITS_PER_PAGE;
+		unsigned long off = block & (BITS_PER_PAGE - 1);
+		unsigned long n = min_t(unsigned long, BITS_PER_PAGE - off,
+					end - block);
+		struct page *page = CBT_PAGE(cbt, idx);
+
+		if (page) {
+			local_irq_save(flags);
+			spin_lock_page(page);
+			set_bits(page_address(page), off, n, 1);
+			unlock_page(page);
+			local_irq_restore(flags);
+		} else {
+			cbt_queue_alloc(cbt, idx);
+			missing = true;
+		}
+		block += n;
+	}
+
+	return missing;
+}
+
+/*
+ * Account a write again when request based dm starts it: dm suspend waits
+ * only for started requests, so one accounted at submission before a
+ * backup point taken under suspend may still reach the disk after it.
+ *
+ * Runs under the dispatch rcu_read_lock() (blk_mq_run_dispatch_ops()), so
+ * it must not sleep: if a bitmap page is missing, its allocation is queued
+ * and true is returned, for dm to retry the request later.
+ */
+bool blk_cbt_rq_start(struct request *rq)
+{
+	struct request_queue *q = rq->q;
+	struct cbt_info *cbt;
+	struct bio *bio;
+	bool retry = false;
+	int idx;
+
+	if (likely(list_empty(&q->cbt_list)))
+		return false;
+	if (rq_data_dir(rq) == READ || blk_rq_is_passthrough(rq))
+		return false;
+
+	idx = srcu_read_lock(&cbt_srcu);
+	list_for_each_entry_srcu(cbt, &q->cbt_list, list,
+				 srcu_read_lock_held(&cbt_srcu)) {
+		if (unlikely(test_bit(CBT_ERROR, &cbt->flags)))
+			continue;
+		__rq_for_each_bio(bio, rq) {
+			if (bio->bi_iter.bi_size &&
+			    cbt_set_present(cbt, bio->bi_iter.bi_sector << 9,
+					    bio->bi_iter.bi_size))
+				retry = true;
+		}
+	}
+	srcu_read_unlock(&cbt_srcu, idx);
+
+	return retry;
+}
+EXPORT_SYMBOL_GPL(blk_cbt_rq_start);
+
 static struct cbt_info *do_cbt_alloc(struct request_queue *q, __u8 *name,
 				     loff_t size, loff_t blocksize)
 {
@@ -339,8 +508,16 @@ static struct cbt_info *do_cbt_alloc(struct request_queue *q, __u8 *name,
 		goto err_pcpu;
 
 	memset(cbt->map, 0, NR_PAGES(cbt->block_max) * sizeof(void*));
+
+	cbt->missing = bitmap_zalloc(NR_PAGES(cbt->block_max), GFP_KERNEL);
+	if (!cbt->missing)
+		goto err_map;
+	INIT_WORK(&cbt->alloc_work, cbt_alloc_work_fn);
+
 	cbt->queue = q;
 	return cbt;
+err_map:
+	vfree(cbt->map);
 err_pcpu:
 	free_percpu(cbt->cache);
 err_cbt:
@@ -722,6 +899,7 @@ static void cbt_release_callback(struct rcu_head *head)
 		free_map(cbt->snp_map, nr_pages);
 	}
 
+	bitmap_free(cbt->missing);
 	free_percpu(cbt->cache);
 	kfree(cbt);
 }
diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
index 20f495bf5399d..5a8cd59ec257f 100644
--- a/drivers/md/dm-rq.c
+++ b/drivers/md/dm-rq.c
@@ -522,6 +522,9 @@ static blk_status_t dm_mq_queue_rq(struct blk_mq_hw_ctx *hctx,
 	if (ti->type->busy && ti->type->busy(ti))
 		return BLK_STS_RESOURCE;
 
+	if (blk_cbt_rq_start(rq))
+		return BLK_STS_RESOURCE;
+
 	dm_start_request(md, rq);
 
 	/* Init tio using md established in .init_request */
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 885555f480462..9698a582356a9 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1628,6 +1628,7 @@ extern void blk_cbt_update_size(struct block_device *bdev);
 extern void blk_cbt_release(struct request_queue *q);
 extern bool blk_cbt_bio_queue(struct request_queue *q, struct bio *bio,
 			      bool nowait);
+extern bool blk_cbt_rq_start(struct request *rq);
 extern int blk_cbt_ioctl(struct block_device *bdev, unsigned cmd, char __user *arg);
 #else /* CONFIG_BLK_DEV_CBT */
 static inline void blk_cbt_update_size(struct block_device *bdev)
@@ -1641,6 +1642,10 @@ static inline bool blk_cbt_bio_queue(struct request_queue *q, struct bio *bio,
 {
 	return false;
 }
+static inline bool blk_cbt_rq_start(struct request *rq)
+{
+	return false;
+}
 static inline int blk_cbt_ioctl(struct block_device *bdev, unsigned cmd,
 				char __user *arg)
 {
-- 
2.55.0

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

Re: [Devel] [PATCH hci-8.1 v7 0/8] block/blk-cbt: don't lose writes held across a dm suspend

[Konstantin Khorenko]

Not applying v7 yet: the REQ_NOWAIT question in the 8/8 reply decides
between applying v7 and a v8.

On 10/28/26 11:11, Pavel Tikhomirov wrote:
> Fix CBT missing writes that request-based dm holds across a suspend,
> which made incremental backups taken with ploop-backup silently lose
> data.

- VSTOR-149268, which all eight commits name, is already resolved.
  Please reopen it or file a separate bug for this series and put that
  into the commits.
- Fixes: tags, in v8 or added on my side if v7 goes in:
  - 1/8, 6/8: 458bca130a8e ("block/blk-cbt: allow multiple cbts in a
    single queue")
  - 2/8, 3/8, 4/8, 5/8, 7/8, 8/8: e713742e2ee6 ("cbt: introduce changed
    block tracking")

The review also found four old CBT bugs, not introduced by this series
and not blocking it. I created tasks for them and assigned them to you,
the details are in the descriptions:

- https://virtuozzo.atlassian.net/browse/VSTOR-153220
  NULL dereference in cbt_page_alloc() on BLKCBTSTOP during a write
- https://virtuozzo.atlassian.net/browse/VSTOR-153221
  crash in CBT_SNAP_CREATE after a failed bitmap page allocation
- https://virtuozzo.atlassian.net/browse/VSTOR-153222
  an extent evicted from the per-cpu cache misses the snapshot
- https://virtuozzo.atlassian.net/browse/VSTOR-153223
  a write allocating a bitmap page during a resize is lost

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

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