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.
e > Therefore in this patch, where force_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 :-)
--
Coly Li
