btrfs: make btrfs subvolume listing consistent

[jelly]

Depending on the given mountpoint listing subvolumes returns different pathnames. The pathnames are returned sort of relative to the provided mountpoint. For example listing with / as mountpoint can return:

['root', 'home', 'home/foo', 'foo']

While giving /home it returns:

['root', 'home', 'root/foo', 'foo']

This gives issues for Cockpit as we want to give users a consistent listing of their subvolumes and UDisks always uses the first MountPoint to look up the subvolumes so Cockpit would either have to figure out how that is relative to the given MountPoint or show inconsistent subvolumes.

---

Some notes for a future situation with libbtrfsutil as I don't want to break the API again, independent of mountpoint this returns the same subvolumes:

In [12]: [path for (path,info) in btrfsutil.SubvolumeIterator('/', 5)]
Out[12]:
['root',
 'root/var/lib/portables',
 'root/var/lib/machines',
 'root/foo',
 'root/bar',
 'home',
 'home/foo']

In [13]: [path for (path,info) in btrfsutil.SubvolumeIterator('/home', 5)]
Out[13]:
['root',
 'root/var/lib/portables',
 'root/var/lib/machines',
 'root/foo',
 'root/bar',
 'home',
 'home/foo']

With this patch libblockdev does that too:

In [6]: [x.path for x in BlockDev.btrfs_list_subvolumes('/home', False)]
Out[6]:
['root',
 'home',
 'root/bar',
 'root/var/lib/portables',
 'root/var/lib/machines',
 'root/foo',
 'home/foo']

In [7]: [x.path for x in BlockDev.btrfs_list_subvolumes('/', False)]
Out[7]:
['root',
 'home',
 'root/bar',
 'root/var/lib/portables',
 'root/var/lib/machines',
 'root/foo',
 'home/foo']

Old behaviour:

In [12]: [x.path for x in BlockDev.btrfs_list_subvolumes('/', False)]
Out[12]:
['root',
 'home',
 'bar',
 'var/lib/portables',
 'var/lib/machines',
 'foo',
 'home/foo']

In [13]: [x.path for x in BlockDev.btrfs_list_subvolumes('/home', False)]
Out[13]:
['root',
 'home',
 'root/bar',
 'root/var/lib/portables',
 'root/var/lib/machines',
 'root/foo',
 'foo']

Some research notes from @mvollmer https://gist.githubusercontent.com/mvollmer/1cf111cfdb4f7ec55248ecd23ea4284d/raw/1711bc17482c8ec777081d14f316816e087ff330/gistfile1.txt

[StorageGhoul]

Can one of the admins verify this patch?

[vojtechtrefny]

Jenkins, ok to test.

[tbzatek]

Interesting finding.

[vojtechtrefny]

I am ok with this change, but this is a behaviour change and it is breaking blivet's feature where we check whether a filesystem is empty/unused or not -- storaged-project/blivet#1188 -- for btrfs this means mounting the subvolume and checking that any subdirectories are subvolumes which now doesn't work with the full/non-relative path we get from libblockdev. We'll need to fix the blivet function first to make sure it works with both versions of libblockdev.

[jelly]

I am ok with this change, but this is a behaviour change and it is breaking blivet's feature where we check whether a filesystem is empty/unused or not -- storaged-project/blivet#1188 -- for btrfs this means mounting the subvolume and checking that any subdirectories are subvolumes which now doesn't work with the full/non-relative path we get from libblockdev. We'll need to fix the blivet function first to make sure it works with both versions of libblockdev.

Do you want me to look into that, I am not too familiar with blivet. Is it possible we can add a test to blivet so it doesn't regress or is that not so easy?

[vojtechtrefny]

Do you want me to look into that, I am not too familiar with blivet.

I'll take care of it.

Is it possible we can add a test to blivet so it doesn't regress or is that not so easy?

We have a test for that, but it cannot run in a container so it's not running against PRs here -- I ran it manually, that's how I found this.

[vojtechtrefny]

Do you want me to look into that, I am not too familiar with blivet.

I'll take care of it.

Done storaged-project/blivet#1208

[vojtechtrefny]

Thank you