dm-raid1: fix deadlock at suspend after suspend was interrupted
cover dm-raid1: fix deadlock at suspend after suspend was interrupted
Hi,
This is a patch set to fix deadlock on suspending of mirror device.
ISSUE
=====
Suspend procedure on a dm-mirror device could cause deadlock on recovery_count semaphore.
When mirror_presuspend is called, recovery_count semaphore is acquired in dm_rh_stop_recovery() to stop recovery routine, but when an signal is caught in dm_wait_for_completion() or an error occurred in in dm_suspend(), the suspend process is interrupted without releasing recovery_count semaphore of a mirror device. This means that another suspend is executed, and then the suspend process gets stuck at dm_rh_stop_recovery().
When suspend procedure is interrupted, the device should work properly since the status of the device is not "suspended."
SOLUTION
========
Introduce a target handler, cancel_presuspend, to cancel status changes done by a target specific presuspend handler.
PATCH SET
=========
1/3: dm: introduce cancel_presuspend framework 2/3: dm-raid1: add cancel_presuspend function 3/3: dm-delay: add cancel_presuspend function
I appreciate your comments.
Thanks,
Signature · 2 lines
Takahiro Yasui Hitachi Computer Products (America), Inc.
Hi Yasui-san,
Quoted 22 lines · Takahiro Yasui
Hi,
This is a patch set to fix deadlock on suspending of mirror device.
ISSUE
=====Suspend procedure on a dm-mirror device could cause deadlock on
recovery_countsemaphore.When
mirror_presuspendis called,recovery_countsemaphore is acquired indm_rh_stop_recovery()to stop recovery routine, but when an signal is caught indm_wait_for_completion()or an error occurred in indm_suspend(), the suspend process is interrupted without releasingrecovery_countsemaphore of a mirror device. This means that another suspend is executed, and then the suspend process gets stuck atdm_rh_stop_recovery().When suspend procedure is interrupted, the device should work properly since the status of the device is not "suspended."
Quoted 5 lines
SOLUTION
========Introduce a target handler,
cancel_presuspend, to cancel status changes done by a target specific presuspend handler.
How about using ->resume as a cancelling method? Though you have to audit existing targets' ->resume handler, I think it's better idea than adding another target handler just for this purpose.
And in your dm-raid1 patch, cancelling log's presuspend which is used by dm-log-userspace is missed.
So it seems that dm-raid1 can use ->resume to cancel presuspend.
Thanks,
Kiyoshi Ueda
Hi Ueda-san,
Quoted 26 lines · Kiyoshi Ueda
Quoted 25 lines · Takahiro Yasui
Hi,
This is a patch set to fix deadlock on suspending of mirror device.
ISSUE
=====Suspend procedure on a dm-mirror device could cause deadlock on
recovery_countsemaphore.When
mirror_presuspendis called,recovery_countsemaphore is acquired indm_rh_stop_recovery()to stop recovery routine, but when an signal is caught indm_wait_for_completion()or an error occurred in indm_suspend(), the suspend process is interrupted without releasingrecovery_countsemaphore of a mirror device. This means that another suspend is executed, and then the suspend process gets stuck atdm_rh_stop_recovery().When suspend procedure is interrupted, the device should work properly since the status of the device is not "suspended."
SOLUTION
========
Quoted 7 lines
Quoted 2 lines
Introduce a target handler,
cancel_presuspend, to cancel status changes done by a target specific presuspend handler.How about using ->resume as a cancelling method? Though you have to audit existing targets' ->resume handler, I think it's better idea than adding another target handler just for this purpose.
A resume method contains a whole resume procedure, but when suspend is interrupted, postsuspend handler is not processed. So the requirements are to restore state changes done by presuspend handler. If a whole resume procedure is executed, at least, dm-log will have a problem.
mirror log is flushed in postsuspend handler and log disk might contain stale data at the moment when suspend is interrupted. If resume handler is used instead of cancel_presuspend handler, log data on memory will be overwritten by stale data on disk.
I'm afraid that we need to modify each target's resume handler so that they work properly even after processing presuspend handler but before postsuspend handler.
Please let me know if there is some oversight.
Quoted 2 lines
And in your dm-raid1 patch, cancelling log's presuspend which is used by dm-log-userspace is missed.
Thank you for telling this. Yes, userspace target should be also handled. I will fix it.
Thanks,
Taka
Hi Yasui-san,
Quoted 10 lines · Takahiro Yasui
Quoted 8 lines · Kiyoshi Ueda
Quoted 2 lines · Takahiro Yasui
Introduce a target handler,
cancel_presuspend, to cancel status changes done by a target specific presuspend handler.How about using ->resume as a cancelling method? Though you have to audit existing targets' ->resume handler, I think it's better idea than adding another target handler just for this purpose.
<snip>
Quoted 6 lines
I'm afraid that we need to modify each target's resume handler so that they work properly even after processing presuspend handler but before postsuspend handler.
Please let me know if there is some oversight.
There is no oversight.
Perhaps I should have said 'audit (and modify if necessary)'.
If auditing and modifying all targets are difficult, I don't object your approach which adds ->cancel_presuspend.
But ->cancel_presuspend should be a subset of ->resume and it should make some code duplication. If ->resume can always restore target state correctly, it may avoid such duplication.
Althrough I'm not sure the following idea is really reasonable, I think it should be worth to consider about it:
When ->postsuspend is called and the device is really suspended, DMF_SUSPENDED flag is set in md->flags.
So targets' ->resume handler can use it to check ->postsuspend had been called or not.
Thanks,
Kiyoshi Ueda
Hi Ueda-san,
Quoted 25 lines · Kiyoshi Ueda
Quoted 8 lines · Takahiro Yasui
Quoted 7 lines · Kiyoshi Ueda
Quoted 2 lines · Takahiro Yasui
Introduce a target handler,
cancel_presuspend, to cancel status changes done by a target specific presuspend handler.How about using ->resume as a cancelling method? Though you have to audit existing targets' ->resume handler, I think it's better idea than adding another target handler just for this purpose.
<snip>
Quoted 5 lines
I'm afraid that we need to modify each target's resume handler so that they work properly even after processing presuspend handler but before postsuspend handler.
Please let me know if there is some oversight.
There is no oversight.
Perhaps I should have said 'audit (and modify if necessary)'.If auditing and modifying all targets are difficult, I don't object your approach which adds ->cancel_presuspend.
But ->cancel_presuspend should be a subset of ->resume and it should make some code duplication. If ->resume can always restore target state correctly, it may avoid such duplication.
Quoted 6 lines
Althrough I'm not sure the following idea is really reasonable, I think it should be worth to consider about it:
When ->postsuspend is called and the device is really suspended,DMF_SUSPENDEDflag is set inmd->flags.
So targets' ->resume handler can use it to check ->postsuspend had been called or not.
Hmm. It looks good idea.
By exporting dm_suspended(), targets running in kernel space, can get the current status and choose actions to cancel state changes done in presuspend(). One execption is dm-log-userspace. It calls user space functions. So other solution is needed.
I will try to update this patch set according to your suggestions.
Thanks,
Taka