Re: [PATCH v2 1/3] md/raid10: fix still_degraded being inverted in raid10_sync_request()

Hi,

> I posted the same one-liner on 19 July -- flagging it only so it is not
> applied twice:
>
>   https://lore.kernel.org/linux-raid/[email protected]/
>
> No objection to yours going in, and I will drop mine if it does:
>
> Reviewed-by: Mykola Marzhan <[email protected]>
>
> Identical hunk and the same Fixes: tag.  Mine picked up Reviewed-by from
> Yu Kuai and Paul Menzel on that posting.
> Your changelog is the better of the two: it carries a measured
> reproducer where mine only reasons through the failure.

Thanks, and apologies for the duplication -- I missed your earlier
posting.  Yours was first on the list and already carries tags from
Kuai and Paul, so I am equally happy if the maintainers take your
copy instead.  If mine is picked up, I will carry your Reviewed-by
and a Link: to your posting should a v3 be needed.

> Your patch 3 and patch 2 of my 19 July series hit the same unit mismatch
> -- raid10's recovery cursor is in per-device space while its bitmap is
> indexed by array sectors -- but at different sites, and they do not
> overlap:
>
>   https://lore.kernel.org/linux-raid/[email protected]/
>
> Yours converts the bitmap-reported span into per-device sectors inside
> raid10_sync_request(); mine stops md_do_sync() handing
> bitmap_ops->skip_sync_blocks() an offset raid10 cannot interpret.  Both
> are still needed.  Yu Kuai has asked me to re-check mine against his
> llbitmap reshape series, so mine is still under discussion.

Agreed.

Your fix addresses the llbitmap mis-skip: md_do_sync() feeds the
per-device recovery cursor to skip_sync_blocks(), which llbitmap
indexes by array sectors, and once it answers for the wrong chunk
md_do_sync() takes "goto update" without ever entering
raid10_sync_request() -- so this has to be fixed at the caller, as
your patch does.

My patch 3 addresses the recovery crawl on a mostly-clean bitmap:
the classic bitmap does not implement skip_sync_blocks() at all, and
with your gate in place raid10 recovery no longer consults the
bitmap from md_do_sync() either way -- the layout-aware bulk skip
inside raid10_sync_request() then serves as raid10's fast path for
both bitmap flavours (it sits on start_sync(), which llbitmap also
implements).

The hunks are disjoint as well -- your series touches the
skip_sync_blocks() gate in md_do_sync() plus md.h/raid1/raid5, mine
touches raid10.c and a cond_resched() ~30 lines below your md.c hunk
-- so they apply cleanly in either order.

So both are needed, whichever order they land in.  I will follow
your discussion with Kuai on the llbitmap reshape series.

Thanks,
Yunye