On Wed, Aug 24 2016, Shaohua Li wrote: > On Wed, Aug 24, 2016 at 02:49:57PM +1000, Neil Brown wrote: >> 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. > > I'm afraid waiting conf->quiesce set isn't safe. The reason to wait for > superblock write isn't because of async IO. discard could zero data, so before > we do discard, we must make sure superblock points to correct log tail, > otherwise recovery will not work. This is the reason we wait for superblock > write. > >> 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. > > Agree, we could ignore discard sometime, which happens occasionally, so impact > is little. I tested something like below recently. Assume this is the solution > we agree on? Yes, this definitely looks like it is heading in the right direction. I thought that > - set_mask_bits(&mddev->flags, 0, > - BIT(MD_CHANGE_DEVS) | BIT(MD_CHANGE_PENDING)); > - md_wakeup_thread(mddev->thread); would still be there in the case that the lock cannot be claimed. You could even record the ->events value before setting the flags, and record the range that needs to be discarded. Next time r5l_do_reclaim is entered, if ->events has moved on, then it should be safe to discard the recorded range. Maybe. Thanks, NeilBrown
Attachment:
signature.asc
Description: PGP signature
