* Re: [PATCH v4 6/9] md/raid1,raid10: keep REQ_NOMERGE on narrow_write_error() retry clones
From: sashiko-bot @ 2026-07-22 19:16 UTC (permalink / raw)
To: Mykola Marzhan; +Cc: linux-raid, linux-pci, yukuai
In-Reply-To: <20260722185841.449934-7-mykola@meshstor.io>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] narrow_write_error() incorrectly coerces dataless operations into data-bearing REQ_OP_WRITE states, causing block layer crashes.
--
--- Patch [6]: [PATCH v4 6/9] md/raid1,raid10: keep REQ_NOMERGE on narrow_write_error() retry clones ---
commit 4017c1a8ef243dcaefd606ee7c22696e4aaccaa1
Author: Mykola Marzhan <mykola@meshstor.io>
md/raid1,raid10: keep REQ_NOMERGE on narrow_write_error() retry clones
This commit fixes a missing REQ_NOMERGE flag on P2PDMA retry clones in
narrow_write_error(). It ensures that member queues do not merge P2PDMA
segments incorrectly when a bio is retried after a write error.
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -2561,6 +2561,9 @@ static void narrow_write_error(struct r1bio *r1_bio, int i)
> }
>
> wbio->bi_opf = REQ_OP_WRITE;
> + /* Keep P2PDMA retry bios unmergeable, like the original */
> + if (md_bio_is_p2pdma(wbio))
> + wbio->bi_opf |= REQ_NOMERGE;
[Severity: High]
This is a pre-existing issue, but does setting wbio->bi_opf unconditionally to
REQ_OP_WRITE here cause a NULL pointer dereference for dataless requests?
If the array is processing a REQ_OP_SECURE_ERASE request (which has bi_size > 0
but bi_io_vec is NULL) and hits a write error, narrow_write_error() will force
the operation to REQ_OP_WRITE. Because REQ_OP_WRITE expects a data vector, the
subsequent call to bio_trim() in this loop will attempt to advance a NULL
bi_io_vec during chunk iteration, resulting in a crash.
> wbio->bi_iter.bi_sector = r1_bio->sector;
> wbio->bi_iter.bi_size = r1_bio->sectors << 9;
[ ... ]
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -2823,6 +2823,9 @@ static void narrow_write_error(struct r10bio *r10_bio, int i)
> wbio->bi_iter.bi_sector = wsector +
> choose_data_offset(r10_bio, rdev);
> wbio->bi_opf = REQ_OP_WRITE;
> + /* Keep P2PDMA retry bios unmergeable, like the original */
> + if (md_bio_is_p2pdma(wbio))
> + wbio->bi_opf |= REQ_NOMERGE;
[Severity: High]
This is a pre-existing issue, but similar to the raid1.c case above, could
this coercion to REQ_OP_WRITE crash lower-level drivers?
If a REQ_OP_SECURE_ERASE request hits a write error and is retried here,
wbio->bi_opf is overridden to REQ_OP_WRITE before calling submit_bio_wait().
When this malformed write bio with a NULL bi_io_vec is sent to lower layers
(like NVMe or SCSI), it can cause a panic during DMA mapping since those
drivers expect a valid bvec for REQ_OP_WRITE.
>
> if (submit_bio_wait(wbio) &&
> !rdev_set_badblocks(rdev, wsector, sectors, 0)) {
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260722185841.449934-1-mykola@meshstor.io?part=6
^ permalink raw reply
* Re: [PATCH v4 7/9] md/raid1,raid10: skip futile retries on P2PDMA mapping failures
From: Logan Gunthorpe @ 2026-07-22 19:18 UTC (permalink / raw)
To: Mykola Marzhan, Jens Axboe, Song Liu, Yu Kuai, Keith Busch,
Christoph Hellwig, Sagi Grimberg, linux-block, linux-raid,
linux-nvme
Cc: Li Nan, Xiao Ni, Guoqing Jiang, Leon Romanovsky, Jason Gunthorpe,
Kiran Kumar Modukuri, Chaitanya Kulkarni, Bjorn Helgaas,
Shivaji Kant, Pranjal Shrivastava, Henrique Carvalho,
linux-kernel, linux-rdma, linux-pci
In-Reply-To: <20260722185841.449934-8-mykola@meshstor.io>
On 2026-07-22 12:58, Mykola Marzhan wrote:
> From: Logan Gunthorpe <logang@deltatee.com>
>
> BLK_STS_P2PDMA reflects the peer/member pairing, not member health:
> retrying the same pages on the same member cannot succeed. Keep
> WriteErrorSeen so the range stays fenced, but skip WantReplacement and
> FailFast eviction, as commit f7b24c7b41f2 ("md/raid1,raid10: don't fail
> devices for invalid IO errors") did for BLK_STS_INVAL.
>
> The failure is whole-range and deterministic, so narrow_write_error()'s
> block-by-block submit_bio_wait() retries are pointless. Record the
> whole range as a bad block in the completion handler instead.
>
> Fixes: 02666132403a ("md: propagate BLK_FEAT_PCI_P2PDMA from member devices to RAID device")
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> Co-developed-by: Mykola Marzhan <mykola@meshstor.io>
> Signed-off-by: Mykola Marzhan <mykola@meshstor.io>
> ---
> drivers/md/raid1-10.c | 3 +++
> drivers/md/raid1.c | 12 +++++++++---
> drivers/md/raid10.c | 8 +++++++-
> 3 files changed, 19 insertions(+), 4 deletions(-)
Maybe drop my authorship from this patch. I only did the first hunk and
don't fully understand the bulk of it.
> diff --git a/drivers/md/raid1-10.c b/drivers/md/raid1-10.c
> index cee21452e066..89daf8558251 100644
> --- a/drivers/md/raid1-10.c
> +++ b/drivers/md/raid1-10.c
> @@ -309,6 +309,9 @@ static inline void raid1_write_error(struct mddev *mddev, struct md_rdev *rdev,
> {
> set_bit(WriteErrorSeen, &rdev->flags);
>
> + if (bio->bi_status == BLK_STS_P2PDMA)
> + return;
> +
> if (!test_and_set_bit(WantReplacement, &rdev->flags))
> set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 19c4dec450de..917d694ef401 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -2624,10 +2624,16 @@ static void handle_write_finished(struct r1conf *conf, struct r1bio *r1_bio)
> * narrow down and record precise write
> * errors.
> */
> + struct md_rdev *rdev = conf->mirrors[m].rdev;
> +
> fail = true;
> - narrow_write_error(r1_bio, m);
> - rdev_dec_pending(conf->mirrors[m].rdev,
> - conf->mddev);
> + /* Mapping failures are whole-range and deterministic */
I'm not sure I follow the point of this new comment here.
Thanks,
Logan
^ permalink raw reply
* Re: [PATCH v4 8/9] md/raid1,raid10: set IO_BLOCKED in case of BLK_STS_P2PDMA
From: Logan Gunthorpe @ 2026-07-22 19:19 UTC (permalink / raw)
To: Mykola Marzhan, Jens Axboe, Song Liu, Yu Kuai, Keith Busch,
Christoph Hellwig, Sagi Grimberg, linux-block, linux-raid,
linux-nvme
Cc: Li Nan, Xiao Ni, Guoqing Jiang, Leon Romanovsky, Jason Gunthorpe,
Kiran Kumar Modukuri, Chaitanya Kulkarni, Bjorn Helgaas,
Shivaji Kant, Pranjal Shrivastava, Henrique Carvalho,
linux-kernel, linux-rdma, linux-pci
In-Reply-To: <20260722185841.449934-9-mykola@meshstor.io>
On 2026-07-22 12:58, Mykola Marzhan wrote:
> From: Logan Gunthorpe <logang@deltatee.com>
>
> A read that fails with BLK_STS_P2PDMA cannot succeed against that
> member, so mark the leg IO_BLOCKED and let the retry redirect elsewhere.
> Skip the read-error machinery: there is nothing on the medium to fix,
> fix_read_error()'s probe reads into host pages and would "succeed", and
> charging the read-error budget would evict a healthy member under a P2P
> read workload. FailFast eviction is skipped for the same reason -- the
> request never reached the wire.
>
> Fixes: 02666132403a ("md: propagate BLK_FEAT_PCI_P2PDMA from member devices to RAID device")
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> Co-developed-by: Mykola Marzhan <mykola@meshstor.io>
> Signed-off-by: Mykola Marzhan <mykola@meshstor.io>
> ---
> drivers/md/raid1.c | 4 +++-
> drivers/md/raid10.c | 5 ++++-
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 917d694ef401..76a1426e64f0 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -2658,6 +2658,8 @@ static void handle_read_error(struct r1conf *conf, struct r1bio *r1_bio)
> {
> struct md_rdev *rdev = conf->mirrors[r1_bio->read_disk].rdev;
> struct bio *bio = r1_bio->bios[r1_bio->read_disk];
> + /* evaluate before the bio_put() below */
> + bool p2pdma_error = bio->bi_status == BLK_STS_P2PDMA;
> struct mddev *mddev = conf->mddev;
> sector_t sector;
>
> @@ -2675,7 +2677,7 @@ static void handle_read_error(struct r1conf *conf, struct r1bio *r1_bio)
> * read error. This is all done synchronously while the array is
> * frozen.
> */
> - if (mddev->ro) {
> + if (mddev->ro || p2pdma_error) {
> r1_bio->bios[r1_bio->read_disk] = IO_BLOCKED;
> } else if (test_bit(FailFast, &rdev->flags)) {
> md_error(mddev, rdev);
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 9045a3f02dae..e35aeac29f06 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -2848,6 +2848,7 @@ static void handle_read_error(struct mddev *mddev, struct r10bio *r10_bio)
> {
> int slot = r10_bio->read_slot;
> struct bio *bio;
> + bool p2pdma_error;
> struct r10conf *conf = mddev->private;
> struct md_rdev *rdev = r10_bio->devs[slot].rdev;
>
> @@ -2860,10 +2861,12 @@ static void handle_read_error(struct mddev *mddev, struct r10bio *r10_bio)
> * frozen.
> */
> bio = r10_bio->devs[slot].bio;
> + /* evaluate before the bio_put() below */
> + p2pdma_error = bio->bi_status == BLK_STS_P2PDMA;
I see why we needed to add the p2pdma_error variable now. I guess I
missed that. But can we maybe do both raid10 and raid1 the same even if
it means moving the `bio =` line up a bit? I think it would be better to
move the code more towards similarity with raid1 instead of diverging it
further. Maybe someday it can be cleaned up into more common code and
that will be easier if we make both changes the same.
Thanks,
Logan
^ permalink raw reply
* Re: [PATCH v4 9/9] nvme-rdma: return BLK_STS_P2PDMA for unsupported P2P transfers
From: sashiko-bot @ 2026-07-22 19:20 UTC (permalink / raw)
To: Mykola Marzhan; +Cc: yukuai, linux-pci, linux-raid
In-Reply-To: <20260722185841.449934-10-mykola@meshstor.io>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] Calling nvme_start_request() after mapping the data for DMA causes stale metadata to be read by the RDMA device on non-coherent architectures.
Pre-existing issues:
- [Critical] Accessing `rq` and `req->sqe.dma` in the error path after `nvme_host_path_error(rq)` completes and frees the request.
- [High] Returning BLK_STS_IOERR or BLK_STS_P2PDMA directly to the block layer after nvme_start_request() has been called leaks the multipath nr_active counter and I/O stats.
--
--- Patch [9]: [PATCH v4 9/9] nvme-rdma: return BLK_STS_P2PDMA for unsupported P2P transfers ---
commit 53acbc82938f48638ba2fe9095582a69fb22439e
Author: Mykola Marzhan <mykola@meshstor.io>
nvme-rdma: return BLK_STS_P2PDMA for unsupported P2P transfers
When the PCIe topology cannot route a P2P transfer between two devices,
the DMA layer fails the mapping with -EREMOTEIO, but ib_dma_map_sg()
returns 0 and the error is lost. nvme-rdma then fails the I/O with a
retryable status, so multipath requeues it forever and a single path
wastes its whole retry budget on an I/O that can never succeed.
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 6909e35427942..2f50509a7a616 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
[ ... ]
> @@ -2026,8 +2029,6 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
> if (ret)
> goto unmap_qe;
>
> - nvme_start_request(rq);
> -
> if (IS_ENABLED(CONFIG_BLK_DEV_INTEGRITY) &&
> queue->pi_support &&
> (c->common.opcode == nvme_cmd_write ||
> @@ -2039,11 +2040,13 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
>
> err = nvme_rdma_map_data(queue, rq, c);
> if (unlikely(err < 0)) {
> - dev_err(queue->ctrl->ctrl.device,
> - "Failed to map data (%d)\n", err);
> + dev_err_ratelimited(queue->ctrl->ctrl.device,
> + "Failed to map data (%d)\n", err);
> goto err;
> }
>
> + nvme_start_request(rq);
> +
[Severity: High]
Does moving nvme_start_request() here cause stale metadata to be read on
non-coherent architectures?
nvme_start_request() invokes blk_integrity_prepare(), which writes to the PI
metadata buffer via the CPU. Since this now happens after the metadata has
already been mapped for DMA in nvme_rdma_map_data(), these CPU writes won't
be flushed to main memory before the RDMA device accesses them.
> sqe->cqe.done = nvme_rdma_send_done;
>
> ib_dma_sync_single_for_device(dev, sqe->dma,
> @@ -2063,6 +2066,9 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
> ret = nvme_host_path_error(rq);
[Severity: Critical]
This is a pre-existing issue, but does this lead to a use-after-free?
nvme_host_path_error() synchronously completes the request via
__nvme_complete_rq(), freeing it back to the block layer where it can be
immediately reallocated.
The error path then continues to dereference rq and req->sqe.dma:
nvme_cleanup_cmd(rq);
unmap_qe:
ib_dma_unmap_single(dev, req->sqe.dma, sizeof(struct nvme_command),
DMA_TO_DEVICE);
Could this corrupt the newly reallocated request if it's reused by another CPU?
> else if (err == -ENOMEM || err == -EAGAIN)
> ret = BLK_STS_RESOURCE;
> + /* Peer memory unreachable from this device: don't retry. */
> + else if (err == -EREMOTEIO)
> + ret = BLK_STS_P2PDMA;
> else
> ret = BLK_STS_IOERR;
> nvme_cleanup_cmd(rq);
[Severity: High]
This isn't a bug introduced by this patch, but does returning an error status
directly to the block layer after nvme_start_request() leak multipath
counters?
When nvme_start_request() is executed, it increments the queue depth counter
(nr_active) and starts I/O accounting via nvme_mpath_start_request().
If the driver then returns an error status (like BLK_STS_P2PDMA or
BLK_STS_IOERR) to blk-mq, blk-mq completes the request directly, bypassing the
driver's nvme_mpath_end_request() hook. This permanently leaks the nr_active
counter.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260722185841.449934-1-mykola@meshstor.io?part=9
^ permalink raw reply
* Re: [PATCH v4 7/9] md/raid1,raid10: skip futile retries on P2PDMA mapping failures
From: Mykola Marzhan @ 2026-07-22 19:50 UTC (permalink / raw)
To: Logan Gunthorpe
Cc: Jens Axboe, Song Liu, Yu Kuai, Keith Busch, Christoph Hellwig,
Sagi Grimberg, linux-block, linux-raid, linux-nvme, Li Nan,
Xiao Ni, Guoqing Jiang, Leon Romanovsky, Jason Gunthorpe,
Kiran Kumar Modukuri, Chaitanya Kulkarni, Bjorn Helgaas,
Shivaji Kant, Pranjal Shrivastava, Henrique Carvalho,
linux-kernel, linux-rdma, linux-pci
In-Reply-To: <38c44536-51c0-43a1-b83c-b94915890aa2@deltatee.com>
> Maybe drop my authorship from this patch.
I will do in v5. Any other changes needed?
> I'm not sure I follow the point of this new comment here.
I will drop the comment; it means the failure covers the whole bio and
won't change on retry, so we record whole range rather than per block.
^ permalink raw reply
* Re: [PATCH v4 8/9] md/raid1,raid10: set IO_BLOCKED in case of BLK_STS_P2PDMA
From: Mykola Marzhan @ 2026-07-22 20:02 UTC (permalink / raw)
To: Logan Gunthorpe
Cc: Jens Axboe, Song Liu, Yu Kuai, Keith Busch, Christoph Hellwig,
Sagi Grimberg, linux-block, linux-raid, linux-nvme, Li Nan,
Xiao Ni, Guoqing Jiang, Leon Romanovsky, Jason Gunthorpe,
Kiran Kumar Modukuri, Chaitanya Kulkarni, Bjorn Helgaas,
Shivaji Kant, Pranjal Shrivastava, Henrique Carvalho,
linux-kernel, linux-rdma, linux-pci
In-Reply-To: <536428fd-9c23-45c3-be0a-5eaaf8f3939a@deltatee.com>
> I think it would be better to
> move the code more towards similarity with raid1 instead of diverging it
> further.
I'll move bio = r10_bio->devs[slot].bio; up into the declaration so
raid10 matches raid1 in v5.
Thank you!
^ permalink raw reply
* Re: [PATCH v4 9/9] nvme-rdma: return BLK_STS_P2PDMA for unsupported P2P transfers
From: Logan Gunthorpe @ 2026-07-22 21:59 UTC (permalink / raw)
To: Mykola Marzhan, Jens Axboe, Song Liu, Yu Kuai, Keith Busch,
Christoph Hellwig, Sagi Grimberg, linux-block, linux-raid,
linux-nvme
Cc: Li Nan, Xiao Ni, Guoqing Jiang, Leon Romanovsky, Jason Gunthorpe,
Kiran Kumar Modukuri, Chaitanya Kulkarni, Bjorn Helgaas,
Shivaji Kant, Pranjal Shrivastava, Henrique Carvalho,
linux-kernel, linux-rdma, linux-pci
In-Reply-To: <20260722185841.449934-10-mykola@meshstor.io>
On 2026-07-22 12:58, Mykola Marzhan wrote:
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 6909e3542794..2f50509a7a61 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -1469,6 +1469,7 @@ static int nvme_rdma_dma_map_req(struct ib_device *ibdev, struct request *rq,
> int *count, int *pi_count)
> {
> struct nvme_rdma_request *req = blk_mq_rq_to_pdu(rq);
> + struct sg_table sgt;
> int ret;
>
> req->data_sgl.sg_table.sgl = (struct scatterlist *)(req + 1);
> @@ -1480,12 +1481,14 @@ static int nvme_rdma_dma_map_req(struct ib_device *ibdev, struct request *rq,
>
> req->data_sgl.nents = blk_rq_map_sg(rq, req->data_sgl.sg_table.sgl);
>
> - *count = ib_dma_map_sg(ibdev, req->data_sgl.sg_table.sgl,
> - req->data_sgl.nents, rq_dma_dir(rq));
> - if (unlikely(*count <= 0)) {
> - ret = -EIO;
> + sgt = (struct sg_table) {
> + .sgl = req->data_sgl.sg_table.sgl,
> + .orig_nents = req->data_sgl.nents,
> + };
Maybe we could make this patch a little clearer by splitting the change
to sg_table into it's own patch? That would make it much easier for me
to review. I've looked through it now a couple times and think it's ok,
but it's a little more complex than I can really untangle in my head.
Per your other email, I think that will make things good from my
perspective for v5, but I reserve the right to find something new when I
look at it ;)
Thanks for your efforts cleaning this up.
Logan
^ permalink raw reply
* Re: [PATCH v4 8/9] md/raid1,raid10: set IO_BLOCKED in case of BLK_STS_P2PDMA
From: Logan Gunthorpe @ 2026-07-23 3:11 UTC (permalink / raw)
To: Mykola Marzhan, Jens Axboe, Song Liu, Yu Kuai, Keith Busch,
Christoph Hellwig, Sagi Grimberg, linux-block, linux-raid,
linux-nvme
Cc: Li Nan, Xiao Ni, Guoqing Jiang, Leon Romanovsky, Jason Gunthorpe,
Kiran Kumar Modukuri, Chaitanya Kulkarni, Bjorn Helgaas,
Shivaji Kant, Pranjal Shrivastava, Henrique Carvalho,
linux-kernel, linux-rdma, linux-pci
In-Reply-To: <20260722185841.449934-9-mykola@meshstor.io>
On 2026-07-22 12:58 p.m., Mykola Marzhan wrote:
> From: Logan Gunthorpe <logang@deltatee.com>
>
> A read that fails with BLK_STS_P2PDMA cannot succeed against that
> member, so mark the leg IO_BLOCKED and let the retry redirect elsewhere.
> Skip the read-error machinery: there is nothing on the medium to fix,
> fix_read_error()'s probe reads into host pages and would "succeed", and
> charging the read-error budget would evict a healthy member under a P2P
> read workload. FailFast eviction is skipped for the same reason -- the
> request never reached the wire.
>
> Fixes: 02666132403a ("md: propagate BLK_FEAT_PCI_P2PDMA from member devices to RAID device")
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> Co-developed-by: Mykola Marzhan <mykola@meshstor.io>
> Signed-off-by: Mykola Marzhan <mykola@meshstor.io>
> ---
> drivers/md/raid1.c | 4 +++-
> drivers/md/raid10.c | 5 ++++-
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 917d694ef401..76a1426e64f0 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -2658,6 +2658,8 @@ static void handle_read_error(struct r1conf *conf, struct r1bio *r1_bio)
> {
> struct md_rdev *rdev = conf->mirrors[r1_bio->read_disk].rdev;
> struct bio *bio = r1_bio->bios[r1_bio->read_disk];
> + /* evaluate before the bio_put() below */
> + bool p2pdma_error = bio->bi_status == BLK_STS_P2PDMA;
Sorry, I had one late nit-picky thought tonight I thought I'd suggest:
It might be better to store bi_status in a local variable instead of the
much more specific p2pdma_error value. I think this would read a lot
better, make the intent a little more clear and be reusable if other
bi_status conditions need to be handled specially.
Thanks,
Logan
^ permalink raw reply
* Re: [PATCH v2 6/7] md/raid1,raid10: skip futile retries on P2PDMA mapping failures
From: Leon Romanovsky @ 2026-07-23 6:11 UTC (permalink / raw)
To: Mykola Marzhan
Cc: Logan Gunthorpe, Jens Axboe, Song Liu, Yu Kuai, Keith Busch,
Christoph Hellwig, Sagi Grimberg, linux-block, linux-raid,
linux-nvme, Li Nan, Xiao Ni, Guoqing Jiang, Jason Gunthorpe,
Kiran Kumar Modukuri, Chaitanya Kulkarni, Bjorn Helgaas,
Shivaji Kant, Pranjal Shrivastava, Henrique Carvalho,
linux-kernel, linux-rdma, linux-pci
In-Reply-To: <CAPzsNDuGfneEmx_nWqw5vYLyPyK8krfWChNRcsxO7icTEDDX5g@mail.gmail.com>
On Wed, Jul 22, 2026 at 09:00:16PM +0200, Mykola Marzhan wrote:
> > I had thoughts about this series too. It seems best to avoid
> > the P2P path entirely unless all legs support P2P.
> >
> > Can the md/raid code determine P2P support in advance?
>
> Yes, and it already does: the block layer ANDs the P2PDMA queue
> feature across all legs, so an array with a non-P2P member
> (e.g. an nvme-tcp leg) never advertises P2P at all.
>
> But that flag can only say "this driver can do P2P in general".
> md cannot see which HCA device does the DMA for a fabrics leg —
> and with multipath failover it can change at any time.
>
> So the up-front gate covers what is knowable up front.
> This series is about the remainder.
> In mixed case, one local P2P leg plus a remote replica, with it:
> 1) a failed read just redirects to a capable mirror;
> 2) a failed write records bad blocks instead of retrying forever.
Thanks for the explanation.
>
> Mykola
>
^ permalink raw reply
* Re: [PATCH v4 7/7] md/raid10: simplify read request error handling
From: Abd-Alrhman Masalkhi @ 2026-07-23 8:43 UTC (permalink / raw)
To: yu kuai, song, magiclinan, xiao, axboe, vverma, john.g.garry,
martin.petersen, linux-kernel, yu kuai
Cc: linux-raid
In-Reply-To: <feae08aa-7ce3-4375-91d0-42d501627908@fygo.io>
Hi Kuai,
Thanks for the feedback.
On Thu, Jul 16, 2026 at 17:52 +0800, yu kuai wrote:
> Hi,
>
> 在 2026/7/10 18:15, Abd-Alrhman Masalkhi 写道:
>> raid10_read_request() currently handles bio completion, barrier
>> handling, and r10_bio lifetime management in several different error
>> paths. This results in duplicated cleanup logic and increases the risk
>> of introducing bugs in future modifications.
>>
>> Make raid10_read_request() return a status to its callers, consolidate
>> the read error paths, and free r10_bio from a single location in the
>> callers. Since the callers allocate r10_bio, they should also be
>> responsible for freeing it when the request fails.
>>
>> This makes the read path follow the same ownership model as the write
>> path and simplifies the error handling flow.
>>
>> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
>> ---
>> Changes in v4:
>> - No changes.
>> - Link to v3: https://lore.kernel.org/linux-raid/20260708101341.473750-8-abd.masalkhi@gmail.com/
>>
>> Changes in v3:
>> - No changes.
>> - Link to v2: https://lore.kernel.org/linux-raid/20260628142420.1051027-8-abd.masalkhi@gmail.com/
>>
>> Changes in v2:
>> - Fix a compilation error (bi -> bio).
>> - Link to v1: https://lore.kernel.org/linux-raid/20260623072456.333437-8-abd.masalkhi@gmail.com/
>> ---
>> drivers/md/raid10.c | 45 +++++++++++++++++++++++++--------------------
>> 1 file changed, 25 insertions(+), 20 deletions(-)
>
> I don't think patch 6 and 7 are actually simplifications, and 7 in particular introduces
> a fragile dependency I'd rather not take as-is. The diff lines also said this and I'd
> rather not take as-is.
>
I see your point about the fragile dependency, especially in
handle_read_error(). I'll drop those patchs from this series and revisit
the cleanup in a future submission if I can find a better approach that
doesn't introduce that dependency.
>>
>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>> index d94c1f28a6f6..01162c483644 100644
>> --- a/drivers/md/raid10.c
>> +++ b/drivers/md/raid10.c
>> @@ -1143,7 +1143,7 @@ static bool regular_request_wait(struct mddev *mddev, struct r10conf *conf,
>> return true;
>> }
>>
>> -static void raid10_read_request(struct mddev *mddev, struct bio *bio,
>> +static bool raid10_read_request(struct mddev *mddev, struct bio *bio,
>> struct r10bio *r10_bio)
>> {
>> struct r10conf *conf = mddev->private;
>> @@ -1191,8 +1191,7 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
>>
>> if (!regular_request_wait(mddev, conf, bio, r10_bio->sectors)) {
>> bio_wouldblock_error(bio);
>> - free_r10bio(r10_bio);
>> - return;
>> + return false;
>> }
>>
>> rdev = read_balance(conf, r10_bio, &max_sectors);
>> @@ -1202,8 +1201,8 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
>> mdname(mddev), b,
>> (unsigned long long)r10_bio->sector);
>> }
>> - raid_end_bio_io(r10_bio);
>> - return;
>> + bio_io_error(bio);
>> + goto err_allow_barrier;
>> }
>> if (err_rdev)
>> pr_err_ratelimited("md/raid10:%s: %pg: redirecting sector %llu to another mirror\n",
>> @@ -1215,10 +1214,8 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
>> bio = bio_submit_split_bioset(bio, max_sectors,
>> &conf->bio_split);
>> wait_barrier(conf, false);
>> - if (!bio) {
>> - set_bit(R10BIO_Returned, &r10_bio->state);
>> - goto err_handle;
>> - }
>> + if (!bio)
>> + goto err_dec_pending;
>>
>> r10_bio->master_bio = bio;
>> r10_bio->sectors = max_sectors;
>> @@ -1244,10 +1241,16 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
>> read_bio->bi_private = r10_bio;
>> mddev_trace_remap(mddev, read_bio, r10_bio->sector);
>> submit_bio_noacct(read_bio);
>> - return;
>> -err_handle:
>> +
>> + return true;
>> +
>> +err_dec_pending:
>> atomic_dec(&rdev->nr_pending);
>> - raid_end_bio_io(r10_bio);
>> +
>> +err_allow_barrier:
>> + allow_barrier(conf);
>> +
>> + return false;
>> }
>>
>> static void raid10_write_one_disk(struct mddev *mddev, struct r10bio *r10_bio,
>> @@ -1538,14 +1541,13 @@ static bool __make_request(struct mddev *mddev, struct bio *bio, int sectors)
>> memset(r10_bio->devs, 0, sizeof(r10_bio->devs[0]) *
>> conf->geo.raid_disks);
>>
>> - ret = true;
>> if (bio_data_dir(bio) == READ)
>> - raid10_read_request(mddev, bio, r10_bio);
>> - else {
>> + ret = raid10_read_request(mddev, bio, r10_bio);
>> + else
>> ret = raid10_write_request(mddev, bio, r10_bio);
>> - if (!ret)
>> - free_r10bio(r10_bio);
>> - }
>> +
>> + if (!ret)
>> + free_r10bio(r10_bio);
>>
>> return ret;
>> }
>> @@ -1875,6 +1877,7 @@ static bool raid10_make_request(struct mddev *mddev, struct bio *bio)
>> sector_t chunk_mask = (conf->geo.chunk_mask & conf->prev.chunk_mask);
>> int chunk_sects = chunk_mask + 1;
>> int sectors = bio_sectors(bio);
>> + bool write = bio_data_dir(bio) == WRITE;
>>
>> if (unlikely(bio->bi_opf & REQ_PREFLUSH)
>> && md_flush_request(mddev, bio))
>> @@ -1898,7 +1901,7 @@ static bool raid10_make_request(struct mddev *mddev, struct bio *bio)
>> sectors = chunk_sects -
>> (bio->bi_iter.bi_sector &
>> (chunk_sects - 1));
>> - if (!__make_request(mddev, bio, sectors))
>> + if (!__make_request(mddev, bio, sectors) && write)
>> md_write_end(mddev);
>>
>> /* In case raid10d snuck in to freeze_array */
>> @@ -2866,7 +2869,9 @@ static void handle_read_error(struct mddev *mddev, struct r10bio *r10_bio)
>>
>> rdev_dec_pending(rdev, mddev);
>> r10_bio->state = 0;
>> - raid10_read_request(mddev, r10_bio->master_bio, r10_bio);
>> + if (!raid10_read_request(mddev, r10_bio->master_bio, r10_bio))
>> + free_r10bio(r10_bio);
>> +
>> /*
>> * allow_barrier after re-submit to ensure no sync io
>> * can be issued while regular io pending.
>
> --
> Thanks,
> Kuai
--
Best Regards,
Abd-Alrhman
^ permalink raw reply
* [PATCH] md/raid1: create serial pool adding rdev to array with serialize_policy=1
From: Martin Wilck @ 2026-07-23 11:27 UTC (permalink / raw)
To: Yu Kuai, Song Liu; +Cc: Xiao Ni, Li Nan, linux-raid, Martin Wilck
The following bug has been observed with kernel 7.1.3 after adding a new
rdev to an existing RAID1 array that had enabled serialize_policy:
[ 1754.091836][T19639] Oops: Oops: 0002 [#1] SMP NOPTI
[ 1754.092679][T19639] CPU: 0 UID: 0 PID: 19639 Comm: ext4lazyinit Not tainted 7.1.3-1-default #1 PREEMPT(full) openSUSE Tumbleweed b041a6527f6e58424f4cd3de0fade8d408b378fd
[ 1754.095245][T19639] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS unknown 02/02/2022
[ 1754.096772][T19639] RIP: 0010:_raw_spin_lock_irqsave+0x27/0x50
[ 1754.097769][T19639] Code: 90 90 90 f3 0f 1e fa 0f 1f 44 00 00 53 9c 58 0f 1f 40 00 48 89 c3 fa 0f 1f 44 00 00 65 ff 05 c0 bc d8 01 31 c0 ba 01 00 00 00 <3e> 0f b1 17 75 09 48 89 d8 5b e9 85 8d fc fe 89 c6 e8 c3 08 00 00
[ 1754.100996][T19639] RSP: 0018:ffffcfbbc0a5bab8 EFLAGS: 00010046
[ 1754.102037][T19639] RAX: 0000000000000000 RBX: 0000000000000212 RCX: 61c8864680b583eb
[ 1754.103367][T19639] RDX: 0000000000000001 RSI: 000000000000092a RDI: 0000000000004960
[ 1754.104723][T19639] RBP: ffff8efae2b12700 R08: 00000000ffffffff R09: 0000000000000000
[ 1754.106038][T19639] R10: ffffffffa0605ac0 R11: 0000000000000001 R12: ffff8efad4199400
[ 1754.107343][T19639] R13: 0000000000004950 R14: ffff8efaf9164600 R15: 000000000004043e
[ 1754.108697][T19639] FS: 0000000000000000(0000) GS:ffff8efbdc754000(0000) knlGS:0000000000000000
[ 1754.110190][T19639] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1754.111280][T19639] CR2: 0000000000004960 CR3: 000000000d830001 CR4: 0000000000770ef0
[ 1754.112580][T19639] PKRU: 55555554
[ 1754.113182][T19639] Call Trace:
[ 1754.113754][T19639] <TASK>
[ 1754.114256][T19639] wait_for_serialization+0xb9/0x260 [raid1 9ad4618e4becc97c5a7140f7aa35cc4730d97dd9]
[ 1754.115869][T19639] raid1_make_request+0x762/0xaff [raid1 9ad4618e4becc97c5a7140f7aa35cc4730d97dd9]
[ 1754.118368][T19639] md_handle_request+0x1c9/0x2e0 [md_mod a261f96e3d61eb9d00ceb34dc9439cf6487db185]
The raid1.c code calls wait_for_serialization() if the MD_SERIALIZE_POLICY
is set, and wait_for_serialization assumes that rdev->serial is
initialized. Normally this will be the case for arrays that have
the serialize_policy sysfs attribute set to 1.
But when a new rdev is added to an existing array in bind_rdev_to_array(),
the condition at mddev_create_serial_pool() causes creation of rdev->serial
to be skipped. Fix it.
Fixes: 69b00b5bb235 ("md: introduce a new struct for IO serialization")
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
drivers/md/md.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index d1465bcd86c8..baf79c501c9e 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -234,7 +234,8 @@ void mddev_create_serial_pool(struct mddev *mddev, struct md_rdev *rdev)
{
int ret = 0;
- if (rdev && !rdev_need_serial(rdev) &&
+ if (!test_bit(MD_SERIALIZE_POLICY, &mddev->flags) &&
+ rdev && !rdev_need_serial(rdev) &&
!test_bit(CollisionCheck, &rdev->flags))
return;
--
2.51.0
^ permalink raw reply related
* Re: [PATCH] md/raid1: create serial pool adding rdev to array with serialize_policy=1
From: sashiko-bot @ 2026-07-23 11:47 UTC (permalink / raw)
To: Martin Wilck; +Cc: linux-raid, yukuai
In-Reply-To: <20260723112741.1206836-1-mwilck@suse.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but `mddev_create_serial_pool` silently ignores memory allocation failures, leading to a guaranteed kernel panic on subsequent I/O.
- [High] This problem wasn't introduced by this patch, but echoing `-writemostly` to a device's sysfs state unconditionally destroys its serialization structures, even if the array-wide `MD_SERIALIZE_POLICY` is active, causing a NULL pointer dereference on the next I/O.
--
--- Patch [1]: [PATCH] md/raid1: create serial pool adding rdev to array with serialize_policy=1 ---
commit ac16670aca465de3c72a1f91b7b1bbb4e463a255
Author: Martin Wilck <martin.wilck@suse.com>
md/raid1: create serial pool adding rdev to array with serialize_policy=1
The raid1.c code calls wait_for_serialization() if the MD_SERIALIZE_POLICY
is set, and wait_for_serialization assumes that rdev->serial is
initialized. Normally this will be the case for arrays that have
the serialize_policy sysfs attribute set to 1.
But when a new rdev is added to an existing array in bind_rdev_to_array(),
the condition at mddev_create_serial_pool() causes creation of rdev->serial
to be skipped. Fix it.
Fixes: 69b00b5bb235 ("md: introduce a new struct for IO serialization")
Signed-off-by: Martin Wilck <mwilck@suse.com>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index d1465bcd86c81..baf79c501c9ec 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -234,7 +234,8 @@ void mddev_create_serial_pool(struct mddev *mddev, struct md_rdev *rdev)
> {
> int ret = 0;
>
> - if (rdev && !rdev_need_serial(rdev) &&
> + if (!test_bit(MD_SERIALIZE_POLICY, &mddev->flags) &&
> + rdev && !rdev_need_serial(rdev) &&
> !test_bit(CollisionCheck, &rdev->flags))
> return;
>
[Severity: High]
This isn't a bug introduced by this patch, but I noticed that
mddev_create_serial_pool() silently ignores memory allocation failures.
If rdev_init_serial() or mempool_create_kmalloc_pool() fails due to memory
pressure, mddev_create_serial_pool() catches the error but returns early
without propagating it:
drivers/md/md.c:mddev_create_serial_pool() {
...
if (ret)
return;
if (mddev->serial_info_pool == NULL) {
...
}
Callers like bind_rdev_to_array() proceed as if successful, setting the
MD_SERIALIZE_POLICY bit. On the next I/O, raid1_make_request() invokes
serialization functions like check_and_add_serial() that unconditionally
dereference the unallocated rdev->serial pointers:
drivers/md/raid1.c:check_and_add_serial() {
...
struct serial_in_rdev *serial = &rdev->serial[idx];
spin_lock_irqsave(&serial->serial_lock, flags);
...
}
Should mddev_create_serial_pool() return an error code instead of void to
prevent this crash?
[Severity: High]
This is another pre-existing issue, but while reviewing the serialization
logic I noticed that echoing "-writemostly" to a device's state via sysfs
unconditionally destroys its serialization structures.
In state_store(), this destroys the serial pool for the specific rdev without
checking the array-wide policy:
drivers/md/md.c:state_store() {
...
} else if (cmd_match(buf, "-writemostly")) {
mddev_destroy_serial_pool(rdev->mddev, rdev);
clear_bit(WriteMostly, &rdev->flags);
...
}
This unconditionally frees rdev->serial for the device:
drivers/md/md.c:mddev_destroy_serial_pool() {
...
if (rdev)
rdev_uninit_serial(rdev);
...
}
If the array-wide MD_SERIALIZE_POLICY is still active, the next I/O will
invoke wait_for_serialization() and remove_serial(), which dereference the
now-freed rdev->serial pointer:
drivers/md/raid1.c:check_and_add_serial() {
...
struct serial_in_rdev *serial = &rdev->serial[idx];
spin_lock_irqsave(&serial->serial_lock, flags);
...
}
Should state_store() check if MD_SERIALIZE_POLICY is still active before
destroying the serial pool?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260723112741.1206836-1-mwilck@suse.com?part=1
^ permalink raw reply
* [PATCH v2 0/3] md/raid10: fix recovery corruption and soft lockup on large arrays
From: Yunye Zhao @ 2026-07-23 13:55 UTC (permalink / raw)
To: Song Liu, Yu Kuai
Cc: Li Nan, Xiao Ni, Joseph Qi, linux-raid, linux-kernel, Yunye Zhao
In-Reply-To: <20260717062743.128189-1-yunye.zhao@linux.alibaba.com>
This series fixes two independent problems in raid10 recovery, plus a
safety net in md core.
Problem 1: silent data corruption when recovering with more than one
mirror missing.
Commit fe6a19d40ceb ("md/md-bitmap: merge md_bitmap_start_sync() into
bitmap_operations") inverted still_degraded in raid10_sync_request()
(int 1 -> bool false; the raid1 side of the same conversion is correct).
Recovering one disk while another is still missing therefore clears
bitmap bits the missing disk still needs; after its re-add the dirty
regions are skipped and the disk is marked In_sync with stale data.
Patch 1 restores the correct value.
Problem 2: soft lockup / CPU burn on recovery of a large sparse array
(the original report behind v1).
Recovering a degraded RAID10 with a mostly-clean bitmap makes
raid10_sync_request() take the "everything skipped" path, which returns
only 128 sectors per call even though the bitmap already reported a much
larger clean span. md_do_sync() then crawls across the whole device in
128-sector steps with no reschedule; on a 2^40-sector array that is 2^33
no-op iterations:
watchdog: BUG: soft lockup - CPU#149 stuck for 313s! [mdX_resync]
md_bitmap_start_sync+0x6f/0xe0
raid10_sync_request+0x2c9/0x1530 [raid10]
md_do_sync+0x810/0x1030
md_thread+0xa7/0x150
Patch 2 adds a cond_resched() to md_do_sync()'s skip path (safety net,
any personality/layout). Patch 3 fixes the root cause for near layouts:
convert the bitmap-reported clean span from array to per-device sectors
and skip it in one step. The conversion cannot be done generically in
the bitmap layer because the recovery cursor is in per-device space
(llbitmap's skip_sync_blocks() likewise returns 0 when degraded), and
resync paths already skip whole clean spans, so the fix lives in raid10.
Tested on QEMU/KVM on the latest mainline (raid10 near=2, 4 disks,
internal bitmap):
- data consistency: write, fail one disk of each pair, write 256MiB
while degraded, re-add both, wait for recovery, run "check":
mismatch_cnt=262272 and changed file hashes before patch 1,
mismatch_cnt=0 and identical hashes after.
- recovery of two re-added disks on a sparse 8T-per-disk array: one
37.2s recovery sweep of 100% CPU spinning before, 6.4 ms and 12.8 ms
for the two recovery passes after patch 3 (dmesg "recover of RAID
array" -> "recover done": 44.886958 -> 44.893378 and
44.923957 -> 44.936716); no soft lockup with softlockup_panic=1.
Changes since v1 (a single cond_resched patch):
- patch 1 (new): fix the still_degraded inversion, a silent data
corruption found while testing.
- patch 2 (was v1): rewrite the changelog and retitle; the v1 claim
that "io_sectors never increases" was wrong (Yu Kuai).
- patch 3 (new): fix the 128-sector recovery crawl at the source
instead of only silencing the watchdog.
v1: https://lore.kernel.org/linux-raid/20260717062743.128189-1-yunye.zhao@linux.alibaba.com/
Yunye Zhao (3):
md/raid10: fix still_degraded being inverted in raid10_sync_request()
md: add cond_resched() to md_do_sync()'s skip path
md/raid10: skip clean regions in bulk during recovery
drivers/md/md.c | 4 ++-
drivers/md/raid10.c | 57 ++++++++++++++++++++++++++++++++++++++++++---
2 files changed, 56 insertions(+), 5 deletions(-)
base-commit: 4539944e515183668109bdf4d0c3d7d228383d88
--
2.19.1.6.gb485710b
^ permalink raw reply
* [PATCH v2 3/3] md/raid10: skip clean regions in bulk during recovery
From: Yunye Zhao @ 2026-07-23 13:55 UTC (permalink / raw)
To: Song Liu, Yu Kuai
Cc: Li Nan, Xiao Ni, Joseph Qi, linux-raid, linux-kernel, Yunye Zhao
In-Reply-To: <20260723135535.101995-1-yunye.zhao@linux.alibaba.com>
During recovery of a degraded RAID10 with a mostly-clean bitmap, the
"everything skipped" path (biolist == NULL) returns max_sync == 128
sectors, even though md_bitmap_start_sync() already reported a much
larger clean range (an unallocated bitmap page spans 2^31 sectors with
a 512M bitmap chunk). On the reported 2^40-sector array md_do_sync()
then crawls through 2^33 no-op iterations, burning a CPU for the whole
sweep; the cond_resched() in md_do_sync()'s skip path only stops the
watchdog firing.
The skip path cannot simply return sync_blocks: sync_blocks is measured
in array sectors while the return value advances the per-device recovery
cursor, and the two spaces differ by the raid10 layout. For a near
layout (far_copies == 1 && !far_offset) the mapping is linear with slope
raid_disks / near_copies; when near_copies evenly divides raid_disks the
conversion is exact. Track the minimum clean span across the skipped
devices, convert it to device sectors and skip it in one step.
far/offset layouts are not a linear scale and keep the previous
behaviour.
Only devices skipped as clean feed that minimum, and the skip is
suppressed when a device needed rebuilding but had no readable source
(missing_source), so the cursor never jumps past sectors that still
need recovery.
The generic bitmap skip_sync_blocks() path cannot be used here: it does
not see mrdev/mreplace (the recovery target and its replacement), and
replacement targets must rebuild even bitmap-clean chunks.
On a mostly-clean array with 8T per device (near=2, 4 disks) the
recovery sweep drops from 37.2s of CPU spinning to about 10ms.
Signed-off-by: Yunye Zhao <yunye.zhao@linux.alibaba.com>
---
drivers/md/raid10.c | 46 ++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 43 insertions(+), 3 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 54cddb3a98cd..8acbd5ef618b 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -3176,6 +3176,9 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
int i;
int max_sync;
sector_t sync_blocks;
+ sector_t min_sync_blocks = MaxSector;
+ sector_t sync_end;
+ bool missing_source = false;
sector_t chunk_mask = conf->geo.chunk_mask;
int page_idx = 0;
@@ -3258,6 +3261,14 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
if (max_sector > mddev->resync_max)
max_sector = mddev->resync_max; /* Don't do IO beyond here */
+ /*
+ * The chunk clamp below caps a single sync I/O to at most one chunk.
+ * The bitmap-clean recovery skip issues no I/O, so remember the real
+ * end here to bound the skip against it rather than the chunk
+ * boundary.
+ */
+ sync_end = max_sector;
+
/* make sure whole request will fit in a chunk - if chunks
* are meaningful
*/
@@ -3334,9 +3345,13 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
if (!must_sync &&
mreplace == NULL &&
!conf->fullsync) {
- /* yep, skip the sync_blocks here, but don't assume
- * that there will never be anything to do here
+ /* Skip the clean sync_blocks here; don't
+ * assume there will never be anything to
+ * do. Only a genuinely skipped clean span
+ * may widen the bulk skip below.
*/
+ if (sync_blocks < min_sync_blocks)
+ min_sync_blocks = sync_blocks;
continue;
}
if (mrdev)
@@ -3459,6 +3474,7 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
if (j == conf->copies) {
/* Cannot recover, so abort the recovery or
* record a bad block */
+ missing_source = true;
if (any_working) {
/* problem is that there are bad blocks
* on other device(s)
@@ -3514,6 +3530,8 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
}
}
if (biolist == NULL) {
+ sector_t skip_sectors = max_sync;
+
while (r10_bio) {
struct r10bio *rb2 = r10_bio;
r10_bio = (struct r10bio*) rb2->master_bio;
@@ -3521,7 +3539,29 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
put_buf(rb2);
}
*skipped = 1;
- return max_sync;
+
+ /*
+ * min_sync_blocks is in array sectors, but the return
+ * value advances the per-device cursor; for a near
+ * layout the two spaces differ by the factor
+ * raid_disks / near_copies.
+ */
+ if (!missing_source &&
+ conf->geo.far_copies == 1 && !conf->geo.far_offset &&
+ conf->geo.raid_disks % conf->geo.near_copies == 0 &&
+ min_sync_blocks != MaxSector) {
+ sector_t clean_sectors = min_sync_blocks;
+
+ clean_sectors *= conf->geo.near_copies;
+ sector_div(clean_sectors, conf->geo.raid_disks);
+ clean_sectors &= ~chunk_mask;
+ if (clean_sectors > sync_end - sector_nr)
+ clean_sectors = sync_end - sector_nr;
+ if (clean_sectors > skip_sectors)
+ skip_sectors = clean_sectors;
+ }
+
+ return skip_sectors;
}
} else {
/* resync. Schedule a read for every block at this virt offset */
--
2.19.1.6.gb485710b
^ permalink raw reply related
* [PATCH v2 1/3] md/raid10: fix still_degraded being inverted in raid10_sync_request()
From: Yunye Zhao @ 2026-07-23 13:55 UTC (permalink / raw)
To: Song Liu, Yu Kuai
Cc: Li Nan, Xiao Ni, Joseph Qi, linux-raid, linux-kernel, Yunye Zhao
In-Reply-To: <20260723135535.101995-1-yunye.zhao@linux.alibaba.com>
Commit fe6a19d40ceb ("md/md-bitmap: merge md_bitmap_start_sync() into
bitmap_operations") converted still_degraded from int to bool, but
inverted the assignment in the loop that checks whether the array will
still be degraded after the current device is recovered:
"still_degraded = 1" became "still_degraded = false".
As a result, recovering a device while another mirror is still missing
calls md_bitmap_start_sync() with degraded == false, which clears bitmap
bits that the still-missing device needs. When that device is re-added,
its bitmap-based recovery finds the bits already cleared and skips every
region written while the array was degraded, so it is marked In_sync
while holding stale data: silent corruption.
Reproducer (raid10 near=2, 4 disks, internal bitmap):
- fail and remove one disk of each mirror pair
- write to the degraded array
- re-add both disks and let recovery finish
- "check" reports mismatch_cnt=262272 after 256 MiB of degraded
writes and file contents differ; the second disk's "recovery"
completes in milliseconds because everything is skipped
The same conversion in raid1 got it right (still_degraded = true).
Restore the correct value.
Fixes: fe6a19d40ceb ("md/md-bitmap: merge md_bitmap_start_sync() into bitmap_operations")
Cc: stable@vger.kernel.org
Signed-off-by: Yunye Zhao <yunye.zhao@linux.alibaba.com>
---
drivers/md/raid10.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 0a3cfdd3f5df..54cddb3a98cd 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -3365,7 +3365,7 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
struct md_rdev *rdev = conf->mirrors[j].rdev;
if (rdev == NULL || test_bit(Faulty, &rdev->flags)) {
- still_degraded = false;
+ still_degraded = true;
break;
}
}
base-commit: 4539944e515183668109bdf4d0c3d7d228383d88
--
2.19.1.6.gb485710b
^ permalink raw reply related
* [PATCH v2 2/3] md: add cond_resched() to md_do_sync()'s skip path
From: Yunye Zhao @ 2026-07-23 13:55 UTC (permalink / raw)
To: Song Liu, Yu Kuai
Cc: Li Nan, Xiao Ni, Joseph Qi, linux-raid, linux-kernel, Yunye Zhao
In-Reply-To: <20260723135535.101995-1-yunye.zhao@linux.alibaba.com>
When sync_request() reports a skipped region (*skipped == 1),
md_do_sync()'s main loop advances the cursor and takes an early
continue:
j += sectors;
...
if (last_check + window > io_sectors || j == max_sectors)
continue;
If the personality returns a small span per call (raid10 recovery
returns only 128 sectors), syncing a large, mostly clean array iterates
this branch an enormous number of times without ever yielding the CPU.
On a non-preemptive kernel the resync thread then trips the soft-lockup
watchdog:
watchdog: BUG: soft lockup - CPU#149 stuck for 313s! [mdX_resync]
md_bitmap_start_sync+0x6f/0xe0
raid10_sync_request+0x2c9/0x1530 [raid10]
md_do_sync+0x810/0x1030
md_thread+0xa7/0x150
Add a cond_resched(). This does not reduce the wasted iterations; the
excessive iteration count is a raid10 problem addressed separately.
Cc: stable@vger.kernel.org
Signed-off-by: Yunye Zhao <yunye.zhao@linux.alibaba.com>
---
drivers/md/md.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index d1465bcd86c8..c2e8b203c4db 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -9881,8 +9881,10 @@ void md_do_sync(struct md_thread *thread)
*/
md_new_event();
- if (last_check + window > io_sectors || j == max_sectors)
+ if (last_check + window > io_sectors || j == max_sectors) {
+ cond_resched();
continue;
+ }
last_check = io_sectors;
repeat:
--
2.19.1.6.gb485710b
^ permalink raw reply related
* Re: [PATCH v2 2/3] md: add cond_resched() to md_do_sync()'s skip path
From: sashiko-bot @ 2026-07-23 14:05 UTC (permalink / raw)
To: Yunye Zhao; +Cc: yukuai, linux-raid
In-Reply-To: <20260723135535.101995-3-yunye.zhao@linux.alibaba.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Unbounded waitqueue spam from md_new_event() during sync skip loops.
--
--- Patch [2]: [PATCH v2 2/3] md: add cond_resched() to md_do_sync()'s skip path ---
commit 6f3b7b59be28b3b7daedbe090461ee12870bd481
Author: Yunye Zhao <yunye.zhao@linux.alibaba.com>
md: add cond_resched() to md_do_sync()'s skip path
This patch adds a cond_resched() to the skip loop in md_do_sync(). It
addresses a soft lockup watchdog trigger during raid10 recovery on large,
mostly clean arrays by yielding the CPU when regions are skipped.
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index d1465bcd86c81..c2e8b203c4db2 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -9881,8 +9881,10 @@ void md_do_sync(struct md_thread *thread)
> */
> md_new_event();
>
> - if (last_check + window > io_sectors || j == max_sectors)
> + if (last_check + window > io_sectors || j == max_sectors) {
> + cond_resched();
> continue;
> + }
[Severity: High]
This isn't a bug introduced by this patch, but does this code cause unbounded
waitqueue spam from md_new_event() during sync skip loops?
When last_check starts at 0 and the personality skips regions, io_sectors
does not increment. Because last_check remains 0, the block just above this
diff checks:
if (last_check == 0)
/* this is the earliest that rebuild will be
* visible in /proc/mdstat
*/
md_new_event();
Since last_check == 0 evaluates to true on every iteration of the skip loop,
md_new_event() is called continuously.
For a large clean array with small skip chunks (like 128 sectors), this could
execute millions of times.
Could this lead to severe waitqueue spam and lock contention on
md_event_waiters, resulting in spurious wakeups for userspace processes
monitoring mdstat?
>
> last_check = io_sectors;
> repeat:
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260723135535.101995-1-yunye.zhao@linux.alibaba.com?part=2
^ permalink raw reply
* Re: [PATCH v2 3/3] md/raid10: skip clean regions in bulk during recovery
From: sashiko-bot @ 2026-07-23 14:16 UTC (permalink / raw)
To: Yunye Zhao; +Cc: linux-raid, yukuai
In-Reply-To: <20260723135535.101995-4-yunye.zhao@linux.alibaba.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Fallback to an unscaled `max_sync` mixes array and device sectors, overshooting the recovery cursor and silently skipping dirty sectors.
--
--- Patch [3]: [PATCH v2 3/3] md/raid10: skip clean regions in bulk during recovery ---
commit 4abce2392f76451810793b3efca0299da30d9898
Author: Yunye Zhao <yunye.zhao@linux.alibaba.com>
md/raid10: skip clean regions in bulk during recovery
During recovery of a degraded RAID10 array with a mostly-clean bitmap, this
patch calculates the skipped clean span in bulk. It tracks the minimum clean
span across skipped devices, converts it to device sectors, and skips it in
one step to reduce CPU spinning.
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 54cddb3a98cd6..8acbd5ef618b4 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
[ ... ]
> @@ -3514,6 +3530,8 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
> }
> }
> if (biolist == NULL) {
> + sector_t skip_sectors = max_sync;
> +
> while (r10_bio) {
> struct r10bio *rb2 = r10_bio;
> r10_bio = (struct r10bio*) rb2->master_bio;
> @@ -3521,7 +3539,29 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
> put_buf(rb2);
> }
> *skipped = 1;
> - return max_sync;
> +
> + /*
> + * min_sync_blocks is in array sectors, but the return
> + * value advances the per-device cursor; for a near
> + * layout the two spaces differ by the factor
> + * raid_disks / near_copies.
> + */
> + if (!missing_source &&
> + conf->geo.far_copies == 1 && !conf->geo.far_offset &&
> + conf->geo.raid_disks % conf->geo.near_copies == 0 &&
> + min_sync_blocks != MaxSector) {
> + sector_t clean_sectors = min_sync_blocks;
> +
> + clean_sectors *= conf->geo.near_copies;
> + sector_div(clean_sectors, conf->geo.raid_disks);
> + clean_sectors &= ~chunk_mask;
> + if (clean_sectors > sync_end - sector_nr)
> + clean_sectors = sync_end - sector_nr;
> + if (clean_sectors > skip_sectors)
> + skip_sectors = clean_sectors;
> + }
> +
> + return skip_sectors;
[Severity: Critical]
This is a pre-existing issue, but does returning the unscaled max_sync mix
array and device sectors here?
When recovering a degraded RAID10 array, md_bitmap_start_sync() returns
sync_blocks in array sectors, which is then assigned to max_sync.
If a clean span is small or not chunk-aligned, the truncation:
clean_sectors &= ~chunk_mask;
can force the scaled clean_sectors to 0. When that happens, the condition:
if (clean_sectors > skip_sectors)
skip_sectors = clean_sectors;
fails, leaving skip_sectors as the unscaled max_sync value.
Because raid10_sync_request() then returns skip_sectors (an array-sector
distance) to advance the physical per-device cursor in md_do_sync(), could
the physical cursor overstep the actual clean physical span?
For layouts where array sectors advance faster than device sectors
(e.g., near_copies > 1), won't this silently skip dirty sectors and leave
mirrors out of sync?
> }
> } else {
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260723135535.101995-1-yunye.zhao@linux.alibaba.com?part=3
^ permalink raw reply
page: | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox