diff mbox series

[md-6.8,v2,2/9] md: export helpers to stop sync_thread

Message ID 20240305072306.2562024-3-yukuai1@huaweicloud.com (mailing list archive)
State Accepted, archived
Headers show
Series dm-raid, md/raid: fix v6.7 regressions part2 | expand

Commit Message

Yu Kuai March 5, 2024, 7:22 a.m. UTC 
From: Yu Kuai <yukuai3@huawei.com>

The new heleprs will be used in dm-raid in later patches to fix
regressions and prevent calling md_reap_sync_thread() directly.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
Signed-off-by: Xiao Ni <xni@redhat.com>
Acked-by: Mike Snitzer <snitzer@kernel.org>
---
 drivers/md/md.c | 29 +++++++++++++++++++++++++++++
 drivers/md/md.h |  3 +++
 2 files changed, 32 insertions(+)

Comments

Paul Menzel March 5, 2024, 8:08 a.m. UTC | #1 
Dear Kuai,


Thank you for your patch.

Am 05.03.24 um 08:22 schrieb Yu Kuai:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> The new heleprs will be used in dm-raid in later patches to fix

hel*pe*rs

> regressions and prevent calling md_reap_sync_thread() directly.

Please list the new functions.

1.  md_idle_sync_thread(struct mddev *mddev);
2.  md_frozen_sync_thread(struct mddev *mddev);
3.  md_unfrozen_sync_thread(struct mddev *mddev);

I do not like the naming so much. `md_reap_sync_thread()` has the verb 
in imperative mood. At least myself, I would not know what the functions 
do exactly without looking at the implementation.


Kind regards,

Paul


> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> Signed-off-by: Xiao Ni <xni@redhat.com>
> Acked-by: Mike Snitzer <snitzer@kernel.org>
> ---
>   drivers/md/md.c | 29 +++++++++++++++++++++++++++++
>   drivers/md/md.h |  3 +++
>   2 files changed, 32 insertions(+)
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 23f31fd1d024..22e7512a5b1e 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -4919,6 +4919,35 @@ static void stop_sync_thread(struct mddev *mddev, bool locked, bool check_seq)
>   		mddev_lock_nointr(mddev);
>   }
>   
> +void md_idle_sync_thread(struct mddev *mddev)
> +{
> +	lockdep_assert_held(&mddev->reconfig_mutex);
> +
> +	clear_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
> +	stop_sync_thread(mddev, true, true);
> +}
> +EXPORT_SYMBOL_GPL(md_idle_sync_thread);
> +
> +void md_frozen_sync_thread(struct mddev *mddev)
> +{
> +	lockdep_assert_held(&mddev->reconfig_mutex);
> +
> +	set_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
> +	stop_sync_thread(mddev, true, false);
> +}
> +EXPORT_SYMBOL_GPL(md_frozen_sync_thread);
> +
> +void md_unfrozen_sync_thread(struct mddev *mddev)
> +{
> +	lockdep_assert_held(&mddev->reconfig_mutex);
> +
> +	clear_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
> +	set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
> +	md_wakeup_thread(mddev->thread);
> +	sysfs_notify_dirent_safe(mddev->sysfs_action);
> +}
> +EXPORT_SYMBOL_GPL(md_unfrozen_sync_thread);
> +
>   static void idle_sync_thread(struct mddev *mddev)
>   {
>   	mutex_lock(&mddev->sync_mutex);
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index 8d881cc59799..437ab70ce79b 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -781,6 +781,9 @@ extern void md_rdev_clear(struct md_rdev *rdev);
>   extern void md_handle_request(struct mddev *mddev, struct bio *bio);
>   extern int mddev_suspend(struct mddev *mddev, bool interruptible);
>   extern void mddev_resume(struct mddev *mddev);
> +extern void md_idle_sync_thread(struct mddev *mddev);
> +extern void md_frozen_sync_thread(struct mddev *mddev);
> +extern void md_unfrozen_sync_thread(struct mddev *mddev);
>   
>   extern void md_reload_sb(struct mddev *mddev, int raid_disk);
>   extern void md_update_sb(struct mddev *mddev, int force);
Yu Kuai March 5, 2024, 8:13 a.m. UTC | #2 
Hi,

在 2024/03/05 16:08, Paul Menzel 写道:
> Dear Kuai,
> 
> 
> Thank you for your patch.
> 
> Am 05.03.24 um 08:22 schrieb Yu Kuai:
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> The new heleprs will be used in dm-raid in later patches to fix
> 
> hel*pe*rs
> 
>> regressions and prevent calling md_reap_sync_thread() directly.
> 
> Please list the new functions.
> 
> 1.  md_idle_sync_thread(struct mddev *mddev);
> 2.  md_frozen_sync_thread(struct mddev *mddev);
> 3.  md_unfrozen_sync_thread(struct mddev *mddev);
> 
> I do not like the naming so much. `md_reap_sync_thread()` has the verb 
> in imperative mood. At least myself, I would not know what the functions 
> do exactly without looking at the implementation.
> 
Thanks for the suggestions, I'm not that good at naming :(

Usually I'll send a new version with updated naming, however, we must
merge this set ASAP now, perhaps can we live with this for now?

Thanks,
Kuai

> 
> Kind regards,
> 
> Paul
> 
> 
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> Signed-off-by: Xiao Ni <xni@redhat.com>
>> Acked-by: Mike Snitzer <snitzer@kernel.org>
>> ---
>>   drivers/md/md.c | 29 +++++++++++++++++++++++++++++
>>   drivers/md/md.h |  3 +++
>>   2 files changed, 32 insertions(+)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 23f31fd1d024..22e7512a5b1e 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -4919,6 +4919,35 @@ static void stop_sync_thread(struct mddev 
>> *mddev, bool locked, bool check_seq)
>>           mddev_lock_nointr(mddev);
>>   }
>> +void md_idle_sync_thread(struct mddev *mddev)
>> +{
>> +    lockdep_assert_held(&mddev->reconfig_mutex);
>> +
>> +    clear_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
>> +    stop_sync_thread(mddev, true, true);
>> +}
>> +EXPORT_SYMBOL_GPL(md_idle_sync_thread);
>> +
>> +void md_frozen_sync_thread(struct mddev *mddev)
>> +{
>> +    lockdep_assert_held(&mddev->reconfig_mutex);
>> +
>> +    set_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
>> +    stop_sync_thread(mddev, true, false);
>> +}
>> +EXPORT_SYMBOL_GPL(md_frozen_sync_thread);
>> +
>> +void md_unfrozen_sync_thread(struct mddev *mddev)
>> +{
>> +    lockdep_assert_held(&mddev->reconfig_mutex);
>> +
>> +    clear_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
>> +    set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
>> +    md_wakeup_thread(mddev->thread);
>> +    sysfs_notify_dirent_safe(mddev->sysfs_action);
>> +}
>> +EXPORT_SYMBOL_GPL(md_unfrozen_sync_thread);
>> +
>>   static void idle_sync_thread(struct mddev *mddev)
>>   {
>>       mutex_lock(&mddev->sync_mutex);
>> diff --git a/drivers/md/md.h b/drivers/md/md.h
>> index 8d881cc59799..437ab70ce79b 100644
>> --- a/drivers/md/md.h
>> +++ b/drivers/md/md.h
>> @@ -781,6 +781,9 @@ extern void md_rdev_clear(struct md_rdev *rdev);
>>   extern void md_handle_request(struct mddev *mddev, struct bio *bio);
>>   extern int mddev_suspend(struct mddev *mddev, bool interruptible);
>>   extern void mddev_resume(struct mddev *mddev);
>> +extern void md_idle_sync_thread(struct mddev *mddev);
>> +extern void md_frozen_sync_thread(struct mddev *mddev);
>> +extern void md_unfrozen_sync_thread(struct mddev *mddev);
>>   extern void md_reload_sb(struct mddev *mddev, int raid_disk);
>>   extern void md_update_sb(struct mddev *mddev, int force);
> .
>
Paul Menzel March 6, 2024, 3:10 p.m. UTC | #3 
Dear Kuai,


Am 05.03.24 um 09:13 schrieb Yu Kuai:

> 在 2024/03/05 16:08, Paul Menzel 写道:

>> Am 05.03.24 um 08:22 schrieb Yu Kuai:
>>> From: Yu Kuai <yukuai3@huawei.com>
>>>
>>> The new heleprs will be used in dm-raid in later patches to fix
>>
>> hel*pe*rs
>>
>>> regressions and prevent calling md_reap_sync_thread() directly.
>>
>> Please list the new functions.
>>
>> 1.  md_idle_sync_thread(struct mddev *mddev);
>> 2.  md_frozen_sync_thread(struct mddev *mddev);
>> 3.  md_unfrozen_sync_thread(struct mddev *mddev);
>>
>> I do not like the naming so much. `md_reap_sync_thread()` has the verb 
>> in imperative mood. At least myself, I would not know what the 
>> functions do exactly without looking at the implementation.
>
> Thanks for the suggestions, I'm not that good at naming :(
> 
> Usually I'll send a new version with updated naming, however, we must
> merge this set ASAP now, perhaps can we live with this for now?

Fine by me. Maybe when applying the typo can be fixed, and the naming 
than later.


Kind regards,

Paul


>>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>>> Signed-off-by: Xiao Ni <xni@redhat.com>
>>> Acked-by: Mike Snitzer <snitzer@kernel.org>
>>> ---
>>>   drivers/md/md.c | 29 +++++++++++++++++++++++++++++
>>>   drivers/md/md.h |  3 +++
>>>   2 files changed, 32 insertions(+)
>>>
>>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>>> index 23f31fd1d024..22e7512a5b1e 100644
>>> --- a/drivers/md/md.c
>>> +++ b/drivers/md/md.c
>>> @@ -4919,6 +4919,35 @@ static void stop_sync_thread(struct mddev 
>>> *mddev, bool locked, bool check_seq)
>>>           mddev_lock_nointr(mddev);
>>>   }
>>> +void md_idle_sync_thread(struct mddev *mddev)
>>> +{
>>> +    lockdep_assert_held(&mddev->reconfig_mutex);
>>> +
>>> +    clear_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
>>> +    stop_sync_thread(mddev, true, true);
>>> +}
>>> +EXPORT_SYMBOL_GPL(md_idle_sync_thread);
>>> +
>>> +void md_frozen_sync_thread(struct mddev *mddev)
>>> +{
>>> +    lockdep_assert_held(&mddev->reconfig_mutex);
>>> +
>>> +    set_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
>>> +    stop_sync_thread(mddev, true, false);
>>> +}
>>> +EXPORT_SYMBOL_GPL(md_frozen_sync_thread);
>>> +
>>> +void md_unfrozen_sync_thread(struct mddev *mddev)
>>> +{
>>> +    lockdep_assert_held(&mddev->reconfig_mutex);
>>> +
>>> +    clear_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
>>> +    set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
>>> +    md_wakeup_thread(mddev->thread);
>>> +    sysfs_notify_dirent_safe(mddev->sysfs_action);
>>> +}
>>> +EXPORT_SYMBOL_GPL(md_unfrozen_sync_thread);
>>> +
>>>   static void idle_sync_thread(struct mddev *mddev)
>>>   {
>>>       mutex_lock(&mddev->sync_mutex);
>>> diff --git a/drivers/md/md.h b/drivers/md/md.h
>>> index 8d881cc59799..437ab70ce79b 100644
>>> --- a/drivers/md/md.h
>>> +++ b/drivers/md/md.h
>>> @@ -781,6 +781,9 @@ extern void md_rdev_clear(struct md_rdev *rdev);
>>>   extern void md_handle_request(struct mddev *mddev, struct bio *bio);
>>>   extern int mddev_suspend(struct mddev *mddev, bool interruptible);
>>>   extern void mddev_resume(struct mddev *mddev);
>>> +extern void md_idle_sync_thread(struct mddev *mddev);
>>> +extern void md_frozen_sync_thread(struct mddev *mddev);
>>> +extern void md_unfrozen_sync_thread(struct mddev *mddev);
>>>   extern void md_reload_sb(struct mddev *mddev, int raid_disk);
>>>   extern void md_update_sb(struct mddev *mddev, int force);
Song Liu March 6, 2024, 4:39 p.m. UTC | #4 
Hi Paul,

Thanks for reviewing the patch!

On Wed, Mar 6, 2024 at 7:10 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> Dear Kuai,
>
>
> Am 05.03.24 um 09:13 schrieb Yu Kuai:
>
> > 在 2024/03/05 16:08, Paul Menzel 写道:
>
> >> Am 05.03.24 um 08:22 schrieb Yu Kuai:
> >>> From: Yu Kuai <yukuai3@huawei.com>
> >>>
> >>> The new heleprs will be used in dm-raid in later patches to fix
> >>
> >> hel*pe*rs
> >>
> >>> regressions and prevent calling md_reap_sync_thread() directly.
> >>
> >> Please list the new functions.
> >>
> >> 1.  md_idle_sync_thread(struct mddev *mddev);
> >> 2.  md_frozen_sync_thread(struct mddev *mddev);
> >> 3.  md_unfrozen_sync_thread(struct mddev *mddev);
> >>
> >> I do not like the naming so much. `md_reap_sync_thread()` has the verb
> >> in imperative mood. At least myself, I would not know what the
> >> functions do exactly without looking at the implementation.
> >
> > Thanks for the suggestions, I'm not that good at naming :(
> >
> > Usually I'll send a new version with updated naming, however, we must
> > merge this set ASAP now, perhaps can we live with this for now?
>
> Fine by me. Maybe when applying the typo can be fixed, and the naming
> than later.

Yes, I did exactly this when applying the patches, but forgot to mention it
here.

Song
diff mbox series

Patch

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 23f31fd1d024..22e7512a5b1e 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -4919,6 +4919,35 @@  static void stop_sync_thread(struct mddev *mddev, bool locked, bool check_seq)
 		mddev_lock_nointr(mddev);
 }
 
