| Message ID | 20251019153922.27208-2-dani.barra25@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fstab xattrs: This update ensures that fstab xattrs are correctly updated without adding a lock to the database. | expand |
On Sun, 19 Oct 2025 at 23:45, dani.barra25 via lists.openembedded.org <dani.barra25=gmail.com@lists.openembedded.org> wrote: > + pseudo_prefix = cls.__get_pseudo(native_sysroot, new_rootfs, new_pseudo) > + > + # Shutdown any existing pseudo server > + # This ensures no other processes are using the database > + shutdown_cmd = "%s -S" % pseudo_prefix > + exec_native_cmd(shutdown_cmd, native_sysroot) > + > + # Database is not locked anymore > + # Repairs inodes related to fstab. This way xattrs and metadata is correctly applied to the new file > + repair_cmd = "%s -B" % pseudo_prefix > + exec_native_cmd(repair_cmd, native_sysroot) > + Is it possible to describe how to reproduce and observe the issue that this patch aims to fix? This snippet makes me *very* nervous as pseudo is used by mulitple bitbake tasks in parallel, and doing shutdowns and repairs in the middle of it can blow up spectacularly. Alex
Hello Alex, Yes I can. Maybe I am doing something wrong since the patch is an alternative to past threads but somehow it gets separated without context. Starting from the beginning. My original patch aimed to fix a simple thing. No matter what fstype you were using, no xattrs from the build process were assigned to /etc/fstab if you had multiple partitions. And this was particularly problematic when you have read-only rootfs like squashfs and you need to have all the metadata available during build, like the SELinux file labels, since they cannot be set during boot time. This is because it creates a temp folder and then creates a new fstab that is later replaced in the image. The replacement is made via 2 options: 1. If it was ext* or msdos, it will fall under specific methods for those types that will open the already built image and remove the fstab and writing the new one, losing all the metadata of that file. 2. It falls under the "install" cmd of rootfs.py and this one was being executed inside the pseudo environment. This means the original entry of fstab on files db is completely replace with this new file and all the xattrs are gone To avoid this, my first patch removed the debugfs specifics of ext* and msdos, since those did not leveragy any "write only content" mechanism without replacing the actual file. Then, all the fstab modifications would lie under rootfs.py which seemed to be the common point for everything. On this I simply replaced "install" with "cp" since I only wanted to modify the contents. https://lists.openembedded.org/g/openembedded-core/topic/115043355 Later in the discussion, Mathieu correctly pointed out that it could not be used because it fails a test due to the usage of copyhardlinktree() for this temporary rootfs folder. This meant that I was not only modifying the temporary fstab, but also the original one, leading to a crash of wic.Wic.test_no_fstab_update. https://lists.openembedded.org/g/openembedded-core/topic/115043356 The second approach I took was looking at how pseudo behaved. This time, I decided to maintain the original install cmd, but instead of running it under fakeroot, I ran it on the host system. This meant that, if a later check of the db occurred, it would correctly detect an inode change on fstab and therefore it could fix it using the -B option. And that is what I did : https://lists.openembedded.org/g/openembedded-core/topic/115805391 I only neglected the fact that, in reality, other tests could have failed because of this. And, in fact, it happened. Because of this, tests regarding including and excluding paths were failing because the db was locked. That's when I reached the current patch alternative. I knew it could cause problems but honestly I never considered it would and when I run wic tests, I didn't see any failure. In reality, I was trying to replicate what was being done a few lines above with the options -B -m and -M. The difference is that pseudo doesn't have a flag to repair a single file, therefore this was the only solution found. A good alternative to this would be pseudo actually having a functionality to repair single file inodes, avoiding to completely check the whole database and therefore better management on this side. In the end this bug rendered all my images that enforced SELinux with read only rootfs (squashfs) useless and I'd assume I might not be the only one with this issue. Another thing I notice is that, for repeatability purposes, there are multiple variables being used to timestamp files. This is just a recommendation/question. Shouldn't all those variable timestamps be aligned also? In any case, if you have some other idea on how to approach this, let me know! Daniel Alexander Kanavin <alex.kanavin@gmail.com> escreveu (segunda, 20/10/2025 à(s) 13:09): > On Sun, 19 Oct 2025 at 23:45, dani.barra25 via lists.openembedded.org > <dani.barra25=gmail.com@lists.openembedded.org> wrote: > > > + pseudo_prefix = cls.__get_pseudo(native_sysroot, > new_rootfs, new_pseudo) > > + > > + # Shutdown any existing pseudo server > > + # This ensures no other processes are using the > database > > + shutdown_cmd = "%s -S" % pseudo_prefix > > + exec_native_cmd(shutdown_cmd, native_sysroot) > > + > > + # Database is not locked anymore > > + # Repairs inodes related to fstab. This way xattrs > and metadata is correctly applied to the new file > > + repair_cmd = "%s -B" % pseudo_prefix > > + exec_native_cmd(repair_cmd, native_sysroot) > > + > > Is it possible to describe how to reproduce and observe the issue that > this patch aims to fix? This snippet makes me *very* nervous as pseudo > is used by mulitple bitbake tasks in parallel, and doing shutdowns and > repairs in the middle of it can blow up spectacularly. > > Alex >
On 20 Oct 2025, at 13:09, Alexander Kanavin via lists.openembedded.org <alex.kanavin=gmail.com@lists.openembedded.org> wrote: > > Is it possible to describe how to reproduce and observe the issue that > this patch aims to fix? This snippet makes me *very* nervous as pseudo > is used by mulitple bitbake tasks in parallel, and doing shutdowns and > repairs in the middle of it can blow up spectacularly. Agreed. If you’re building a single image then yes, the wic tasks will typically happen at the end with nothing else in parallel. But this is not a guarantee and killing pseudo whilst other recipes might be mid-build is not acceptable. Ross
diff --git a/scripts/lib/wic/partition.py b/scripts/lib/wic/partition.py index bf2c34d594..82d754835c 100644 --- a/scripts/lib/wic/partition.py +++ b/scripts/lib/wic/partition.py @@ -131,7 +131,7 @@ class Partition(): partition command parameters. """ self.updated_fstab_path = updated_fstab_path - if self.updated_fstab_path and not (self.fstype.startswith("ext") or self.fstype == "msdos"): + if self.updated_fstab_path: self.update_fstab_in_rootfs = True if not self.source: @@ -295,15 +295,6 @@ class Partition(): (self.fstype, extraopts, rootfs, label_str, self.fsuuid, rootfs_dir) exec_native_cmd(mkfs_cmd, native_sysroot, pseudo=pseudo) - if self.updated_fstab_path and self.has_fstab and not self.no_fstab_update: - debugfs_script_path = os.path.join(cr_workdir, "debugfs_script") - with open(debugfs_script_path, "w") as f: - f.write("cd etc\n") - f.write("rm fstab\n") - f.write("write %s fstab\n" % (self.updated_fstab_path)) - debugfs_cmd = "debugfs -w -f %s %s" % (debugfs_script_path, rootfs) - exec_native_cmd(debugfs_cmd, native_sysroot) - mkfs_cmd = "fsck.%s -pvfD %s" % (self.fstype, rootfs) exec_native_cmd(mkfs_cmd, native_sysroot, pseudo=pseudo) @@ -400,10 +391,6 @@ class Partition(): mcopy_cmd = "mcopy -i %s -s %s/* ::/" % (rootfs, rootfs_dir) exec_native_cmd(mcopy_cmd, native_sysroot) - if self.updated_fstab_path and self.has_fstab and not self.no_fstab_update: - mcopy_cmd = "mcopy -m -i %s %s ::/etc/fstab" % (rootfs, self.updated_fstab_path) - exec_native_cmd(mcopy_cmd, native_sysroot) - chmod_cmd = "chmod 644 %s" % rootfs exec_cmd(chmod_cmd) diff --git a/scripts/lib/wic/plugins/source/rootfs.py b/scripts/lib/wic/plugins/source/rootfs.py index e29f3a4c2f..09446baef2 100644 --- a/scripts/lib/wic/plugins/source/rootfs.py +++ b/scripts/lib/wic/plugins/source/rootfs.py @@ -225,12 +225,21 @@ class RootfsPlugin(SourcePlugin): fstab_path = os.path.join(new_rootfs, "etc/fstab") # Assume that fstab should always be owned by root with fixed permissions install_cmd = "install -m 0644 -p %s %s" % (part.updated_fstab_path, fstab_path) + exec_native_cmd(install_cmd, native_sysroot) + if new_pseudo: - pseudo = cls.__get_pseudo(native_sysroot, new_rootfs, new_pseudo) - else: - pseudo = None - exec_native_cmd(install_cmd, native_sysroot, pseudo) - + pseudo_prefix = cls.__get_pseudo(native_sysroot, new_rootfs, new_pseudo) + + # Shutdown any existing pseudo server + # This ensures no other processes are using the database + shutdown_cmd = "%s -S" % pseudo_prefix + exec_native_cmd(shutdown_cmd, native_sysroot) + + # Database is not locked anymore + # Repairs inodes related to fstab. This way xattrs and metadata is correctly applied to the new file + repair_cmd = "%s -B" % pseudo_prefix + exec_native_cmd(repair_cmd, native_sysroot) + part.prepare_rootfs(cr_workdir, oe_builddir, new_rootfs or part.rootfs_dir, native_sysroot, pseudo_dir = new_pseudo or pseudo_dir)