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

[no-reply at sashiko.virtuozzo.com]

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