On Thu, Oct 19 2017, Shaohua Li wrote:

quoted

quoted

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