Re: [Devel] [PATCH hci-8.1 v2] 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:
- [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