Re: [PATCH v6 08/12] md/raid1,raid10: set IO_BLOCKED in case of BLK_STS_P2PDMA

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] A pre-existing ABBA deadlock can occur if a read error is processed by `raid1d` or `raid10d` concurrently with a management thread calling `freeze_array(conf, 0)`.
--

--- Patch [8]: [PATCH v6 08/12] md/raid1,raid10: set IO_BLOCKED in case of BLK_STS_P2PDMA ---
commit f4692375c384d52b343af556c8a63ea0e3e48c0f
Author: Mykola Marzhan <[email protected]>

md/raid1,raid10: set IO_BLOCKED in case of BLK_STS_P2PDMA
    
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.
This skips the read-error machinery to avoid evicting a healthy member
under a P2P workload.

> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 83ebf40237d57..16a47568679f0 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c

[ ... ]

> @@ -2674,7 +2676,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 || status == BLK_STS_P2PDMA) {

[Severity: High]
This isn't a bug introduced by this patch, but does extending this condition
expose a pre-existing ABBA deadlock to P2PDMA failures?

If a management thread concurrently calls freeze_array(conf, 0) (setting
conf->array_frozen = 1 and waiting for nr_pending to drop to 0), and
raid1d takes this branch, it skips calling freeze_array(conf, 1) and
unfreeze_array(conf).

Later in handle_read_error(), the daemon thread invokes
raid1_read_request(), which blocks on wait_read_barrier() waiting for
conf->array_frozen to be cleared.

The management thread is blocked waiting for the daemon to drop its
nr_pending count, while the daemon is blocked waiting for the management
thread to unfreeze the array. Can this cause both threads to hang
indefinitely?

>  		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 77c7d8a599160..462f622556ed0 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c

[ ... ]

> @@ -2859,11 +2861,10 @@ static void handle_read_error(struct mddev *mddev, struct r10bio *r10_bio)
>  	 * This is all done synchronously while the array is
>  	 * frozen.
>  	 */
> -	bio = r10_bio->devs[slot].bio;
>  	bio_put(bio);
>  	r10_bio->devs[slot].bio = NULL;
>  
> -	if (mddev->ro)
> +	if (mddev->ro || status == BLK_STS_P2PDMA)

[Severity: High]
This isn't a bug introduced by this patch, but does this branch suffer from
a similar pre-existing deadlock in raid10?

If the daemon thread takes this path, it skips the freeze/unfreeze cycle
and later calls raid10_read_request(). This blocks on wait_barrier(),
waiting for the concurrent management freeze_array(conf, 0) to clear,
creating the same deadlock between the daemon and management thread.

>  		r10_bio->devs[slot].bio = IO_BLOCKED;
>  	else if (!test_bit(FailFast, &rdev->flags)) {
>  		freeze_array(conf, 1);

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