fix: btrfs mountpoint check in makechrootpkg
Fix the branch condition so that btrfs subvolume manipulation happens when the path is a mountpoint.
Signed-off-by: Maciek Borzecki maciek.borzecki@gmail.com
Merge request reports
Activity
added 1 commit
- 7dbf1e5c - fix: btrfs mountpoint check in makechrootpkg
I'm not quite sure I understand whats wrong with the current code. From my perspective everything seems to be working and we do not go into the non-btrfs branch on btrfs file-systems.
The subvolume copydir (idk how you actually call that) is not a mount, so the check seems correct:
$ mount | grep btrfs /dev/mapper/archlinux on / type btrfs (rw,relatime,ssd,space_cache=v2,subvolid=5,subvol=/)
I'm not sure I follow, the branch should only be taken if btrfs is used on chrootdir and copydir is a mount point (hopefully of a subvolume but that's probably implied). The code as it is without the fix tries to manipulate subvolumes on something that isn't a subvolume mount, and in fact does not work for me.
I think I'll need more information about your setup or how to replicate the faulty behaviour
I posted the output of mount above in order to show that there is no mountpoint in the devtools directory, but here in more detail maybe:
$ sudo btrfs subvolume list / | grep "core" ID 311 gen 9919 top level 5 path var/lib/archbuild/core-testing-x86_64/root ID 312 gen 9925 top level 5 path var/lib/archbuild/core-testing-x86_64/chris-6 ID 313 gen 9920 top level 312 path var/lib/archbuild/core-testing-x86_64/chris-6/var/lib/portables ID 314 gen 9920 top level 312 path var/lib/archbuild/core-testing-x86_64/chris-6/var/lib/machines
While it is a subvolume, it is not a mountpoint:
$ sudo mountpoint /var/lib/archbuild/core-testing-x86_64/chris-6 /var/lib/archbuild/core-testing-x86_64/chris-6 is not a mountpoint $ sudo mount | grep core <empty result>
df
disagrees here, but there is a recent thread about this on the btrfs mailing list:$ sudo df -T /var/lib/archbuild/extra-x86_64/chris-3 Filesystem Type 1K-blocks Used Available Use% Mounted on - - 499577856 383669340 114229252 78% /var/lib/archbuild/extra-x86_64/chris-3
Then again I'm using btrfs for just a few days on this machine now, so it could just be that I don't understand it properly yet
Edited by Christian Heusel$ sudo btrfs subvolume list / ID 256 gen 139600 top level 5 path @ ID 259 gen 137507 top level 256 path root ID 260 gen 139600 top level 256 path home ID 261 gen 139341 top level 256 path opt ID 262 gen 139600 top level 256 path var ID 264 gen 11 top level 256 path swap ID 265 gen 139341 top level 256 path usr/local
my build chroot path is at
$HOME/chroot
, so the code will sync$HOME/chroot/root
to$HOME/chroot/maciek
, hence:maciek@galeon:~ mountpoint $HOME/chroot/maciek /home/maciek/chroot/maciek is not a mountpoint maciek@galeon:~ echo $? 32
but the code as it is written now, which is:
if is_btrfs "$chrootdir" && ! mountpoint -q "$copydir"; then subvolume_delete_recursive "$copydir" || die "Unable to delete subvolume %s" "$copydir" btrfs subvolume snapshot "$chrootdir/root" "$copydir" >/dev/null || die "Unable to create subvolume %s" "$copydir" else
is wrong. The
! mounpoint ...
condition is incorrect, the branch is taken whenmountpoint
exits with non-0 status, which happens only when the path in question is not a mountpoint. So what you really want isis_btrfs "$chrootdir" && mountpoint -q "$copydir"
, i.e.$chrootdir
is on btrfs, and$copydir
is a mountpoint, otherwise the whole manipulation of subvolumes makes no sense. FWIW, I'm guessing your setup is actually incorrectly hitting the else branch which does rsync.FWIW, I'm guessing your setup is actually incorrectly hitting the else branch which does rsync.
This does not seem to be the case, see the following:
--- /home/chris/Documents/shared_projects/arch-devtools/src/makechrootpkg.in 2024-06-21 00:20:36.151695108 +0200 +++ /usr/bin/makechrootpkg 2024-06-26 12:08:35.049083513 +0200 @@ -106,11 +106,13 @@ sync_chroot() { stat_busy "Synchronizing chroot copy [%s] -> [%s]" "$chrootdir/root" "$copy" if is_btrfs "$chrootdir" && ! mountpoint -q "$copydir"; then + echo "Using btrfs to sync" subvolume_delete_recursive "$copydir" || die "Unable to delete subvolume %s" "$copydir" btrfs subvolume snapshot "$chrootdir/root" "$copydir" >/dev/null || die "Unable to create subvolume %s" "$copydir" else + echo "Using rsync to sync" mkdir -p "$copydir" rsync -a --delete -q -W -x "$chrootdir/root/" "$copydir" fi
Which gives this output:
$ pkgctl build --rebuild nsxiv ==> Updating pacman database cache :: Synchronizing package databases... core downloading... extra downloading... multilib downloading... ==> Building nsxiv -> repo: extra -> arch: x86_64 -> worker: chris-0 ==> WARNING: ignoring --rebuild as pkgrel has already been incremented from 4 to 5 ==> Building nsxiv for [extra] (x86_64) :: Synchronizing package databases... core downloading... extra downloading... :: Starting full system upgrade... there is nothing to do ==> Building in chroot for [extra] (x86_64)... ==> Synchronizing chroot copy [/var/lib/archbuild/extra-x86_64/root] -> [chris-0]...Using btrfs to sync done
if is_btrfs "$chrootdir" && ! mountpoint -q "$copydir"; then
Assuming that
$copydir
is always below$chrootdir
, it makes sense: when$copydir
is not a mountpoint, it must be on the same device/filesystem as$chrootdir
, but when it is a mountpoint, it might be a different non-btrfs filesystem.Note that btrfs subvolumes don't have to be mounted. The code does not handle mounted subvolumes, but if only devtools is supposed to create subvolumes for
$copydir
, this should still be non-issue.Edited by Jakub Klinkovský
diff --git a/src/makechrootpkg.in b/src/makechrootpkg.in old mode 100644 new mode 100755 index 9df14e8..5b17bf9 --- a/src/makechrootpkg.in +++ b/src/makechrootpkg.in @@ -106,6 +106,7 @@ sync_chroot() { stat_busy "Synchronizing chroot copy [%s] -> [%s]" "$chrootdir/root" "$copy" if is_btrfs "$chrootdir" && ! mountpoint -q "$copydir"; then + echo "-- oops wrong branch" subvolume_delete_recursive "$copydir" || die "Unable to delete subvolume %s" "$copydir" btrfs subvolume snapshot "$chrootdir/root" "$copydir" >/dev/null ||
and so:
$ /usr/local/bin/makechrootpkg -r ~/chroot ==> Synchronizing chroot copy [/home/maciek/chroot/root] -> [maciek]...-- oops wrong branch ERROR: Not a Btrfs subvolume: Invalid argument ==> ERROR: Unable to create subvolume /home/maciek/chroot/maciek
But then the issue is not that the mountpoint check is wrong but rather that we would need an additional check if "$copydir/root" is actually a subvolume
yes, that would be preferable, but I don't know if there's a non-ugly way of doing so. FWIW,
btrfs subvolume show <path>
doesn't fail for subvolumes, but returns EINVAL for other paths, so maybe that's the solution?Subvolumes apparently have a fixed inode number of 256:
$ stat --format=%i /var/lib/archbuild/extra-x86_64/root 256
Taken from: https://stackoverflow.com/questions/25908149/how-to-test-if-location-is-a-btrfs-subvolume
Proper reference: https://btrfs.readthedocs.io/en/latest/Subvolumes.html#inode-numbers
Edit: I will create a Merge Request
Edited by Christian Heusel
mentioned in commit 5d2e31ae
mentioned in merge request !271 (merged)
mentioned in commit c9d82144
Superseded by !271 (merged)