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

[Devel] [PATCH hci-8.1 v4 0/3] 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 10/10 with it. CBT
start/stop and device resize under fio ran without warnings.

Known pre-existing problem, not fixed here: when blk_cbt_add() evicts an
extent from the per-cpu cache (or in CBT_NOCACHE mode), __blk_cbt_set()
may allocate a bitmap page with the RCU lock dropped in the middle of
the cbt_list walk, so a concurrent CBT stop or resize can free the next
cbt under the walk. It also sleeps there for REQ_NOWAIT bios. With this
series the page is usually allocated by then, so it is rarer.

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

Signed-off-by: Pavel Tikhomirov <ptikhomirov@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 (3):
      block/blk-cbt: shift the block size as unsigned
      block/blk-throttle: account a throttled write in CBT
      block/blk-cbt: account a write request also when dm starts it

 block/blk-cbt.c        | 176 ++++++++++++++++++++++++++++++++++++++++++++++++-
 block/blk-throttle.c   |   5 +-
 drivers/md/dm-rq.c     |   3 +
 include/linux/blkdev.h |   5 ++
 4 files changed, 185 insertions(+), 4 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 v4 1/3] 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: the sign extended block size
makes the rounded up block counts wrong. Shift an unsigned one instead.

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 90219f58f1ae5..1066b6a33b6b5 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

[Devel] [PATCH hci-8.1 v4 2/3] 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

[Devel] [PATCH hci-8.1 v4 3/3] 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        | 170 +++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/md/dm-rq.c     |   3 +
 include/linux/blkdev.h |   5 ++
 3 files changed, 178 insertions(+)

diff --git a/block/blk-cbt.c b/block/blk-cbt.c
index 1066b6a33b6b5..ae941ec7e7a1d 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>
@@ -52,6 +53,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;
 };
@@ -221,6 +226,82 @@ 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_rcu(&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) {
+				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;
+
+	set_bit(idx, cbt->missing);
+	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);
+}
+
 static void blk_cbt_add(struct cbt_info *cbt, blkcnt_t start, blkcnt_t len)
 {
 	struct cbt_extent *ex;
@@ -236,6 +317,8 @@ static void blk_cbt_add(struct cbt_info *cbt, blkcnt_t start, blkcnt_t len)
 		__blk_cbt_set(cbt, start, len, 1, 1, NULL, NULL);
 		return;
 	}
+	/* 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;
@@ -266,6 +349,84 @@ void blk_cbt_bio_queue(struct request_queue *q, struct bio *bio)
 	rcu_read_unlock();
 }
 
+/*
+ * 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;
+
+	if (likely(list_empty(&q->cbt_list)))
+		return false;
+	if (rq_data_dir(rq) == READ || blk_rq_is_passthrough(rq))
+		return false;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(cbt, &q->cbt_list, list) {
+		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;
+		}
+	}
+	rcu_read_unlock();
+
+	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)
 {
@@ -288,8 +449,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:
@@ -671,6 +840,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 1284bb0e4ebc0..d76a0c9be28f0 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1627,6 +1627,7 @@ static inline bool blk_crypto_register(struct blk_crypto_profile *profile,
 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_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)
@@ -1638,6 +1639,10 @@ 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_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 v4 1/3] 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: the sign extended block size
> makes the rounded up block counts wrong. Shift an unsigned one instead.
> 
> 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/20261027-ptikhomirov-vstor-149268-cbt-account-at-rq-start-v4-0-b4d92264f6a4@virtuozzo.com?part=1


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

Re: [Devel] [PATCH hci-8.1 v4 2/3] 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/20261027-ptikhomirov-vstor-149268-cbt-account-at-rq-start-v4-0-b4d92264f6a4@virtuozzo.com?part=2


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

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

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] block: cbt_alloc_work_fn() races with unlocked CBT_PAGE_MISSED store in __blk_cbt_set()
--

commit e384a8a44cb8b1957ca1f16104c5b091e997663e
Author: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>

block/blk-cbt: account a write request also when dm starts it

This patch accounts a write again when request based dm starts it, by setting bits directly in the already-present bitmap pages from blk_cbt_rq_start(). Allocation of missing bitmap pages is queued to kblockd at submission time through cbt_alloc_work_fn(). If a page is still missing at dm start time, dm_mq_queue_rq() returns BLK_STS_RESOURCE so blk-mq retries the request later.

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

> diff --git a/block/blk-cbt.c b/block/blk-cbt.c
> --- a/block/blk-cbt.c
> +++ b/block/blk-cbt.c

[ ... ]

> +/* 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) {
> +				cbt->map[idx] = page;
> +				page = NULL;

[Severity: High]
Can this store race with the CBT_PAGE_MISSED store in __blk_cbt_set()?

cbt_alloc_work_fn() installs the new page under cbt->lock. However, the
sentinel store in __blk_cbt_set() takes no cbt->lock:

block/blk-cbt.c:__blk_cbt_set() {
    ...
	} else if (pages_missed) {
		(*pages_missed)++;
		if (!*idx_first)
			*idx_first = idx;
		cbt->map[idx] = CBT_PAGE_MISSED;
    ...
}

__blk_cbt_set() is reached with a non-NULL pages_missed from the
on_each_cpu() IPI in cbt_flush_cache():

    on_each_cpu(__cbt_flush_cpu_cache, &ctx, 1);

A possible interleaving:

CPU B (IPI, __blk_cbt_set)
    page = CBT_PAGE(cbt, idx);       /* NULL */

CPU A (kblockd, cbt_alloc_work_fn)
    spin_lock_irq(&cbt->lock);
    cbt->map[idx] = page;

CPU B
    cbt->map[idx] = CBT_PAGE_MISSED; /* overwrites the new page */

The new page is then no longer reachable, since CBT_PAGE() treats the slot
as empty and cbt_release_callback() skips it, so it is leaked.

Any dirty bits already in that page are also lost. This includes bits set
by cbt_set_present() from blk_cbt_rq_start(), which is the case this patch
is meant to protect. A later cbt_page_alloc() from cbt_flush_cache() would
install a fresh zeroed page.

Before this patch the window was much narrower. Now cbt_queue_alloc_pages()
queues an asynchronous installer on every write that touches a missing
page, unless CBT_NOCACHE is set. Neither cbt_mutex nor any other exclusion
keeps cbt_alloc_work_fn() out while cbt_flush_cache() runs.

Could the sentinel store be done under cbt->lock, or could the flush path
be serialized against alloc_work (for example with flush_work())?

[ ... ]

> +static bool cbt_set_present(struct cbt_info *cbt, blkcnt_t start, blkcnt_t len)
> +{

[ ... ]

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

[Severity: High]
Related to the comment above: bits set here in cbt_set_present() are lost
if the page is subsequently discarded by the unlocked CBT_PAGE_MISSED store
in __blk_cbt_set(). That would leave an incremental backup missing changed
data for a write that dm has already started.

[ ... ]

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

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