Unregister sync_thread doesn't need to hold reconfig_mutex since it
doesn't reconfigure array.
And it could cause deadlock problem for raid5 as follows:
1. process A tried to reap sync thread with reconfig_mutex held after echo
idle to sync_action.
2. raid5 sync thread was blocked if there were too many active stripes.
3. SB_CHANGE_PENDING was set (because of write IO comes from upper layer)
which causes the number of active stripes can't be decreased.
4. SB_CHANGE_PENDING can't be cleared since md_check_recovery was not able
to hold reconfig_mutex.
More details in the link:
https://lore.kernel.org/linux-raid/5ed54ffc-ce82-bf66-4eff-390cb23bc1ac@molgen.mpg.de/T/#t
And add one parameter to md_reap_sync_thread since it could be called by
dm-raid which doesn't hold reconfig_mutex.
Reported-and-tested-by: Donald Buczek <redacted>
Signed-off-by: Guoqing Jiang <redacted>
---
V2:
1. add one parameter to md_reap_sync_thread per Jack's suggestion.
drivers/md/dm-raid.c | 2 +-
drivers/md/md.c | 14 +++++++++-----
drivers/md/md.h | 2 +-
3 files changed, 11 insertions(+), 7 deletions(-)
@@ -9291,7 +9291,7 @@ void md_check_recovery(struct mddev *mddev)gotounlock;}if(mddev->sync_thread){-md_reap_sync_thread(mddev);+md_reap_sync_thread(mddev,true);gotounlock;}/* Set RUNNING before clearing NEEDED to avoid
@@ -9364,14 +9364,18 @@ void md_check_recovery(struct mddev *mddev)}EXPORT_SYMBOL(md_check_recovery);-voidmd_reap_sync_thread(structmddev*mddev)+voidmd_reap_sync_thread(structmddev*mddev,boolreconfig_mutex_held){structmd_rdev*rdev;sector_told_dev_sectors=mddev->dev_sectors;boolis_reshaped=false;/* resync has finished, collect result */+if(reconfig_mutex_held)+mddev_unlock(mddev);md_unregister_thread(&mddev->sync_thread);+if(reconfig_mutex_held)+mddev_lock_nointr(mddev);if(!test_bit(MD_RECOVERY_INTR,&mddev->recovery)&&!test_bit(MD_RECOVERY_REQUESTED,&mddev->recovery)&&mddev->degraded!=mddev->raid_disks){
From: Paul Menzel <hidden> Date: 2021-02-15 11:08:44
[+cc Donald]
Am 13.02.21 um 01:49 schrieb Guoqing Jiang:
quoted hunk
Unregister sync_thread doesn't need to hold reconfig_mutex since it
doesn't reconfigure array.
And it could cause deadlock problem for raid5 as follows:
1. process A tried to reap sync thread with reconfig_mutex held after echo
idle to sync_action.
2. raid5 sync thread was blocked if there were too many active stripes.
3. SB_CHANGE_PENDING was set (because of write IO comes from upper layer)
which causes the number of active stripes can't be decreased.
4. SB_CHANGE_PENDING can't be cleared since md_check_recovery was not able
to hold reconfig_mutex.
More details in the link:
https://lore.kernel.org/linux-raid/5ed54ffc-ce82-bf66-4eff-390cb23bc1ac@molgen.mpg.de/T/#t
And add one parameter to md_reap_sync_thread since it could be called by
dm-raid which doesn't hold reconfig_mutex.
Reported-and-tested-by: Donald Buczek <redacted>
Signed-off-by: Guoqing Jiang <redacted>
---
V2:
1. add one parameter to md_reap_sync_thread per Jack's suggestion.
drivers/md/dm-raid.c | 2 +-
drivers/md/md.c | 14 +++++++++-----
drivers/md/md.h | 2 +-
3 files changed, 11 insertions(+), 7 deletions(-)
@@ -9291,7 +9291,7 @@ void md_check_recovery(struct mddev *mddev)gotounlock;}if(mddev->sync_thread){-md_reap_sync_thread(mddev);+md_reap_sync_thread(mddev,true);gotounlock;}/* Set RUNNING before clearing NEEDED to avoid
@@ -9364,14 +9364,18 @@ void md_check_recovery(struct mddev *mddev)}EXPORT_SYMBOL(md_check_recovery);-voidmd_reap_sync_thread(structmddev*mddev)+voidmd_reap_sync_thread(structmddev*mddev,boolreconfig_mutex_held){structmd_rdev*rdev;sector_told_dev_sectors=mddev->dev_sectors;boolis_reshaped=false;/* resync has finished, collect result */+if(reconfig_mutex_held)+mddev_unlock(mddev);md_unregister_thread(&mddev->sync_thread);+if(reconfig_mutex_held)+mddev_lock_nointr(mddev);if(!test_bit(MD_RECOVERY_INTR,&mddev->recovery)&&!test_bit(MD_RECOVERY_REQUESTED,&mddev->recovery)&&mddev->degraded!=mddev->raid_disks){
From: Song Liu <song@kernel.org> Date: 2021-02-24 09:12:08
On Mon, Feb 15, 2021 at 3:08 AM Paul Menzel [off-list ref] wrote:
[+cc Donald]
Am 13.02.21 um 01:49 schrieb Guoqing Jiang:
quoted
Unregister sync_thread doesn't need to hold reconfig_mutex since it
doesn't reconfigure array.
And it could cause deadlock problem for raid5 as follows:
1. process A tried to reap sync thread with reconfig_mutex held after echo
idle to sync_action.
2. raid5 sync thread was blocked if there were too many active stripes.
3. SB_CHANGE_PENDING was set (because of write IO comes from upper layer)
which causes the number of active stripes can't be decreased.
4. SB_CHANGE_PENDING can't be cleared since md_check_recovery was not able
to hold reconfig_mutex.
More details in the link:
https://lore.kernel.org/linux-raid/5ed54ffc-ce82-bf66-4eff-390cb23bc1ac@molgen.mpg.de/T/#t
And add one parameter to md_reap_sync_thread since it could be called by
dm-raid which doesn't hold reconfig_mutex.
Reported-and-tested-by: Donald Buczek <redacted>
Signed-off-by: Guoqing Jiang <redacted>
I don't really like this fix. But I haven't got a better (and not too
complicated)
alternative.
quoted
---
V2:
1. add one parameter to md_reap_sync_thread per Jack's suggestion.
drivers/md/dm-raid.c | 2 +-
drivers/md/md.c | 14 +++++++++-----
drivers/md/md.h | 2 +-
3 files changed, 11 insertions(+), 7 deletions(-)
On Mon, Feb 15, 2021 at 3:08 AM Paul Menzel [off-list ref] wrote:
quoted
[+cc Donald]
Am 13.02.21 um 01:49 schrieb Guoqing Jiang:
quoted
Unregister sync_thread doesn't need to hold reconfig_mutex since it
doesn't reconfigure array.
And it could cause deadlock problem for raid5 as follows:
1. process A tried to reap sync thread with reconfig_mutex held after echo
idle to sync_action.
2. raid5 sync thread was blocked if there were too many active stripes.
3. SB_CHANGE_PENDING was set (because of write IO comes from upper layer)
which causes the number of active stripes can't be decreased.
4. SB_CHANGE_PENDING can't be cleared since md_check_recovery was not able
to hold reconfig_mutex.
More details in the link:
https://lore.kernel.org/linux-raid/5ed54ffc-ce82-bf66-4eff-390cb23bc1ac@molgen.mpg.de/T/#t
And add one parameter to md_reap_sync_thread since it could be called by
dm-raid which doesn't hold reconfig_mutex.
Reported-and-tested-by: Donald Buczek <redacted>
Signed-off-by: Guoqing Jiang <redacted>
I don't really like this fix. But I haven't got a better (and not too
complicated)
alternative.
quoted
quoted
---
V2:
1. add one parameter to md_reap_sync_thread per Jack's suggestion.
drivers/md/dm-raid.c | 2 +-
drivers/md/md.c | 14 +++++++++-----
drivers/md/md.h | 2 +-
3 files changed, 11 insertions(+), 7 deletions(-)
From: Song Liu <song@kernel.org> Date: 2021-03-19 23:01:23
On Wed, Feb 24, 2021 at 1:26 AM Guoqing Jiang
[off-list ref] wrote:
On 2/24/21 10:09, Song Liu wrote:
quoted
On Mon, Feb 15, 2021 at 3:08 AM Paul Menzel [off-list ref] wrote:
quoted
[+cc Donald]
Am 13.02.21 um 01:49 schrieb Guoqing Jiang:
quoted
Unregister sync_thread doesn't need to hold reconfig_mutex since it
doesn't reconfigure array.
And it could cause deadlock problem for raid5 as follows:
1. process A tried to reap sync thread with reconfig_mutex held after echo
idle to sync_action.
2. raid5 sync thread was blocked if there were too many active stripes.
3. SB_CHANGE_PENDING was set (because of write IO comes from upper layer)
which causes the number of active stripes can't be decreased.
4. SB_CHANGE_PENDING can't be cleared since md_check_recovery was not able
to hold reconfig_mutex.
More details in the link:
https://lore.kernel.org/linux-raid/5ed54ffc-ce82-bf66-4eff-390cb23bc1ac@molgen.mpg.de/T/#t
And add one parameter to md_reap_sync_thread since it could be called by
dm-raid which doesn't hold reconfig_mutex.
Reported-and-tested-by: Donald Buczek <redacted>
Signed-off-by: Guoqing Jiang <redacted>
I don't really like this fix. But I haven't got a better (and not too
complicated)
alternative.
quoted
quoted
---
V2:
1. add one parameter to md_reap_sync_thread per Jack's suggestion.
drivers/md/dm-raid.c | 2 +-
drivers/md/md.c | 14 +++++++++-----
drivers/md/md.h | 2 +-
3 files changed, 11 insertions(+), 7 deletions(-)
@@ -3668,7 +3668,7 @@ static int raid_message(struct dm_target *ti, unsigned int argc, char **argv,if(!strcasecmp(argv[0],"idle")||!strcasecmp(argv[0],"frozen")){if(mddev->sync_thread){set_bit(MD_RECOVERY_INTR,&mddev->recovery);-md_reap_sync_thread(mddev);+md_reap_sync_thread(mddev,false);
I think we can add mddev_lock() and mddev_unlock() here and then we don't
need the extra parameter?
I thought it too, but I would prefer get the input from DM people first.
@ Mike or Alasdair
Hi Mike and Alasdair,
Could you please comment on this option: adding mddev_lock() and mddev_unlock()
to raid_message() around md_reap_sync_thread()?
Thanks,
Song
From: Paul Menzel <hidden> Date: 2021-11-30 17:25:10
Dear Linux folks,
Am 20.03.21 um 00:00 schrieb Song Liu:
On Wed, Feb 24, 2021 at 1:26 AM Guoqing Jiang wrote:
quoted
On 2/24/21 10:09, Song Liu wrote:
quoted
On Mon, Feb 15, 2021 at 3:08 AM Paul Menzel wrote:
quoted
[+cc Donald]
Am 13.02.21 um 01:49 schrieb Guoqing Jiang:
quoted
Unregister sync_thread doesn't need to hold reconfig_mutex since it
doesn't reconfigure array.
And it could cause deadlock problem for raid5 as follows:
1. process A tried to reap sync thread with reconfig_mutex held after echo
idle to sync_action.
2. raid5 sync thread was blocked if there were too many active stripes.
3. SB_CHANGE_PENDING was set (because of write IO comes from upper layer)
which causes the number of active stripes can't be decreased.
4. SB_CHANGE_PENDING can't be cleared since md_check_recovery was not able
to hold reconfig_mutex.
More details in the link:
https://lore.kernel.org/linux-raid/5ed54ffc-ce82-bf66-4eff-390cb23bc1ac@molgen.mpg.de/T/#t
And add one parameter to md_reap_sync_thread since it could be called by
dm-raid which doesn't hold reconfig_mutex.
Reported-and-tested-by: Donald Buczek <redacted>
Signed-off-by: Guoqing Jiang <redacted>
I don't really like this fix. But I haven't got a better (and not too
complicated)
alternative.
quoted
quoted
---
V2:
1. add one parameter to md_reap_sync_thread per Jack's suggestion.
drivers/md/dm-raid.c | 2 +-
drivers/md/md.c | 14 +++++++++-----
drivers/md/md.h | 2 +-
3 files changed, 11 insertions(+), 7 deletions(-)
From: Paul Menzel <hidden> Date: 2021-11-30 17:27:18
[Update Guoqing’s email address]
Am 30.11.21 um 18:25 schrieb Paul Menzel:
Dear Linux folks,
Am 20.03.21 um 00:00 schrieb Song Liu:
quoted
On Wed, Feb 24, 2021 at 1:26 AM Guoqing Jiang wrote:
quoted
quoted
On 2/24/21 10:09, Song Liu wrote:
quoted
On Mon, Feb 15, 2021 at 3:08 AM Paul Menzel wrote:
quoted
[+cc Donald]
Am 13.02.21 um 01:49 schrieb Guoqing Jiang:
quoted
Unregister sync_thread doesn't need to hold reconfig_mutex since it
doesn't reconfigure array.
And it could cause deadlock problem for raid5 as follows:
1. process A tried to reap sync thread with reconfig_mutex held
after echo
idle to sync_action.
2. raid5 sync thread was blocked if there were too many active
stripes.
3. SB_CHANGE_PENDING was set (because of write IO comes from upper
layer)
which causes the number of active stripes can't be decreased.
4. SB_CHANGE_PENDING can't be cleared since md_check_recovery was
not able
to hold reconfig_mutex.
More details in the link:
https://lore.kernel.org/linux-raid/5ed54ffc-ce82-bf66-4eff-390cb23bc1ac@molgen.mpg.de/T/#t
And add one parameter to md_reap_sync_thread since it could be
called by
dm-raid which doesn't hold reconfig_mutex.
Reported-and-tested-by: Donald Buczek <redacted>
Signed-off-by: Guoqing Jiang <redacted>
I don't really like this fix. But I haven't got a better (and not too
complicated)
alternative.
quoted
quoted
---
V2:
1. add one parameter to md_reap_sync_thread per Jack's suggestion.
drivers/md/dm-raid.c | 2 +-
drivers/md/md.c | 14 +++++++++-----
drivers/md/md.h | 2 +-
3 files changed, 11 insertions(+), 7 deletions(-)
From: Donald Buczek <hidden> Date: 2021-12-09 12:54:42
On 15.02.21 12:07, Paul Menzel wrote:
[+cc Donald]
Am 13.02.21 um 01:49 schrieb Guoqing Jiang:
quoted
Unregister sync_thread doesn't need to hold reconfig_mutex since it
doesn't reconfigure array.
And it could cause deadlock problem for raid5 as follows:
1. process A tried to reap sync thread with reconfig_mutex held after echo
idle to sync_action.
2. raid5 sync thread was blocked if there were too many active stripes.
3. SB_CHANGE_PENDING was set (because of write IO comes from upper layer)
which causes the number of active stripes can't be decreased.
4. SB_CHANGE_PENDING can't be cleared since md_check_recovery was not able
to hold reconfig_mutex.
More details in the link:
https://lore.kernel.org/linux-raid/5ed54ffc-ce82-bf66-4eff-390cb23bc1ac@molgen.mpg.de/T/#t
And add one parameter to md_reap_sync_thread since it could be called by
dm-raid which doesn't hold reconfig_mutex.
Reported-and-tested-by: Donald Buczek <redacted>
Thanks, Paul, for putting me into the cc.
Guoqing, I don't think, I've tested this patch. Please remove the tested-by.
btw: We have the fix I suggested [1] running on 59 production raid6 sets with 16 disk each with various loads and with monthly mdcheck (paused during daytime, so a few transitions each month) on several kernel versions running for nearly a year now. Many more transitions during testing. That doesn't mean the fix is correct, of course. The configurations of our systems are almost identical and we don't do suspend or anything. But maybe you might want to reconsider.
[1]: https://lore.kernel.org/linux-raid/bc342de0-98d2-1733-39cd-cc1999777ff3@molgen.mpg.de/
If you want me to test V3 of your patch, please put me in the cc.
Best
Donald
From: Donald Buczek <hidden> Date: 2021-12-09 12:57:37
[Update Guoqing’s email address]
On 15.02.21 12:07, Paul Menzel wrote:
[+cc Donald]
Am 13.02.21 um 01:49 schrieb Guoqing Jiang:
quoted
Unregister sync_thread doesn't need to hold reconfig_mutex since it
doesn't reconfigure array.
And it could cause deadlock problem for raid5 as follows:
1. process A tried to reap sync thread with reconfig_mutex held after echo
idle to sync_action.
2. raid5 sync thread was blocked if there were too many active stripes.
3. SB_CHANGE_PENDING was set (because of write IO comes from upper layer)
which causes the number of active stripes can't be decreased.
4. SB_CHANGE_PENDING can't be cleared since md_check_recovery was not able
to hold reconfig_mutex.
More details in the link:
https://lore.kernel.org/linux-raid/5ed54ffc-ce82-bf66-4eff-390cb23bc1ac@molgen.mpg.de/T/#t
And add one parameter to md_reap_sync_thread since it could be called by
dm-raid which doesn't hold reconfig_mutex.
Reported-and-tested-by: Donald Buczek <redacted>
Thanks, Paul, for putting me into the cc.
Guoqing, I don't think, I've tested this patch. Please remove the tested-by.
btw: We have the fix I suggested [1] running on 59 production raid6 sets with 16 disk each with various loads and with monthly mdcheck (paused during daytime, so a few transitions each month) on several kernel versions running for nearly a year now. Many more transitions during testing. That doesn't mean the fix is correct, of course. The configurations of our systems are almost identical and we don't do suspend or anything. But maybe you might want to reconsider.
[1]: https://lore.kernel.org/linux-raid/bc342de0-98d2-1733-39cd-cc1999777ff3@molgen.mpg.de/
If you want me to test V3 of your patch, please put me in the cc.
Best
Donald
[Update Guoqing’s email address]
On 15.02.21 12:07, Paul Menzel wrote:
quoted
[+cc Donald]
Am 13.02.21 um 01:49 schrieb Guoqing Jiang:
quoted
Unregister sync_thread doesn't need to hold reconfig_mutex since it
doesn't reconfigure array.
And it could cause deadlock problem for raid5 as follows:
1. process A tried to reap sync thread with reconfig_mutex held
after echo
idle to sync_action.
2. raid5 sync thread was blocked if there were too many active stripes.
3. SB_CHANGE_PENDING was set (because of write IO comes from upper
layer)
which causes the number of active stripes can't be decreased.
4. SB_CHANGE_PENDING can't be cleared since md_check_recovery was
not able
to hold reconfig_mutex.
More details in the link:
https://lore.kernel.org/linux-raid/5ed54ffc-ce82-bf66-4eff-390cb23bc1ac@molgen.mpg.de/T/#t
And add one parameter to md_reap_sync_thread since it could be
called by
dm-raid which doesn't hold reconfig_mutex.
Reported-and-tested-by: Donald Buczek <redacted>
Thanks, Paul, for putting me into the cc.
Guoqing, I don't think, I've tested this patch. Please remove the
tested-by.
From: Donald Buczek <hidden> Date: 2021-12-10 14:16:35
Dear Guoqing,
On 13.02.21 01:49, Guoqing Jiang wrote:
quoted hunk
Unregister sync_thread doesn't need to hold reconfig_mutex since it
doesn't reconfigure array.
And it could cause deadlock problem for raid5 as follows:
1. process A tried to reap sync thread with reconfig_mutex held after echo
idle to sync_action.
2. raid5 sync thread was blocked if there were too many active stripes.
3. SB_CHANGE_PENDING was set (because of write IO comes from upper layer)
which causes the number of active stripes can't be decreased.
4. SB_CHANGE_PENDING can't be cleared since md_check_recovery was not able
to hold reconfig_mutex.
More details in the link:
https://lore.kernel.org/linux-raid/5ed54ffc-ce82-bf66-4eff-390cb23bc1ac@molgen.mpg.de/T/#t
And add one parameter to md_reap_sync_thread since it could be called by
dm-raid which doesn't hold reconfig_mutex.
Reported-and-tested-by: Donald Buczek <redacted>
Signed-off-by: Guoqing Jiang <redacted>
---
V2:
1. add one parameter to md_reap_sync_thread per Jack's suggestion.
drivers/md/dm-raid.c | 2 +-
drivers/md/md.c | 14 +++++++++-----
drivers/md/md.h | 2 +-
3 files changed, 11 insertions(+), 7 deletions(-)
@@ -9291,7 +9291,7 @@ void md_check_recovery(struct mddev *mddev)gotounlock;}if(mddev->sync_thread){-md_reap_sync_thread(mddev);+md_reap_sync_thread(mddev,true);gotounlock;}/* Set RUNNING before clearing NEEDED to avoid
@@ -9364,14 +9364,18 @@ void md_check_recovery(struct mddev *mddev)}EXPORT_SYMBOL(md_check_recovery);-voidmd_reap_sync_thread(structmddev*mddev)+voidmd_reap_sync_thread(structmddev*mddev,boolreconfig_mutex_held){structmd_rdev*rdev;sector_told_dev_sectors=mddev->dev_sectors;boolis_reshaped=false;/* resync has finished, collect result */+if(reconfig_mutex_held)+mddev_unlock(mddev);
If one thread got here, e.g. via action_store( /* "idle" */ ), now that the mutex is unlocked, is there anything which would prevent another thread getting here as well, e.g. via the same path?
If not, they both might call
md_unregister_thread(&mddev->sync_thread);
Which is not reentrant:
void md_unregister_thread(struct md_thread **threadp)
{
struct md_thread *thread = *threadp;
if (!thread)
return;
pr_debug("interrupting MD-thread pid %d\n", task_pid_nr(thread->tsk));
/* Locking ensures that mddev_unlock does not wake_up a
* non-existent thread
*/
spin_lock(&pers_lock);
*threadp = NULL;
spin_unlock(&pers_lock);
kthread_stop(thread->tsk);
kfree(thread);
}
This might be a preexisting problem, because the call site in dm-raid.c, which you updated to `md_reap_sync_thread(mddev, false);`, didn't hold the mutex anyway.
Am I missing something? Probably, I do.
Otherwise: Move the deref of threadp in md_unregister_thread() into the spinlock scope?
Best
Donald
quoted hunk
+ if (reconfig_mutex_held)
+ mddev_lock_nointr(mddev);
if (!test_bit(MD_RECOVERY_INTR, &mddev->recovery) &&
!test_bit(MD_RECOVERY_REQUESTED, &mddev->recovery) &&
mddev->degraded != mddev->raid_disks) {
Dear Guoqing,
On 13.02.21 01:49, Guoqing Jiang wrote:
quoted
Unregister sync_thread doesn't need to hold reconfig_mutex since it
doesn't reconfigure array.
And it could cause deadlock problem for raid5 as follows:
1. process A tried to reap sync thread with reconfig_mutex held after
echo
idle to sync_action.
2. raid5 sync thread was blocked if there were too many active stripes.
3. SB_CHANGE_PENDING was set (because of write IO comes from upper
layer)
which causes the number of active stripes can't be decreased.
4. SB_CHANGE_PENDING can't be cleared since md_check_recovery was not
able
to hold reconfig_mutex.
More details in the link:
https://lore.kernel.org/linux-raid/5ed54ffc-ce82-bf66-4eff-390cb23bc1ac@molgen.mpg.de/T/#t
And add one parameter to md_reap_sync_thread since it could be called by
dm-raid which doesn't hold reconfig_mutex.
Reported-and-tested-by: Donald Buczek <redacted>
Signed-off-by: Guoqing Jiang <redacted>
---
V2:
1. add one parameter to md_reap_sync_thread per Jack's suggestion.
drivers/md/dm-raid.c | 2 +-
drivers/md/md.c | 14 +++++++++-----
drivers/md/md.h | 2 +-
3 files changed, 11 insertions(+), 7 deletions(-)
If one thread got here, e.g. via action_store( /* "idle" */ ), now
that the mutex is unlocked, is there anything which would prevent
another thread getting here as well, e.g. via the same path?
If not, they both might call
quoted
md_unregister_thread(&mddev->sync_thread);
Which is not reentrant:
void md_unregister_thread(struct md_thread **threadp)
{
struct md_thread *thread = *threadp;
if (!thread)
return;
pr_debug("interrupting MD-thread pid %d\n",
task_pid_nr(thread->tsk));
/* Locking ensures that mddev_unlock does not wake_up a
* non-existent thread
*/
spin_lock(&pers_lock);
*threadp = NULL;
spin_unlock(&pers_lock);
kthread_stop(thread->tsk);
kfree(thread);
}
This might be a preexisting problem, because the call site in
dm-raid.c, which you updated to `md_reap_sync_thread(mddev, false);`,
didn't hold the mutex anyway.
Am I missing something? Probably, I do.
Otherwise: Move the deref of threadp in md_unregister_thread() into
the spinlock scope?
Good point, I think you are right.
And actually pers_lock does extra service to protect accesses to
mddev->thread (I think it
also suitable for mddev->sync_thread ) when the mutex can't be held.
Care to send a patch
for it?
Thanks,
Guoqing
From: Donald Buczek <hidden> Date: 2021-12-14 09:31:20
On 14.12.21 03:34, Guoqing Jiang wrote:
On 12/10/21 10:16 PM, Donald Buczek wrote:
quoted
Dear Guoqing,
On 13.02.21 01:49, Guoqing Jiang wrote:
quoted
Unregister sync_thread doesn't need to hold reconfig_mutex since it
doesn't reconfigure array.
And it could cause deadlock problem for raid5 as follows:
1. process A tried to reap sync thread with reconfig_mutex held after echo
idle to sync_action.
2. raid5 sync thread was blocked if there were too many active stripes.
3. SB_CHANGE_PENDING was set (because of write IO comes from upper layer)
which causes the number of active stripes can't be decreased.
4. SB_CHANGE_PENDING can't be cleared since md_check_recovery was not able
to hold reconfig_mutex.
More details in the link:
https://lore.kernel.org/linux-raid/5ed54ffc-ce82-bf66-4eff-390cb23bc1ac@molgen.mpg.de/T/#t
And add one parameter to md_reap_sync_thread since it could be called by
dm-raid which doesn't hold reconfig_mutex.
Reported-and-tested-by: Donald Buczek <redacted>
Signed-off-by: Guoqing Jiang <redacted>
---
V2:
1. add one parameter to md_reap_sync_thread per Jack's suggestion.
drivers/md/dm-raid.c | 2 +-
drivers/md/md.c | 14 +++++++++-----
drivers/md/md.h | 2 +-
3 files changed, 11 insertions(+), 7 deletions(-)
If one thread got here, e.g. via action_store( /* "idle" */ ), now that the mutex is unlocked, is there anything which would prevent another thread getting here as well, e.g. via the same path?
If not, they both might call
quoted
md_unregister_thread(&mddev->sync_thread);
Which is not reentrant:
void md_unregister_thread(struct md_thread **threadp)
{
struct md_thread *thread = *threadp;
if (!thread)
return;
pr_debug("interrupting MD-thread pid %d\n", task_pid_nr(thread->tsk));
/* Locking ensures that mddev_unlock does not wake_up a
* non-existent thread
*/
spin_lock(&pers_lock);
*threadp = NULL;
spin_unlock(&pers_lock);
kthread_stop(thread->tsk);
kfree(thread);
}
This might be a preexisting problem, because the call site in dm-raid.c, which you updated to `md_reap_sync_thread(mddev, false);`, didn't hold the mutex anyway.
Am I missing something? Probably, I do.
Otherwise: Move the deref of threadp in md_unregister_thread() into the spinlock scope?
Good point, I think you are right.
And actually pers_lock does extra service to protect accesses to mddev->thread (I think it
also suitable for mddev->sync_thread ) when the mutex can't be held. Care to send a patch
for it?
I'm really sorry, but it's one thing to point to a possible problem and another thing to come up with a correct solution.
While I think it would be easy to avoid the double free with the spinlock (or maybe atomic RMW) , we surely don't want to hold the spinlock while we are sleeping in kthread_stop(). If we don't hold some kind of lock, what are the side effects of another sync thread being started or any other reconfiguration? Are the existing flags enough to protect us from this? If we do want to hold the lock while waiting for the thread to terminate, should it be made into a mutex? If so, it probably shouldn't be static but moved into the mddev structure. I'd need weeks if not month to figure that out and to feel bold enough to post it.
I don't want to push work to others, but my own my understanding of md is to narrow.
Best
Donald
Thanks,
Guoqing
--
Donald Buczek
buczek@molgen.mpg.de
Tel: +49 30 8413 1433
If one thread got here, e.g. via action_store( /* "idle" */ ), now
that the mutex is unlocked, is there anything which would prevent
another thread getting here as well, e.g. via the same path?
If not, they both might call
quoted
md_unregister_thread(&mddev->sync_thread);
Which is not reentrant:
void md_unregister_thread(struct md_thread **threadp)
{
struct md_thread *thread = *threadp;
if (!thread)
return;
pr_debug("interrupting MD-thread pid %d\n",
task_pid_nr(thread->tsk));
/* Locking ensures that mddev_unlock does not wake_up a
* non-existent thread
*/
spin_lock(&pers_lock);
*threadp = NULL;
spin_unlock(&pers_lock);
kthread_stop(thread->tsk);
kfree(thread);
}
This might be a preexisting problem, because the call site in
dm-raid.c, which you updated to `md_reap_sync_thread(mddev,
false);`, didn't hold the mutex anyway.
Am I missing something? Probably, I do.
Otherwise: Move the deref of threadp in md_unregister_thread() into
the spinlock scope?
Good point, I think you are right.
And actually pers_lock does extra service to protect accesses to
mddev->thread (I think it
also suitable for mddev->sync_thread ) when the mutex can't be held.
Care to send a patch
for it?
I'm really sorry, but it's one thing to point to a possible problem
and another thing to come up with a correct solution.
Yes, it is often the reality of life, and we can always correct
ourselves if there is problem 😎.
While I think it would be easy to avoid the double free with the
spinlock (or maybe atomic RMW) , we surely don't want to hold the
spinlock while we are sleeping in kthread_stop(). If we don't hold
some kind of lock, what are the side effects of another sync thread
being started or any other reconfiguration? Are the existing flags
enough to protect us from this? If we do want to hold the lock while
waiting for the thread to terminate, should it be made into a mutex?
If so, it probably shouldn't be static but moved into the mddev
structure. I'd need weeks if not month to figure that out and to feel
bold enough to post it.
Thanks for deep thinking about it, I think we can avoid to call
kthread_stop with spinlock
held. Maybe something like this, but just my raw idea, please have a
thorough review.
void md_unregister_thread(struct md_thread **threadp)
{
- struct md_thread *thread = *threadp;
- if (!thread)
+ struct md_thread *thread = READ_ONCE(*threadp);
+
+ spin_lock(&pers_lock);
+ if (!thread) {
+ spin_unlock(&pers_lock);
return;
+ }
pr_debug("interrupting MD-thread pid %d\n",
task_pid_nr(thread->tsk));
/* Locking ensures that mddev_unlock does not wake_up a
* non-existent thread
*/
- spin_lock(&pers_lock);
*threadp = NULL;
spin_unlock(&pers_lock);
- kthread_stop(thread->tsk);
+ if (IS_ERR_OR_NULL(thread->tsk)) {
+ kthread_stop(thread->tsk);
+ thread->tsk = NULL;
+ }
kfree(thread);
}
EXPORT_SYMBOL(md_unregister_thread);
I don't want to push work to others, but my own my understanding of md
is to narrow.
If one thread got here, e.g. via action_store( /* "idle" */ ), now that the mutex is unlocked, is there anything which would prevent another thread getting here as well, e.g. via the same path?
If not, they both might call
quoted
md_unregister_thread(&mddev->sync_thread);
Which is not reentrant:
void md_unregister_thread(struct md_thread **threadp)
{
struct md_thread *thread = *threadp;
if (!thread)
return;
pr_debug("interrupting MD-thread pid %d\n", task_pid_nr(thread->tsk));
/* Locking ensures that mddev_unlock does not wake_up a
* non-existent thread
*/
spin_lock(&pers_lock);
*threadp = NULL;
spin_unlock(&pers_lock);
kthread_stop(thread->tsk);
kfree(thread);
}
This might be a preexisting problem, because the call site in dm-raid.c, which you updated to `md_reap_sync_thread(mddev, false);`, didn't hold the mutex anyway.
Am I missing something? Probably, I do.
Otherwise: Move the deref of threadp in md_unregister_thread() into the spinlock scope?
Good point, I think you are right.
And actually pers_lock does extra service to protect accesses to mddev->thread (I think it
also suitable for mddev->sync_thread ) when the mutex can't be held. Care to send a patch
for it?
I'm really sorry, but it's one thing to point to a possible problem and another thing to come up with a correct solution.
Yes, it is often the reality of life, and we can always correct ourselves if there is problem 😎.
quoted
While I think it would be easy to avoid the double free with the spinlock (or maybe atomic RMW) , we surely don't want to hold the spinlock while we are sleeping in kthread_stop(). If we don't hold some kind of lock, what are the side effects of another sync thread being started or any other reconfiguration? Are the existing flags enough to protect us from this? If we do want to hold the lock while waiting for the thread to terminate, should it be made into a mutex? If so, it probably shouldn't be static but moved into the mddev structure. I'd need weeks if not month to figure that out and to feel bold enough to post it.
Thanks for deep thinking about it, I think we can avoid to call kthread_stop with spinlock
held. Maybe something like this, but just my raw idea, please have a thorough review.
void md_unregister_thread(struct md_thread **threadp)
{
- struct md_thread *thread = *threadp;
- if (!thread)
+ struct md_thread *thread = READ_ONCE(*threadp);
- The access to *threadp needs to be after the spin_lock(). Otherwise two CPUs might read non-NULL here.
- If it was after spin_lock(), I think (!= I know), we don't need the READ_ONCE, because spin_lock() implies a compiler barrier.
+
+ spin_lock(&pers_lock);
+ if (!thread) {
+ spin_unlock(&pers_lock);
return;
+ }
pr_debug("interrupting MD-thread pid %d\n", task_pid_nr(thread->tsk));
/* Locking ensures that mddev_unlock does not wake_up a
* non-existent thread
*/
- spin_lock(&pers_lock);
*threadp = NULL;
spin_unlock(&pers_lock);
- kthread_stop(thread->tsk);
+ if (IS_ERR_OR_NULL(thread->tsk)) {
- Test accidentally negated? This test is new. Is this an unrelated change? Anyway, I don't get it.
From: Mikko Rantalainen <hidden> Date: 2022-01-11 12:34:43
Guoqing Jiang (2021-02-13 02:49 Europe/Helsinki):
Unregister sync_thread doesn't need to hold reconfig_mutex since it
doesn't reconfigure array.
And it could cause deadlock problem for raid5 as follows:
1. process A tried to reap sync thread with reconfig_mutex held after echo
idle to sync_action.
2. raid5 sync thread was blocked if there were too many active stripes.
3. SB_CHANGE_PENDING was set (because of write IO comes from upper layer)
which causes the number of active stripes can't be decreased.
4. SB_CHANGE_PENDING can't be cleared since md_check_recovery was not able
to hold reconfig_mutex.
More details in the link:
https://lore.kernel.org/linux-raid/5ed54ffc-ce82-bf66-4eff-390cb23bc1ac@molgen.mpg.de/T/#t
I don't understand md well enough to suggest a patch but isn't this
logically a classic two lock deadlock problem where
thread 1:
- lock reconfig_mutex
- blocked for sync that requires SB_CHANGE_PENDING
thread 2:
- (logically) acquire lock SB_CHANGE_PENDING
- blocked for reconfig_mutex before SB_CHANGE_PENDING can be released
?
The classic fix for this kind of deadlock is to require these locks to
be always acquired in constant order and released in reverse order.
If you had a rule that SB_CHANGE_PENDING cannot be set or cleared
without already having reconfig_mutex, wouldn't that prevent this
deadlock? (If I understood the issue correctly, it's currently possible
to set but not clear the SB_CHANGE_PENDING without having reconfig_mutex.)
Another possibility is to expect SB_CHANGE_PENDING to be set as part of
sync process required change to "idle" (write to sync_action). In that
case the logic would be you cannot have reconfig_mutex already locked
before setting (logically acquiring lock) SB_CHANGE_PENDING. So the
transfer from active to idle would require first setting
SB_CHANGE_PENDING, doing the required processing (getting and freeing
reconfig_mutex in process) and then clearing SB_CHANGE_PENDING.
Basically the rule would be you must lock SB_CHANGE_PENDING before you
can lock reconfig_mutex.
If I've understood correctly SB_CHANGE_PENDING is not technically a lock
but it's logically used like it were a lock.
Would either of these make sense for the overall design?
Obviously, if it doesn't hurt the performance too much, using a single
lock for everything that needs to be serialized would be much easier.
--
Mikko