Re: [PATCH v5 21/29] md/md-llbitmap: remap checkpointed bits as reshape progresses

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing read_lock(&llbitmap->reshape_lock) in background daemon and sysfs paths causes read-modify-write data races with reshape checkpointing.
- [High] llbitmap_rmerge_state() fails to handle BitCleanUnwritten and other unwritten sync states, incorrectly downgrading them to BitUnwritten.
- [High] The newly introduced reshape_mark feature to record checkpoint remaps is dead code as it is never invoked by the MD subsystem.
--

--- Patch [21]: [PATCH v5 21/29] md/md-llbitmap: remap checkpointed bits as reshape progresses ---
commit f911f407605f9d24ba29497958177e6d0f9e041f
Author: Yu Kuai <yukuai@fygo.io>
Subject: md/md-llbitmap: remap checkpointed bits as reshape progresses

This commit introduces a mechanism to merge checkpointed old llbitmap state
forward as reshape_position advances. It synchronizes reshape checkpoint
remapping against normal I/O bitmap updates using a read-write lock to avoid
data loss during concurrent read/modify/write paths.

> diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
> index a20e55fdf82b0..5d95627ff9830 100644
> --- a/drivers/md/md-llbitmap.c
> +++ b/drivers/md/md-llbitmap.c

[ ... ]

> @@ -930,6 +943,33 @@ static int llbitmap_prepare_resize(struct llbitmap *llbitmap,
>  	return 0;
>  }
>  
> +static enum llbitmap_state
> +llbitmap_rmerge_state(struct llbitmap *llbitmap,
> +		      enum llbitmap_state dst,
> +		      enum llbitmap_state src)
> +{
> +	bool level_456 = raid_is_456(llbitmap->mddev);
> +
> +	if (dst == BitNeedSync || dst == BitSyncing ||
> +	    src == BitNeedSync || src == BitSyncing)
> +		return BitNeedSync;
> +
> +	if (dst == BitDirty || src == BitDirty)
> +		return BitDirty;
> +
> +	/*
> +	 * Reshape generates valid target parity/data for both already-written
> +	 * and not-yet-written regions in the checkpointed range, so a mix of
> +	 * clean and unwritten still results in a clean destination bit.
> +	 */
> +	if (level_456 && ((dst == BitClean && src == BitUnwritten) ||
> +			  (src == BitClean && dst == BitUnwritten)))
> +		return BitClean;
> +	if (dst == BitClean || src == BitClean)
> +		return BitClean;
> +	return BitUnwritten;
> +}

[Severity: High]
Does this logic handle unwritten sync states like BitCleanUnwritten?

If a chunk in BitCleanUnwritten, BitNeedSyncUnwritten, or
BitSyncingUnwritten is processed during a reshape on a RAID4/5/6 array,
the state evaluates through these checks and returns BitUnwritten.

Can this cause the array to lose track of valid parity or pending proactive
syncs, potentially triggering a full read-modify-write lazy recovery on the
next write, or leaving unwritten regions permanently unsynced?

[ ... ]

> @@ -1306,6 +1346,7 @@ static void md_llbitmap_daemon_fn(struct work_struct *work)
>  
>  	if (llbitmap->mddev->degraded)
>  		return;
> +
>  retry:
>  	start = 0;
>  	end = min(llbitmap->chunks, PAGE_SIZE - BITMAP_DATA_OFFSET) - 1;

[Severity: High]
Are there missing read locks for the reshape_lock in the background daemon
and sysfs paths?

The reshape_mark() function explicitly bypasses page suspension and uses
write_lock(&llbitmap->reshape_lock). However, the background daemon in
md_llbitmap_daemon_fn() calls llbitmap_state_machine() without acquiring
the read lock:

    llbitmap_state_machine(llbitmap, start, end, BitmapActionDaemon);

Similarly, sysfs writes via llbitmap_dirty_bits() call it without the lock:

    llbitmap_state_machine(mddev->bitmap, s, e, BitmapActionStartwrite);

Could this result in read-modify-write data races? For example, if reshape
copies a BitNeedSync state and writes it, while the background daemon
concurrently evaluates a BitDirty to BitClean transition for the same chunk,
the daemon's write could overwrite the reshape's write and lose the
BitNeedSync state.

[ ... ]

> @@ -2181,6 +2384,7 @@ static struct bitmap_operations llbitmap_ops = {
>  	.prepare_range		= llbitmap_prepare_range,
>  	.reshape_finish		= llbitmap_reshape_finish,
>  	.reshape_can_start	= llbitmap_reshape_can_start,
> +	.reshape_mark		= llbitmap_reshape_mark,
>  	.write_all		= llbitmap_write_all,
>  
>  	.groups			= md_llbitmap_groups,

[Severity: High]
Is the new llbitmap_reshape_mark() function dead code?

Although the reshape_mark operation is implemented and wired up in
struct bitmap_operations, it appears there are no callers in the MD core
subsystem (such as in md.c or raid5.c) that invoke
mddev->bitmap_ops->reshape_mark().

If the system crashes during reshape, could this missing hook prevent the
bitmap state from being correctly recovered because the remaps were never
recorded?

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