On Thu, Oct 19 2017, Shaohua Li wrote: >> > >> > For this one, my point is: >> > >> > wait_event(mddev->sb_wait, conf->log == NULL || >> > !test_bit(MD_SB_CHANGE_PENDING, &mddev->sb_flags)); >> > if (conf->log == NULL) >> > return; >> > >> > mddev_suspend(mddev); >> > log->r5c_journal_mode = R5C_JOURNAL_MODE_WRITE_THROUGH; >> > mddev_resume(mddev); >> > >> > does it work? >> >> The >> lockdep_assert_held(&mddev->reconfig_mutex); >> in mddev_suspend() will complain. >> >> If you put an mddev_lock() call in there to stop the complaint, and if >> the work item doesn't start before the reconfig_mutex is taken prior to >> stopping the array, then r5l_exit_log() can deadlock at >> flush_work(&log->disable_writeback_work); > > Ok, got it now. But really don't like this patch. The mddev_unlock is strange, > r5c_disable_writeback_async could skip disabling writeback too. Could we add a > new callback like .prepare_free, and flush workqueue there. After we drop the > reconfig_mutex in do_md_stop, we call the prepare_free. We can probably set a > flag, so later r5c_disable_writeback_async will bail out doing nothing. I think > this should work, right? Might work, though it sounds more messy to me (assuming I understand). I would like to get rid of disable_writeback_work altogether. Just set log->r5c_journal_mode = R5C_JOURNAL_MODE_WRITE_THROUGH in r5c_update_on_rdev_error(), and make sure that does the right thing. The distinction between write-through and write-back should be able to be a per-stripe_head distinction. Once we set r5c_journal_mode, new stripe_heads will get the new mode, old ones are allowed to continue how they are. Maybe we could keep a counter of how many stripes are in WRITE_BACK mode, and test that counter in r5c_is_writeback()?? I don't know what all the issues are so it would need careful review, preferably by someone familiar with the code. Short of that, I think my current patch is the best interim step. I agree that it isn't the most elegant thing ever, but it is localized and I believe it works correctly. The "mddev_unlock()" shouldn't look too strange, it perfectly balances he mddev_try_lock(). Thanks, NeilBrown
Attachment:
signature.asc
Description: PGP signature
