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

[Devel] [PATCH hci-8.1] 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 in dm_start_request(). It runs under the dispatch
rcu_read_lock(), so a missing bitmap page is allocated with GFP_ATOMIC
(CBT_ERROR on failure). This only happens for requests submitted before
BLKCBTSTART: any later write has its page allocated already by its
submission time accounting.

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.

https://virtuozzo.atlassian.net/browse/VSTOR-149268
---
 block/blk-cbt.c        | 82 +++++++++++++++++++++++++++++++++++++++-----------
 drivers/md/dm-rq.c     |  1 +
 include/linux/blkdev.h |  4 +++
 3 files changed, 70 insertions(+), 17 deletions(-)

diff --git a/block/blk-cbt.c b/block/blk-cbt.c
index 90219f58f1ae5..7513acd713b00 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>
@@ -63,6 +64,13 @@ enum CBT_FLAGS
 	CBT_DEAD  = 1,
 	CBT_NOCACHE  = 2,
 };
+
+/* How a missing bitmap page may be allocated */
+enum cbt_alloc {
+	CBT_ALLOC_SLEEP = 0,	/* may sleep */
+	CBT_ALLOC_RCU = 1,	/* may sleep, dropping our own rcu_read_lock() */
+	CBT_ALLOC_ATOMIC = 2,	/* must not sleep */
+};
 static void cbt_release_callback(struct rcu_head *head);
 static void cbt_flush_cache(struct cbt_info *cbt);
 
@@ -103,7 +111,7 @@ static void set_bits(void *bm, int cur, int len, bool is_set)
  * -ENOMEM if alloc_page failed.
  */
 static int cbt_page_alloc(struct cbt_info  **cbt_pp, unsigned long idx,
-			  int in_rcu)
+			  enum cbt_alloc alloc)
 {
 	struct cbt_info	 *cbt = *cbt_pp;
 	struct page *page;
@@ -129,11 +137,15 @@ 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();
+	if (alloc == CBT_ALLOC_ATOMIC) {
+		page = alloc_page(GFP_ATOMIC | __GFP_ZERO | __GFP_NOWARN);
+	} else {
+		if (alloc == CBT_ALLOC_RCU)
+			rcu_read_unlock();
+		page = alloc_page(GFP_NOIO | __GFP_ZERO);
+		if (alloc == CBT_ALLOC_RCU)
+			rcu_read_lock();
+	}
 	spin_lock_irq(&cbt->lock);
 	if (unlikely(!--(cbt->count) && test_bit(CBT_DEAD, &cbt->flags))) {
 		spin_unlock_irq(&cbt->lock);
@@ -160,7 +172,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, enum cbt_alloc alloc, bool set,
 			 unsigned long *pages_missed,
 			 unsigned long *idx_first)
 {
@@ -210,7 +222,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, alloc);
 		if (ret == -EAGAIN) /* new cbt */
 			continue;
 		else if (ret == -EBADF) /* dead cbt */
@@ -221,7 +233,8 @@ 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)
+static void blk_cbt_add(struct cbt_info *cbt, blkcnt_t start, blkcnt_t len,
+			enum cbt_alloc alloc)
 {
 	struct cbt_extent *ex;
 	struct cbt_extent old;
@@ -233,7 +246,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, alloc, 1, NULL, NULL);
 		return;
 	}
 	ex = get_cpu_ptr(cbt->cache);
@@ -248,7 +261,7 @@ 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, alloc, 1, NULL, NULL);
 }
 
 void blk_cbt_bio_queue(struct request_queue *q, struct bio *bio)
@@ -260,12 +273,47 @@ void blk_cbt_bio_queue(struct request_queue *q, struct bio *bio)
 		goto out_rcu;
 
 	list_for_each_entry_rcu(cbt, &q->cbt_list, list)
-		blk_cbt_add(cbt, bio->bi_iter.bi_sector << 9, bio->bi_iter.bi_size);
+		blk_cbt_add(cbt, bio->bi_iter.bi_sector << 9, bio->bi_iter.bi_size,
+			    CBT_ALLOC_RCU);
 
 out_rcu:
 	rcu_read_unlock();
 }
 
+/*
+ * 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.
+ *
+ * Account bio by bio, as a discard request may hold non-contiguous bios.
+ *
+ * Runs under the dispatch rcu_read_lock() (blk_mq_run_dispatch_ops()),
+ * hence GFP_ATOMIC.
+ */
+void blk_cbt_rq_start(struct request *rq)
+{
+	struct request_queue *q = rq->q;
+	struct cbt_info *cbt;
+	struct bio *bio;
+
+	if (likely(list_empty(&q->cbt_list)))
+		return;
+	if (rq_data_dir(rq) == READ || blk_rq_is_passthrough(rq))
+		return;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(cbt, &q->cbt_list, list) {
+		__rq_for_each_bio(bio, rq) {
+			if (!bio->bi_iter.bi_size)
+				continue;
+			blk_cbt_add(cbt, bio->bi_iter.bi_sector << 9,
+				    bio->bi_iter.bi_size, CBT_ALLOC_ATOMIC);
+		}
+	}
+	rcu_read_unlock();
+}
+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)
 {
@@ -545,7 +593,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, CBT_ALLOC_SLEEP))
 				goto out;
 
 			page_main = cbt->map[i];
