-
Notifications
You must be signed in to change notification settings - Fork 150
Add ZFS filesystem reflink support #748
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
base: master
Are you sure you want to change the base?
Conversation
|
I'm not very familiar with the codebase here, but formally there are tests and maybe something needs to be adapted here as well to provide/enable testing? Line 49 in a7e1df4
|
|
Damn, I just re-implemented it on my side before seeing that this PR was open... I should really have checked first! I didn't go as far as differentiating between FIDEDUPERANGE and FICLONERANGE, but clearly that's a good thing to do. diff --git a/lib/utilities.c b/lib/utilities.c
index ef52d347..a3c50c94 100644
--- a/lib/utilities.c
+++ b/lib/utilities.c
@@ -597,7 +597,7 @@ static bool fs_supports_dedupe(char *fstype, char *mountpoint) {
return false;
}
-static bool fs_supports_reflinks(char *fstype, char *mountpoint) {
+static bool fs_supports_reflinks(char *fstype, char *mountpoint, char *fsname) {
/* Check if filesystem supports any form of reflinks/clones */
if(fs_supports_dedupe(fstype, mountpoint)) {
/* If it supports dedupe ioctl, it supports reflinks */
@@ -609,7 +609,20 @@ static bool fs_supports_reflinks(char *fstype, char *mountpoint) {
* 'rmlint --dedupe', but does support 'cp --reflink=always'.
* For sh:handler=reflink, the generated script will use cp --reflink.
* For sh:handler=clone, the dedupe operation will fail gracefully. */
- return true;
+
+ /* However we need to check that the zpool has the feature enabled,
+ * so get the pool name from the fsname (this is the part before the
+ * first '/', if any). It can either be disabled, or not supported
+ * because the pool has not been upgraded yet, or because ZFS is too old. */
+ char *fs = g_strdup(fsname);
+ g_strdelimit(fs, "/", '\0'); /* modifies 'fs' inplace */
+ char *pool = g_shell_quote(fs);
+ char *cmd = g_strdup_printf("zpool get -H -o value feature@block_cloning %s 2>/dev/null | grep -q -e active -e enabled", pool);
+ int res = system(cmd);
+ g_free(cmd);
+ g_free(pool);
+ g_free(fs);
+ return(res==0);
}
return false;
}
@@ -693,7 +706,7 @@ static RmMountEntries *rm_mount_list_open(RmMountTable *table) {
evilfs_found->name, wrap_entry->dir, (unsigned)dir_stat.st_dev);
}
- if(fs_supports_reflinks(wrap_entry->type, wrap_entry->dir)) {
+ if(fs_supports_reflinks(wrap_entry->type, wrap_entry->dir, wrap_entry->fsname)) {
RmStat dir_stat;
if(rm_sys_stat(wrap_entry->dir, &dir_stat) == 0) {
g_hash_table_insert(table->reflinkfs_table,I'm not extremely fond of using the shell version of |
|
Hey, Finally back from my hiatus. I'll give this a readover, though as @jdehaan says, unit tests should probably be added for this. I may commit to this in the future with this. |
ZFS now supports file cloning/reflinks via cp --reflink in OpenZFS 2.2+. This commit adds proper support for ZFS in rmlint by distinguishing between filesystems that support the FIDEDUPERANGE ioctl (used by 'rmlint --dedupe' and sh:handler=clone) and those that only support cp --reflink. Changes: - Split filesystem detection into fs_supports_dedupe() and fs_supports_reflinks() - Add separate dedupefs_table to track filesystems supporting FIDEDUPERANGE - Update clone handler to check dedupe support specifically - Add rm_mounts_can_dedupe() function for checking dedupe capability - Document that ZFS supports reflinks but not the dedupe ioctl This ensures that: - sh:handler=reflink works correctly on ZFS (uses cp --reflink) - sh:handler=clone is skipped on ZFS (no FIDEDUPERANGE support) - The 'link' shortcut now works properly on ZFS by falling back to reflink Documentation has been updated to clarify the behavior on ZFS. Fixes: sahib#745
|
You might want to use |
| gint exit_status = 0; | ||
| bool supports_cloning = false; | ||
|
|
||
| if(g_spawn_sync(NULL, argv, NULL, G_SPAWN_SEARCH_PATH | G_SPAWN_STDERR_TO_DEV_NULL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative would be not to check reflink-capability at all here and let the coreutils commands used in the script to emit error if the ioctl calls fail.
cp seems to try for FICLONE by default and use copy_file_range on failure.
Our fs_supports_dedupe() uses system() for XFS so what we do here is not so bad, at least we don't invoke the shell.
|
@vassilit saw your commit with zfs tests, iirc ubuntu 24 has zfs 2.2, it has disabled block cloning by default. It may be enabled with |
|
@gmelikov Спасибо! But I saw your message too late (just now). I plan to add FreeBSD tests on 14.4 (ZFS 2.2) and 15.0 (2.4) soon that will cover more recent versions if 2.2.2 on Linux is failing. |
|
@vassilit usually get FreeBSD in github actions is not so straightforward, but yeah, it's too should work :) Although better to have both Linux and FreeBSD. JFYI openzfs in the end just started qemu in CI for non-Ubuntu OSes https://github.com/openzfs/zfs/blob/master/.github/workflows/zfs-qemu.yml |
|
@gmelikov it's a bit off-topic here but for FreeBSD runners on Github I used Cirrus-CI in the past, then switched to vmactions, but it is very slow. The quite new Firecracker seems promising but I have not tested it yet. |
@gmelikov, ping. Your help is appreciated, could you open a separate PR (I don't know how to give you rights on Mac92's branch) and we will rebase this one on yours ? |
|
@vassilit got it, I'll do that in several days. |
|
zfs 2.4 didn't help, and looks like problem is not with zfs 2.2 from Ubuntu, here you can see run with 2.4:
I've built this PR, read ubuntu@ubuntu:~/rmlint$ sudo zfs -V
zfs-2.2.2-0ubuntu9.2
zfs-kmod-2.2.2-0ubuntu9.1
ubuntu@ubuntu:~/rmlint$ echo 1 | sudo tee /sys/module/zfs/parameters/zfs_bclone_enabled
1
# Create original file
ubuntu@ubuntu:~/rmlint$ dd if=/dev/urandom of=/tmp/a bs=4K count=1
# Before: block table empty
ubuntu@ubuntu:~/rmlint$ sudo zdb -T rpool
BRT: empty
# Run test
ubuntu@ubuntu:~/rmlint$ cp --reflink=auto /tmp/a /tmp/b
# After, ZFS sees duplicates, reflink was a success
ubuntu@ubuntu:~/rmlint$ sudo zdb -T rpool
BRT: used 4K; saved 4K; ratio 2.00x
ubuntu@ubuntu:~/rmlint$ ./rmlint --is-reflink /tmp/a /tmp/b
ubuntu@ubuntu:~/rmlint$ echo $?
5from docs exit code 5 is "fiemaps can't be read". Ah, I thought ZFS supported it but it's not openzfs/zfs#9554 So, reflink works apparently, BUT rmlint can't easily test it with FIOMAP. |
|
2.4 helped not getting EAGAIN: Resource temporarily unavailable. At least, we can test now. Thank you ! |
Signed-off-by: George Melikov <mail@gmelikov.ru>
|
@vassilit ok, here is clean commit to use zfs 2.4 gmelikov@86ed254 |
|
I guess we could add needs_fiemap to some tests and modify the code so that reflink operations are best-efforts on ZFS (no easy way to check, I don't think we should call zdb or import headers from OpenZFS and use their non-stable API). It is kind of already supported, as we can make the generated script to use What are your thoughts on this? |
ZFS now supports file cloning/reflinks via cp --reflink in OpenZFS 2.2+. This commit adds proper support for ZFS in rmlint by distinguishing between filesystems that support the FIDEDUPERANGE ioctl (used by 'rmlint --dedupe' and sh:handler=clone) and those that only support cp --reflink.
Changes:
This ensures that:
Documentation has been updated to clarify the behavior on ZFS.
Fixes: #745