Re: mdadm: one question about the readonly and readwrite feature

[NeilBrown <neilb@suse.com>]

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

On Thu, Mar 23 2017, Zhilong Liu wrote:

> On 03/23/2017 03:06 PM, NeilBrown wrote:
>> Why?
>> How do the actions performed by set_disk_ro() interact with the actions
>> performed by md_wakeup_thread()?
>>
>> I'm not saying the code shouldn't be changed, just that there needs to
>> be a clear explanation of the benefit.
>
> here just according to my understanding for readonly code path, welcome
> the correction in my comments if I misunderstand this feature, :-).

I'm sorry if this sounds harsh, but I get the impression that you aren't
really trying to understand the code.  You are just guessing about what
things might be doing, rather than doing careful research to determine
exactly what the code does.

Do you know what "set_disk_ro()" does?  What are the consequences of
call it?  Until you know that, you cannot make any reasonable assessment
on where the call should go.

Do you know why we set MD_RECOVERY_NEEDED and wake up the thread?
You seem to expect that it might cause some write out, however it
happens just after a call to __md_stop_writes() which aims to stop all
writes happening to the array.  So that seems unlikely (but is worth
checking).


> ... ...
> static int md_set_readonly(struct mddev *mddev, struct block_device *bdev)
> {
>          int err = 0;
>          int did_freeze = 0;
>
>          if (!test_bit(MD_RECOVERY_FROZEN, &mddev->recovery)) {
>                  did_freeze = 1;
>                  set_bit(MD_RECOVERY_FROZEN, &mddev->recovery);  --> 
> here has set bit as FROZEN
>                  md_wakeup_thread(mddev->thread);
>          }
> ... ...
>
>          if (mddev->pers) {
>                  __md_stop_writes(mddev);
> /*
>   *  set FROZEN again, maybe can use test_and_set_bit(FROZEN) better, it 
> doesn't matter.

Correct, it doesn't matter.


>   *  it flushed and synced all data, bitmap and superblock to array.

Correct again.


>   */
>                  err  = -ENXIO;
>                  if (mddev->ro==1)
>                          goto out;
>                  mddev->ro = 1;
> /*
>   *  Here, I means that set_disk_ro until the daemon thread has 
> completed all operations
>   *  include of sync and recovery progress. set_disk_ro when the array 
> has cleaned totally,
>   *  then it would be safer.

Completed which operations exactly?  __md_stop_writes() has called
md_reap_sync_thread() so there cannot still be any recovery.  It has
called pers->quiesce() so there cannot be any outstanding io.
And just moving the call to afterwards would't cause it to wait for those
supposed operations to complete.

>   *  I'm not sure MD_RECOVERY_NEEDED would be running once the array has 
> set_disk_ro,
>   *  actually I don't know how to test this scenario, thus asked this 
> question.

The first step is to understand the code.
Your questions was "should we move this line to here", without asking any
questions about what the code does, or showing much understanding of it.

I do encourage you to ask questions, but it is best to start with
questions that make sense.
And once you have framed a question, try to answer it yourself.
Read the code (e.g. the code for set_disk_ro()) if you haven't read it
before, to be sure you understand what it does.
And if you don't know why some code does something, it often helps to
look through the git logs, or use "git blame" to find out when the code
was added or changed.  Often the changelog of the patch will explain the
purpose of the code.

NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]