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
>
> 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,
Mariusz
