* [PATCH] md: cleanup md_check_recovery()
@ 2025-09-04 11:13 Wu Guanghao
2025-09-04 12:08 ` Paul Menzel
2025-09-05 8:54 ` Yu Kuai
0 siblings, 2 replies; 5+ messages in thread
From: Wu Guanghao @ 2025-09-04 11:13 UTC (permalink / raw)
To: song, yukuai (C); +Cc: linux-raid, yangyun50
In md_check_recovery(), use new helpers to make code cleaner.
Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
---
drivers/md/md.c | 41 +++++++++++++++++++++++++++++------------
1 file changed, 29 insertions(+), 12 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 1baaf52c603c..cbbb9ac14cf6 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -9741,6 +9741,34 @@ static void unregister_sync_thread(struct mddev *mddev)
md_reap_sync_thread(mddev);
}
+
+
+static bool md_should_do_recovery(struct mddev *mddev)
+{
+ /*
+ * As long as one of the following flags is set,
+ * recovery needs to do.
+ */
+ if (test_bit(MD_RECOVERY_NEEDED, &mddev->recovery) ||
+ test_bit(MD_RECOVERY_DONE, &mddev->recovery))
+ return true;
+
+ /*
+ * If no flags are set and it is in read-only status,
+ * there is nothing to do.
+ */
+ if (!md_is_rdwr(mddev))
+ return false;
+
+ if ((mddev->sb_flags & ~ (1<<MD_SB_CHANGE_PENDING)) ||
+ (mddev->external == 0 && mddev->safemode == 1) ||
+ (mddev->safemode == 2 && !mddev->in_sync &&
+ mddev->resync_offset == MaxSector))
+ return true;
+
+ return false;
+}
+
/*
* This routine is regularly called by all per-raid-array threads to
* deal with generic issues like resync and super-block update.
@@ -9777,18 +9805,7 @@ void md_check_recovery(struct mddev *mddev)
flush_signals(current);
}
- if (!md_is_rdwr(mddev) &&
- !test_bit(MD_RECOVERY_NEEDED, &mddev->recovery) &&
- !test_bit(MD_RECOVERY_DONE, &mddev->recovery))
- return;
- if ( ! (
- (mddev->sb_flags & ~ (1<<MD_SB_CHANGE_PENDING)) ||
- test_bit(MD_RECOVERY_NEEDED, &mddev->recovery) ||
- test_bit(MD_RECOVERY_DONE, &mddev->recovery) ||
- (mddev->external == 0 && mddev->safemode == 1) ||
- (mddev->safemode == 2
- && !mddev->in_sync && mddev->resync_offset == MaxSector)
- ))
+ if (!md_should_do_recovery(mddev))
return;
if (mddev_trylock(mddev)) {
--
2.27.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] md: cleanup md_check_recovery()
2025-09-04 11:13 [PATCH] md: cleanup md_check_recovery() Wu Guanghao
@ 2025-09-04 12:08 ` Paul Menzel
2025-09-08 2:09 ` Wu Guanghao
2025-09-05 8:54 ` Yu Kuai
1 sibling, 1 reply; 5+ messages in thread
From: Paul Menzel @ 2025-09-04 12:08 UTC (permalink / raw)
To: Wu Guanghao; +Cc: song, yukuai3, linux-raid, yangyun50
Dear Guanghao,
Thank you for your patch. For the summary I suggest:
Factor out code into md_should_do_recovery()
Am 04.09.25 um 13:13 schrieb Wu Guanghao:
> In md_check_recovery(), use new helpers to make code cleaner.
Singular *helper*?
> Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
> ---
> drivers/md/md.c | 41 +++++++++++++++++++++++++++++------------
> 1 file changed, 29 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 1baaf52c603c..cbbb9ac14cf6 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -9741,6 +9741,34 @@ static void unregister_sync_thread(struct mddev *mddev)
> md_reap_sync_thread(mddev);
> }
>
> +
> +
Why so many blank lines.
> +static bool md_should_do_recovery(struct mddev *mddev)
> +{
> + /*
> + * As long as one of the following flags is set,
> + * recovery needs to do.
… recovery needs to be do*ne*?
> + */
> + if (test_bit(MD_RECOVERY_NEEDED, &mddev->recovery) ||
> + test_bit(MD_RECOVERY_DONE, &mddev->recovery))
> + return true;
> +
> + /*
> + * If no flags are set and it is in read-only status,
> + * there is nothing to do.
Ditto.
> + */
> + if (!md_is_rdwr(mddev))
> + return false;
> +
> + if ((mddev->sb_flags & ~ (1<<MD_SB_CHANGE_PENDING)) ||
> + (mddev->external == 0 && mddev->safemode == 1) ||
> + (mddev->safemode == 2 && !mddev->in_sync &&
> + mddev->resync_offset == MaxSector))
> + return true;
> +
> + return false;
> +}
> +
> /*
> * This routine is regularly called by all per-raid-array threads to
> * deal with generic issues like resync and super-block update.
> @@ -9777,18 +9805,7 @@ void md_check_recovery(struct mddev *mddev)
> flush_signals(current);
> }
>
> - if (!md_is_rdwr(mddev) &&
> - !test_bit(MD_RECOVERY_NEEDED, &mddev->recovery) &&
> - !test_bit(MD_RECOVERY_DONE, &mddev->recovery))
> - return;
> - if ( ! (
> - (mddev->sb_flags & ~ (1<<MD_SB_CHANGE_PENDING)) ||
> - test_bit(MD_RECOVERY_NEEDED, &mddev->recovery) ||
> - test_bit(MD_RECOVERY_DONE, &mddev->recovery) ||
> - (mddev->external == 0 && mddev->safemode == 1) ||
> - (mddev->safemode == 2
> - && !mddev->in_sync && mddev->resync_offset == MaxSector)
> - ))
> + if (!md_should_do_recovery(mddev))
> return;
>
> if (mddev_trylock(mddev)) {
The code diff looks good.
Kind regards,
Paul
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] md: cleanup md_check_recovery()
2025-09-04 12:08 ` Paul Menzel
@ 2025-09-08 2:09 ` Wu Guanghao
0 siblings, 0 replies; 5+ messages in thread
From: Wu Guanghao @ 2025-09-08 2:09 UTC (permalink / raw)
To: Paul Menzel; +Cc: song, yukuai3, linux-raid, yangyun50
Thank you for your suggestion, I will modify it in the next version.
在 2025/9/4 20:08, Paul Menzel 写道:
> Dear Guanghao,
>
>
> Thank you for your patch. For the summary I suggest:
>
> Factor out code into md_should_do_recovery()
>
> Am 04.09.25 um 13:13 schrieb Wu Guanghao:
>> In md_check_recovery(), use new helpers to make code cleaner.
>
> Singular *helper*?
>
>> Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
>> ---
>> drivers/md/md.c | 41 +++++++++++++++++++++++++++++------------
>> 1 file changed, 29 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 1baaf52c603c..cbbb9ac14cf6 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -9741,6 +9741,34 @@ static void unregister_sync_thread(struct mddev *mddev)
>> md_reap_sync_thread(mddev);
>> }
>>
>> +
>> +
>
> Why so many blank lines.
>
>> +static bool md_should_do_recovery(struct mddev *mddev)
>> +{
>> + /*
>> + * As long as one of the following flags is set,
>> + * recovery needs to do.
>
> … recovery needs to be do*ne*?
>
>> + */
>> + if (test_bit(MD_RECOVERY_NEEDED, &mddev->recovery) ||
>> + test_bit(MD_RECOVERY_DONE, &mddev->recovery))
>> + return true;
>> +
>> + /*
>> + * If no flags are set and it is in read-only status,
>> + * there is nothing to do.
>
> Ditto.
>
>> + */
>> + if (!md_is_rdwr(mddev))
>> + return false;
>> +
>> + if ((mddev->sb_flags & ~ (1<<MD_SB_CHANGE_PENDING)) ||
>> + (mddev->external == 0 && mddev->safemode == 1) ||
>> + (mddev->safemode == 2 && !mddev->in_sync &&
>> + mddev->resync_offset == MaxSector))
>> + return true;
>> +
>> + return false;
>> +}
>> +
>> /*
>> * This routine is regularly called by all per-raid-array threads to
>> * deal with generic issues like resync and super-block update.
>> @@ -9777,18 +9805,7 @@ void md_check_recovery(struct mddev *mddev)
>> flush_signals(current);
>> }
>>
>> - if (!md_is_rdwr(mddev) &&
>> - !test_bit(MD_RECOVERY_NEEDED, &mddev->recovery) &&
>> - !test_bit(MD_RECOVERY_DONE, &mddev->recovery))
>> - return;
>> - if ( ! (
>> - (mddev->sb_flags & ~ (1<<MD_SB_CHANGE_PENDING)) ||
>> - test_bit(MD_RECOVERY_NEEDED, &mddev->recovery) ||
>> - test_bit(MD_RECOVERY_DONE, &mddev->recovery) ||
>> - (mddev->external == 0 && mddev->safemode == 1) ||
>> - (mddev->safemode == 2
>> - && !mddev->in_sync && mddev->resync_offset == MaxSector)
>> - ))
>> + if (!md_should_do_recovery(mddev))
>> return;
>>
>> if (mddev_trylock(mddev)) {
>
> The code diff looks good.
>
>
> Kind regards,
>
> Paul
> .
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] md: cleanup md_check_recovery()
2025-09-04 11:13 [PATCH] md: cleanup md_check_recovery() Wu Guanghao
2025-09-04 12:08 ` Paul Menzel
@ 2025-09-05 8:54 ` Yu Kuai
2025-09-08 2:12 ` Wu Guanghao
1 sibling, 1 reply; 5+ messages in thread
From: Yu Kuai @ 2025-09-05 8:54 UTC (permalink / raw)
To: Wu Guanghao, song; +Cc: linux-raid, yangyun50, yukuai (C)
Hi,
在 2025/09/04 19:13, Wu Guanghao 写道:
> In md_check_recovery(), use new helpers to make code cleaner.
>
> Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
> ---
> drivers/md/md.c | 41 +++++++++++++++++++++++++++++------------
> 1 file changed, 29 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 1baaf52c603c..cbbb9ac14cf6 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -9741,6 +9741,34 @@ static void unregister_sync_thread(struct mddev *mddev)
> md_reap_sync_thread(mddev);
> }
>
> +
> +
> +static bool md_should_do_recovery(struct mddev *mddev)
> +{
> + /*
> + * As long as one of the following flags is set,
> + * recovery needs to do.
> + */
> + if (test_bit(MD_RECOVERY_NEEDED, &mddev->recovery) ||
> + test_bit(MD_RECOVERY_DONE, &mddev->recovery))
> + return true;
> +
> + /*
> + * If no flags are set and it is in read-only status,
> + * there is nothing to do.
> + */
> + if (!md_is_rdwr(mddev))
> + return false;
> +
> + if ((mddev->sb_flags & ~ (1<<MD_SB_CHANGE_PENDING)) ||
> + (mddev->external == 0 && mddev->safemode == 1) ||
> + (mddev->safemode == 2 && !mddev->in_sync &&
> + mddev->resync_offset == MaxSector))
> + return true;
Plese also split abouve conditions and add comments.
Thanks,
Kuai
> +
> + return false;
> +}
> +
> /*
> * This routine is regularly called by all per-raid-array threads to
> * deal with generic issues like resync and super-block update.
> @@ -9777,18 +9805,7 @@ void md_check_recovery(struct mddev *mddev)
> flush_signals(current);
> }
>
> - if (!md_is_rdwr(mddev) &&
> - !test_bit(MD_RECOVERY_NEEDED, &mddev->recovery) &&
> - !test_bit(MD_RECOVERY_DONE, &mddev->recovery))
> - return;
> - if ( ! (
> - (mddev->sb_flags & ~ (1<<MD_SB_CHANGE_PENDING)) ||
> - test_bit(MD_RECOVERY_NEEDED, &mddev->recovery) ||
> - test_bit(MD_RECOVERY_DONE, &mddev->recovery) ||
> - (mddev->external == 0 && mddev->safemode == 1) ||
> - (mddev->safemode == 2
> - && !mddev->in_sync && mddev->resync_offset == MaxSector)
> - ))
> + if (!md_should_do_recovery(mddev))
> return;
>
> if (mddev_trylock(mddev)) {
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] md: cleanup md_check_recovery()
2025-09-05 8:54 ` Yu Kuai
@ 2025-09-08 2:12 ` Wu Guanghao
0 siblings, 0 replies; 5+ messages in thread
From: Wu Guanghao @ 2025-09-08 2:12 UTC (permalink / raw)
To: Yu Kuai, song; +Cc: linux-raid, yangyun50, yukuai (C)
在 2025/9/5 16:54, Yu Kuai 写道:
> Hi,
>
> 在 2025/09/04 19:13, Wu Guanghao 写道:
>> In md_check_recovery(), use new helpers to make code cleaner.
>>
>> Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
>> ---
>> drivers/md/md.c | 41 +++++++++++++++++++++++++++++------------
>> 1 file changed, 29 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 1baaf52c603c..cbbb9ac14cf6 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -9741,6 +9741,34 @@ static void unregister_sync_thread(struct mddev *mddev)
>> md_reap_sync_thread(mddev);
>> }
>>
>> +
>> +
>> +static bool md_should_do_recovery(struct mddev *mddev)
>> +{
>> + /*
>> + * As long as one of the following flags is set,
>> + * recovery needs to do.
>> + */
>> + if (test_bit(MD_RECOVERY_NEEDED, &mddev->recovery) ||
>> + test_bit(MD_RECOVERY_DONE, &mddev->recovery))
>> + return true;
>> +
>> + /*
>> + * If no flags are set and it is in read-only status,
>> + * there is nothing to do.
>> + */
>> + if (!md_is_rdwr(mddev))
>> + return false;
>> +
>> + if ((mddev->sb_flags & ~ (1<<MD_SB_CHANGE_PENDING)) ||
>> + (mddev->external == 0 && mddev->safemode == 1) ||
>> + (mddev->safemode == 2 && !mddev->in_sync &&
>> + mddev->resync_offset == MaxSector))
>> + return true;
>
> Plese also split abouve conditions and add comments.
>
> Thanks,
> Kuai
>
OK，I will make the changes in the next version.
Thanks,
Guanghao
>> +
>> + return false;
>> +}
>> +
>> /*
>> * This routine is regularly called by all per-raid-array threads to
>> * deal with generic issues like resync and super-block update.
>> @@ -9777,18 +9805,7 @@ void md_check_recovery(struct mddev *mddev)
>> flush_signals(current);
>> }
>>
>> - if (!md_is_rdwr(mddev) &&
>> - !test_bit(MD_RECOVERY_NEEDED, &mddev->recovery) &&
>> - !test_bit(MD_RECOVERY_DONE, &mddev->recovery))
>> - return;
>> - if ( ! (
>> - (mddev->sb_flags & ~ (1<<MD_SB_CHANGE_PENDING)) ||
>> - test_bit(MD_RECOVERY_NEEDED, &mddev->recovery) ||
>> - test_bit(MD_RECOVERY_DONE, &mddev->recovery) ||
>> - (mddev->external == 0 && mddev->safemode == 1) ||
>> - (mddev->safemode == 2
>> - && !mddev->in_sync && mddev->resync_offset == MaxSector)
>> - ))
>> + if (!md_should_do_recovery(mddev))
>> return;
>>
>> if (mddev_trylock(mddev)) {
>>
>
> .
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-09-08 2:12 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-04 11:13 [PATCH] md: cleanup md_check_recovery() Wu Guanghao
2025-09-04 12:08 ` Paul Menzel
2025-09-08 2:09 ` Wu Guanghao
2025-09-05 8:54 ` Yu Kuai
2025-09-08 2:12 ` Wu Guanghao
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox