Re: [PATCH v5] Incremental: remove obsoleted calls to udisks

[Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>]

On Fri, 1 Sep 2023 11:47:09 -0400
Jes Sorensen <jes@trained-monkey.org> wrote:

> On 8/13/23 12:46, Coly Li wrote:
> > Utility 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.
> > 
> > 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@suse.de>
> > Reviewed-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
> > Cc: Jes Sorensen <jes@trained-monkey.org>
> > ---
> > Changelog,
> > v5: change Mariusz's email address as he suggested
> > v4: add Reviewed-by from Mariusz.
> > 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.
> > 
> >  Incremental.c | 64 +++++++++++----------------------------------------
> >  1 file changed, 13 insertions(+), 51 deletions(-)  
> 
> Been out of the loop for a while, trying to catch up.
> 
> Mariusz, do you consider this one good to go now? You were the one
> providing feedback multiple times.
> 
> Thanks,
> Jes
> 
> 

Hi Jes,

Yes, I see this as a good change. The current behavior is not stable, because
udev is not able to "umount"- if array is not mounted it is stopped, otherwise
not.

With the change, we will not try to stop it at all- fair for me, behavior is
same every time. If we cannot stop array every time we should not try to.

Thanks,
Mariusz