Skip to content
Snippets Groups Projects

fix: btrfs mountpoint check in makechrootpkg

Closed bboozzoo requested to merge bboozzoo/devtools:bboozzoo/fix-btrfs into master
2 unresolved threads

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

Edited by bboozzoo

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Please fix the butrfs typo in your commit message :wink:

  • Haha, missed that :smile: Let me force push

  • bboozzoo added 1 commit

    added 1 commit

    • 7dbf1e5c - fix: btrfs mountpoint check in makechrootpkg

    Compare with previous version

  • bboozzoo changed the description

    changed the description

  • 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 :thinking:

    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 :laughing:

    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 when mountpoint exits with non-0 status, which happens only when the path in question is not a mountpoint. So what you really want is is_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ý
    • Please register or sign in to reply
    • 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 :thinking:

    • 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 :thinking:

      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 :blush:

      Edited by Christian Heusel
    • Please register or sign in to reply
  • This whole thing also got me wondering: How did you create the chroot? :thinking: mkrarchroot should setup the subvolume for you if executed on a btrfs path :blush:

  • This is an old install I have been moving around since roughly 2007. The chroot was probably created some years ago and I simply kept updating it and rebuilding packages I work on.

  • mentioned in commit 5d2e31ae

  • Christian Heusel mentioned in merge request !271 (merged)

    mentioned in merge request !271 (merged)

  • mentioned in commit c9d82144

  • Superseded by !271 (merged)

  • closed

Please register or sign in to reply
Loading