On Mon, Oct 23, 2017 at 11:02:23AM +1100, Neil Brown wrote: > 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. We check the writeback in several steps, would be problematical, but I didn't take a close look yet. > 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(). Alright, we need this patch to avoid the lockdep warning, so I queued the patch now. Once I got time, I'll check if the proposal works. Thanks, Shaohua -- To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
