Re: [PATCH v2 12/20] md/md-llbitmap: remap checkpointed bits as reshape progresses

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] In-place array modification during reshape overwrites un-reshaped source data before it can be read.
- [High] Non-atomic read-modify-write cycle in `llbitmap_reshape_merge` races with concurrent normal I/O, dropping `BitDirty` flags.
- [High] Missing handling for `BitNeedSyncUnwritten` and `BitCleanUnwritten` in `llbitmap_rmerge_state` silently demotes them.
- [High] The `level_456` condition in `llbitmap_rmerge_state` is completely ineffective and subsumed by the subsequent fallback.
--

commit a1307817c41ef916bb59f41a3af5b2d7a709caa2
Author: Yu Kuai <[email protected]>

md/md-llbitmap: remap checkpointed bits as reshape progresses

Merge checkpointed old llbitmap state forward as reshape_position
advances and record the checkpoint remap through reshape_mark().

> diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
> index c62397743d9a0..00b77ecd600be 100644
> --- a/drivers/md/md-llbitmap.c
> +++ b/drivers/md/md-llbitmap.c

[ ... ]

> @@ -906,6 +914,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;

[Severity: High]
Are BitNeedSyncUnwritten and BitCleanUnwritten states meant to be handled
here?

Because they do not match BitNeedSync, BitSyncing, BitDirty, or BitClean,
they fall through these conditions. If paired with BitClean, they
incorrectly evaluate to BitClean; otherwise, they fall through to
BitUnwritten. Both outcomes silently drop the sync requirement.

> +
> +	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;

[Severity: High]
Does the fallback condition "dst == BitClean || src == BitClean" render the
preceding level_456 check dead code?

The fallback unconditionally forces any BitClean mix to BitClean, which
entirely overrides the level_456 restriction. Could this lead to RAID10
incorrectly resolving mixed BitClean and BitUnwritten states to BitClean,
causing skipped resync operations?

> +	return BitUnwritten;
> +}

[ ... ]

> +static void llbitmap_reshape_merge(struct llbitmap *llbitmap,
> +				   const struct llbitmap_reshape_range *old,
> +				   const struct llbitmap_reshape_range *new)
> +{
> +	unsigned long dst_start;
> +	unsigned long dst_end;
> +	unsigned long dst;
> +
> +	if (!new->sectors)
> +		return;
> +
> +	dst_start = div64_u64(new->offset, llbitmap->reshape_chunksize);
> +	dst_end = div64_u64(new->end - 1, llbitmap->reshape_chunksize);
> +
> +	for (dst = dst_start; dst <= dst_end; dst++) {
> +		struct llbitmap_reshape_range dst_range;
> +		struct llbitmap_reshape_range src;
> +		enum llbitmap_state state;
> +
> +		llbitmap_reshape_dst_range(llbitmap, dst, new, &dst_range);
> +		state = llbitmap_reshape_init_dst(llbitmap, dst, new);

[Severity: High]
Does this non-atomic read-modify-write cycle race with concurrent normal I/O?

llbitmap_reshape_init_dst() reads the chunk state, computes the merged state
through llbitmap_rmerge_src(), and writes it back via llbitmap_write() below
without being protected by locks.

Concurrent normal I/O operating on stripes outside the reshape window but
within the same bitmap chunk can set BitDirty. The llbitmap_reshape_merge()
thread will blindly overwrite this BitDirty flag with its locally computed
state, potentially causing silent data corruption due to lost BitDirty
flags.

> +		if (llbitmap_reshape_src_range(old, new, &dst_range, &src))
> +			state = llbitmap_rmerge_src(llbitmap, state, &src);
> +		else
> +			state = llbitmap_rmerge_state(llbitmap, state, BitUnwritten);
> +		llbitmap_write(llbitmap, state, dst);

[Severity: Critical]
Does writing directly to the shared pctl array overwrite un-reshaped source
data before it can be read?

When adding disks to a RAID5 array, new->sectors > old->sectors. This causes
the forward iteration of dst to outpace bit in llbitmap_rmerge_src()
(e.g., dst = 10, bit = 5).

Because llbitmap_write() updates the array in place, the state at index 5
is overwritten before it is read for dst = 10, destroying the original chunk
state.

> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12