Re: [md PATCH 1/5 v2] md: always hold reconfig_mutex when calling mddev_suspend()

[NeilBrown <neilb@suse.com>]

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

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]