Re: [PATCH v3 2/3] md-cluster: fix error handling and superblock consistency in update_size

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

New issues:
- [High] When the cluster rejects a resize and the local revert succeeds, `update_size()` erroneously returns 0 (success), misleading userspace.
- [High] If the local revert resize fails after the cluster rejects the new size, the code unconditionally updates the on-disk superblock with the rejected new size.

Pre-existing issues:
- [High] Early failure paths in `update_size()` commit the new size to the on-disk superblock but fail to revert it on error, causing persistent cluster split-brain.
--

commit faab74a4740b5da9226b76ad0d7a819261f23143
Author: Cao Guanghui <[email protected]>
Subject: md-cluster: fix error handling and superblock consistency in update_size

This commit attempts to fix error handling in update_size() by returning
proper error codes and ensuring the on-disk superblock is reverted if
other nodes in the cluster reject the size update.

> diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
> index fb62fdb492350..0ba25053198d5 100644
> --- a/drivers/md/md-cluster.c
> +++ b/drivers/md/md-cluster.c
> @@ -1302,18 +1302,19 @@ static int cluster_check_sync_size(struct mddev *mddev)
[ ... ]
> -static void update_size(struct mddev *mddev, sector_t old_dev_sectors)
> +static int update_size(struct mddev *mddev, sector_t old_dev_sectors)
>  {
>  	struct md_cluster_info *cinfo = mddev->cluster_info;
> +	bool reverted = false;
>  	struct cluster_msg cmsg;
>  	struct md_rdev *rdev;
> -	int ret = 0;
> +	int ret = 0, msg_ret = 0;
>  	int raid_slot = -1;
>  
>  	md_update_sb(mddev, 1);
>  	if (lock_comm(cinfo, 1)) {
>  		pr_err("%s: lock_comm failed\n", __func__);
> -		return;
> +		return -EIO;
>  	}

[Severity: High]
This is a pre-existing issue, but since md_update_sb() has already
written the new array size to the on-disk superblock, does this early
error path need to revert that change?

If the cluster operation fails early here, or in the subsequent
__sendmsg() calls, the cluster is never notified of the size change but
the local node has already committed it to disk. On the next reboot,
this node might assemble the array with the new size while others use
the old size, causing a persistent cluster split-brain.

[ ... ]
> @@ -1359,12 +1360,28 @@ static void update_size(struct mddev *mddev, sector_t old_dev_sectors)
>  	} else {
>  		/* revert to previous sectors */
>  		ret = mddev->pers->resize(mddev, old_dev_sectors);

[Severity: High]
When the cluster rejects a resize and the code enters this else block to
revert the size, does a successful local revert (where
mddev->pers->resize() returns 0) cause the function to return 0?

Returning 0 incorrectly signals a successful resize to the caller. This
might mislead userspace tools and administrators into assuming the
storage capacity has increased, when it was actually rejected by the
cluster and rolled back locally.

> -		ret = __sendmsg(cinfo, &cmsg);
>  		if (ret)
> +			pr_err("%s:%d: failed to revert array size\n",
> +			       __func__, __LINE__);
> +		reverted = true;

[Severity: High]
If the local revert resize fails (returning a non-zero error), the
reverted flag is still unconditionally set to true.

Because the reverted flag is true, md_update_sb() will be called at the
end of the function. Will this cause the local node to permanently store
the rejected new size in its on-disk superblock, leading to a cluster
split-brain?

> +		msg_ret = __sendmsg(cinfo, &cmsg);
> +		if (msg_ret) {
>  			pr_err("%s:%d: failed to send METADATA_UPDATED msg\n",
>  			       __func__, __LINE__);
> +			if (!ret)
> +				ret = msg_ret;
> +		}
>  	}
>  	unlock_comm(cinfo);
> +
> +	if (reverted)
> +		/* Update on-disk superblock to match reverted in-memory
> +		 * size. Must be after unlock_comm() to avoid self-deadlock
> +		 * since md_update_sb() acquires the cluster send lock.
> +		 */
> +		md_update_sb(mddev, 1);
> +
> +	return ret;
>  }

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