Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libbtrfsutil: add btrfs_util_get_label #736

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jelly
Copy link

@jelly jelly commented Jan 26, 2024

Extend libbtrfs with a method to obtain to btrfs filesystem label.


Hey, this is a first PR of hopefully more to come. I'm looking at extending libbtrfsutil as we intend to use it in libblockdev to avoid parsing btrfs CLI output.

This is just a simple starter PR, some more interesting such as volume information (devices, RAID data, etc. basically btrfs filesystem show), creating a volume and adding/removing devices. And of course, btrfs_util_set_label happy to include that in this PR if required.

Extend libbtrfs with a method to obtain to btrfs filesystem label.

Signed-off-by: Jelle van der Waa <[email protected]>
@kdave
Copy link
Owner

kdave commented Jan 29, 2024

Thanks. I'm not sure if you're aware of my attempt to do implement the label get/set support, it looks it's mostly the same https://github.com/kdave/btrfs-progs/commits/dev/libbtrfsutil/labels-wip/ .

However one thing is missing from your code, this is adding a new symbol to the library and must be thus versioned. It would be best to add more new calls than one by one (each requiring a version update). Thre's also a pending update of the API naming scheme #574 .

@jelly
Copy link
Author

jelly commented Jan 30, 2024

Hi,

Thanks. I'm not sure if you're aware of my attempt to do implement the label get/set support, it looks it's mostly the same https://github.com/kdave/btrfs-progs/commits/dev/libbtrfsutil/labels-wip/ .

Woops, I was fully unaware of this I've looked through the issues and pull requests but not other branches.

However one thing is missing from your code, this is adding a new symbol to the library and must be thus versioned. It would be best to add more new calls than one by one (each requiring a version update). Thre's also a pending update of the API naming scheme #574 .

Shall I take your commit and work on a branch with for example adding AddDevice/RemoveDevice/CreateVolume that should be fairly trivial.

Something I really would like however is to replace our usage of btrfs filesystem show parsed by regex, so something like list devices of a btrfs volume. That will require some more API design, I guess it's useful to think of how the Python API should look like and then create the C API?

@kdave
Copy link
Owner

kdave commented Mar 6, 2024

I've pushed library updated 1.3 that updates the naming and sets the ground for future extensions. The release 6.7 will have only that.

Regarding the label, yes use my patch as a base, it needs some changes and I don't remember if it's complete (C code is, not sure about python and tests). This can be in 1.4 update along with some other easy ioctl wrappers, there are too many to choose from so it can be anything.

@jelly
Copy link
Author

jelly commented Mar 18, 2024

The Python code for the label has some issues, the tests don't succeed as there is extra data after the label. I'll take a look at it.

I have been looking into potential new functions to expose in libbtrfsutil which would be useful for libblockdev but I've found out that none of them are really trivial to implement:

  • btrfs device add/remove, this would be great to have. The ioctl is quite simple however, the API might not be:

The command line utility discards when required, detects if there is a filesystem on the target and optionally can enqueue by checking the sysfs file /sys/fs/btrfs/$fs_id/exclusive_operation . As no code can be re-used, it would either have to be written again or not handled.

btrfs_util_device_add("/dev/sda", "/mountpoint", force?, enque?, nodiscard?)

Remove seems more trivial, but not entirely useful with an add I'd say.

btrfs_util_device_remove("/dev/sda", "/mountpoint", enqueue)
  • btrfs filesystem show, this would be great to have as an API. Looking at the ioctl this requires a lot of thought and code. I have learned that for unmounted filesystems this wouldn't work, as that doesn't use the ioctl but reads the ctree via shared kernel code which is GPLv2 so can't be re-used afaik.

Maybe a better alternative for my use case is to add json output to btrfs filesystem show. I assume that is a welcome change regardless.

So for further extending libbttrfsutil adding the following functionality would probably be "easier":

  • BTRFS_IOC_GET_DEV_STATS - requires looking up the devid via FS_INFO
  • BTRFS_IOC_DEV_STATS_RESET

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants