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

[Devel] [PATCH hci-8.1 v5 0/6] 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.

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

Signed-off-by: Pavel Tikhomirov <ptikhomirov@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 (6):
      block/blk-cbt: shift the block size as unsigned
      block/blk-throttle: account a throttled write in CBT
      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        | 289 +++++++++++++++++++++++++++++++++++++++++++------
 block/blk-core.c       |   5 +-
 block/blk-throttle.c   |   5 +-
 drivers/md/dm-rq.c     |   3 +
 include/linux/blkdev.h |  12 +-
 5 files changed, 276 insertions(+), 38 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 v5 1/6] 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 v5 2/6] 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 v5 3/6] 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, without cbt->lock. A page installed under cbt->lock in between, by
cbt_page_alloc(), is then overwritten: it leaks, and the bits already
set in it are lost.

Store the marker under cbt->lock, and set the bits in the page if it
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 | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/block/blk-cbt.c b/block/blk-cbt.c
index 1066b6a33b6b5..01114362bb2bc 100644
--- a/block/blk-cbt.c
+++ b/block/blk-cbt.c
@@ -194,10 +194,19 @@ static int __blk_cbt_set(struct cbt_info  *cbt, blkcnt_t block,
 			block += len;
 			continue;
 		} else if (pages_missed) {
+			bool installed;
+
+			/* Don't overwrite a page installed meanwhile */
+			spin_lock_irqsave(&cbt->lock, flags);
+			installed = CBT_PAGE(cbt, idx);
+			if (!installed)
+				cbt->map[idx] = CBT_PAGE_MISSED;
+			spin_unlock_irqrestore(&cbt->lock, flags);
+			if (installed)
+				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

[Devel] [PATCH hci-8.1 v5 4/6] 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.

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, 26 insertions(+), 24 deletions(-)

diff --git a/block/blk-cbt.c b/block/blk-cbt.c
index 01114362bb2bc..c87c7624f3800 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>
@@ -31,6 +32,8 @@
 			    NULL : cbt->map[idx])
 
 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;
@@ -102,8 +105,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;
@@ -115,7 +117,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;
 
@@ -129,15 +133,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;
@@ -160,7 +160,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)
 {
@@ -219,7 +219,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 */
@@ -242,7 +242,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);
@@ -257,22 +257,24 @@ 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();
+	idx = srcu_read_lock(&cbt_srcu);
 	if (list_empty(&q->cbt_list) || bio_data_dir(bio) == READ || !bio->bi_iter.bi_size)
-		goto out_rcu;
+		goto out_srcu;
 
-	list_for_each_entry_rcu(cbt, &q->cbt_list, list)
+	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();
+out_srcu:
+	srcu_read_unlock(&cbt_srcu, idx);
 }
 
 static struct cbt_info *do_cbt_alloc(struct request_queue *q, __u8 *name,
@@ -554,7 +556,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];
@@ -607,7 +609,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)
@@ -695,7 +697,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)
@@ -743,7 +745,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) {
@@ -774,7 +776,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 */
@@ -896,7 +898,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;
@@ -980,7 +982,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

[Devel] [PATCH hci-8.1 v5 5/6] 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        | 52 +++++++++++++++++++++++++++++++++++++++++++-------
 block/blk-core.c       |  5 ++++-
 block/blk-throttle.c   |  2 +-
 include/linux/blkdev.h |  7 +++++--
 4 files changed, 55 insertions(+), 11 deletions(-)

diff --git a/block/blk-cbt.c b/block/blk-cbt.c
index c87c7624f3800..7636602398d1d 100644
--- a/block/blk-cbt.c
+++ b/block/blk-cbt.c
@@ -230,26 +230,52 @@ 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 */
+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;
@@ -258,11 +284,17 @@ 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;
 
 	idx = srcu_read_lock(&cbt_srcu);
@@ -270,11 +302,17 @@ void blk_cbt_bio_queue(struct request_queue *q, struct bio *bio)
 		goto out_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;
+		}
+	}
 
 out_srcu:
 	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

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

diff --git a/block/blk-cbt.c b/block/blk-cbt.c
index 7636602398d1d..50aa0557eb987 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>
@@ -55,6 +56,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;
 };
@@ -230,6 +235,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_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) {
+				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);
+}
+
 /* Return true if all the map pages for [block, block + count) are there */
 static bool cbt_pages_present(struct cbt_info *cbt, blkcnt_t block,
 			      blkcnt_t count)
@@ -271,6 +352,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;
@@ -315,6 +398,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)
 {
@@ -337,8 +500,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:
@@ -720,6 +891,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 v5 1/6] 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-v5-0-88dccc74b48b@virtuozzo.com?part=1


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

Re: [Devel] [PATCH hci-8.1 v5 2/6] 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-v5-0-88dccc74b48b@virtuozzo.com?part=2


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

Re: [Devel] [PATCH hci-8.1 v5 4/6] 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.
> 
> 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-v5-0-88dccc74b48b@virtuozzo.com?part=4


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

Re: [Devel] [PATCH hci-8.1 v5 3/6] block/blk-cbt: don't overwrite a map page with CBT_PAGE_MISSED

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] blk-cbt: spinlock_t cbt->lock taken in on_each_cpu() IPI callback, invalid wait context
--

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

block/blk-cbt: don't overwrite a map page with CBT_PAGE_MISSED

This patch changes __blk_cbt_set() so that the CBT_PAGE_MISSED marker is stored under cbt->lock. If a map page was installed in the meantime, the bits are set in that page instead. The aim is to stop the cache flush from overwriting, and leaking, a page installed by cbt_page_alloc().

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

> diff --git a/block/blk-cbt.c b/block/blk-cbt.c
> index 1066b6a33b6b..01114362bb2b 100644
> --- a/block/blk-cbt.c
> +++ b/block/blk-cbt.c

[ ... ]

> @@ -194,10 +194,19 @@ static int __blk_cbt_set(struct cbt_info  *cbt, blkcnt_t block,
>  			block += len;
>  			continue;
>  		} else if (pages_missed) {
> +			bool installed;
> +
> +			/* Don't overwrite a page installed meanwhile */
> +			spin_lock_irqsave(&cbt->lock, flags);

[Severity: Medium]
Is it safe to take cbt->lock here? The pages_missed branch of __blk_cbt_set() is reached from the IPI callback:

cbt_flush_cache()
  on_each_cpu(__cbt_flush_cpu_cache, &ctx, 1)
    __cbt_flush_cpu_cache()
      __blk_cbt_set(..., &ctx->pages_missed, ...)

On remote CPUs this runs in hardirq context.

cbt->lock is declared in struct cbt_info as a plain spinlock_t, not a raw_spinlock_t. A spinlock_t is a sleeping lock on PREEMPT_RT. It is also an invalid wait context in hardirq under CONFIG_PROVE_RAW_LOCK_NESTING.

The previous code stored the marker without taking any lock. With this change, kernels built with CONFIG_PROVE_LOCKING and CONFIG_PROVE_RAW_LOCK_NESTING (as in the shipped debug config) could report an "Invalid wait context" splat whenever a flush finds a missing map page. On a PREEMPT_RT build this would be a sleeping lock taken in hardirq context.

Would it be better to make cbt->lock a raw_spinlock_t? Alternatively, the IPI path could install the marker lock-free, for example with cmpxchg(&cbt->map[idx], NULL, CBT_PAGE_MISSED).

> +			installed = CBT_PAGE(cbt, idx);
> +			if (!installed)
> +				cbt->map[idx] = CBT_PAGE_MISSED;
> +			spin_unlock_irqrestore(&cbt->lock, flags);
> +			if (installed)
> +				continue;
>  			(*pages_missed)++;
>  			if (!*idx_first)
>  				*idx_first = idx;
> -			cbt->map[idx] = CBT_PAGE_MISSED;
>  			count -= len;
>  			block += len;
>  			continue;

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

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

Re: [Devel] [PATCH hci-8.1 v5 5/6] block/blk-cbt: don't sleep in CBT for a REQ_NOWAIT write

[no-reply]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] block: blk-cbt: CBT_PAGE() double load lets nowait path see MISSED sentinel as a page
--

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

block/blk-cbt: don't sleep in CBT for a REQ_NOWAIT write

For a REQ_NOWAIT write, this patch makes blk_cbt_add() skip the per-cpu
extent cache and set the bits directly. It first checks with the new
cbt_pages_present() that every bitmap page exists. If one is missing,
submit_bio_noacct() fails the bio with BLK_STS_AGAIN. The throttle
dispatch work keeps the old, possibly sleeping, behaviour.

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

[ ... ]

> +/* Return true if all the map pages for [block, block + count) are there */
> +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;
> +}

[Severity: Medium]
Can this lockless check be fooled by a concurrent flush?

CBT_PAGE() expands to two plain loads of cbt->map[idx]:

    (cbt->map[idx] == CBT_PAGE_MISSED ? NULL : cbt->map[idx])

There is no READ_ONCE().  The writer is __blk_cbt_set(), reached from
cbt_flush_cache() with pages_missed set.  It stores the sentinel under
cbt->lock:

    if (!installed)
            cbt->map[idx] = CBT_PAGE_MISSED;

