RFC: btrfs plugin: use libbtrfsutil #999

jelly · 2024-01-22T13:13:52Z

This is a quick hack to start off the discussion if libbtrfsutil would be a good fit for extending further features in the btrfs plugin for libblockdev. ( I recall there was a discussion somewhere, but couldn't find the issue)

Some notable things which needs to be verified:

License, libbtrfsutil itself is licensed LGPL, is it compatible with libblockdev?
Feature completeness, can everything currently be replaced with libbtrfsutil and what is missing, can it be supported in libbtrfsutil and is upstream willing too?
Is libbtrfsutil packaged everywhere libblockdev is at the moment used.

Important note:

libbtrfsutil only includes operations that are done through the filesystem and ioctl interface, not operations that modify the filesystem directly (e.g., mkfs or fsck). This is by design but also a legal necessity, as the filesystem implementation is GPL but libbtrfsutil is LGPL. That is also why the libbtrfsutil code is a reimplementation of the btrfs-progs code rather than a refactoring. Be wary of this when porting functionality.

Porting todo

btrfs default subvolume get/set
listing subvolumes
create subvolume
delete subvolume

Missing

volume information (bd_btrfs_filesystem_info) (ideally also RAID/ multi device information which is now lacking (btrfs device usage /))
device add/remove support
set label?
bd_list_devices
create volume

StorageGhoul · 2024-01-22T13:17:02Z

Can one of the admins verify this patch?

vojtechtrefny · 2024-01-22T13:40:57Z

This is a quick hack to start off the discussion if libbtrfsutil would be a good fit for extending further features in the btrfs plugin for libblockdev.

Definitely something we'd like to do. Every command call we can replace with a library call is a huge improvement.

( I recall there was a discussion somewhere, but couldn't find the issue)

That would be #552

Some notable things which needs to be verified:
* [ ]  License, libbtrfsutil itself is licensed LGPL, is it compatible with libblockdev?

Yes, libblockdev is also licensed under LGPL 2.1 so there shouldn't be a problem.

* [ ]  Feature completeness, can everything currently be replaced with libbtrfsutil and what is missing, can it be supported in libbtrfsutil and is upstream willing too?

It would be great if we could replace everything with libbtrfsutil, but I am ok with replacing only parts of the command calls with the library.

* [ ]  Is libbtrfsutil packaged everywhere libblockdev is at the moment used.

Libblockdev is packaged pretty much everywhere these days, but libbtrfsutil seems to be available in both Fedora and Debian/Ubuntu and that's good enough at least for our CI :)

vojtechtrefny · 2024-01-22T13:45:34Z

configure.ac

@@ -229,6 +229,10 @@ AS_IF([test "x$with_fs" != "xno" -o "x$with_crypto" != "xno" -o "x$with_swap" !=
             [Define as neutral value if libblkid doesn't provide the definition])])]
      [])

+AS_IF([test "x$with_btrfs" != "xno"],
+      [LIBBLOCKDEV_PKG_CHECK_MODULES([LIBBTRFSUTIL], [libbtrfsutil >= 6.6])],
+      [])


To unblock some of the CI, you'll need to libbtrfsutil dependency to few places:

dist/libblockdev.spec.in

misc/install-test-dependencies.yml

.packit.yaml

This will cover Packit and the GH actions checks (we'll also need to add it to our internal CI, but that's something we need to do manually).

Thanks, applied it.

jelly · 2024-01-22T13:51:29Z

This is a quick hack to start off the discussion if libbtrfsutil would be a good fit for extending further features in the btrfs plugin for libblockdev.

Definitely something we'd like to do. Every command call we can replace with a library call is a huge improvement.

( I recall there was a discussion somewhere, but couldn't find the issue)

That would be #552
Some notable things which needs to be verified:
* [ ]  License, libbtrfsutil itself is licensed LGPL, is it compatible with libblockdev?
Yes, libblockdev is also licensed under LGPL 2.1 so there shouldn't be a problem.

Nice!

* [ ]  Feature completeness, can everything currently be replaced with libbtrfsutil and what is missing, can it be supported in libbtrfsutil and is upstream willing too?
It would be great if we could replace everything with libbtrfsutil, but I am ok with replacing only parts of the command calls with the library.

So from the LGPL/GPL note in the documentation, I can make up that it is basically everything mkfs, repair and possibly btrfs device add.

* [ ]  Is libbtrfsutil packaged everywhere libblockdev is at the moment used.
Libblockdev is packaged pretty much everywhere these days, but libbtrfsutil seems to be available in both Fedora and Debian/Ubuntu and that's good enough at least for our CI :)

It's also available in Arch Linux. I should add the required CI changes, for Fedora this would mean a dependency on libbtrfsutil which btrfs-progs does not depend on (due to the licensing afaik).

tbzatek

Very nice!

tbzatek · 2024-01-22T13:57:39Z

src/plugins/btrfs.c

-    g_match_info_free (match_info);
-    g_regex_unref (regex);
-    g_free (output);
+    err = btrfs_util_get_default_subvolume(mountpoint, &ret);


Minor nitpick: please use Gnome-like codestyle, i.e. a space before opening parenthesis.

tbzatek · 2024-01-22T14:01:15Z

src/plugins/btrfs.c

-    g_free (output);
+    err = btrfs_util_get_default_subvolume(mountpoint, &ret);
+    if (err)
+        g_set_error (error, BD_BTRFS_ERROR, BD_BTRFS_ERROR_PARSE, btrfs_util_strerror(err));


I think we can afford changing error codes a little bit, if needed. While it would be nice to keep most of them consistent, I imagine the code path would be so different anyway and it would be good to distinguish between minor errors (i.e. "device open error", "parse error", "invalid argument", etc.)

Agreed, currently the error is rather strange when given a path which is not mounted. So this needs more work.

AssertionError: ".*(can't|cannot) access.*" does not match "g-bd-btrfs-error-quark: Could not open (2)"

It's a C style API so the test of the error was in errno.

Different error messages are fine, there's generally no guarantee of stability and clients should never match any strings (it's okay for tests where it is intentional) - that's what the error codes are for.

jelly · 2024-01-22T20:28:04Z

configure.ac

@@ -229,6 +229,10 @@ AS_IF([test "x$with_fs" != "xno" -o "x$with_crypto" != "xno" -o "x$with_swap" !=
             [Define as neutral value if libblkid doesn't provide the definition])])]
      [])

+AS_IF([test "x$with_btrfs" != "xno"],
+      [LIBBLOCKDEV_PKG_CHECK_MODULES([LIBBTRFSUTIL], [libbtrfsutil >= 5.1])],


This is a bit arbitrary, needs some investigation which version is really needed, then again 5.1 is from 2019.

jelly · 2024-01-22T20:29:30Z

src/plugins/btrfs.c

 * @error: (out) (optional): place to store error (if any)
 *
 * Returns: whether the @mountpoint volume's default subvolume was correctly set
 * to @subvol_id or not
 *
 * Tech category: %BD_BTRFS_TECH_SUBVOL-%BD_BTRFS_TECH_MODE_MODIFY
 */
-gboolean bd_btrfs_set_default_subvolume (const gchar *mountpoint, guint64 subvol_id, const BDExtraArg **extra, GError **error) {
-    const gchar *argv[6] = {"btrfs", "subvol", "set-default", NULL, mountpoint, NULL};


This breaks udisks, which I think we rather not? But -Werror will scream at me if I keep it around. I guess I should mark it with something __unused?

Yes, existing public API cannot be changed. Please add G_GNUC_UNUSED annotation.

If you want to keep full API compatibility, you'd need to translate any known extra args to library calls/arguments. Porting existing API is tough, creating new API from scratch is much easier.

I think deprecating the argument would be the best, I am not sure what extra options can be given as btrfs subvolume set default only takes:

btrfs subvolume set-default <subvolid> <path>

So I think for backwards compatibility this would actually work out quite well in this instance. Should I add G_DEPRECATED as well to the argument and update the comment?

No need to special annotation for function arguments I guess.

libblockdev has historically used btrfs-progs CLI utilities in the meanwhile libbtrfsutil has been developed and used by snapper. Not parsing CLI output and using the library functions should provide a more stable way to gather information from btrfs.

libbtrfsutil provides it's own error enum for libbtrfsutil functions and as we no longer parse output this introduces a BD_BTRFS_ERROR_NOT_FOUND error type. As the mount point not being found is the most likely failure scenario.

Use libbtrfsutil's subvolume iterator to obtain a list of subvolumes in order. In the future this would allow exposing more information about a subvolume to list_subvolumes.

Avoids having to exec btrfs-progs and allows us to implement recursive subvolume deletion in the future.

jelly · 2024-03-06T11:03:28Z

I've rebased and ported all functions which can use libbtrfsutil. I haven't managed to land a new feature yet in libbtrfsutil. But I don't think btrfs-progs wants to block new additions to the API.

This is all still a bit WiP, there are some TODO's regarding error checking and one test which cannot be ported as it overrides btrfs subvolume list (The weird docker subvolumes one). But in all it seems to shave off ~ 100 loc.

vojtechtrefny reviewed Jan 22, 2024

View reviewed changes

tbzatek reviewed Jan 22, 2024

View reviewed changes

jelly force-pushed the libbtrfsutil branch 2 times, most recently from d9b9b8f to 65eac48 Compare January 22, 2024 20:27

jelly commented Jan 22, 2024

View reviewed changes

jelly force-pushed the libbtrfsutil branch 2 times, most recently from 0321b45 to 8ac2504 Compare January 25, 2024 13:35

jelly mentioned this pull request Jan 26, 2024

libbtrfsutil: add btrfs_util_get_label kdave/btrfs-progs#736

Open

jelly added 2 commits March 4, 2024 14:02

btrfs: link against libbtrfsutil

40f1df9

libblockdev has historically used btrfs-progs CLI utilities in the meanwhile libbtrfsutil has been developed and used by snapper. Not parsing CLI output and using the library functions should provide a more stable way to gather information from btrfs.

btrfs: use libbtrfsutil for get/set default subvolume

2b47e34

libbtrfsutil provides it's own error enum for libbtrfsutil functions and as we no longer parse output this introduces a BD_BTRFS_ERROR_NOT_FOUND error type. As the mount point not being found is the most likely failure scenario.

jelly force-pushed the libbtrfsutil branch 2 times, most recently from a81a7a7 to 4108205 Compare March 5, 2024 13:30

jelly added 2 commits March 6, 2024 11:58

btrfs: use libbtrfsutil for listing subvolumes

3857501

Use libbtrfsutil's subvolume iterator to obtain a list of subvolumes in order. In the future this would allow exposing more information about a subvolume to list_subvolumes.

btrfs: port create/delete subvolume to libbtrfsutil

b7587d9

Avoids having to exec btrfs-progs and allows us to implement recursive subvolume deletion in the future.

jelly force-pushed the libbtrfsutil branch from 4108205 to b7587d9 Compare March 6, 2024 11:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: btrfs plugin: use libbtrfsutil #999

RFC: btrfs plugin: use libbtrfsutil #999

jelly commented Jan 22, 2024 •

edited

Loading

StorageGhoul commented Jan 22, 2024

vojtechtrefny commented Jan 22, 2024

vojtechtrefny Jan 22, 2024

jelly Jan 22, 2024

jelly commented Jan 22, 2024

tbzatek left a comment

tbzatek Jan 22, 2024

tbzatek Jan 22, 2024

jelly Jan 22, 2024

jelly Jan 22, 2024

tbzatek Jan 23, 2024

jelly Jan 22, 2024 •

edited

Loading

jelly Jan 22, 2024

tbzatek Jan 23, 2024

jelly Jan 25, 2024

tbzatek Jan 25, 2024

jelly commented Mar 6, 2024

RFC: btrfs plugin: use libbtrfsutil #999

Are you sure you want to change the base?

RFC: btrfs plugin: use libbtrfsutil #999

Conversation

jelly commented Jan 22, 2024 • edited Loading

Porting todo

Missing

StorageGhoul commented Jan 22, 2024

vojtechtrefny commented Jan 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelly commented Jan 22, 2024

tbzatek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelly Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelly commented Mar 6, 2024

jelly commented Jan 22, 2024 •

edited

Loading

jelly Jan 22, 2024 •

edited

Loading