+void md_idle_sync_thread(struct mddev *mddev)
+{
+	lockdep_assert_held(&mddev->reconfig_mutex);
+
+	clear_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
+	stop_sync_thread(mddev, true, true);
+}
+EXPORT_SYMBOL_GPL(md_idle_sync_thread);
+
+void md_frozen_sync_thread(struct mddev *mddev)
+{
+	lockdep_assert_held(&mddev->reconfig_mutex);
+
+	set_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
+	stop_sync_thread(mddev, true, false);
+}
+EXPORT_SYMBOL_GPL(md_frozen_sync_thread);
+
+void md_unfrozen_sync_thread(struct mddev *mddev)
+{
+	lockdep_assert_held(&mddev->reconfig_mutex);
+
+	clear_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
+	set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
+	md_wakeup_thread(mddev->thread);
+	sysfs_notify_dirent_safe(mddev->sysfs_action);
+}
+EXPORT_SYMBOL_GPL(md_unfrozen_sync_thread);
+
 static void idle_sync_thread(struct mddev *mddev)
 {
 	mutex_lock(&mddev->sync_mutex);
diff --git a/drivers/md/md.h b/drivers/md/md.h
index 8d881cc59799..437ab70ce79b 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -781,6 +781,9 @@  extern void md_rdev_clear(struct md_rdev *rdev);
 extern void md_handle_request(struct mddev *mddev, struct bio *bio);
 extern int mddev_suspend(struct mddev *mddev, bool interruptible);
 extern void mddev_resume(struct mddev *mddev);
+extern void md_idle_sync_thread(struct mddev *mddev);
+extern void md_frozen_sync_thread(struct mddev *mddev);
+extern void md_unfrozen_sync_thread(struct mddev *mddev);
 
 extern void md_reload_sb(struct mddev *mddev, int raid_disk);
 extern void md_update_sb(struct mddev *mddev, int force);