[PATCH] md: cleanup md_check_recovery()

[PATCH] md: cleanup md_check_recovery()

[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;
+
+	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

Re: [PATCH] md: cleanup md_check_recovery()

[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

Re: [PATCH] md: cleanup md_check_recovery()

[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

> +
> +	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)) {
> 

Re: [PATCH] md: cleanup md_check_recovery()

[Wu Guanghao]

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
> .

Re: [PATCH] md: cleanup md_check_recovery()

[Wu Guanghao]

在 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)) {
>>
> 
> .