> 2023年5月30日 21:05，Mariusz Tkaczyk <mariusz.tkaczyk@xxxxxxxxxxxxxxx> 写道：
>
> On Tue, 30 May 2023 18:59:46 +0800
> Coly Li <colyli@xxxxxxx> wrote:
>
>> On Tue, May 30, 2023 at 08:17:18AM +0200, Mariusz Tkaczyk wrote:
>>> On Tue, 30 May 2023 00:07:54 +0800
>>> Coly Li <colyli@xxxxxxx> wrote:
>>>
>>>> Utilility udisks is removed from udev upstream, calling this obsoleted
>>>> command in run_udisks() doesn't make any sense now.
>>>>
>>>> This patch removes the calls chain of udisks, which includes routines
>>>> run_udisk(), force_remove(), and 2 locations where force_remove() are
>>>> called. Considering force_remove() is removed with udisks util, it is
>>>> fair to remove Manage_stop() inside force_remove() as well.
>>>>
>>>> In the two modifications where calling force_remove() are removed,
>>>> the failure from Manage_subdevs() can be safely ignored, because,
>>>> 1) udisks doesn't exist, no need to check the return value to umount
>>>> the file system by udisks and remove the component disk again.
>>>> 2) After the 'I' inremental remove, there is another 'r' hot remove
>>>> following up. The first incremental remove is a best-try effort.
>>> Hi Coly,
>>>
>>> I'm not sure what you meant here. I know that on "remove" event udev will
>>> call mdadm -If <devname>. And that is all I'm familiar with. I don't see
>>> another branch executed in code to handle "remove" event, no second attempt
>>> for clean up is made. Could you clarify? How is it executed?
>>> Perhaps, I understand it incorrectly as second action that is always
>>> executed automatically. I know that there is an action "--remove" which can
>>> be manually triggered. Is that what you meant?
>>>
>>
>> What I mentioned was only related to the source code.
>>
>> Before force_remove() is removed, it was called on 2 locations, one was from
>> remove_from_member_array(), another was from IncrementalRemove().
>>
>> But remove_from_member_array() was called from IncrementalRemove() too. The
>> code flow is,
>>
>> if (container) {
>> call remove_from_member_array()
>> } else {
>> call Manage_subdevs() with 'I' operation.
>> if (return 2)
>> call force_remove()
>> }
>>
>> call Manage_subdevs() with 'r' operation
>>
>> From the above bogus code, the first call to Manage_subdevs() was an
>> Incremental remove, and the second call to Manage_subdevs() was a hot remove.
>> No matter it succeed or failed on the first call, the second call for hot
>> remove will always happen.
>>
>> Therefore, after removing force_remove(), it is unnecessary to check the
>> return value of the first call to IncrementalRemove(). Because always the
>> second call to Manage_subdevs() with 'r' operation will follow up.
>>
>> This was what I meant, it was only related to the code I modified.
>>
>
> Ok, now I get this. Thanks! It make sense now. And I know who made it this way:
> https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/commit/?id=cb8f5371352f6c16af5aab8a40861e13aa50fc2b
>
> This second independent Manage_subdevs call is needed for external metadata
> because at the end we need to remove the device from the container. It seems
> that I made a mistake here and doubled call for native (nobody have been
> noticed for years LOL). The goto end; should be independent from if (rv & 2).
>
> Feel free to clear this up. I think that else branch is not needed now.
> In incremental remove we should stay with 'I' disposition because in this mode
> kernel should not be blocked from setting broken state as it is with 'f'
> disposition.
> https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/commit/?id=461fae7e7809670d286cc19aac5bfa861c29f93a
OK, I will post another patch for the cleanup later. Now I submit this patch to Jes firstly.
>>
>> e > Therefore in this patch, where foorce_remove() is removed, the return
>>>> value of calling Manage_subdevs() is not checked too.
>>>>
>>>> Signed-off-by: Coly Li <colyli@xxxxxxx>
>>>> Cc: Mariusz Tkaczyk <mariusz.tkaczyk@xxxxxxxxx>
>>>> Cc: Jes Sorensen <jes@xxxxxxxxxxxxxxxxxx>
>>>> ---
>>>> Changelog,
>>>> v3: remove the almost-useless warning message, and make the change
>>>> more simplified.
>>>> v2: improve based on code review comments from Mariusz.
>>>> v1: initial version.
>>>
>>> For the code:
>>> Reviewed-by: Mariusz Tkaczyk <mariusz.tkaczyk@xxxxxxxxxxxxxxx>
>>
>> Thanks.
>>
>> BTW, do you have any suggestion for the commit log? It seems current commit
>> message is not that clear, and I want to listen to your input :-)
>
> It is fine, I read that at the morning so it seems that my brain was not
> working yet. This is another example why I should not write mail before coffee
> :)
Thanks.
Coly Li
