On 2022-06-05 21:29, Guoqing Jiang wrote:
>
>
> On 6/3/22 2:03 AM, Logan Gunthorpe wrote:
>>
>>
>> On 2022-06-02 07:45, Guoqing Jiang wrote:
>>> The 07reshape5intr test is broke because of below path.
>>>
>>> md_reap_sync_thread
>>> -> mddev_unlock
>>> -> md_unregister_thread(&mddev->sync_thread)
>>>
>>> And md_check_recovery is triggered by,
>>>
>>> mddev_unlock -> md_wakeup_thread(mddev->thread)
>>>
>>> then mddev->reshape_position is set to MaxSector in raid5_finish_reshape
>>> since MD_RECOVERY_INTR is cleared in md_check_recovery, which means
>>> feature_map is not set with MD_FEATURE_RESHAPE_ACTIVE and superblock's
>>> reshape_position can't be updated accordingly.
>>>
>>> Since the bug which commit 8b48ec23cc51a ("md: don't unregister sync_thread
>>> with reconfig_mutex held") fixed is related with action_store path, other
>>> callers which reap sync_thread didn't need to be changed, let's
>>>
>>> 1. only unlock mddev in md_reap_sync_thread if caller is action_store,
>>> so the parameter is renamed to reflect the change.
>>> 2. save some contexts (MD_RECOVERY_INTR and reshape_position) since they
>>> could be changed by other processes, then restore them after get lock
>>> again.
>>>
>>> Fixes: 8b48ec23cc51a ("md: don't unregister sync_thread with reconfig_mutex held")
>>> Reported-by: Logan Gunthorpe <logang@xxxxxxxxxxxx>
>>> Signed-off-by: Guoqing Jiang <guoqing.jiang@xxxxxxxxx>
>>> ---
>>> I suppose the previous bug still can be fixed with the change, but it is
>>> better to verify it. Donald, could you help to test the new code?
>>>
>>> Thanks,
>>> Guoqing
>>>
>>> drivers/md/md.c | 24 ++++++++++++++++++------
>>> drivers/md/md.h | 2 +-
>>> 2 files changed, 19 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>>> index 5c8efef13881..3387260dd55b 100644
>>> --- a/drivers/md/md.c
>>> +++ b/drivers/md/md.c
>>> @@ -6197,7 +6197,7 @@ static void __md_stop_writes(struct mddev *mddev)
>>> flush_workqueue(md_misc_wq);
>>> if (mddev->sync_thread) {
>>> set_bit(MD_RECOVERY_INTR, &mddev->recovery);
>>> - md_reap_sync_thread(mddev, true);
>>> + md_reap_sync_thread(mddev, false);
>>> }
>>>
>>> del_timer_sync(&mddev->safemode_timer);
>>> @@ -9303,7 +9303,7 @@ void md_check_recovery(struct mddev *mddev)
>>> * ->spare_active and clear saved_raid_disk
>>> */
>>> set_bit(MD_RECOVERY_INTR, &mddev->recovery);
>>> - md_reap_sync_thread(mddev, true);
>>> + md_reap_sync_thread(mddev, false);
>>> clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
>>> clear_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
>>> clear_bit(MD_SB_CHANGE_PENDING, &mddev->sb_flags);
>>> @@ -9338,7 +9338,7 @@ void md_check_recovery(struct mddev *mddev)
>>> goto unlock;
>>> }
>>> if (mddev->sync_thread) {
>>> - md_reap_sync_thread(mddev, true);
>>> + md_reap_sync_thread(mddev, false);
>>> goto unlock;
>>> }
>>> /* Set RUNNING before clearing NEEDED to avoid
>>> @@ -9411,18 +9411,30 @@ void md_check_recovery(struct mddev *mddev)
>>> }
>>> EXPORT_SYMBOL(md_check_recovery);
>>>
>>> -void md_reap_sync_thread(struct mddev *mddev, bool reconfig_mutex_held)
>>> +void md_reap_sync_thread(struct mddev *mddev, bool unlock_mddev)
>>> {
>>> struct md_rdev *rdev;
>>> sector_t old_dev_sectors = mddev->dev_sectors;
>>> + sector_t old_reshape_position;
>>> bool is_reshaped = false;
>>> + bool is_interrupted = false;
>>>
>>> - if (reconfig_mutex_held)
>>> + if (unlock_mddev) {
>>> + old_reshape_position = mddev->reshape_position;
>>> + if (test_bit(MD_RECOVERY_INTR, &mddev->recovery))
>>> + is_interrupted = true;
>>> mddev_unlock(mddev);
>>> + }
>>> /* resync has finished, collect result */
>>> md_unregister_thread(&mddev->sync_thread);
>>> - if (reconfig_mutex_held)
>>> + if (unlock_mddev) {
>>> mddev_lock_nointr(mddev);
>>> + /* restore the previous flag and position */
>>> + mddev->reshape_position = old_reshape_position;
>>> + if (is_interrupted)
>>> + set_bit(MD_RECOVERY_INTR, &mddev->recovery);
>>> + }
>> Maybe instead of the ugly boolean argument we could pull
>> md_unregister_thread() into all the callers and explicitly unlock in the
>> single call site that needs it?
>
> After move "md_unregister_thread(&mddev->sync_thread)", then we need to
> rename md_reap_sync_thread given it doesn't unregister sync_thread, any
> suggestion about the new name? md_behind_reap_sync_thread?
I don't like the "behind"... Which would be a name suggesting when the
function should be called, not what the function does.
I'd maybe go with something like md_cleanup_sync_thread()
Logan