The reader here takes no lock, only cbt_srcu.  Suppose the first load in
CBT_PAGE() sees NULL and the flusher then stores CBT_PAGE_MISSED.  The
second load returns CBT_PAGE_MISSED, which is (struct page *)1.  The
compiler may also legally reload the slot.

In that case cbt_pages_present() treats a missing page as present.

[ ... ]

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

[Severity: Medium]
If cbt_pages_present() was fooled as described above, what happens in
this __blk_cbt_set() call with pages_missed == NULL?

__blk_cbt_set() reads the slot again through CBT_PAGE().  If it now sees
CBT_PAGE_MISSED, page is NULL and pages_missed is NULL, so it falls
through to cbt_page_alloc().  That allocates with GFP_NOIO and can sleep
in a REQ_NOWAIT submission, which is the case this patch is meant to
avoid.

If the double load tears again, page_address() and the page lock are
applied to (struct page *)1, which looks like it would oops.

The window is narrow and only exists while a flush is installing the
sentinel.  The same CBT_PAGE() race already existed in __blk_cbt_set().
This patch adds a new lockless reader and makes the no-sleep guarantee
depend on it being exact.

Would it make sense to load the slot once with READ_ONCE() in CBT_PAGE()
(or in cbt_pages_present()), and to make the nowait path in
__blk_cbt_set() fail instead of allocating when a page is missing?

The follow-up patch "block/blk-cbt: account a write request also when dm
starts it" does not change CBT_PAGE(), add READ_ONCE(), or add locking,
so the issue remains in the final series state.

[ ... ]

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

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

Re: [Devel] [PATCH hci-8.1 v5 6/6] 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:
- [High] block/cbt: cbt_set_present() double-reads cbt->map[idx] via CBT_PAGE() locklessly
- [Low] block/cbt: blk_cbt_add() queues page allocation under lock for every write bio
--

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

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

This patch makes request based dm call a new blk_cbt_rq_start() hook from
dm_mq_queue_rq(), so a write is accounted in CBT again when dm starts it.
The hook sets bits only in bitmap pages that already exist. Missing pages
are queued for allocation to kblockd, and dm returns BLK_STS_RESOURCE so
the request is dispatched again later.

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

[ ... ]

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

[ ... ]

> @@ -271,6 +352,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);

[Severity: Low]
This isn't a correctness problem, but is the per-bio cost here intended?

blk_cbt_add() now calls cbt_queue_alloc_pages() for every write bio before
the per-CPU extent cache fast path. While a bitmap page is still missing,
each such bio goes through cbt_queue_alloc():

    set_bit(idx, cbt->missing);
    spin_lock_irqsave(&cbt->lock, flags);
    cbt->count++;
    ...
    if (!kblockd_schedule_work(&cbt->alloc_work))
        cbt_put(cbt);

When the work is already pending, kblockd_schedule_work() returns 0 and
cbt_put() takes cbt->lock a second time.

Nothing dedupes the requests, for example by testing the bit with
test_and_set_bit() on cbt->missing before taking the lock. The window lasts
until the kblockd work installs the page, and that can be delayed because
the allocation is GFP_NOIO. Would it make sense to skip the locked
reference and queue_work() when the bit is already set?

[ ... ]

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

[ ... ]

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

[Severity: High]
Can this read of the page pointer race with __blk_cbt_set() and hand an
invalid page pointer to spin_lock_page()?

CBT_PAGE(cbt, idx) expands to:

    (cbt->map[idx] == CBT_PAGE_MISSED ? NULL : cbt->map[idx])

so it loads the slot twice, with no READ_ONCE() and no cbt->lock held. The
compiler is also free to reload it.

Concurrently, __blk_cbt_set() with pages_missed set (reached from the
BLKCBTSTART / CBT_SNAP_CREATE flush path) does this under cbt->lock, only
for a slot that was NULL:

    if (!installed)
        cbt->map[idx] = CBT_PAGE_MISSED;

One possible interleaving:

    reader: first load of cbt->map[idx] sees NULL, so not MISSED
    writer: stores 0x1 into cbt->map[idx]
    reader: second load returns 0x1

page is then the bogus non-NULL value 0x1, and trylock_page() and
page_address() in cbt_set_present() dereference it. cbt_set_present() never
takes cbt->lock, so nothing excludes the writer.

This runs from blk_cbt_rq_start() in the dm_mq_queue_rq() dispatch path and
would hit ordinary writers, during exactly the backup-point operations this
patch is meant to cover. Should the slot be read once into a local variable
with READ_ONCE() and then compared against CBT_PAGE_MISSED, or should the
lookup be done under cbt->lock?

[ ... ]

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

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