Re: [PATCH V2] md: don't unregister sync_thread with reconfig_mutex held

[Guoqing Jiang <guoqing.jiang@linux.dev>]

On 12/1/21 1:27 AM, Paul Menzel wrote:
>
>>>>>>> diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
>>>>>>> index cab12b2..0c4cbba 100644
>>>>>>> --- a/drivers/md/dm-raid.c
>>>>>>> +++ b/drivers/md/dm-raid.c
>>>>>>> @@ -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()?

Add Heinz and Jonathan, could you comment about this? Thanks.

>>
>> The issue is unfortunately still unresolved (at least Linux 5.10.82). 
>> How can we move forward?

If it is not applicable to change dm-raid, another alternative could be 
like this

--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -9409,8 +9409,12 @@ void md_reap_sync_thread(struct mdev *mddev)
         sector_t old_dev_sectors = mddev->dev_sectors;
         bool is_reshaped = false;

+       if (mddev_is_locked(mddev))
+               mddev_unlock(mddev);
         /* resync has finished, collect result */
         md_unregister_thread(&mddev->sync_thread);
+       if (mddev_is_locked(mddev))
+               mddev_lock(mddev);
         if (!test_bit(MD_RECOVERY_INTR, &mddev->recovery) &&
             !test_bit(MD_RECOVERY_REQUESTED, &mddev->recovery) &&
             mddev->degraded != mddev->raid_disks) {
diff --git a/drivers/md/md.h b/drivers/md/md.h
index 53ea7a6961de..96a88b7681d6 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -549,6 +549,11 @@ static inline int mddev_trylock(struct mddev *mddev)
  }
  extern void mddev_unlock(struct mddev *mddev);

+static inline int mddev_is_locked(struct mddev *mddev)
+{
+       return mutex_is_locked(&mddev->reconfig_mutex);
+}
+

BTW, it is holiday season,  so people are probably on vacation.

Thanks,
Guoqing