@@ -734,7 +782,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, CBT_ALLOC_SLEEP, 1,
 					&ctx->pages_missed,
 					&ctx->idx_first);
 		if (!ret) {
@@ -765,7 +813,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, CBT_ALLOC_SLEEP);
 			if (ret == -EAGAIN) /* new cbt */
 				goto try_again;
 			else if (ret) /* dead cbt or alloc_page failed */
@@ -887,7 +935,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, CBT_ALLOC_SLEEP, 0, NULL, NULL);
 		cur_ex++;
 		ci.ci_mapped_extents++;
 		block = ex.start + ex.len;
@@ -971,7 +1019,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, CBT_ALLOC_SLEEP, set, NULL, NULL);
 		if (ret)
 			break;
 		cur_ex++;
diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
index 20f495bf5399d..9354e1e0c5a2d 100644
--- a/drivers/md/dm-rq.c
+++ b/drivers/md/dm-rq.c
@@ -444,6 +444,7 @@ ssize_t dm_attr_rq_based_seq_io_merge_deadline_store(struct mapped_device *md,
 
 static void dm_start_request(struct mapped_device *md, struct request *orig)
 {
+	blk_cbt_rq_start(orig);
 	blk_mq_start_request(orig);
 
 	if (unlikely(dm_stats_used(&md->stats))) {
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 1284bb0e4ebc0..848fac73e451f 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 void 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,9 @@ 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 void blk_cbt_rq_start(struct request *rq)
+{
+}
 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] 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:
- [Medium] block: cbt_page_alloc() GFP_ATOMIC failure sets CBT_ERROR on dm dispatch path
--

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

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

This patch adds blk_cbt_rq_start(), called from dm_start_request(), so a
write is accounted in CBT a second time when request based dm starts it.
A missing bitmap page is allocated with GFP_ATOMIC in this path, and
CBT_ERROR is set if that allocation fails.

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

[ ... ]

> @@ -129,11 +137,15 @@ 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();
> +	if (alloc == CBT_ALLOC_ATOMIC) {
> +		page = alloc_page(GFP_ATOMIC | __GFP_ZERO | __GFP_NOWARN);

[Severity: Medium]
Can this GFP_ATOMIC allocation fail often enough to matter?

Under memory pressure alloc_page(GFP_ATOMIC) has no direct reclaim and
can fail.  When it does, a few lines further down in cbt_page_alloc():

	if (unlikely(!page)) {
		set_bit(CBT_ERROR, &cbt->flags);
		spin_unlock_irq(&cbt->lock);
		return -ENOMEM;
	}

CBT_ERROR is a sticky flag, and __GFP_NOWARN makes the failure silent.
The call chain is:

dm_mq_queue_rq()
  dm_start_request()
    blk_cbt_rq_start()
      blk_cbt_add(..., CBT_ALLOC_ATOMIC)
        __blk_cbt_set()
          cbt_page_alloc()

Neither blk_cbt_rq_start() nor blk_cbt_add() checks the return value.

After the failure, blk_cbt_add() returns early for this CBT on every
later write:

	if (unlikely(test_bit(CBT_ERROR, &cbt->flags)))
		return;

cbt_ioc_get() and cbt_ioc_set() then return -EIO.

So a transient atomic allocation failure permanently invalidates the CBT,
and with it the incremental backup chain, until the CBT is re-created.
The old submission time path used GFP_NOIO with the RCU lock dropped,
which does not fail in practice.

Is there a reason not to retry from a sleepable context, or to avoid
setting CBT_ERROR in the atomic case?

[ ... ]

> @@ -248,7 +261,7 @@ 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, alloc, 1, NULL, NULL);

[Severity: Medium]
Is the commit message claim correct that a page can be missing here only
for requests submitted before BLKCBTSTART?

The commit message says:

  "This only happens for requests submitted before BLKCBTSTART: any later
  write has its page allocated already by its submission time accounting."

On the normal (cached) path, blk_cbt_add() only records the extent in the
per-CPU cache.  It calls __blk_cbt_set(), and so allocates pages, only
for the evicted old extent.

That means a write submitted after BLKCBTSTART can still have no bitmap
page when dm_start_request() runs.

In blk_cbt_rq_start(), re-adding a range that is not contiguous with the
cached extent evicts the previous cached extent, which is often an
unrelated earlier write.  That extent is flushed with CBT_ALLOC_ATOMIC.

So GFP_ATOMIC page allocation looks possible on ordinary write-heavy
workloads, not only in the narrow pre-BLKCBTSTART window.

Also, re-adding the same range from blk_cbt_rq_start() never coalesces,
since ex->start + ex->len != start for it.  Each started request
therefore causes an extra __blk_cbt_set() call and bitmap page lock.

Could the commit message and the allocation strategy be revisited in
light of this?

[ ... ]

> +void blk_cbt_rq_start(struct request *rq)
> +{

[ ... ]

> +			blk_cbt_add(cbt, bio->bi_iter.bi_sector << 9,
> +				    bio->bi_iter.bi_size, CBT_ALLOC_ATOMIC);

[Severity: Medium]
This is the same issue as above, seen from the caller.  Errors from
blk_cbt_add() are dropped here, so a CBT_ERROR set by a failed
GFP_ATOMIC allocation in blk_cbt_rq_start() is not reported to the dm
dispatch path.  Is that intended?

[ ... ]

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

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