* [PATCH] md-cluster: check pers->resize() return value in update_size()
@ 2026-08-27 2:52 ghuicao
2026-08-27 3:07 ` sashiko-bot
2026-08-27 6:11 ` [PATCH v2 1/3] md-cluster: fix error handling and superblock update in update_size revert ghuicao
0 siblings, 2 replies; 14+ messages in thread
From: ghuicao @ 2026-08-27 2:52 UTC (permalink / raw)
To: Song Liu; +Cc: Yu Kuai, Li Nan, Xiao Ni, linux-raid, linux-kernel, Cao Guanghui
From: Cao Guanghui <caoguanghui@kylinos.cn>
In update_size(), when cluster_check_sync_size() detects that not all
nodes have updated their sync_size, the initiator reverts the array
size via pers->resize(). However, the return value of resize() is
immediately overwritten by __sendmsg(), so a resize failure is
silently lost.
If the revert resize fails, the array remains at the new (larger)
size while other nodes have not confirmed the change, leaving the
cluster in an inconsistent state with no error logged.
Check the resize() return value and log an error before sending the
METADATA_UPDATED message, so that a resize failure is visible to the
user.
Signed-off-by: Cao Guanghui <caoguanghui@kylinos.cn>
---
drivers/md/md-cluster.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
--- a/drivers/md/md-cluster.c
+++ b/drivers/md/md-cluster.c
@@ -1349,7 +1349,10 @@ 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__);
ret = __sendmsg(cinfo, &cmsg);
if (ret)
pr_err("%s:%d: failed to send METADATA_UPDATED msg\n",
__func__, __LINE__);
--
2.34.1
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] md-cluster: check pers->resize() return value in update_size() 2026-08-27 2:52 [PATCH] md-cluster: check pers->resize() return value in update_size() ghuicao @ 2026-08-27 3:07 ` sashiko-bot 2026-08-27 6:11 ` [PATCH v2 1/3] md-cluster: fix error handling and superblock update in update_size revert ghuicao 1 sibling, 0 replies; 14+ messages in thread From: sashiko-bot @ 2026-08-27 3:07 UTC (permalink / raw) To: ghuicao; +Cc: linux-raid, yukuai 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 <caoguanghui@kylinos.cn> 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/20260827025229.40138-1-ghuicao@163.com?part=1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/3] md-cluster: fix error handling and superblock update in update_size revert 2026-08-27 2:52 [PATCH] md-cluster: check pers->resize() return value in update_size() ghuicao 2026-08-27 3:07 ` sashiko-bot @ 2026-08-27 6:11 ` ghuicao 2026-08-27 6:11 ` [PATCH v2 2/3] md-cluster: propagate update_size() errors to callers ghuicao ` (3 more replies) 1 sibling, 4 replies; 14+ messages in thread From: ghuicao @ 2026-08-27 6:11 UTC (permalink / raw) To: Song Liu Cc: Yu Kuai, Li Nan, Xiao Ni, linux-raid, linux-kernel, stable, Cao Guanghui From: Cao Guanghui <caoguanghui@kylinos.cn> In update_size(), when cluster_check_sync_size() detects that not all nodes have updated their sync_size, the initiator reverts the array size via pers->resize(). Two issues exist in this revert path: 1. The return value of resize() is immediately overwritten by __sendmsg(), so a resize failure is silently lost. The array remains at the new size while other nodes have not confirmed the change, with no error logged. 2. The on-disk superblock still retains the new size written by the earlier md_update_sb() call. Other nodes that receive the METADATA_UPDATED message will re-read the on-disk superblock and adopt the new size, while the initiator runs with the reverted old size, causing a cluster-wide metadata inconsistency. Fix by checking the resize() return value and logging an error, then calling md_update_sb() so the on-disk superblock matches the in-memory array size before broadcasting the message. Fixes: 818da59f97d6 ("md-cluster: add the support for resize") Cc: stable@vger.kernel.org Signed-off-by: Cao Guanghui <caoguanghui@kylinos.cn> --- Changes in v2: - Fold in the md_update_sb() fix (was a separate patch in v1) - Combine error check and superblock update into one coherent fix drivers/md/md-cluster.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c --- a/drivers/md/md-cluster.c +++ b/drivers/md/md-cluster.c @@ -1349,7 +1349,11 @@ 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__); + md_update_sb(mddev, 1); ret = __sendmsg(cinfo, &cmsg); if (ret) pr_err("%s:%d: failed to send METADATA_UPDATED msg\n", __func__, __LINE__); } -- 2.34.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 2/3] md-cluster: propagate update_size() errors to callers 2026-08-27 6:11 ` [PATCH v2 1/3] md-cluster: fix error handling and superblock update in update_size revert ghuicao @ 2026-08-27 6:11 ` ghuicao 2026-08-27 6:23 ` sashiko-bot 2026-08-27 6:12 ` [PATCH v2 3/3] md-cluster: fix ack_lockres leak in __sendmsg error path ghuicao ` (2 subsequent siblings) 3 siblings, 1 reply; 14+ messages in thread From: ghuicao @ 2026-08-27 6:11 UTC (permalink / raw) To: Song Liu Cc: Yu Kuai, Li Nan, Xiao Ni, linux-raid, linux-kernel, stable, Cao Guanghui From: Cao Guanghui <caoguanghui@kylinos.cn> update_size() in md-cluster.c has a void return type, so the caller in md.c cannot detect cluster resize failures. A capacity revert or message delivery failure is silently lost, and the MD layer reports success to userspace even when the cluster operation failed. Change the update_size() callback in struct md_cluster_ops and its implementation to return int, and propagate the error at the call sites in md.c. Fixes: 818da59f97d6 ("md-cluster: add the support for resize") Cc: stable@vger.kernel.org Signed-off-by: Cao Guanghui <caoguanghui@kylinos.cn> --- drivers/md/md-cluster.c | 11 ++++++----- drivers/md/md-cluster.h | 2 +- drivers/md/md.c | 7 +++++-- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c --- a/drivers/md/md-cluster.c +++ b/drivers/md/md-cluster.c @@ -1292,7 +1292,7 @@ static int cluster_check_sync_size(struct mddev *mddev) * let other nodes to perform it. If one node can't update sync_size * accordingly, we need to revert to previous value. */ -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; struct cluster_msg cmsg; @@ -1303,7 +1303,7 @@ static int update_size(struct mddev *mddev, sector_t old_dev_sectors) md_update_sb(mddev, 1); if (lock_comm(cinfo, 1)) { pr_err("%s: lock_comm failed\n", __func__); - return; + return -1; } memset(&cmsg, 0, sizeof(cmsg)); @@ -1325,12 +1325,12 @@ static int update_size(struct mddev *mddev, sector_t old_dev_sectors) pr_err("%s:%d: failed to send METADATA_UPDATED msg\n", __func__, __LINE__); unlock_comm(cinfo); - return; + return ret; } } else { pr_err("md-cluster: No good device id found to send\n"); unlock_comm(cinfo); - return; + return -1; } /* @@ -1355,6 +1355,7 @@ static int update_size(struct mddev *mddev, sector_t old_dev_sectors) __func__, __LINE__); } unlock_comm(cinfo); + return ret; } static int resync_start(struct mddev *mddev) diff --git a/drivers/md/md-cluster.h b/drivers/md/md-cluster.h --- a/drivers/md/md-cluster.h +++ b/drivers/md/md-cluster.h @@ -34,7 +34,7 @@ struct md_cluster_operations { int (*resize_bitmaps)(struct mddev *mddev, sector_t newsize, sector_t oldsize); int (*lock_all_bitmaps)(struct mddev *mddev); void (*unlock_all_bitmaps)(struct mddev *mddev); - void (*update_size)(struct mddev *mddev, sector_t old_dev_sectors); + int (*update_size)(struct mddev *mddev, sector_t old_dev_sectors); }; extern int md_setup_cluster(struct mddev *mddev, int nodes); diff --git a/drivers/md/md.c b/drivers/md/md.c --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -8023,7 +8023,7 @@ 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); + rv = mddev->cluster_ops->update_size(mddev, old_dev_sectors); else if (!mddev_is_dm(mddev)) set_capacity_and_notify(mddev->gendisk, mddev->array_sectors); @@ -10615,8 +10615,12 @@ void md_reap_sync_thread(struct mddev *mddev) */ if (mddev_is_clustered(mddev) && is_reshaped && mddev->pers->finish_reshape && - !test_bit(MD_CLOSING, &mddev->flags)) - mddev->cluster_ops->update_size(mddev, old_dev_sectors); + !test_bit(MD_CLOSING, &mddev->flags)) { + int ret = mddev->cluster_ops->update_size(mddev, old_dev_sectors); + + if (ret) + pr_warn("md: cluster update_size failed after reshape: %d\n", ret); + } /* flag recovery needed just to double check */ set_bit(MD_RECOVERY_NEEDED, &mddev->recovery); sysfs_notify_dirent_safe(mddev->sysfs_completed); -- 2.34.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/3] md-cluster: propagate update_size() errors to callers 2026-08-27 6:11 ` [PATCH v2 2/3] md-cluster: propagate update_size() errors to callers ghuicao @ 2026-08-27 6:23 ` sashiko-bot 0 siblings, 0 replies; 14+ messages in thread From: sashiko-bot @ 2026-08-27 6:23 UTC (permalink / raw) To: ghuicao; +Cc: yukuai, linux-raid Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: New issues: - [High] Self-deadlock in error path of update_size() when calling md_update_sb() while holding cluster locks. - [High] The patch fails its stated goal by returning success when a cluster capacity revert occurs. - [High] Early cluster communication error paths return an error without reverting the local array size, leaving the cluster in a split-brain state. - [Medium] The patch propagates hardcoded `-1` returns from update_size() which translates to `-EPERM` for userspace, violating standard kernel error codes. Pre-existing issues: - [High] lock_comm() leaks the MD_CLUSTER_SEND_LOCK bit on lock_token() failure, causing permanent hangs for future cluster operations. -- commit 10a3e68b8ff2647112ced9ea72fb738bc9b14bb0 Author: Cao Guanghui <caoguanghui@kylinos.cn> md-cluster: propagate update_size() errors to callers This patch updates the update_size() callback in the md_cluster_ops structure to return an integer instead of void. This allows error codes from cluster operations to be propagated up to the caller in md.c, instead of failing silently. > diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c > index 9f404660a1fb7..bc4118d8c5a9f 100644 > --- a/drivers/md/md-cluster.c > +++ b/drivers/md/md-cluster.c > @@ -1292,7 +1292,7 @@ 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; > struct cluster_msg cmsg; > @@ -1303,7 +1303,7 @@ static void update_size(struct mddev *mddev, sector_t old_dev_sectors) > md_update_sb(mddev, 1); > if (lock_comm(cinfo, 1)) { [Severity: High] This is a pre-existing issue, but does lock_comm() leak the MD_CLUSTER_SEND_LOCK bit on failure? Looking at lock_comm(), if test_and_set_bit(MD_CLUSTER_SEND_LOCK) succeeds, but lock_token() fails, lock_comm() directly returns the error without clearing the bit: lock_comm() { ... wait_event(cinfo->wait, !test_and_set_bit(MD_CLUSTER_SEND_LOCK, &cinfo->state)); rv = lock_token(cinfo); if (set_bit) clear_bit_unlock(MD_CLUSTER_HOLDING_MUTEX_FOR_RECVD, &cinfo->state); return rv; } If this occurs, will subsequent cluster operations permanently hang in wait_event() waiting for the bit to clear? > pr_err("%s: lock_comm failed\n", __func__); > - return; > + return -1; [Severity: Medium] Could returning -1 here cause userspace to see a confusing -EPERM error? Since the patch changes this callback to return int, using standard error codes like -EIO, -EAGAIN, or -ENODEV might be more appropriate than a hardcoded -1. [ ... ] > @@ -1325,12 +1325,12 @@ static void update_size(struct mddev *mddev, sector_t old_dev_sectors) > pr_err("%s:%d: failed to send METADATA_UPDATED msg\n", > __func__, __LINE__); > unlock_comm(cinfo); > - return; > + return ret; > } > } else { > pr_err("md-cluster: No good device id found to send\n"); > unlock_comm(cinfo); > - return; > + return -1; [Severity: Medium] Does returning -1 here also similarly risk propagating -EPERM to userspace for an internal configuration or device availability error? > } > > /* [ ... ] > 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); > } 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__); > md_update_sb(mddev, 1); [Severity: High] Could this call to md_update_sb() cause a self-deadlock? The current thread holds the cluster send lock because lock_comm() was called successfully at the start of update_size(). When md_update_sb() is called, it triggers a cluster metadata update: md_update_sb() -> mddev->cluster_ops->metadata_update_start() The metadata_update_start() path appears to wait for MD_CLUSTER_SEND_LOCK to be cleared. Since the lock is held by the same thread, will it wait forever? > ret = __sendmsg(cinfo, &cmsg); [Severity: High] Does this __sendmsg() call overwrite the failure state from the capacity revert? If the cluster sync size check fails, the result of the local revert operation is stored in 'ret' right above. By reassigning 'ret' to the result of __sendmsg() here, would a successful broadcast incorrectly return 0 (success) to userspace instead of propagating the failure of the resize? > if (ret) > pr_err("%s:%d: failed to send METADATA_UPDATED msg\n", > __func__, __LINE__); > } > unlock_comm(cinfo); > + return ret; > } > diff --git a/drivers/md/md.c b/drivers/md/md.c > index 680b34a63cb3d..b723fd9eccc0b 100644 > --- a/drivers/md/md.c > +++ b/drivers/md/md.c > @@ -8023,7 +8023,7 @@ 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); > + rv = mddev->cluster_ops->update_size(mddev, old_dev_sectors); [Severity: High] If the cluster update_size() operation fails early (for example, if lock_comm fails or it cannot find an active raid device), we now propagate the error back to userspace. However, the local array has already been resized by mddev->pers->resize() right above this call. Does this local resize need to be reverted if the cluster update fails, to prevent a split-brain state where the local node has a different capacity than the rest of the cluster? > else if (!mddev_is_dm(mddev)) > set_capacity_and_notify(mddev->gendisk, > mddev->array_sectors); -- Sashiko AI review · https://sashiko.dev/#/patchset/20260827061200.79753-1-ghuicao@163.com?part=2 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 3/3] md-cluster: fix ack_lockres leak in __sendmsg error path 2026-08-27 6:11 ` [PATCH v2 1/3] md-cluster: fix error handling and superblock update in update_size revert ghuicao 2026-08-27 6:11 ` [PATCH v2 2/3] md-cluster: propagate update_size() errors to callers ghuicao @ 2026-08-27 6:12 ` ghuicao 2026-08-27 6:24 ` sashiko-bot 2026-08-27 6:24 ` [PATCH v2 1/3] md-cluster: fix error handling and superblock update in update_size revert sashiko-bot 2026-08-27 8:44 ` [PATCH v3 1/3] md-cluster: fix lock_comm leak and __sendmsg error path ghuicao 3 siblings, 1 reply; 14+ messages in thread From: ghuicao @ 2026-08-27 6:12 UTC (permalink / raw) To: Song Liu Cc: Yu Kuai, Li Nan, Xiao Ni, linux-raid, linux-kernel, stable, Cao Guanghui From: Cao Guanghui <caoguanghui@kylinos.cn> In __sendmsg(), if the down-conversion of ack_lockres from EX to CR fails (step 5), the code jumps to failed_ack which only unlocks message_lockres. The ack_lockres is left in EX state, causing a cluster-wide deadlock as other nodes cannot acquire the ack lock. Restore ack_lockres to CR in the failed_ack path when an error occurred, so the lock is not leaked in EX state. Fixes: 601b515c5dcc ("Communication Framework: Sending functions") Cc: stable@vger.kernel.org Signed-off-by: Cao Guanghui <caoguanghui@kylinos.cn> --- drivers/md/md-cluster.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c --- a/drivers/md/md-cluster.c +++ b/drivers/md/md-cluster.c @@ -801,8 +801,10 @@ static int __sendmsg(struct md_cluster_info *cinfo, struct cluster_msg *cmsg) } failed_ack: + if (error) + dlm_lock_sync(cinfo->ack_lockres, DLM_LOCK_CR); 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; -- 2.34.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] md-cluster: fix ack_lockres leak in __sendmsg error path 2026-08-27 6:12 ` [PATCH v2 3/3] md-cluster: fix ack_lockres leak in __sendmsg error path ghuicao @ 2026-08-27 6:24 ` sashiko-bot 0 siblings, 0 replies; 14+ messages in thread From: sashiko-bot @ 2026-08-27 6:24 UTC (permalink / raw) To: ghuicao; +Cc: yukuai, linux-raid Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: New issues: - [High] The newly added error recovery logic silently fails if the original error was a timeout, leaving the lock leaked in EX state and defeating the patch's purpose. Pre-existing issues: - [High] An infinite loop and CPU soft lockup occur in the `failed_ack` cleanup path if a previous DLM lock operation timed out. -- commit aed4dde7a766dd41cf7dc88c61e2ae9c8fd7a612 Author: Cao Guanghui <caoguanghui@kylinos.cn> md-cluster: fix ack_lockres leak in __sendmsg error path In __sendmsg(), if the down-conversion of ack_lockres from EX to CR fails, the original code jumps to failed_ack and leaves ack_lockres in the EX state. This commit attempts to fix this by adding a conversion of ack_lockres to CR in the failed_ack path. > diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c > index bc4118d8c5a9f..7c6aa759d0c23 100644 > --- a/drivers/md/md-cluster.c > +++ b/drivers/md/md-cluster.c > @@ -801,6 +801,8 @@ static int __sendmsg(struct md_cluster_info *cinfo, struct cluster_msg *cmsg) > } > > failed_ack: > + if (error) > + dlm_lock_sync(cinfo->ack_lockres, DLM_LOCK_CR); [Severity: High] Will this error recovery logic silently fail if the original error was a timeout? If dlm_lock_sync() times out when acquiring DLM_LOCK_EX for ack_lockres earlier in the function, it leaves a pending DLM lock request in the background. Because of the uncancelled pending request, calling dlm_lock_sync(cinfo->ack_lockres, DLM_LOCK_CR) here immediately fails with -EBUSY and its return value is ignored. When the background EX request eventually completes, the lock is left in the EX state, which could still lead to a cluster-wide deadlock. > while ((unlock_error = dlm_unlock_sync(cinfo->message_lockres))) > pr_err("md-cluster: failed convert to NL on MESSAGE(%d)\n", > unlock_error); [Severity: High] This is a pre-existing issue, but does this create an infinite loop and CPU soft lockup if a previous DLM lock operation timed out? If a timeout occurs during dlm_lock_sync(cinfo->message_lockres, DLM_LOCK_CW) earlier in __sendmsg(), it leaves a pending lock conversion request in the DLM lockspace. Because of the pending request, dlm_unlock_sync() will immediately return -EBUSY without blocking, causing this to become an infinite loop that floods the kernel log and soft lockups the CPU core. -- Sashiko AI review · https://sashiko.dev/#/patchset/20260827061200.79753-1-ghuicao@163.com?part=3 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] md-cluster: fix error handling and superblock update in update_size revert 2026-08-27 6:11 ` [PATCH v2 1/3] md-cluster: fix error handling and superblock update in update_size revert ghuicao 2026-08-27 6:11 ` [PATCH v2 2/3] md-cluster: propagate update_size() errors to callers ghuicao 2026-08-27 6:12 ` [PATCH v2 3/3] md-cluster: fix ack_lockres leak in __sendmsg error path ghuicao @ 2026-08-27 6:24 ` sashiko-bot 2026-08-27 8:44 ` [PATCH v3 1/3] md-cluster: fix lock_comm leak and __sendmsg error path ghuicao 3 siblings, 0 replies; 14+ messages in thread From: sashiko-bot @ 2026-08-27 6:24 UTC (permalink / raw) To: ghuicao; +Cc: yukuai, linux-raid Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Guaranteed self-deadlock and potential double-unlocks in `update_size` due to calling `md_update_sb()` while the cluster communication lock is held. -- commit bae2db67348747b3864a225bb7b24be69937b7a2 Author: Cao Guanghui <caoguanghui@kylinos.cn> md-cluster: fix error handling and superblock update in update_size revert This commit addresses error handling and metadata updates when reverting an array resize operation in a cluster. It checks the return value of resize() and updates the superblock to match the in-memory array size. > diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c > index 24dac6f09da82..9f404660a1fb7 100644 > --- a/drivers/md/md-cluster.c > +++ b/drivers/md/md-cluster.c > @@ -1349,6 +1349,10 @@ 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__); > + md_update_sb(mddev, 1); [Severity: High] Does this call to md_update_sb() cause a self-deadlock? At this point in update_size(), the cluster communication lock (MD_CLUSTER_SEND_LOCK) is already held via lock_comm(). When md_update_sb() is called, it enters mddev->cluster_ops->metadata_update_start(mddev), which waits for MD_CLUSTER_SEND_LOCK to be cleared. Since the lock is held by the same thread, this appears to result in a strict self-deadlock during a failed array resize operation. Additionally, if the deadlock could somehow be bypassed, md_update_sb() would complete by calling metadata_update_finish(), which prematurely calls unlock_comm(), leading to a fatal double-unlock when update_size() later attempts to call unlock_comm() itself. > ret = __sendmsg(cinfo, &cmsg); > if (ret) > pr_err("%s:%d: failed to send METADATA_UPDATED msg\n", -- Sashiko AI review · https://sashiko.dev/#/patchset/20260827061200.79753-1-ghuicao@163.com?part=1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 1/3] md-cluster: fix lock_comm leak and __sendmsg error path 2026-08-27 6:11 ` [PATCH v2 1/3] md-cluster: fix error handling and superblock update in update_size revert ghuicao ` (2 preceding siblings ...) 2026-08-27 6:24 ` [PATCH v2 1/3] md-cluster: fix error handling and superblock update in update_size revert sashiko-bot @ 2026-08-27 8:44 ` ghuicao 2026-08-27 8:44 ` [PATCH v3 2/3] md-cluster: fix error handling and superblock consistency in update_size ghuicao ` (2 more replies) 3 siblings, 3 replies; 14+ messages in thread From: ghuicao @ 2026-08-27 8:44 UTC (permalink / raw) To: Song Liu Cc: Yu Kuai, Li Nan, Xiao Ni, linux-raid, linux-kernel, stable, Cao Guanghui From: Cao Guanghui <caoguanghui@kylinos.cn> Fix two error handling issues in cluster communication: 1. lock_comm() leaks MD_CLUSTER_SEND_LOCK if lock_token() fails. The bit is set by test_and_set_bit but never cleared on error, causing all subsequent cluster operations to hang permanently in wait_event(). Clear the bit before returning the error. 2. __sendmsg() has two problems in the failed_ack cleanup path: - ack_lockres is left in EX state if the down-conversion to CR fails, causing a cluster-wide deadlock. Attempt to restore it to CR and log if that also fails. - The while loop for message_lockres unlock spins forever if a previous DLM operation timed out and left a pending request (dlm_unlock_sync returns -EBUSY immediately). Change to a single attempt with error logging. Fixes: 818da59f97d6 ("md-cluster: add the support for resize") (lock_comm) Fixes: 601b515c5dcc ("Communication Framework: Sending functions") (__sendmsg) Cc: stable@vger.kernel.org Signed-off-by: Cao Guanghui <caoguanghui@kylinos.cn> --- drivers/md/md-cluster.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c --- a/drivers/md/md-cluster.c +++ b/drivers/md/md-cluster.c @@ -735,6 +735,8 @@ static int lock_comm(struct md_cluster_info *cinfo, bool mddev_locked) wait_event(cinfo->wait, !test_and_set_bit(MD_CLUSTER_SEND_LOCK, &cinfo->state)); rv = lock_token(cinfo); + if (rv) + clear_bit_unlock(MD_CLUSTER_SEND_LOCK, &cinfo->state); if (set_bit) clear_bit_unlock(MD_CLUSTER_HOLDING_MUTEX_FOR_RECVD, &cinfo->state); return rv; @@ -801,7 +803,15 @@ static int __sendmsg(struct md_cluster_info *cinfo, struct cluster_msg *cmsg) } failed_ack: - while ((unlock_error = dlm_unlock_sync(cinfo->message_lockres))) + if (error) { + int ack_ret = dlm_lock_sync(cinfo->ack_lockres, DLM_LOCK_CR); + + if (ack_ret) + pr_err("md-cluster: failed to restore ACK to CR (%d)\n", + ack_ret); + } + unlock_error = dlm_unlock_sync(cinfo->message_lockres); + if (unlock_error) pr_err("md-cluster: failed convert to NL on MESSAGE(%d)\n", unlock_error); -- 2.34.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 2/3] md-cluster: fix error handling and superblock consistency in update_size 2026-08-27 8:44 ` [PATCH v3 1/3] md-cluster: fix lock_comm leak and __sendmsg error path ghuicao @ 2026-08-27 8:44 ` ghuicao 2026-08-27 8:58 ` sashiko-bot 2026-08-27 8:44 ` [PATCH v3 3/3] md-cluster: revert local resize and propagate cluster errors ghuicao 2026-08-27 8:58 ` [PATCH v3 1/3] md-cluster: fix lock_comm leak and __sendmsg error path sashiko-bot 2 siblings, 1 reply; 14+ messages in thread From: ghuicao @ 2026-08-27 8:44 UTC (permalink / raw) To: Song Liu Cc: Yu Kuai, Li Nan, Xiao Ni, linux-raid, linux-kernel, stable, Cao Guanghui From: Cao Guanghui <caoguanghui@kylinos.cn> update_size() has multiple issues in the revert path when cluster_check_sync_size() detects that not all nodes have confirmed the new size: 1. (pre-existing) The return value of resize() is immediately overwritten by __sendmsg(), so a resize failure is silently lost. 2. (pre-existing) The on-disk superblock still retains the new size from the earlier md_update_sb() call. Other nodes re-read it and adopt the new size while the initiator runs with the reverted old size. 3. (pre-existing) The function returns void, so callers cannot detect failures. Fix all of the above by: - Using a separate variable for __sendmsg result so resize failure is not overwritten - Calling md_update_sb() after unlock_comm() to write the reverted size back to disk. This must be outside the locked section because md_update_sb() internally acquires MD_CLUSTER_SEND_LOCK via metadata_update_start(), which would self-deadlock if already held. - Changing return type from void to int with proper error codes (-EIO for lock failure, -ENODEV for no device, ret for others) Fixes: 818da59f97d6 ("md-cluster: add the support for resize") Cc: stable@vger.kernel.org Signed-off-by: Cao Guanghui <caoguanghui@kylinos.cn> --- drivers/md/md-cluster.c | 28 +++++++++++++++++++++++++--- drivers/md/md-cluster.h | 2 +- 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c --- 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) * let other nodes to perform it. If one node can't update sync_size * accordingly, we need to revert to previous value. */ -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; } memset(&cmsg, 0, sizeof(cmsg)); @@ -1335,12 +1336,12 @@ static int update_size(struct mddev *mddev, sector_t old_dev_sectors) pr_err("%s:%d: failed to send METADATA_UPDATED msg\n", __func__, __LINE__); unlock_comm(cinfo); - return; + return ret; } } else { pr_err("md-cluster: No good device id found to send\n"); unlock_comm(cinfo); - return; + return -ENODEV; } /* @@ -1359,12 +1360,28 @@ static int update_size(struct mddev *mddev, sector_t old_dev_sectors) } else { /* revert to previous sectors */ ret = mddev->pers->resize(mddev, old_dev_sectors); - ret = __sendmsg(cinfo, &cmsg); if (ret) + pr_err("%s:%d: failed to revert array size\n", + __func__, __LINE__); + reverted = true; + 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; } static int resync_start(struct mddev *mddev) diff --git a/drivers/md/md-cluster.h b/drivers/md/md-cluster.h --- a/drivers/md/md-cluster.h +++ b/drivers/md/md-cluster.h @@ -34,7 +34,7 @@ struct md_cluster_operations { int (*resize_bitmaps)(struct mddev *mddev, sector_t newsize, sector_t oldsize); int (*lock_all_bitmaps)(struct mddev *mddev); void (*unlock_all_bitmaps)(struct mddev *mddev); - void (*update_size)(struct mddev *mddev, sector_t old_dev_sectors); + int (*update_size)(struct mddev *mddev, sector_t old_dev_sectors); }; extern int md_setup_cluster(struct mddev *mddev, int nodes); -- 2.34.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/3] md-cluster: fix error handling and superblock consistency in update_size 2026-08-27 8:44 ` [PATCH v3 2/3] md-cluster: fix error handling and superblock consistency in update_size ghuicao @ 2026-08-27 8:58 ` sashiko-bot 0 siblings, 0 replies; 14+ messages in thread From: sashiko-bot @ 2026-08-27 8:58 UTC (permalink / raw) To: ghuicao; +Cc: yukuai, linux-raid 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 <caoguanghui@kylinos.cn> 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/20260827084453.124629-1-ghuicao@163.com?part=2 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 3/3] md-cluster: revert local resize and propagate cluster errors 2026-08-27 8:44 ` [PATCH v3 1/3] md-cluster: fix lock_comm leak and __sendmsg error path ghuicao 2026-08-27 8:44 ` [PATCH v3 2/3] md-cluster: fix error handling and superblock consistency in update_size ghuicao @ 2026-08-27 8:44 ` ghuicao 2026-08-27 9:04 ` sashiko-bot 2026-08-27 8:58 ` [PATCH v3 1/3] md-cluster: fix lock_comm leak and __sendmsg error path sashiko-bot 2 siblings, 1 reply; 14+ messages in thread From: ghuicao @ 2026-08-27 8:44 UTC (permalink / raw) To: Song Liu Cc: Yu Kuai, Li Nan, Xiao Ni, linux-raid, linux-kernel, stable, Cao Guanghui From: Cao Guanghui <caoguanghui@kylinos.cn> In md.c:update_size(), the local array is resized via pers->resize() before calling the cluster update_size() callback. If the cluster operation fails, the local node has the new size while the rest of the cluster does not, causing a split-brain state. Propagate the cluster update_size() return value and revert the local resize on failure. Also check the return value in the reshape completion path (md_reap_sync_thread) and log a warning on failure. Fixes: 818da59f97d6 ("md-cluster: add the support for resize") Cc: stable@vger.kernel.org Signed-off-by: Cao Guanghui <caoguanghui@kylinos.cn> --- drivers/md/md.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/drivers/md/md.c b/drivers/md/md.c --- 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); + if (rv) + mddev->pers->resize(mddev, old_dev_sectors); + } else if (!mddev_is_dm(mddev)) set_capacity_and_notify(mddev->gendisk, mddev->array_sectors); } @@ -10615,8 +10617,12 @@ void md_reap_sync_thread(struct mddev *mddev) */ if (mddev_is_clustered(mddev) && is_reshaped && mddev->pers->finish_reshape && - !test_bit(MD_CLOSING, &mddev->flags)) - mddev->cluster_ops->update_size(mddev, old_dev_sectors); + !test_bit(MD_CLOSING, &mddev->flags)) { + int ret = mddev->cluster_ops->update_size(mddev, old_dev_sectors); + + if (ret) + pr_warn("md: cluster update_size failed after reshape: %d\n", ret); + } /* flag recovery needed just to double check */ set_bit(MD_RECOVERY_NEEDED, &mddev->recovery); sysfs_notify_dirent_safe(mddev->sysfs_completed); -- 2.34.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/3] md-cluster: revert local resize and propagate cluster errors 2026-08-27 8:44 ` [PATCH v3 3/3] md-cluster: revert local resize and propagate cluster errors ghuicao @ 2026-08-27 9:04 ` sashiko-bot 0 siblings, 0 replies; 14+ messages in thread From: sashiko-bot @ 2026-08-27 9:04 UTC (permalink / raw) To: ghuicao; +Cc: linux-raid, yukuai 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 <caoguanghui@kylinos.cn> 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/20260827084453.124629-1-ghuicao@163.com?part=3 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/3] md-cluster: fix lock_comm leak and __sendmsg error path 2026-08-27 8:44 ` [PATCH v3 1/3] md-cluster: fix lock_comm leak and __sendmsg error path ghuicao 2026-08-27 8:44 ` [PATCH v3 2/3] md-cluster: fix error handling and superblock consistency in update_size ghuicao 2026-08-27 8:44 ` [PATCH v3 3/3] md-cluster: revert local resize and propagate cluster errors ghuicao @ 2026-08-27 8:58 ` sashiko-bot 2 siblings, 0 replies; 14+ messages in thread From: sashiko-bot @ 2026-08-27 8:58 UTC (permalink / raw) To: ghuicao; +Cc: yukuai, linux-raid Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: New issues: - [High] Missing `wake_up(&cinfo->wait)` after clearing `MD_CLUSTER_SEND_LOCK` on the error path in `lock_comm()`. - [High] The single-attempt lock release strategy leaves the DLM lock permanently stuck when there is a pending timed-out conversion. - [Medium] The fallback restoration of `ack_lockres` to `CR` incorrectly executes on all error paths, leading to redundant or synchronously failing requests. Pre-existing issues: - [High] `metadata_update_start()` suffers from the exact same lock leak pattern that the patch attempts to fix in `lock_comm()`. -- commit ab42e35df8bfdb28c9d4472b6f7586349538824c Author: Cao Guanghui <caoguanghui@kylinos.cn> md-cluster: fix lock_comm leak and __sendmsg error path This commit addresses error handling issues in cluster communication. It clears MD_CLUSTER_SEND_LOCK when lock_token() fails in lock_comm() to avoid hangs. It also modifies __sendmsg() to attempt restoring ack_lockres to CR and replaces an infinite loop with a single attempt when unlocking message_lockres. > diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c > index 24dac6f09da82..fb62fdb492350 100644 > --- a/drivers/md/md-cluster.c > +++ b/drivers/md/md-cluster.c > @@ -735,6 +735,8 @@ static int lock_comm(struct md_cluster_info *cinfo, bool mddev_locked) > wait_event(cinfo->wait, > !test_and_set_bit(MD_CLUSTER_SEND_LOCK, &cinfo->state)); > rv = lock_token(cinfo); > + if (rv) > + clear_bit_unlock(MD_CLUSTER_SEND_LOCK, &cinfo->state); [Severity: High] Does this clear operation also require a wake_up(&cinfo->wait)? When lock_token() fails, this patch clears the lock bit but appears to miss the wake_up() call. Any thread currently asleep in wait_event() waiting to acquire MD_CLUSTER_SEND_LOCK could remain asleep indefinitely because waitqueues require an explicit wakeup. > if (set_bit) > clear_bit_unlock(MD_CLUSTER_HOLDING_MUTEX_FOR_RECVD, &cinfo->state); > return rv; [ ... ] > @@ -801,7 +803,15 @@ static int __sendmsg(struct md_cluster_info *cinfo, struct cluster_msg *cmsg) > } > > failed_ack: > - while ((unlock_error = dlm_unlock_sync(cinfo->message_lockres))) > + if (error) { > + int ack_ret = dlm_lock_sync(cinfo->ack_lockres, DLM_LOCK_CR); [Severity: Medium] Will this unconditional fallback restoration fail synchronously? If __sendmsg() fails on message_lockres, ack_lockres is still in CR, making this conversion redundant. However, if it fails on ack_lockres due to a timeout, the DLM resource is already busy with a pending request. The immediate dlm_lock_sync() fallback to CR will fail synchronously with -EBUSY and fail to recover the lock state. > + > + if (ack_ret) > + pr_err("md-cluster: failed to restore ACK to CR (%d)\n", > + ack_ret); > + } > + unlock_error = dlm_unlock_sync(cinfo->message_lockres); > + if (unlock_error) > pr_err("md-cluster: failed convert to NL on MESSAGE(%d)\n", > unlock_error); [Severity: High] Could this single-attempt lock release strategy leave the DLM lock stuck? If dlm_lock_sync() times out (for example, when getting EX lock on MESSAGE), it returns -EBUSY but leaves the lock request active in DLM. This single dlm_unlock_sync() attempt translates to a new DLM lock request which is immediately rejected with -EBUSY by DLM due to the pending request. Proper recovery might require using dlm_unlock() with DLM_LKF_CANCEL to cancel the pending operation. [Severity: High] This isn't a bug introduced by this patch, but metadata_update_start() appears to suffer from the exact same lock leak pattern that this patch attempts to fix in lock_comm(). Looking at metadata_update_start() in drivers/md/md-cluster.c: ret = lock_token(cinfo); clear_bit_unlock(MD_CLUSTER_HOLDING_MUTEX_FOR_RECVD, &cinfo->state); return ret; If lock_token() fails here, it returns without clearing MD_CLUSTER_SEND_LOCK and without calling wake_up(&cinfo->wait), which could cause a permanent cluster-wide hang during metadata updates. -- Sashiko AI review · https://sashiko.dev/#/patchset/20260827084453.124629-1-ghuicao@163.com?part=1 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2026-08-27 9:05 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-08-27 2:52 [PATCH] md-cluster: check pers->resize() return value in update_size() ghuicao 2026-08-27 3:07 ` sashiko-bot 2026-08-27 6:11 ` [PATCH v2 1/3] md-cluster: fix error handling and superblock update in update_size revert ghuicao 2026-08-27 6:11 ` [PATCH v2 2/3] md-cluster: propagate update_size() errors to callers ghuicao 2026-08-27 6:23 ` sashiko-bot 2026-08-27 6:12 ` [PATCH v2 3/3] md-cluster: fix ack_lockres leak in __sendmsg error path ghuicao 2026-08-27 6:24 ` sashiko-bot 2026-08-27 6:24 ` [PATCH v2 1/3] md-cluster: fix error handling and superblock update in update_size revert sashiko-bot 2026-08-27 8:44 ` [PATCH v3 1/3] md-cluster: fix lock_comm leak and __sendmsg error path ghuicao 2026-08-27 8:44 ` [PATCH v3 2/3] md-cluster: fix error handling and superblock consistency in update_size ghuicao 2026-08-27 8:58 ` sashiko-bot 2026-08-27 8:44 ` [PATCH v3 3/3] md-cluster: revert local resize and propagate cluster errors ghuicao 2026-08-27 9:04 ` sashiko-bot 2026-08-27 8:58 ` [PATCH v3 1/3] md-cluster: fix lock_comm leak and __sendmsg error path sashiko-bot
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox