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

[Devel] [PATCH hci-8.1 v2] 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, which are now allocated already at submission. A page can
still be missing only for a write submitted before BLKCBTSTART, then dm
returns BLK_STS_RESOURCE, so that blk-mq dispatches the request again
later, and the page is allocated from the kblockd workqueue.

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

Feature: cbt: changed block tracking (for backup)
Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
---
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.

https://virtuozzo.atlassian.net/browse/VSTOR-149268
---
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
---
 block/blk-cbt.c        | 177 +++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/md/dm-rq.c     |   3 +
 include/linux/blkdev.h |   5 ++
 3 files changed, 185 insertions(+)

diff --git a/block/blk-cbt.c b/block/blk-cbt.c
index 90219f58f1ae5..30252027c4578 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 blk_cbt_rq_start() found missing, see cbt_alloc_work_fn() */
+	unsigned long *missing;
+	struct work_struct alloc_work;
+
 	spinlock_t lock;
 	struct list_head list;
 };
@@ -221,6 +226,24 @@ static int __blk_cbt_set(struct cbt_info  *cbt, blkcnt_t block,
 	return (pages_missed && *pages_missed) ? -EAGAIN : 0;
 }
 
+/* Allocate the missing map pages for blocks [block, block + count) */
+static void cbt_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++) {
+		struct cbt_info *c = cbt;
+
+		if (!CBT_PAGE(cbt, idx) && cbt_page_alloc(&c, idx, 1))
+			return;
+	}
+}
+
 static void blk_cbt_add(struct cbt_info *cbt, blkcnt_t start, blkcnt_t len)
 {
 	struct cbt_extent *ex;
@@ -236,6 +259,12 @@ 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;
 	}
+	/*
+	 * Preallocate the pages for blk_cbt_rq_start(): it runs under the
+	 * dispatch rcu_read_lock(), and a missing page there means delaying
+	 * the request and allocating the page from the kblockd workqueue.
+	 */
+	cbt_alloc_pages(cbt, start, len);
 	ex = get_cpu_ptr(cbt->cache);
 	if (ex->start + ex->len == start) {
 		ex->len += len;
@@ -266,6 +295,145 @@ void blk_cbt_bio_queue(struct request_queue *q, struct bio *bio)
 	rcu_read_unlock();
 }
 
+/* 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 blk_cbt_rq_start() found 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);
+}
+
+/*
+ * 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, 1 << 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 +456,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 +847,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)
 {

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

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

[no-reply]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] block: cbt_set_present() uses signed 1 << block_bits, broken for 2^31 block size
- [Low] block: blk_cbt_rq_start() page-preallocation assumption fails for throttled bios
--

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

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

This patch makes CBT account a write a second time when request-based dm
starts the request, so that a request dispatched after a suspend-time
backup point is still recorded. The dispatch hook only sets bits in bitmap
pages that already exist. If a page is missing, it queues an allocation on
kblockd and dm-rq returns BLK_STS_RESOURCE so that 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

[ ... ]

> @@ -236,6 +259,12 @@ 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;
>  	}
> +	/*
> +	 * Preallocate the pages for blk_cbt_rq_start(): it runs under the
> +	 * dispatch rcu_read_lock(), and a missing page there means delaying
> +	 * the request and allocating the page from the kblockd workqueue.
> +	 */
> +	cbt_alloc_pages(cbt, start, len);
>  	ex = get_cpu_ptr(cbt->cache);

[Severity: Low]
The comment here, and the commit message ("A page can still be missing
only for a write submitted before BLKCBTSTART"), say that preallocation
at submission covers every write except those submitted before
BLKCBTSTART.

Is that accurate for throttled bios? In submit_bio_noacct() a throttled
write returns before the CBT hook:

    if (blk_throtl_bio(bio))
        return;
    blk_cbt_bio_queue(q, bio);
    submit_bio_noacct_nocheck(bio);

Later blk_throtl_dispatch_work_fn() resubmits it with:

    while ((bio = bio_list_pop(&bio_list_on_stack)))
        submit_bio_noacct_nocheck(bio);

This path never calls blk_cbt_bio_queue(), so blk_cbt_add() never runs
cbt_alloc_pages() for that bio. When the request reaches dm_mq_queue_rq(),
blk_cbt_rq_start() can find the page missing. It then queues kblockd work
and dm_mq_queue_rq() returns BLK_STS_RESOURCE.

The retry does terminate, so this looks like a one-time delayed dispatch.
Could the commit message and this comment be updated to mention that case?

[ ... ]

> +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, 1 << cbt->block_bits);

[Severity: Low]
This problem wasn't introduced by this patch in blk_cbt_add(), but the
same expression is repeated here. 1 << cbt->block_bits is a signed int
shift.

If block_bits is 31, the shift overflows. cbt_ioc_init() only checks
is_power_of_2(ci.ci_blksize), so a privileged caller can pass
ci_blksize = 0x80000000, and do_cbt_alloc() then sets block_bits to 31.
In practice 1 << 31 becomes INT_MIN and is sign-extended into the 64-bit
DIV_ROUND_UP().

Can end then come out wrong, so that either the "end > cbt->block_max"
check sets CBT_ERROR spuriously, or the "while (block < end)" loop skips
every page and no writes are recorded?

Would 1ULL << cbt->block_bits, or a shift on a blkcnt_t, avoid this in
both cbt_set_present() and blk_cbt_add()?

> +	block = start >> cbt->block_bits;
> +	if (unlikely(end > cbt->block_max)) {
> +		set_bit(CBT_ERROR, &cbt->flags);
> +		return false;
> +	}

[ ... ]

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

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