[PATCH - RFC] MD: Sync thread not properly shutdown after mddev_suspend()

[PATCH - RFC] MD: Sync thread not properly shutdown after mddev_suspend()

[Jonathan Brassow]

MD: Sync thread not properly shutdown after mddev_suspend()

After performing an 'md_stop_writes' followed by an 'mddev_suspend',
it is possible to have 'MD_RECOVERY_RUNNING' set in mddev->recovery.
It doesn't happen often, but when it does, the recovery thread does
not restart properly after a resume.

The problem seems to come from 'md_stop_writes'.  This function is a
wrapper around '__md_stop_writes' - surrounding it with mddev_[un]lock
calls.  While '__md_stop_writes' properly cleans up the sync thread,
the subsequent 'mddev_unlock' call will wake up the personality thread,
which in turn calls 'md_check_recovery' - a function that sets
mddev->recovery flags and potentially launches the sync thread.
Effectively, this can undo what has just been done.

When 'mddev_suspend' is called, it sets the mddev->suspended variable.
This variable causes 'md_check_recovery' to simply return if set.  Thus,
it is better to reap the sync thread in mddev_suspend, because it cannot
be respawned until mddev_resume is called.

There are probably several ways to solve this problem.  The simplest way
was to add 'md_reap_sync_thread' to mddev_suspend.  It may be
better fixed in 'md_stop_writes' though.  We could also combine
'md_stop_writes' and 'mddev_suspend' by calling '__md_stop_writes' from
within 'mddev_suspend' after mddev->suspended has been set.

Thoughts?

Signed-off-by: Jonathan Brassow <jbrassow@redhat.com>

Index: linux-upstream/drivers/md/md.c
===================================================================
--- linux-upstream.orig/drivers/md/md.c
+++ linux-upstream/drivers/md/md.c
@@ -360,6 +360,7 @@ void mddev_suspend(struct mddev *mddev)
 	mddev->pers->quiesce(mddev, 1);
 
 	del_timer_sync(&mddev->safemode_timer);
+	md_reap_sync_thread(mddev);
 }
 EXPORT_SYMBOL_GPL(mddev_suspend);
 

Re: [PATCH - RFC] MD: Sync thread not properly shutdown after mddev_suspend()

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3223 bytes --]

On Thu, 02 May 2013 15:19:23 -0500 Jonathan Brassow <jbrassow@redhat.com>
wrote:

> MD: Sync thread not properly shutdown after mddev_suspend()
> 
> After performing an 'md_stop_writes' followed by an 'mddev_suspend',
> it is possible to have 'MD_RECOVERY_RUNNING' set in mddev->recovery.
> It doesn't happen often, but when it does, the recovery thread does
> not restart properly after a resume.
> 
> The problem seems to come from 'md_stop_writes'.  This function is a
> wrapper around '__md_stop_writes' - surrounding it with mddev_[un]lock
> calls.  While '__md_stop_writes' properly cleans up the sync thread,
> the subsequent 'mddev_unlock' call will wake up the personality thread,
> which in turn calls 'md_check_recovery' - a function that sets
> mddev->recovery flags and potentially launches the sync thread.
> Effectively, this can undo what has just been done.
> 
> When 'mddev_suspend' is called, it sets the mddev->suspended variable.
> This variable causes 'md_check_recovery' to simply return if set.  Thus,
> it is better to reap the sync thread in mddev_suspend, because it cannot
> be respawned until mddev_resume is called.
> 
> There are probably several ways to solve this problem.  The simplest way
> was to add 'md_reap_sync_thread' to mddev_suspend.  It may be
> better fixed in 'md_stop_writes' though.  We could also combine
> 'md_stop_writes' and 'mddev_suspend' by calling '__md_stop_writes' from
> within 'mddev_suspend' after mddev->suspended has been set.
> 
> Thoughts?

Thanks for the thorough analysis.

Your patch looks like it would work,  but it involves calling
md_reap_sync_thread() twice which is a little ugly.

