Re: [PATCH 2/3] md: only consult skip_sync_blocks when 'j' is in the bitmap's domain

[yu kuai]

Hi,

在 2026/7/19 22:42, Mykola Marzhan 写道:
> Rebuilding a raid10 member with llbitmap can complete in a few
> hundred milliseconds having copied nothing: md_do_sync() offers
> every position 'j' to bitmap_ops->skip_sync_blocks(), the bitmap
> answers for the wrong chunks, the whole rebuild is skipped as
> "unwritten", and the array reports optimal with a stale member.  The
> staleness is typically noticed only when the mirror partner fails;
> writes made while the array was degraded vanish.
>
> The bitmap spans [0, resync_max_sectors) in the personality's
> sync-cursor space.  Resync walks exactly that range for every
> personality and may always consult the bitmap.  Recovery walks
> per-device offsets, which match the bitmap's space only for raid1
> (device space is array space) and raid456 (the bitmap is indexed by
> the per-device sync cursor; bitmap_sector() translates array IO
> into it).  raid10's bitmap is in array space -- near-2 on four
> disks spans twice a member's size; far-2 on two disks puts array
> chunk 2c+d at device chunk c of disk d -- so
> llbitmap_skip_sync_blocks() was consulted with offsets it cannot
> interpret.
>
> Only call the hook when the offsets match what the bitmap expects.
> Reshape must visit every stripe regardless of bitmap state --
> skipping would desync reshape_position from 'j' -- so never skip
> there.  For recovery, add an md_personality capability,
> recovery_in_bitmap_space, set by raid1 and raid456.  raid10 leaves
> it unset (raid10_find_virt() needs the disk number, which
> md_do_sync() does not have); raid0 and linear too, but no recovery
> path of theirs reaches the hook.  No performance is lost against any
> released kernel: the hook is llbitmap-only and new in v6.18, so
> raid10 recovery returns to its earlier path -- walk every stripe,
> skip clean chunks inside raid10_sync_request().
>
> A capability rather than a geometry test: for raid10 layouts with
> raid_disks == near_copies * far_copies, resync_max_sectors equals
> dev_sectors while the spaces can still differ (far and offset
> variants), so an arithmetic proxy reopens the same hole on those
> layouts.  Disabling the hook for all recovery instead would
> forfeit llbitmap's fast-forward over unwritten chunks for raid1 and
> raid456 rebuilds.  Opt-in is also the safe default: a new
> personality gets no bitmap consultation during recovery until it
> declares otherwise.
>
> Verified for near-2 and far-2 layouts: a rebuild onto an added spare
> copies the writes made while the array was degraded.
>
> Fixes: f196d7288864 ("md/md-bitmap: add a new method skip_sync_blocks() in bitmap_operations")
> Cc: stable@xxxxxxxxxxxxxxx # v6.18+
> Assisted-by: Claude-Code:claude-opus-4-8
> Signed-off-by: Mykola Marzhan <mykola@xxxxxxxxxxx>
> ---
>   drivers/md/md.c    | 13 ++++++++++++-
>   drivers/md/md.h    | 11 +++++++++++
>   drivers/md/raid1.c |  1 +
>   drivers/md/raid5.c |  3 +++
>   4 files changed, 27 insertions(+), 1 deletion(-)

There is already a patchset to support llbitmap reshape for raid10 and raid5:

[PATCH v2 00/20] md/md-llbitmap: support reshape for RAID10 and RAID5 - 
Yu Kuai <https://lore.kernel.org/linux-raid/cover.1782282042.git.yukuai@xxxxxxxxxx/>

Before this set, reshape for raid10 or raid5 is not safe. Kernel panic or data lost
is observed. Please check if your problems still exist with this set.

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index d1465bcd86c8..d60ea7aaca3a 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -9846,7 +9846,18 @@ void md_do_sync(struct md_thread *thread)
>   		if (test_bit(MD_RECOVERY_INTR, &mddev->recovery))
>   			break;
>   
> -		if (mddev->bitmap_ops && mddev->bitmap_ops->skip_sync_blocks) {
> +		/*
> +		 * The bitmap may be consulted with 'j' for a plain resync,
> +		 * but for recovery only when the personality declares that
> +		 * its recovery cursor addresses the bitmap's space; raid10
> +		 * recovery walks per-device offsets the bitmap cannot
> +		 * interpret.  Reshape must relocate every stripe regardless
> +		 * of bitmap state, so never skip there either.
> +		 */
> +		if (mddev->bitmap_ops && mddev->bitmap_ops->skip_sync_blocks &&
> +		    !test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery) &&
> +		    (!test_bit(MD_RECOVERY_RECOVER, &mddev->recovery) ||
> +		     mddev->pers->recovery_in_bitmap_space)) {
>   			sectors = mddev->bitmap_ops->skip_sync_blocks(mddev, j);
>   			if (sectors)
>   				goto update;
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index d8daf0f75cbb..a490a47736b0 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -798,6 +798,17 @@ struct md_personality
>   	/* convert io ranges from array to bitmap */
>   	void (*bitmap_sector)(struct mddev *mddev, sector_t *offset,
>   			      unsigned long *sectors);
> +	/*
> +	 * The position md_do_sync() walks during MD_RECOVERY_RECOVER
> +	 * addresses the space the write-intent bitmap is indexed by, so
> +	 * recovery may consult bitmap_ops->skip_sync_blocks() directly.
> +	 * True for raid1 (device space is array space) and raid456 (the
> +	 * bitmap is indexed by the per-device sync cursor).  raid10 must
> +	 * leave this unset: its recovery walks per-device offsets while
> +	 * its bitmap is indexed by array sectors, and translating needs
> +	 * the disk number, which md_do_sync() does not have.
> +	 */
> +	bool recovery_in_bitmap_space;
>   };
>   
>   struct md_sysfs_entry {
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index afe2ca96ad8c..1222c0470ab6 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -3518,6 +3518,7 @@ static struct md_personality raid1_personality =
>   	.check_reshape	= raid1_reshape,
>   	.quiesce	= raid1_quiesce,
>   	.takeover	= raid1_takeover,
> +	.recovery_in_bitmap_space = true,
>   };
>   
>   static int __init raid1_init(void)
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index ffb5fcde54a9..25f8d829a986 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -9078,6 +9078,7 @@ static struct md_personality raid6_personality =
>   	.change_consistency_policy = raid5_change_consistency_policy,
>   	.prepare_suspend = raid5_prepare_suspend,
>   	.bitmap_sector	= raid5_bitmap_sector,
> +	.recovery_in_bitmap_space = true,
>   };
>   static struct md_personality raid5_personality =
>   {
> @@ -9108,6 +9109,7 @@ static struct md_personality raid5_personality =
>   	.change_consistency_policy = raid5_change_consistency_policy,
>   	.prepare_suspend = raid5_prepare_suspend,
>   	.bitmap_sector	= raid5_bitmap_sector,
> +	.recovery_in_bitmap_space = true,
>   };
>   
>   static struct md_personality raid4_personality =
> @@ -9139,6 +9141,7 @@ static struct md_personality raid4_personality =
>   	.change_consistency_policy = raid5_change_consistency_policy,
>   	.prepare_suspend = raid5_prepare_suspend,
>   	.bitmap_sector	= raid5_bitmap_sector,
> +	.recovery_in_bitmap_space = true,
>   };
>   
>   static int __init raid5_init(void)

-- 
Thanks,
Kuai