linux-kernel - Re: [PATCH -next v2 3/7] md: delay choosing sync direction to md_start

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <937878d9-7c43-a3e3-9208-d3ff1bb3df20@huaweicloud.com>
Date:   Sun, 20 Aug 2023 10:04:23 +0800
From:   Yu Kuai <yukuai1@...weicloud.com>
To:     Xiao Ni <xni@...hat.com>, Yu Kuai <yukuai1@...weicloud.com>
Cc:     song@...nel.org, linux-raid@...r.kernel.org,
        linux-kernel@...r.kernel.org, yi.zhang@...wei.com,
        yangerkun@...wei.com, "yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH -next v2 3/7] md: delay choosing sync direction to
 md_start_sync()

Hi,

在 2023/08/16 14:38, Xiao Ni 写道:
> On Tue, Aug 15, 2023 at 11:13 AM Yu Kuai <yukuai1@...weicloud.com> wrote:
>>
>> From: Yu Kuai <yukuai3@...wei.com>
>>
>> Before this patch, for read-write array:
>>
>> 1) md_check_recover() found that something need to be done, and it'll
>>     try to grab 'reconfig_mutex'. The case that md_check_recover() need
>>     to do something:
>>     - array is not suspend;
>>     - super_block need to be updated;
>>     - 'MD_RECOVERY_NEEDED' or ''MD_RECOVERY_DONE' is set;
>>     - unusual case related to safemode;
>>
>> 2) if 'MD_RECOVERY_RUNNING' is not set, and 'MD_RECOVERY_NEEDED' is set,
>>     md_check_recover() will try to choose a sync direction, and then
>>     queue a work md_start_sync().
>>
>> 3) md_start_sync() register sync_thread;
>>
>> After this patch,
>>
>> 1) is the same;
>> 2) if 'MD_RECOVERY_RUNNING' is not set, and 'MD_RECOVERY_NEEDED' is set,
>>     queue a work md_start_sync() directly;
>> 3) md_start_sync() will try to choose a sync direction, and then
>>     register sync_thread();
>>
>> Because 'MD_RECOVERY_RUNNING' is cleared when sync_thread is done, 2)
>> and 3) is always ran in serial and they can never concurrent, this
>> change should not introduce any behavior change for now.
>>
>> Also fix a problem that md_start_sync() can clear 'MD_RECOVERY_RUNNING'
>> without protection in error path, which might affect the logical in
>> md_check_recovery().
>>
>> The advantage to change this is that array reconfiguration is
>> independent from daemon now, and it'll be much easier to synchronize it
>> with io, consider that io may rely on daemon thread to be done.
> 
> Hi Kuai
> 
> What's the meaning of "array reconfiguration" here? "mdadm -f/-r/-a"
> something like this, right?. Because before and after this patch, only
> one sync thread can be running, so If we don't do this change, are
> there bugs or performance problems?

As we discussed([1]), and explained in patch 0, the purpose of this
change is to prepare to synchronize io with array reconfiguration(add
or remove rdev from array, for example, modify
conf->mirrors[].rdev/replacement for raid10).

Without this change, normal io can rely on daemon thread, while daemone
thread can change array configuration. raid1/raid10 record such io as
'io_queued', and can use freeze_array() to do synchronization in daemon
thread. However, other personalities have to implement such logical as
well, and I found it quite complicated, at least for raid456.

[1] 
https://lore.kernel.org/all/cb390b39-1b1e-04e1-55ad-2ff8afc47e1b@huawei.com/

Thanks,
Kuai

> 
> If it's only a patch that wants to make md_check_recovery more clearer
> and easier, I'm good with this idea too.
>