How about this:

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 4c74424..3e2acfa 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -5277,8 +5277,8 @@ static void md_clean(struct mddev *mddev)
 
 static void __md_stop_writes(struct mddev *mddev)
 {
+	set_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
 	if (mddev->sync_thread) {
-		set_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
 		set_bit(MD_RECOVERY_INTR, &mddev->recovery);
 		md_reap_sync_thread(mddev);
 	}


Callers of md_stop_writes() already need to be prepared for
MD_RECOVERY_FROZEN to get set, and raid_resume() clears it for dm-raid.c, so
it should be safe.
An md_check_recovery won't start anything while MD_RECOVERY_FROZEN is set.
So this should *really* stop writes going to the devices.

Make sense?

Thanks,
NeilBrown



> 
> Signed-off-by: Jonathan Brassow <jbrassow@redhat.com>
> 
> Index: linux-upstream/drivers/md/md.c
> ===================================================================
> --- linux-upstream.orig/drivers/md/md.c
> +++ linux-upstream/drivers/md/md.c
> @@ -360,6 +360,7 @@ void mddev_suspend(struct mddev *mddev)
>  	mddev->pers->quiesce(mddev, 1);
>  
>  	del_timer_sync(&mddev->safemode_timer);
> +	md_reap_sync_thread(mddev);
>  }
>  EXPORT_SYMBOL_GPL(mddev_suspend);
>  
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH - RFC] MD: Sync thread not properly shutdown after mddev_suspend()

[Brassow Jonathan]

On May 6, 2013, at 1:12 AM, NeilBrown wrote:

