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