Re: [PATCH] md-cluster: check pers->resize() return value in update_size()

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

Pre-existing issues:
- [Critical] Missing superblock update after reverting array size leads to cluster-wide metadata inconsistency.
- [High] Cluster capacity change failures are incorrectly masked from userspace.
- [High] DLM lock leaked in EX state if down-conversion fails in `__sendmsg()`.
--

commit 716b34ca47de8d73645de2da63206339cac7a868
Author: Cao Guanghui <[email protected]>

md-cluster: check pers->resize() return value in update_size()

This commit checks the return value of pers->resize() during an array capacity
revert and logs an error if it fails, ensuring the failure is visible.

> diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
> index 24dac6f09da82..0c6da16aecbfa 100644
> --- a/drivers/md/md-cluster.c
> +++ b/drivers/md/md-cluster.c
> @@ -1349,6 +1349,9 @@ 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);
> +		if (ret)
> +			pr_err("%s:%d: failed to revert array size\n",
> +			       __func__, __LINE__);

[Severity: High]
This is a pre-existing issue, but since update_size() has a void return type,
cluster capacity change failures like this are not propagated back to the core
MD layer.

In drivers/md/md.c:update_size(), the return state of the cluster size
update is ignored:

	if (mddev_is_clustered(mddev))
		mddev->cluster_ops->update_size(mddev, old_dev_sectors);

This allows the function to return success (0) even if the cluster resize
failed and was reverted here. Should we change the signature of update_size()
so userspace is accurately informed of the failure?

[Severity: Critical]
This is a pre-existing issue, but is there a missing superblock update before
sending the METADATA_UPDATED message?

If the resize operation fails and the in-memory size is reverted via
mddev->pers->resize(), the on-disk superblock still retains the new size
written earlier.

Because md_update_sb() is not called before broadcasting METADATA_UPDATED,
other nodes reading the on-disk superblock will adopt the new size while the
initiator runs with the old size. Can this lead to cluster-wide metadata
inconsistency and data corruption?

>  		ret = __sendmsg(cinfo, &cmsg);

[Severity: High]
This is a pre-existing issue, but I have a question about the __sendmsg()
error handling.

Looking at __sendmsg(), if the down-conversion of the lock fails:

	/*down-convert EX to CR on Ack*/
	error = dlm_lock_sync(cinfo->ack_lockres, DLM_LOCK_CR);
	if (error) {
		pr_err("md-cluster: failed to convert EX to CR on ACK(%d)\n",
				error);
		goto failed_ack;
	}

failed_ack:
	while ((unlock_error = dlm_unlock_sync(cinfo->message_lockres)))
		pr_err("md-cluster: failed convert to NL on MESSAGE(%d)\n",
			unlock_error);
	return error;

The cleanup block handles message_lockres but completely ignores ack_lockres.
Does this permanently leak the DLM lock in EX state if the down-conversion
fails, potentially causing a cluster-wide deadlock?

>  		if (ret)
>  			pr_err("%s:%d: failed to send METADATA_UPDATED msg\n",

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