> On Thu, 02 May 2013 15:19:23 -0500 Jonathan Brassow <jbrassow@redhat.com>
> wrote:
> 
>> MD: Sync thread not properly shutdown after mddev_suspend()
>> 
>> After performing an 'md_stop_writes' followed by an 'mddev_suspend',
>> it is possible to have 'MD_RECOVERY_RUNNING' set in mddev->recovery.
>> It doesn't happen often, but when it does, the recovery thread does
>> not restart properly after a resume.
>> 
>> The problem seems to come from 'md_stop_writes'.  This function is a
>> wrapper around '__md_stop_writes' - surrounding it with mddev_[un]lock
>> calls.  While '__md_stop_writes' properly cleans up the sync thread,
>> the subsequent 'mddev_unlock' call will wake up the personality thread,
>> which in turn calls 'md_check_recovery' - a function that sets
>> mddev->recovery flags and potentially launches the sync thread.
>> Effectively, this can undo what has just been done.
>> 
>> When 'mddev_suspend' is called, it sets the mddev->suspended variable.
>> This variable causes 'md_check_recovery' to simply return if set.  Thus,
>> it is better to reap the sync thread in mddev_suspend, because it cannot
>> be respawned until mddev_resume is called.
>> 
>> There are probably several ways to solve this problem.  The simplest way
>> was to add 'md_reap_sync_thread' to mddev_suspend.  It may be
>> better fixed in 'md_stop_writes' though.  We could also combine
>> 'md_stop_writes' and 'mddev_suspend' by calling '__md_stop_writes' from
>> within 'mddev_suspend' after mddev->suspended has been set.
>> 
>> Thoughts?
> 
> Thanks for the thorough analysis.
> 
> Your patch looks like it would work,  but it involves calling
> md_reap_sync_thread() twice which is a little ugly.
> 
> How about this:
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 4c74424..3e2acfa 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -5277,8 +5277,8 @@ static void md_clean(struct mddev *mddev)
> 
> static void __md_stop_writes(struct mddev *mddev)
> {
> +	set_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
> 	if (mddev->sync_thread) {
> -		set_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
> 		set_bit(MD_RECOVERY_INTR, &mddev->recovery);
> 		md_reap_sync_thread(mddev);
> 	}
> 
> 
> Callers of md_stop_writes() already need to be prepared for
> MD_RECOVERY_FROZEN to get set, and raid_resume() clears it for dm-raid.c, so
> it should be safe.
> An md_check_recovery won't start anything while MD_RECOVERY_FROZEN is set.
> So this should *really* stop writes going to the devices.
> 
> Make sense?

Yeah, that looks good, but give me a day or two to test it.   It seems that with the addition of this patch, the previous patch we added to revive failed devices on raid_resume sometimes fails.  I can't reproduce it by hand, but some of my automated tests will hit it ~ 1 out of 100 times.  So let me investigate a bit more.

 brassow

Re: [PATCH - RFC] MD: Sync thread not properly shutdown after mddev_suspend()

[Brassow Jonathan]

On May 7, 2013, at 8:25 AM, Brassow Jonathan wrote:

> 
> On May 6, 2013, at 1:12 AM, NeilBrown wrote:
> 
>> On Thu, 02 May 2013 15:19:23 -0500 Jonathan Brassow <jbrassow@redhat.com>
>> wrote:
>> 
>>> MD: Sync thread not properly shutdown after mddev_suspend()
>>> 
>>> After performing an 'md_stop_writes' followed by an 'mddev_suspend',
>>> it is possible to have 'MD_RECOVERY_RUNNING' set in mddev->recovery.
>>> It doesn't happen often, but when it does, the recovery thread does
>>> not restart properly after a resume.
>>> 
>>> The problem seems to come from 'md_stop_writes'.  This function is a
>>> wrapper around '__md_stop_writes' - surrounding it with mddev_[un]lock
>>> calls.  While '__md_stop_writes' properly cleans up the sync thread,
>>> the subsequent 'mddev_unlock' call will wake up the personality thread,
>>> which in turn calls 'md_check_recovery' - a function that sets
>>> mddev->recovery flags and potentially launches the sync thread.
>>> Effectively, this can undo what has just been done.
>>> 
>>> When 'mddev_suspend' is called, it sets the mddev->suspended variable.
>>> This variable causes 'md_check_recovery' to simply return if set.  Thus,
>>> it is better to reap the sync thread in mddev_suspend, because it cannot
>>> be respawned until mddev_resume is called.
>>> 
>>> There are probably several ways to solve this problem.  The simplest way
>>> was to add 'md_reap_sync_thread' to mddev_suspend.  It may be
>>> better fixed in 'md_stop_writes' though.  We could also combine
>>> 'md_stop_writes' and 'mddev_suspend' by calling '__md_stop_writes' from
>>> within 'mddev_suspend' after mddev->suspended has been set.
>>> 
>>> Thoughts?
>> 
>> Thanks for the thorough analysis.
>> 
>> Your patch looks like it would work,  but it involves calling
>> md_reap_sync_thread() twice which is a little ugly.
>> 
>> How about this:
>> 
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 4c74424..3e2acfa 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -5277,8 +5277,8 @@ static void md_clean(struct mddev *mddev)
>> 
>> static void __md_stop_writes(struct mddev *mddev)
>> {
>> +	set_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
>> 	if (mddev->sync_thread) {
>> -		set_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
>> 		set_bit(MD_RECOVERY_INTR, &mddev->recovery);
>> 		md_reap_sync_thread(mddev);
>> 	}
>> 
>> 
>> Callers of md_stop_writes() already need to be prepared for
>> MD_RECOVERY_FROZEN to get set, and raid_resume() clears it for dm-raid.c, so
>> it should be safe.
>> An md_check_recovery won't start anything while MD_RECOVERY_FROZEN is set.
>> So this should *really* stop writes going to the devices.
>> 
>> Make sense?
> 
> Yeah, that looks good, but give me a day or two to test it.   It seems that with the addition of this patch, the previous patch we added to revive failed devices on raid_resume sometimes fails.  I can't reproduce it by hand, but some of my automated tests will hit it ~ 1 out of 100 times.  So let me investigate a bit more.

Yes.  This solution works well please apply.

I've also discovered the source of the problem I was seeing with the patch that attempts to revive failed devices in raid_resume().  Sometimes a device can be set as Faulty but have the array suspend before the personality can call 'hot_remove_disk'.  This causes problems when it comes time to attempt 'hot_add_disk' in raid_resume.  I'll have two follow-up patches to resolve this soon.

 brassow