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

[no-reply@sashiko.virtuozzo.com]

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