Re: [PATCH v5 20/29] md/md-llbitmap: don't skip reshape ranges from bitmap state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but a lockless race condition in `llbitmap_set_page_dirty` causes dirty bitmap blocks to be lost in memory and never flushed to disk.
- [High] This problem wasn't introduced by this patch, but integer truncation of `blocks` can cause an infinite loop in the `md_do_sync` thread.
--

--- Patch [20]: [PATCH v5 20/29] md/md-llbitmap: don't skip reshape ranges from bitmap state ---
commit c6db76f6bc970107f68d37fc602584cc2c0e0a1b
Author: Yu Kuai <yukuai@fygo.io>

md/md-llbitmap: don't skip reshape ranges from bitmap state

This commit modifies md-llbitmap to stop using stale bitmap state for
skipping reshape ranges. It ensures that reshape progress is tracked purely
by array metadata. This prevents data loss after a restart before checkpoints
are persisted to disk.

Tested-by: Mykola Marzhan <mykola@meshstor.io>

> diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
> index fa16a4224c45a..a20e55fdf82b0 100644
> --- a/drivers/md/md-llbitmap.c
> +++ b/drivers/md/md-llbitmap.c
> @@ -1707,6 +1707,14 @@ static sector_t llbitmap_skip_sync_blocks(struct mddev *mddev, sector_t offset)

[Severity: High]
This isn't a bug introduced by this patch, but can integer truncation of
blocks cause an infinite loop in the md_do_sync() thread?

In llbitmap_skip_sync_blocks(), blocks is declared as an int:

    int blocks = llbitmap->chunksize - (offset & (llbitmap->chunksize - 1));

If the array size and bitmap space require chunksize to exceed INT_MAX (e.g.,
a >8TB array with a small bitmap space), chunksize will be >= 2^31. Can the
subtraction yield a value >= 2^31, which truncates into a negative 32-bit int
(e.g., -1024)?

When this negative int is returned as a 64-bit sector_t, does it sign-extend
to a massive positive value? If md_do_sync() adds this value to the current
sync sector index, could it cause the index to overflow and decrease, looping
endlessly back and forth?

>  		return 0;
>  	c = llbitmap_read(llbitmap, p);
>  
> +	/*
> +	 * Reshape progress is tracked by array metadata rather than llbitmap.
> +	 * Skipping reshape ranges from stale bitmap state can lose data after a
> +	 * restart before the corresponding bits are checkpointed to disk.
> +	 */
> +	if (test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery))
> +		return 0;
> +
>  	/* always skip unwritten blocks */
>  	if (c == BitUnwritten)
>  		return blocks;

[Severity: Critical]
This is a pre-existing issue, but a lockless race condition in
llbitmap_set_page_dirty() can cause dirty bitmap blocks to be lost in memory
and never flushed to disk.

Can this sequence actually occur? If llbitmap_set_page_dirty() is called
concurrently with the llbitmap_unplug_fn() flusher thread, the writer sets
the LLPageDirty flag before setting the specific block bit:

drivers/md/md-llbitmap.c:llbitmap_set_page_dirty() {
    ...
    if (!test_bit(LLPageDirty, &pctl->flags))
        set_bit(LLPageDirty, &pctl->flags);
    
    // race window here
    
    set_bit(block, pctl->dirty);
    ...
}

If the flusher thread clears LLPageDirty and checks pctl->dirty between these
two operations:

drivers/md/md-llbitmap.c:llbitmap_unplug_fn() {
    ...
    if (!test_bit(LLPageDirty, &llbitmap->pctl[i]->flags) ||
        !test_and_clear_bit(LLPageDirty, &llbitmap->pctl[i]->flags))
        continue;
    ...
}

Will it miss the dirty block? The writer then sets the block bit, but
because LLPageDirty is now 0, the flusher will ignore this page indefinitely
until another block happens to be dirtied.

If a dirty block is not flushed to the disk bitmap, will the array fail to
resync that block after a crash or unclean shutdown, leaving stale data on
one of the drives?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260802195038.164272-1-yukuai@kernel.org?part=20