| Message ID | 20230107020424.1703752-1-computersforpeace@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [fstools,v2] partname: Ignore root=PARTUUID... | expand |
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@gmail.com> Can you elaborate this a bit more? This is dependent of the google based devices with ipq806x but why? In theory we should have other device with sd card/eMMC that use uuid to select the partition... Also can't we just ignore the device bootargs and provide a custom one? > --- > > 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@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
On Mon, Jan 9, 2023 at 1:53 PM Christian Marangi <ansuelsmth@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@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. > 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.) 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@lists.openwrt.org > > https://lists.openwrt.org/mailman/listinfo/openwrt-devel > > -- > Ansuel
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@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@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. 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. > > 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 > > > > --- > > > > > > 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@lists.openwrt.org > > > https://lists.openwrt.org/mailman/listinfo/openwrt-devel > > > > -- > > Ansuel
On Mon, Jan 9, 2023 at 3:20 PM Christian Marangi <ansuelsmth@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@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@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@lists.openwrt.org > > > > https://lists.openwrt.org/mailman/listinfo/openwrt-devel > > > > > > -- > > > Ansuel > > -- > Ansuel
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); }
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@gmail.com> --- Changes in v2: * fstools, not fsutils (sorry for the noise) libfstools/partname.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)