Re: [PATCH 1/3] MD: hold mddev lock for .quiesce in md_do_sync

[NeilBrown <neilb@suse.com>]

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

On Wed, Aug 17 2016, Shaohua Li wrote:
>> >
>> > We will have the same deadlock issue with just stopping/restarting the reclaim
>> > thread. As stopping the thread will wait for the thread, which probably is
>> > doing r5l_do_reclaim and writting the superblock. Since we are writting the
>> > superblock, we must hold the reconfig_mutex.
>> 
>> When you say "writing the superblock" you presumably mean "blocked in
>> r5l_write_super_and_discard_space(), waiting for  MD_CHANGE_PENDING to
>> be cleared" ??
> right
>> With a bit of care you could wait for MD_CHANGE_PENDING to clear, or for
>> ->quiesce to be set, and then exit gracefully.
>
> Can you give details about this please? .quiesce is called with reconfig_mutex
> hold, so the MD_CHANGE_PENDING will never get cleared.

raid5_quiesce(mddev, 1) sets conf->quiesce and then calls r5l_quiesce().

r5l_quiesce() tells the reclaim_thread to exit and waits for it to do so.

But the reclaim thread might be in
   r5l_do_reclaim() -> r5l_write_super_and_discard_space()
waiting for MD_CHANGE_PENDING to clear.  That will only get cleared when
the main thread can get the reconfig_mutex, which the thread calling
raid5_quiesce() might hold.  So we get a deadlock.

My suggestion is to change r5l_write_super_and_discard_space() so that
it waits for *either* MD_CHANGE_PENDING to be clear, or conf->quiesce
to be set.  That will avoid the deadlock.

Whatever thread called raid5_quiesce() will now be in control of the
array without any async IO going on.  If it needs the metadata to be
sync, it can do that itself.  If not, then it doesn't really matter that
r5l_write_super_and_discard_space() didn't wait.

r5l_write_super_and_discard_space() shouldn't call discard if the
superblock write didn't complete, and probably r5l_do_reclaim()
shouldn't update last_checkpoint and last_cp_seq in that case.
This is what I mean by "with a bit of care" and "exit gracefully".
Maybe I should have said "abort cleanly".  The goal is to get the thread
to exit.  It doesn't need to complete what it was doing, it just needs
to make sure that it leaves things in a tidy state so that when it
starts up again, it can pick up where it left off.

NeilBrown

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