Re: [PATCH v3 3/3] md-cluster: revert local resize and propagate cluster errors

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

New issues:
- [High] Reverting the local array resize on cluster failure leaves the on-disk superblock inconsistent with the in-memory state.
- [High] Missing revert of block device capacity when cluster broadcast fails creates a split-brain state between the block layer and the MD layer.
- [High] The return value of the revert call to `pers->resize()` is ignored, which can hide a failed revert and leave the array in an unexpected state.

Pre-existing issues:
- [High] The patch fails to propagate the error when the cluster resize is rejected by other nodes, resulting in a silent failure reported as success to user-space.
--

commit cc96df1db446b526c4a40bb6ed18c53c2fa44fb6
Author: Cao Guanghui <[email protected]>

md-cluster: revert local resize and propagate cluster errors

This patch attempts to fix a split-brain state during an array resize by
propagating the return value of the cluster update_size() operation and
reverting the local resize if the cluster operation fails. It also logs
a warning if the cluster update fails during the reshape completion path.

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 680b34a63cb3d..10b9880e64580 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -8022,9 +8022,11 @@ static int update_size(struct mddev *mddev, sector_t num_sectors)
>  	}
>  	rv = mddev->pers->resize(mddev, num_sectors);
>  	if (!rv) {
> -		if (mddev_is_clustered(mddev))
> -			mddev->cluster_ops->update_size(mddev, old_dev_sectors);
> -		else if (!mddev_is_dm(mddev))
> +		if (mddev_is_clustered(mddev)) {
> +			rv = mddev->cluster_ops->update_size(mddev, old_dev_sectors);

[Severity: High]
This is a pre-existing issue, but does mddev->cluster_ops->update_size()
correctly propagate errors?

If cluster_check_sync_size() returns an error, md-cluster.c:update_size()
executes a fallback path that overwrites the error state with the result of
the revert:

drivers/md/md-cluster.c:update_size() {
...
	} else {
		/* revert to previous sectors */
		ret = mddev->pers->resize(mddev, old_dev_sectors);
		if (ret)
			pr_err("%s:%d: failed to revert array size\n",
			       __func__, __LINE__);
		reverted = true;
...
}

If the local revert succeeds, ret becomes 0, silencing the error from
cluster_check_sync_size(). When this code receives rv = 0, it will
incorrectly assume the cluster update succeeded, bypassing the new revert
logic and reporting a false success to user-space.

> +			if (rv)

[Severity: High]
The new revert logic handles the in-memory array size, but misses correcting
the on-disk superblock if cluster communication fails.

In md-cluster.c:update_size(), md_update_sb() synchronously writes the new
size to disk. If lock_comm() fails, it returns -EIO without reverting:

drivers/md/md-cluster.c:update_size() {
	md_update_sb(mddev, 1);
	if (lock_comm(cinfo, 1)) {
		pr_err("%s: lock_comm failed\n", __func__);
		return -EIO;
	}
...
}

If cluster_ops->update_size() returns -EIO, we revert the internal MD array
size here, but shouldn't we also call md_update_sb(mddev, 1) to avoid
leaving the array in a split-brain state with a corrupted on-disk
superblock?

[Severity: High]
Does this sequence cause a split-brain state between the block layer and the
MD layer?

If the cluster broadcast fails right after checking the sync size,
md-cluster.c:update_size() unconditionally updates the block capacity before
returning the error:

drivers/md/md-cluster.c:update_size() {
...
	if (cluster_check_sync_size(mddev) == 0) {
		memset(&cmsg, 0, sizeof(cmsg));
		cmsg.type = cpu_to_le32(CHANGE_CAPACITY);
		ret = __sendmsg(cinfo, &cmsg);
		if (ret)
			pr_err("%s:%d: failed to send CHANGE_CAPACITY msg\n",
			       __func__, __LINE__);
		set_capacity_and_notify(mddev->gendisk, mddev->array_sectors);
	}
...
}

This code catches the __sendmsg() error and reverts the MD layer capacity
via mddev->pers->resize(), but there is no corresponding call to revert the
block layer capacity. Could this lead to out-of-bounds IOs if the block
layer exposes more capacity than the MD array limits?

> +				mddev->pers->resize(mddev, old_dev_sectors);

[Severity: High]
Is it safe to ignore the return value of the revert operation here?

If mddev->pers->resize() fails (e.g., raid1_resize() returning -ENOMEM due
to bitmap allocation failure), the MD array will silently remain stuck at
the new size internally while user-space receives an error code. Shouldn't
we check the return value to handle a failed revert properly?

> +		} else if (!mddev_is_dm(mddev))
>  			set_capacity_and_notify(mddev->gendisk,
>  						mddev->array_sectors);
>  	}

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