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

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 an=
d sysfs paths causes read-modify-write data races with reshape checkpointin=
g.
- [High] llbitmap_rmerge_state() fails to handle BitCleanUnwritten and othe=
r unwritten sync states, incorrectly downgrading them to BitUnwritten.
- [High] The newly introduced reshape_mark feature to record checkpoint rem=
aps 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 <[email protected]>
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;
>  }
> =20
> +static enum llbitmap_state
> +llbitmap_rmerge_state(struct llbitmap *llbitmap,
> +		      enum llbitmap_state dst,
> +		      enum llbitmap_state src)
> +{
> +	bool level_456 =3D raid_is_456(llbitmap->mddev);
> +
> +	if (dst =3D=3D BitNeedSync || dst =3D=3D BitSyncing ||
> +	    src =3D=3D BitNeedSync || src =3D=3D BitSyncing)
> +		return BitNeedSync;
> +
> +	if (dst =3D=3D BitDirty || src =3D=3D 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 =3D=3D BitClean && src =3D=3D BitUnwritten) ||
> +			  (src =3D=3D BitClean && dst =3D=3D BitUnwritten)))
> +		return BitClean;
> +	if (dst =3D=3D BitClean || src =3D=3D 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_struc=
t *work)
> =20
>  	if (llbitmap->mddev->degraded)
>  		return;
> +
>  retry:
>  	start =3D 0;
>  	end =3D 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 =3D {
>  	.prepare_range		=3D llbitmap_prepare_range,
>  	.reshape_finish		=3D llbitmap_reshape_finish,
>  	.reshape_can_start	=3D llbitmap_reshape_can_start,
> +	.reshape_mark		=3D llbitmap_reshape_mark,
>  	.write_all		=3D llbitmap_write_all,
> =20
>  	.groups			=3D 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?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260802195038.1642=
[email protected]?part=3D21