[PATCH fstools v2] partname: Ignore root=PARTUUID...

Brian Norris computersforpeace at gmail.com
Mon Jan 9 16:24:22 PST 2023 

On Mon, Jan 9, 2023 at 3:20 PM Christian Marangi <ansuelsmth at gmail.com> wrote:
> On Mon, Jan 09, 2023 at 02:34:48PM -0800, Brian Norris wrote:
> > On Mon, Jan 9, 2023 at 1:53 PM Christian Marangi <ansuelsmth at gmail.com> wrote:
> > >
> > > On Fri, Jan 06, 2023 at 06:04:22PM -0800, Brian Norris wrote:
> > > > We're assuming all root= arguments are /dev/ paths, but many targets
> > > > utilize root=PARTUUID=<xxx> strategies. At least allow them to fall back
> > > > to scanning all block devices.
> > > >
> > > > Signed-off-by: Brian Norris <computersforpeace at gmail.com>
> > >
> > > Can you elaborate this a bit more?
> >
> > This function currently assumes that if it can find a "root=" line,
> > then it can parse it and use it to find "rootfs_data" on the same
> > disk. But the rootdevname() function really chokes (does completely
> > the wrong thing) when given "PARTUUID=<blah>". So this parser becomes
> > useless.
> >
> > > This is dependent of the google based
> > > devices with ipq806x but why?
> >
> > I guess it's not *strictly* a dependency, but as-is, things aren't great.
> >
> > With the above choking, I believe fstools will fall back to
> > rootdisk.c, which will place rootfs_data in a different place --
> > appended to the squashfs filesystem, via a custom loopback device.
> > This works OK I suppose, but has its downsides.
> >
> > But the real kicker is that if fstools / partname.c eventually learns
> > how to find the right 'rootfs_data' partition, then suddenly our data
> > filesystem will move, and that's not great for sysupgrade. So I'd like
> > to get it right from the start, rather than change between rootdisk.c
> > and partname.c approaches arbitrarily.
> >
> > > In theory we should have other device with sd card/eMMC that use uuid to
> > > select the partition...
> >
> > Right. Search the tree for the "root=PARTUUID=" string, and you'll
> > find several. (Some breadcrumbs in rockchip, x86, imx, and more.)
> > Presumably they would run into the same problem if they tried to use a
> > dedicated data partition on a block device -- right now, I believe
> > Rockchip restricts itself to a 2-partition layout, for instance. Which
> > is why I didn't specifically call this a ipq806x / Google-specific
> > thing.
> >
>
> This is what gets me most confused... The patch is correct and looks
> good... Just what I can't understand is how things worked till today...
>
> They all fallback to rootdisk.c? It's worth to check and have some proof
> of this theory.
>
> Since we are playing with the function mounting root the main concern
> here is that we brake any device using PARTUUID that somehow are working
> now so we need to be carefull in what we change as the risk of causing
> regression is behind the corner.

Totally understood.

> So we should just find a way to understand why thing are working (or are
> not working and are using an alternative way currently) Just that and
> this can be merged.

I don't have any of these targets. (I suppose I could try to figure
out how, if at all, the x86/generic target is handling this. But it
seems like a very flexible target that can be used in many ways, and
I'm not sure I'd find the Right(TM) ones to test.)

But looking at sources, I see imx (Build/imx-combined-image) and
rockchip (Build/pine64-img) are doing 2-partition layouts, with
$(SCRIPT_DIR)/gen_image_generic.sh. So that skips "partname" and just
does "rootdisk".

On the other hand, Device/glinet_gl-b2200 is one of the few I could
find with a 3-partition (with rootfs_data partition) layout
(qsdk-ipq-app-gpt), and it has an explicit "root=/dev/mmcblk0p2" in
its bootargs. (A related key point: it's the only one using
`CI_DATAPART` for emmc.sh sysupgrade.)

Most (all?) remaining rootfs_data references I see are related to
MTD/UBI, and shouldn't really be relevant.

So I don't have a full proof of it, but it appears that all relevant
users are either 2-partition layouts that use rootdisk.c, or else
using partname.c with a rootfs_data partition but only using /dev
paths for root=. Exceptions would be if someone was manually modifying
their partition layout with a spare rootfs_data partition, in which
case it's possible this could pick it up now. I'm not sure we can
guard against all local customizations though.

I can try to look or play around some more, if you have hints on what
I should investigate. Or wait around for someone (Daniel?) who has
more background in this area?

> > > Also can't we just ignore the device bootargs
> > > and provide a custom one?
> >
> > This isn't about the device (as in, baked into a bootloader) bootargs;
> > see "root=PARTUUID=%U/PARTNROFF=1" in my patch 7:
> > https://patchwork.ozlabs.org/project/openwrt/patch/20230107074945.2140362-7-computersforpeace@gmail.com/
> > This is a better way of specifying root devices (say, rather than
> > memorizing a "/dev/mmcblk0" or similar, which is *not* a stable
> > contract; it also means boot-from-USB won't work), except that fstools
> > doesn't like it. (And again, there are several other existing
> > openwrt.git users for it.)
> >
>
> I understand and I want this fixed. Sorry if I look confused but you can
> understand how this was broken from ages and still we have many target
> using the root=PARTUUID format makes me question a lot of thing ahahhaha

Understood, I question things all the time, especially when they
*seem* to be especially broken :)


Brian

> > > > ---
> > > >
> > > > Changes in v2:
> > > >  * fstools, not fsutils (sorry for the noise)
> > > >
> > > >  libfstools/partname.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/libfstools/partname.c b/libfstools/partname.c
> > > > index f59c52eb8f3c..9c27015643ad 100644
> > > > --- a/libfstools/partname.c
> > > > +++ b/libfstools/partname.c
> > > > @@ -128,12 +128,12 @@ static struct volume *partname_volume_find(char *name)
> > > >                       return NULL;
> > > >       }
> > > >
> > > > -     if (get_var_from_file("/proc/cmdline", "root", rootparam, sizeof(rootparam))) {
> > > > +     if (get_var_from_file("/proc/cmdline", "root", rootparam, sizeof(rootparam)) && rootparam[0] == '/') {
> > > >               rootdev = rootdevname(rootparam);
> > > >               /* find partition on same device as rootfs */
> > > >               snprintf(ueventgstr, sizeof(ueventgstr), "%s/%s/*/uevent", block_dir_name, rootdev);
> > > >       } else {
> > > > -             /* no 'root=' kernel cmdline parameter, find on any block device */
> > > > +             /* no useful 'root=' kernel cmdline parameter, find on any block device */
> > > >               snprintf(ueventgstr, sizeof(ueventgstr), "%s/*/uevent", block_dir_name);
> > > >       }
> > > >
> > > > --
> > > > 2.39.0
> > > >
> > > >
> > > > _______________________________________________
> > > > openwrt-devel mailing list
> > > > openwrt-devel at lists.openwrt.org
> > > > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
> > >
> > > --
> > >         Ansuel
>
> --
>         Ansuel

More information about the openwrt-devel mailing list