| Message ID | 20230529160754.26849-1-colyli@suse.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v3] Incremental: remove obsoleted calls to udisks | expand |
On Tue, 30 May 2023 00:07:54 +0800 Coly Li <colyli@suse.de> 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? > 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> > Cc: Mariusz Tkaczyk <mariusz.tkaczyk@intel.com> > Cc: Jes Sorensen <jes@trained-monkey.org> > --- > 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@linux.intel.com> Mariusz
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@suse.de> 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@suse.de> > > Cc: Mariusz Tkaczyk <mariusz.tkaczyk@intel.com> > > Cc: Jes Sorensen <jes@trained-monkey.org> > > --- > > 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@linux.intel.com> 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 :-)
On Tue, 30 May 2023 18:59:46 +0800 Coly Li <colyli@suse.de> 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@suse.de> 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@suse.de> > > > Cc: Mariusz Tkaczyk <mariusz.tkaczyk@intel.com> > > > Cc: Jes Sorensen <jes@trained-monkey.org> > > > --- > > > 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@linux.intel.com> > > 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
> 2023年5月30日 21:05，Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com> 写道： > > On Tue, 30 May 2023 18:59:46 +0800 > Coly Li <colyli@suse.de> 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@suse.de> 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@suse.de> >>>> Cc: Mariusz Tkaczyk <mariusz.tkaczyk@intel.com> >>>> Cc: Jes Sorensen <jes@trained-monkey.org> >>>> --- >>>> 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@linux.intel.com> >> >> 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
diff --git a/Incremental.c b/Incremental.c index f13ce02..05b33c4 100644 --- a/Incremental.c +++ b/Incremental.c @@ -1628,54 +1628,18 @@ release: return rv; } -static void run_udisks(char *arg1, char *arg2) -{ - int pid = fork(); - int status; - if (pid == 0) { - manage_fork_fds(1); - execl("/usr/bin/udisks", "udisks", arg1, arg2, NULL); - execl("/bin/udisks", "udisks", arg1, arg2, NULL); - exit(1); - } - while (pid > 0 && wait(&status) != pid) - ; -} - -static int force_remove(char *devnm, int fd, struct mdinfo *mdi, int verbose) -{ - int rv; - int devid = devnm2devid(devnm); - - run_udisks("--unmount", map_dev(major(devid), minor(devid), 0)); - rv = Manage_stop(devnm, fd, verbose, 1); - if (rv) { - /* At least we can try to trigger a 'remove' */ - sysfs_uevent(mdi, "remove"); - if (verbose) - pr_err("Fail to stop %s too.\n", devnm); - } - return rv; -} - static void remove_from_member_array(struct mdstat_ent *memb, struct mddev_dev *devlist, int verbose) { - int rv; - struct mdinfo mmdi; int subfd = open_dev(memb->devnm); if (subfd >= 0) { - rv = Manage_subdevs(memb->devnm, subfd, devlist, verbose, - 0, UOPT_UNDEFINED, 0); - if (rv & 2) { - if (sysfs_init(&mmdi, -1, memb->devnm)) - pr_err("unable to initialize sysfs for: %s\n", - memb->devnm); - else - force_remove(memb->devnm, subfd, &mmdi, - verbose); - } + /* + * Ignore the return value because it's necessary + * to handle failure condition here. + */ + Manage_subdevs(memb->devnm, subfd, devlist, verbose, + 0, UOPT_UNDEFINED, 0); close(subfd); } } @@ -1758,21 +1722,19 @@ int IncrementalRemove(char *devname, char *id_path, int verbose) } free_mdstat(mdstat); } else { - rv |= Manage_subdevs(ent->devnm, mdfd, &devlist, - verbose, 0, UOPT_UNDEFINED, 0); - if (rv & 2) { - /* Failed due to EBUSY, try to stop the array. - * Give udisks a chance to unmount it first. + /* + * This 'I' incremental remove is a try-best effort, + * the failure condition can be safely ignored + * because of the following up 'r' remove. */ - rv = force_remove(ent->devnm, mdfd, &mdi, verbose); - goto end; - } + Manage_subdevs(ent->devnm, mdfd, &devlist, + verbose, 0, UOPT_UNDEFINED, 0); } devlist.disposition = 'r'; rv = Manage_subdevs(ent->devnm, mdfd, &devlist, verbose, 0, UOPT_UNDEFINED, 0); -end: + close(mdfd); free_mdstat(ent); return rv;
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. 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> Cc: Mariusz Tkaczyk <mariusz.tkaczyk@intel.com> Cc: Jes Sorensen <jes@trained-monkey.org> --- 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. Incremental.c | 64 +++++++++++---------------------------------------- 1 file changed, 13 insertions(+), 51 deletions(-)