* [PATCH] md: avoid modifying spares while the array is not suspended
@ 2026-06-30 7:56 Abd-Alrhman Masalkhi
2026-06-30 10:18 ` sashiko-bot
2026-07-05 16:16 ` yu kuai
0 siblings, 2 replies; 8+ messages in thread
From: Abd-Alrhman Masalkhi @ 2026-06-30 7:56 UTC (permalink / raw)
To: song, yukuai, magiclinan, xiao
Cc: linux-raid, linux-kernel, Abd-Alrhman Masalkhi, sashiko-bot
remove_spares() and remove_and_add_spares() modify the array's rdev
configuration. These operations are only safe after the array has been
suspended.
Today, md_start_sync() can call md_choose_sync_action() even when the
array has not been suspended. As a result, md_choose_sync_action() may
remove or replace rdevs while normal I/O is still accessing them.
The race can occur as follows:
raid10d Worker Normal IO
____________ _______________________ ______________________
raid10_write_request()
wait_blocked_dev()
set Blocked
set Faulty
Skip Faulty rdev
rrdev->nr_pending++
.repl_bio = bio
removeable_rdev = false .
array not suspended .
lock mddev goto err_handle
lock mddev (wait)
.
update sb .
clear Blocked .
.
unlock mddev .
lock mddev (acquires)
remove_spares()
removeable_rdev = true
raid10_remove_disk()
rdev = replacement
replacement = NULL
rdev_dec_pending(NULL)
unlock mddev (NULL)->nr_pending--
In this case, rdev_dec_pending() is called with a NULL pointer,
resulting in a NULL pointer dereference when attempting to decrement
nr_pending.
Fix this by ensuring that remove_spares() and remove_and_add_spares()
are only called after the array has been suspended, preventing
concurrent rdev configuration changes while normal I/O is in progress.
Fixes: bc08041b32ab ("md: suspend array in md_start_sync() if array need reconfiguration")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260628142420.1051027-1-abd.masalkhi@gmail.com?part=3
Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
drivers/md/md.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 66a41d482e59..c85ebb59535b 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -10116,7 +10116,8 @@ static int remove_and_add_spares(struct mddev *mddev,
return spares;
}
-static bool md_choose_sync_action(struct mddev *mddev, int *spares)
+static bool md_choose_sync_action(struct mddev *mddev, int *spares,
+ bool array_suspended)
{
/* Check if reshape is in progress first. */
if (mddev->reshape_position != MaxSector) {
@@ -10132,7 +10133,9 @@ static bool md_choose_sync_action(struct mddev *mddev, int *spares)
/* Check if resync is in progress. */
if (mddev->resync_offset < MaxSector) {
- remove_spares(mddev, NULL);
+ if (array_suspended)
+ remove_spares(mddev, NULL);
+
set_bit(MD_RECOVERY_SYNC, &mddev->recovery);
clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
clear_bit(MD_RECOVERY_LAZY_RECOVER, &mddev->recovery);
@@ -10144,7 +10147,11 @@ static bool md_choose_sync_action(struct mddev *mddev, int *spares)
* also removed and re-added, to allow the personality to fail the
* re-add.
*/
- *spares = remove_and_add_spares(mddev, NULL);
+ if (array_suspended)
+ *spares = remove_and_add_spares(mddev, NULL);
+ else
+ *spares = 0;
+
if (*spares || test_bit(MD_RECOVERY_LAZY_RECOVER, &mddev->recovery)) {
clear_bit(MD_RECOVERY_SYNC, &mddev->recovery);
clear_bit(MD_RECOVERY_CHECK, &mddev->recovery);
@@ -10189,11 +10196,12 @@ static void md_start_sync(struct work_struct *ws)
* As we only add devices that are already in-sync, we can
* activate the spares immediately.
*/
- remove_and_add_spares(mddev, NULL);
+ if (suspend)
+ remove_and_add_spares(mddev, NULL);
goto not_running;
}
- if (!md_choose_sync_action(mddev, &spares))
+ if (!md_choose_sync_action(mddev, &spares, suspend))
goto not_running;
if (!mddev->pers->sync_request)
--
2.43.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] md: avoid modifying spares while the array is not suspended
2026-06-30 7:56 [PATCH] md: avoid modifying spares while the array is not suspended Abd-Alrhman Masalkhi
@ 2026-06-30 10:18 ` sashiko-bot
2026-07-05 16:16 ` yu kuai
1 sibling, 0 replies; 8+ messages in thread
From: sashiko-bot @ 2026-06-30 10:18 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi; +Cc: linux-raid, yukuai
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [High] The patch prevents the recovery thread from starting or resuming for existing rebuilding spares, silently abandoning recovery and leaving the array permanently degraded.
Pre-existing issues:
- [High] The patch addresses a TOCTOU race for `state_store` but misses other vulnerable paths like `slot_store` and cluster updates, which still call `remove_and_add_spares()` without array suspension.
--
commit 16cf64f20d25bc671dc52422ec0a44672674e524
Author: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
md: avoid modifying spares while the array is not suspended
This commit ensures that remove_spares() and remove_and_add_spares() are
only called after the array has been suspended. This prevents a race
condition where normal I/O could trigger a NULL pointer dereference when
rdevs are removed or replaced concurrently.
Closes: https://sashiko.dev/#/patchset/20260628142420.1051027-1-abd.masalkhi@gmail.com?part=3
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index d1465bcd86c81..f555eb5aca5d8 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -10151,7 +10154,11 @@ static bool md_choose_sync_action(struct mddev *mddev, int *spares)
> * also removed and re-added, to allow the personality to fail the
> * re-add.
> */
> - *spares = remove_and_add_spares(mddev, NULL);
> + if (array_suspended)
> + *spares = remove_and_add_spares(mddev, NULL);
> + else
> + *spares = 0;
> +
[Severity: High]
Does this else branch prevent the recovery thread from starting or resuming
for existing rebuilding spares?
In md_start_sync(), suspend is determined by checking
md_spares_need_change(mddev). For an existing recovering spare receiving
normal I/O (where nr_pending > 0), both rdev_removeable() and
rdev_addable() can evaluate to false. This causes md_spares_need_change()
to return false, leaving suspend as false.
When md_choose_sync_action() is called with array_suspended == false, this
new else branch forces *spares = 0. By completely bypassing the counting of
existing recovering spares, md_choose_sync_action() will return false and
skip starting the sync thread.
Since nr_pending dropping to zero on a non-faulty spare doesn't trigger a
new MD_RECOVERY_NEEDED event, could this silently abandon recovery and
leave the array permanently degraded?
[Severity: High]
This is a pre-existing issue, but does restricting remove_and_add_spares()
calls to suspended arrays in md_choose_sync_action() leave other
vulnerable paths exposed?
The commit addresses a TOCTOU race by ensuring remove_and_add_spares() is
only called when the array is suspended, preventing concurrent I/O from
encountering a cleared replacement pointer.
However, slot_store() can still call remove_and_add_spares() without an
array suspension when a user writes "none" (slot == -1):
drivers/md/md.c:slot_store() {
...
clear_bit(Blocked, &rdev->flags);
remove_and_add_spares(rdev->mddev, rdev);
if (rdev->raid_disk >= 0)
return -EBUSY;
...
}
Similarly, the MD cluster component updates the array configuration in
check_sb_changes() by calling remove_and_add_spares() while the array is
not suspended.
Could normal I/O still increment nr_pending on a replacement drive just
before remove_spares() removes the faulty drive and clears the replacement
pointer in these paths, reproducing the exact TOCTOU crash this patch
aims to fix?
> @@ -10196,11 +10203,12 @@ static void md_start_sync(struct work_struct *ws)
> * As we only add devices that are already in-sync, we can
> * activate the spares immediately.
> */
> - remove_and_add_spares(mddev, NULL);
> + if (suspend)
> + remove_and_add_spares(mddev, NULL);
> goto not_running;
> }
>
> - if (!md_choose_sync_action(mddev, &spares))
> + if (!md_choose_sync_action(mddev, &spares, suspend))
> goto not_running;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260630075640.1081634-1-abd.masalkhi@gmail.com?part=1
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] md: avoid modifying spares while the array is not suspended
2026-06-30 7:56 [PATCH] md: avoid modifying spares while the array is not suspended Abd-Alrhman Masalkhi
2026-06-30 10:18 ` sashiko-bot
@ 2026-07-05 16:16 ` yu kuai
2026-07-05 19:58 ` Abd-Alrhman Masalkhi
1 sibling, 1 reply; 8+ messages in thread
From: yu kuai @ 2026-07-05 16:16 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi, song, magiclinan, xiao, yukuai
Cc: linux-raid, linux-kernel, sashiko-bot
Hi,
在 2026/6/30 15:56, Abd-Alrhman Masalkhi 写道:
> remove_spares() and remove_and_add_spares() modify the array's rdev
> configuration. These operations are only safe after the array has been
> suspended.
>
> Today, md_start_sync() can call md_choose_sync_action() even when the
> array has not been suspended. As a result, md_choose_sync_action() may
> remove or replace rdevs while normal I/O is still accessing them.
>
> The race can occur as follows:
>
> raid10d Worker Normal IO
> ____________ _______________________ ______________________
>
> raid10_write_request()
> wait_blocked_dev()
> set Blocked
> set Faulty
> Skip Faulty rdev
> rrdev->nr_pending++
> .repl_bio = bio
> removeable_rdev = false .
> array not suspended .
> lock mddev goto err_handle
> lock mddev (wait)
> .
> update sb .
> clear Blocked .
> .
> unlock mddev .
> lock mddev (acquires)
> remove_spares()
> removeable_rdev = true
>
> raid10_remove_disk()
> rdev = replacement
> replacement = NULL
> rdev_dec_pending(NULL)
> unlock mddev (NULL)->nr_pending--
>
> In this case, rdev_dec_pending() is called with a NULL pointer,
> resulting in a NULL pointer dereference when attempting to decrement
> nr_pending.
>
> Fix this by ensuring that remove_spares() and remove_and_add_spares()
> are only called after the array has been suspended, preventing
> concurrent rdev configuration changes while normal I/O is in progress.
>
> Fixes: bc08041b32ab ("md: suspend array in md_start_sync() if array need reconfiguration")
> Reported-by: sashiko-bot <sashiko-bot@kernel.org>
> Closes: https://sashiko.dev/#/patchset/20260628142420.1051027-1-abd.masalkhi@gmail.com?part=3
> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
> ---
> drivers/md/md.c | 18 +++++++++++++-----
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 66a41d482e59..c85ebb59535b 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -10116,7 +10116,8 @@ static int remove_and_add_spares(struct mddev *mddev,
> return spares;
> }
>
> -static bool md_choose_sync_action(struct mddev *mddev, int *spares)
> +static bool md_choose_sync_action(struct mddev *mddev, int *spares,
> + bool array_suspended)
> {
> /* Check if reshape is in progress first. */
> if (mddev->reshape_position != MaxSector) {
> @@ -10132,7 +10133,9 @@ static bool md_choose_sync_action(struct mddev *mddev, int *spares)
>
> /* Check if resync is in progress. */
> if (mddev->resync_offset < MaxSector) {
> - remove_spares(mddev, NULL);
> + if (array_suspended)
> + remove_spares(mddev, NULL);
The problem looks real, however, I think this will cause a change that user will be awared,
if there are really spares that can be removed from conf, but array is not suspended here,
user will still expect rdev will be removed from conf automatically.
In md_start_sync, if suspend is false, can we check again after mddev_lock? If suspend is
supposed to be true, we can release the lock and retry with suspend = true.
> +
> set_bit(MD_RECOVERY_SYNC, &mddev->recovery);
> clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
> clear_bit(MD_RECOVERY_LAZY_RECOVER, &mddev->recovery);
> @@ -10144,7 +10147,11 @@ static bool md_choose_sync_action(struct mddev *mddev, int *spares)
> * also removed and re-added, to allow the personality to fail the
> * re-add.
> */
> - *spares = remove_and_add_spares(mddev, NULL);
> + if (array_suspended)
> + *spares = remove_and_add_spares(mddev, NULL);
> + else
> + *spares = 0;
> +
> if (*spares || test_bit(MD_RECOVERY_LAZY_RECOVER, &mddev->recovery)) {
> clear_bit(MD_RECOVERY_SYNC, &mddev->recovery);
> clear_bit(MD_RECOVERY_CHECK, &mddev->recovery);
> @@ -10189,11 +10196,12 @@ static void md_start_sync(struct work_struct *ws)
> * As we only add devices that are already in-sync, we can
> * activate the spares immediately.
> */
> - remove_and_add_spares(mddev, NULL);
> + if (suspend)
> + remove_and_add_spares(mddev, NULL);
> goto not_running;
> }
>
> - if (!md_choose_sync_action(mddev, &spares))
> + if (!md_choose_sync_action(mddev, &spares, suspend))
> goto not_running;
>
> if (!mddev->pers->sync_request)
--
Thanks,
Kuai
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] md: avoid modifying spares while the array is not suspended
2026-07-05 16:16 ` yu kuai
@ 2026-07-05 19:58 ` Abd-Alrhman Masalkhi
2026-07-07 1:12 ` yu kuai
0 siblings, 1 reply; 8+ messages in thread
From: Abd-Alrhman Masalkhi @ 2026-07-05 19:58 UTC (permalink / raw)
To: yu kuai, song, magiclinan, xiao, yukuai
Cc: linux-raid, linux-kernel, sashiko-bot
Hi Kuai,
On Mon, Jul 06, 2026 at 00:16 +0800, yu kuai wrote:
> Hi,
>
> 在 2026/6/30 15:56, Abd-Alrhman Masalkhi 写道:
>> remove_spares() and remove_and_add_spares() modify the array's rdev
>> configuration. These operations are only safe after the array has been
>> suspended.
>>
>> Today, md_start_sync() can call md_choose_sync_action() even when the
>> array has not been suspended. As a result, md_choose_sync_action() may
>> remove or replace rdevs while normal I/O is still accessing them.
>>
>> The race can occur as follows:
>>
>> raid10d Worker Normal IO
>> ____________ _______________________ ______________________
>>
>> raid10_write_request()
>> wait_blocked_dev()
>> set Blocked
>> set Faulty
>> Skip Faulty rdev
>> rrdev->nr_pending++
>> .repl_bio = bio
>> removeable_rdev = false .
>> array not suspended .
>> lock mddev goto err_handle
>> lock mddev (wait)
>> .
>> update sb .
>> clear Blocked .
>> .
>> unlock mddev .
>> lock mddev (acquires)
>> remove_spares()
>> removeable_rdev = true
>>
>> raid10_remove_disk()
>> rdev = replacement
>> replacement = NULL
>> rdev_dec_pending(NULL)
>> unlock mddev (NULL)->nr_pending--
>>
>> In this case, rdev_dec_pending() is called with a NULL pointer,
>> resulting in a NULL pointer dereference when attempting to decrement
>> nr_pending.
>>
>> Fix this by ensuring that remove_spares() and remove_and_add_spares()
>> are only called after the array has been suspended, preventing
>> concurrent rdev configuration changes while normal I/O is in progress.
>>
>> Fixes: bc08041b32ab ("md: suspend array in md_start_sync() if array need reconfiguration")
>> Reported-by: sashiko-bot <sashiko-bot@kernel.org>
>> Closes: https://sashiko.dev/#/patchset/20260628142420.1051027-1-abd.masalkhi@gmail.com?part=3
>> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
>> ---
>> drivers/md/md.c | 18 +++++++++++++-----
>> 1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 66a41d482e59..c85ebb59535b 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -10116,7 +10116,8 @@ static int remove_and_add_spares(struct mddev *mddev,
>> return spares;
>> }
>>
>> -static bool md_choose_sync_action(struct mddev *mddev, int *spares)
>> +static bool md_choose_sync_action(struct mddev *mddev, int *spares,
>> + bool array_suspended)
>> {
>> /* Check if reshape is in progress first. */
>> if (mddev->reshape_position != MaxSector) {
>> @@ -10132,7 +10133,9 @@ static bool md_choose_sync_action(struct mddev *mddev, int *spares)
>>
>> /* Check if resync is in progress. */
>> if (mddev->resync_offset < MaxSector) {
>> - remove_spares(mddev, NULL);
>> + if (array_suspended)
>> + remove_spares(mddev, NULL);
>
> The problem looks real, however, I think this will cause a change that user will be awared,
> if there are really spares that can be removed from conf, but array is not suspended here,
> user will still expect rdev will be removed from conf automatically.
>
> In md_start_sync, if suspend is false, can we check again after mddev_lock? If suspend is
> supposed to be true, we can release the lock and retry with suspend = true.
>
Yes, I see, and your approach is much better. But what do you think
about taking the lock first and then checking only once?
>> +
>> set_bit(MD_RECOVERY_SYNC, &mddev->recovery);
>> clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
>> clear_bit(MD_RECOVERY_LAZY_RECOVER, &mddev->recovery);
>> @@ -10144,7 +10147,11 @@ static bool md_choose_sync_action(struct mddev *mddev, int *spares)
>> * also removed and re-added, to allow the personality to fail the
>> * re-add.
>> */
>> - *spares = remove_and_add_spares(mddev, NULL);
>> + if (array_suspended)
>> + *spares = remove_and_add_spares(mddev, NULL);
>> + else
>> + *spares = 0;
>> +
>> if (*spares || test_bit(MD_RECOVERY_LAZY_RECOVER, &mddev->recovery)) {
>> clear_bit(MD_RECOVERY_SYNC, &mddev->recovery);
>> clear_bit(MD_RECOVERY_CHECK, &mddev->recovery);
>> @@ -10189,11 +10196,12 @@ static void md_start_sync(struct work_struct *ws)
>> * As we only add devices that are already in-sync, we can
>> * activate the spares immediately.
>> */
>> - remove_and_add_spares(mddev, NULL);
>> + if (suspend)
>> + remove_and_add_spares(mddev, NULL);
>> goto not_running;
>> }
>>
>> - if (!md_choose_sync_action(mddev, &spares))
>> + if (!md_choose_sync_action(mddev, &spares, suspend))
>> goto not_running;
>>
>> if (!mddev->pers->sync_request)
>
> --
> Thanks,
> Kuai
--
Best Regards,
Abd-Alrhman
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] md: avoid modifying spares while the array is not suspended
2026-07-05 19:58 ` Abd-Alrhman Masalkhi
@ 2026-07-07 1:12 ` yu kuai
2026-07-07 10:35 ` Abd-Alrhman Masalkhi
0 siblings, 1 reply; 8+ messages in thread
From: yu kuai @ 2026-07-07 1:12 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi, yu kuai, song, magiclinan, xiao
Cc: linux-raid, linux-kernel, sashiko-bot
Hi,
在 2026/7/6 3:58, Abd-Alrhman Masalkhi 写道:
>> The problem looks real, however, I think this will cause a change that user will be awared,
>> if there are really spares that can be removed from conf, but array is not suspended here,
>> user will still expect rdev will be removed from conf automatically.
>>
>> In md_start_sync, if suspend is false, can we check again after mddev_lock? If suspend is
>> supposed to be true, we can release the lock and retry with suspend = true.
>>
> Yes, I see, and your approach is much better. But what do you think
> about taking the lock first and then checking only once?
I don't get what you mean. If we take the lock and then check that array should
suspend, we still have to release the lock before we suspend the array.
--
Thanks,
Kuai
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] md: avoid modifying spares while the array is not suspended
2026-07-07 1:12 ` yu kuai
@ 2026-07-07 10:35 ` Abd-Alrhman Masalkhi
2026-07-08 8:32 ` yu kuai
0 siblings, 1 reply; 8+ messages in thread
From: Abd-Alrhman Masalkhi @ 2026-07-07 10:35 UTC (permalink / raw)
To: yu kuai, yu kuai, song, magiclinan, xiao
Cc: linux-raid, linux-kernel, sashiko-bot
Hi Kuai,
On Tue, Jul 07, 2026 at 09:12 +0800, yu kuai wrote:
> Hi,
>
> 在 2026/7/6 3:58, Abd-Alrhman Masalkhi 写道:
>>> The problem looks real, however, I think this will cause a change that user will be awared,
>>> if there are really spares that can be removed from conf, but array is not suspended here,
>>> user will still expect rdev will be removed from conf automatically.
>>>
>>> In md_start_sync, if suspend is false, can we check again after mddev_lock? If suspend is
>>> supposed to be true, we can release the lock and retry with suspend = true.
>>>
>> Yes, I see, and your approach is much better. But what do you think
>> about taking the lock first and then checking only once?
>
> I don't get what you mean. If we take the lock and then check that array should
> suspend, we still have to release the lock before we suspend the array.
>
Sorry, I was not clear. I meant, do we need to check twice, once before
taking the lock and once after? It seems that the check before taking the
lock is redundant. since the result would need to be checked again after
taking the lock anyway.
Could we drop the check before taking the lock and only check whether
suspension is needed once while holding it? If suspension is needed, we
would release the lock, suspend the array, and then reacquire the lock.
> --
> Thanks,
> Kuai
--
Best Regards,
Abd-Alrhman
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] md: avoid modifying spares while the array is not suspended
2026-07-07 10:35 ` Abd-Alrhman Masalkhi
@ 2026-07-08 8:32 ` yu kuai
2026-07-08 8:59 ` Abd-Alrhman Masalkhi
0 siblings, 1 reply; 8+ messages in thread
From: yu kuai @ 2026-07-08 8:32 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi, yu kuai, song, magiclinan, xiao
Cc: linux-raid, linux-kernel, sashiko-bot
Hi,
在 2026/7/7 18:35, Abd-Alrhman Masalkhi 写道:
> Hi Kuai,
>
> On Tue, Jul 07, 2026 at 09:12 +0800, yu kuai wrote:
>> Hi,
>>
>> 在 2026/7/6 3:58, Abd-Alrhman Masalkhi 写道:
>>>> The problem looks real, however, I think this will cause a change that user will be awared,
>>>> if there are really spares that can be removed from conf, but array is not suspended here,
>>>> user will still expect rdev will be removed from conf automatically.
>>>>
>>>> In md_start_sync, if suspend is false, can we check again after mddev_lock? If suspend is
>>>> supposed to be true, we can release the lock and retry with suspend = true.
>>>>
>>> Yes, I see, and your approach is much better. But what do you think
>>> about taking the lock first and then checking only once?
>> I don't get what you mean. If we take the lock and then check that array should
>> suspend, we still have to release the lock before we suspend the array.
>>
> Sorry, I was not clear. I meant, do we need to check twice, once before
> taking the lock and once after? It seems that the check before taking the
> lock is redundant. since the result would need to be checked again after
> taking the lock anyway.
>
> Could we drop the check before taking the lock and only check whether
> suspension is needed once while holding it? If suspension is needed, we
> would release the lock, suspend the array, and then reacquire the lock.
Thanks for the explanation, I understand now. Howerver, I still prefer to check
first before holding the lock. Because the checking is much lower overhead than
acquire reconfig_mutex, and the race window that rdev become spare is small, so
it's unlikely we'll acquire reconfig_mtuex twice.
>
>> --
>> Thanks,
>> Kuai
--
Thanks,
Kuai
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] md: avoid modifying spares while the array is not suspended
2026-07-08 8:32 ` yu kuai
@ 2026-07-08 8:59 ` Abd-Alrhman Masalkhi
0 siblings, 0 replies; 8+ messages in thread
From: Abd-Alrhman Masalkhi @ 2026-07-08 8:59 UTC (permalink / raw)
To: yu kuai, yu kuai, song, magiclinan, xiao
Cc: linux-raid, linux-kernel, sashiko-bot
Hi Kuai,
On Wed, Jul 08, 2026 at 16:32 +0800, yu kuai wrote:
> Hi,
>
> 在 2026/7/7 18:35, Abd-Alrhman Masalkhi 写道:
>> Hi Kuai,
>>
>> On Tue, Jul 07, 2026 at 09:12 +0800, yu kuai wrote:
>>> Hi,
>>>
>>> 在 2026/7/6 3:58, Abd-Alrhman Masalkhi 写道:
>>>>> The problem looks real, however, I think this will cause a change that user will be awared,
>>>>> if there are really spares that can be removed from conf, but array is not suspended here,
>>>>> user will still expect rdev will be removed from conf automatically.
>>>>>
>>>>> In md_start_sync, if suspend is false, can we check again after mddev_lock? If suspend is
>>>>> supposed to be true, we can release the lock and retry with suspend = true.
>>>>>
>>>> Yes, I see, and your approach is much better. But what do you think
>>>> about taking the lock first and then checking only once?
>>> I don't get what you mean. If we take the lock and then check that array should
>>> suspend, we still have to release the lock before we suspend the array.
>>>
>> Sorry, I was not clear. I meant, do we need to check twice, once before
>> taking the lock and once after? It seems that the check before taking the
>> lock is redundant. since the result would need to be checked again after
>> taking the lock anyway.
>>
>> Could we drop the check before taking the lock and only check whether
>> suspension is needed once while holding it? If suspension is needed, we
>> would release the lock, suspend the array, and then reacquire the lock.
>
> Thanks for the explanation, I understand now. Howerver, I still prefer to check
> first before holding the lock. Because the checking is much lower overhead than
> acquire reconfig_mutex, and the race window that rdev become spare is small, so
> it's unlikely we'll acquire reconfig_mtuex twice.
>
I see, thanks for the explanation. I'll send a v2 shortly.
>>
>>> --
>>> Thanks,
>>> Kuai
>
> --
> Thanks,
> Kuai
--
Best Regards,
Abd-Alrhman
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2026-07-08 8:59 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-06-30 7:56 [PATCH] md: avoid modifying spares while the array is not suspended Abd-Alrhman Masalkhi
2026-06-30 10:18 ` sashiko-bot
2026-07-05 16:16 ` yu kuai
2026-07-05 19:58 ` Abd-Alrhman Masalkhi
2026-07-07 1:12 ` yu kuai
2026-07-07 10:35 ` Abd-Alrhman Masalkhi
2026-07-08 8:32 ` yu kuai
2026-07-08 8:59 ` Abd-Alrhman Masalkhi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox