On Thu, 02 Mar 2023, Mariusz Tkaczyk wrote:
> Hi Neil,
> We found typo. We fixed that to test the change.
> Other comments are less important.
>
> On Mon, 27 Feb 2023 11:13:07 +1100
> NeilBrown <neilb@xxxxxxx> wrote:
>
> > We need a new mdmon@mdfoo instance to run in the root filesystem after
> > switch root, as /sys and /dev are removed from the initrd.
> >
> > systemd will not start a new unit with the same name running while the
> > old unit is still active, and we want the two mdmon processes to overlap
> > in time to avoid any risk of deadlock which a write is attempted with no
> > mdmon running.
> >
> > So we need a different unit name in the initrd than in the root. Apart
> > from the name, everything else should be the same.
> >
> > This is easily achieved using a different instance name as the
> > mdmon@.service unit file already supports multiple instances (for
> > different arrays).
> >
> > So start "mdmon@mdfoo.service" from root, but
> > "mdmon@initrd-mdfoo.service" from the initrd. udev can tell which
> > circumstance is the case by looking for /etc/initrd-release.
> > continue_from_systemd() is enhanced so that the "initrd-" prefix can be
> > requested.
> >
> > Teach mdmon that a container name like "initrd/foo" should be treated
> > just like "foo". Note that systemd passes the instance name
> > "initrd-foo" as "initrd/foo".
> >
> > We don't need a similar machanism at shutdown because dracut runs
> > "mdmon --takeover --all" when appropriate.
> >
> > Signed-off-by: NeilBrown <neilb@xxxxxxx>
>
> > diff --git a/mdmon.c b/mdmon.c
> > index 6d37b17c3f53..25abdd71fb1e 100644
> > --- a/mdmon.c
> > +++ b/mdmon.c
> > @@ -368,7 +368,11 @@ int main(int argc, char *argv[])
> > }
> >
> > if (!all && argv[optind]) {
> > - container_name = get_md_name(argv[optind]);
> > + static const char prefix[] = "initrd/";
> > + container_name = argv[optind];
> > + if (strncmp(container_name, prefix, sizeof(prefix)-1) == 0)
> > + container_name += sizeof(prefix)-1;
> > + container_name = get_md_name(container_name);
>
> "sizeof(prefix)-1" there should be spaces before and after operator.
>
> You are defining similar literals in 2 places:
> prefix[] = "initrd/"
> *prefix = in_initrd() ? "initrd-", "";
>
> When I see something like this, I need to ask why it is not globally defined
> because in the future we would need to define it for the firth and fourth time.
> I see the difference in last sign ('/' and '-'). We can omit that.
> I would like propose something like:
>
> in mdadm.h:
> #DEFINE MDMON_PREFIX "initrd"
To my mind this is "premature optimisation". I think it makes the core
harder to read. If it makes the code easier to maintain then it might
be worth the cost. I don't think it does.
>
> in mdmon, do not check last sign. whatever it is, we don't really care, just
> skip it. All we need to know is that it not belongs to container name.
> Hope it works correctly:
> if (strncmp(container_name, MDMON_PREFIX, sizeof(prefix) - 1) == 0)
> container_name += sizeof(MDMON_PREFIX);
>
> And later in start_mdmon include '-' in snprintf:
> "%s@%s%s.service", service_name, MDMON_PREFIX"-" ?: "",
>
> I think that we don't need to pass whole char* value, we can use bool, the one
> possibility is "initrd" now. If that would be changed, we can use enum and maps
> interface:
> https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/tree/maps.c
Passing bools makes the calling code hard to read. "What does that
"true" or "false" mean??". Sometimes it really is best, but I think
passing the string "initrd" makes more sense to the reader than passing "true".
Passing enums is better than simple bools - and has the benefit of
spelling out the meaning of NULL. These still feel a bit clumsy to me
so I would only use them when there is a clear benefit.
>
> This is lesson learned by code study, we needed to put big effort to correct
> similar case with reshapes because pointers become overkill through
> years:
> https://lore.kernel.org/linux-raid/20230102083524.28893-1-mateusz.kusiak@xxxxxxxxx/
>
> It my my personal view so you are free to make decision. I will accept it but
> please note that mdadm is full of same literals (just find /dev/md or /dev/md/)
> so that is why I'm especially sensitive in that cases.
>
> > --git a/util.c b/util.c index 6b44662db7cd..1d433d1826b5 100644 --- a/util.c
> > +++ b/util.c @@ -1906,6 +1906,7 @@ int start_mdmon(char *devnm)
> > int len;
> > pid_t pid;
> > int status;
> > + char *prefix = in_initrd() ? "initrd-", "";
>
> The most important thing:
> typo, should be in_initrd() ? "initrd-": "";
Certainly - thanks for catching that!
Jes - should I resent the whole series, or just this patch, or will you
edit before applying?
Thanks,
NeilBrown
>
> > char pathbuf[1024];
> > char *paths[4] = {
> > pathbuf,
> > @@ -1916,7 +1917,7 @@ int start_mdmon(char *devnm)
> >
> > if (check_env("MDADM_NO_MDMON"))
> > return 0;
> > - if (continue_via_systemd(devnm, MDMON_SERVICE))
> > + if (continue_via_systemd(devnm, MDMON_SERVICE, prefix))
> > return 0;
> >
> > /* That failed, try running mdmon directly */
> > @@ -2187,7 +2188,7 @@ void manage_fork_fds(int close_all)
> > * 1- if systemd service has been started
> > * 0- otherwise
> > */
> > -int continue_via_systemd(char *devnm, char *service_name)
> > +int continue_via_systemd(char *devnm, char *service_name, char *prefix)
> > {
> > int pid, status;
> > char pathbuf[1024];
> > @@ -2199,7 +2200,7 @@ int continue_via_systemd(char *devnm, char
> > *service_name) case 0:
> > manage_fork_fds(1);
> > snprintf(pathbuf, sizeof(pathbuf),
> > - "%s@%s.service", service_name, devnm);
> > + "%s@%s%s.service", service_name, prefix ?: "",
> > devnm); status = execl("/usr/bin/systemctl", "systemctl", "restart",
> > pathbuf, NULL);
> > status = execl("/bin/systemctl", "systemctl", "restart",
> >
> >
>
> Thanks,
> Mariusz
>
