Skip to content

bugfix: fix cross-subvolume rename failure on bcachefs#527

Merged
andy5995 merged 3 commits intomasterfrom
iss-526
Apr 28, 2026
Merged

bugfix: fix cross-subvolume rename failure on bcachefs#527
andy5995 merged 3 commits intomasterfrom
iss-526

Conversation

@andy5995
Copy link
Copy Markdown
Member

On bcachefs, subvolumes share the same st_dev (unlike btrfs), so rmw correctly matched the waste folder by device number but rename() still returned EXDEV when moving across subvolumes. Add a fallback to safe_mv_via_exec() when rename() fails with EXDEV on a same-device move.

Also switch from BTRFS_IOC_CLONE (linux/btrfs.h) to the generic FICLONE ioctl (linux/fs.h), which is supported by any reflink-capable filesystem (btrfs, bcachefs, xfs, etc.), and update the meson header check accordingly.

Add test/test_bcachefs.sh which creates an 8M bcachefs image on-the-fly, exercises cross-subvolume file and directory moves, and cleans up after itself. Skips gracefully when bcachefs kernel support is unavailable.

Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com

Fixes #526

@andy5995 andy5995 added this to the v0.9.6 milestone Apr 25, 2026
@andy5995
Copy link
Copy Markdown
Member Author

@mlawren Thanks for the suggestion. Wanna test or review this?

andy5995 and others added 3 commits April 25, 2026 12:42
On bcachefs, subvolumes share the same st_dev (unlike btrfs), so rmw
correctly matched the waste folder by device number but rename() still
returned EXDEV when moving across subvolumes. Add a fallback to
safe_mv_via_exec() when rename() fails with EXDEV on a same-device move.

Also switch from BTRFS_IOC_CLONE (linux/btrfs.h) to the generic FICLONE
ioctl (linux/fs.h), which is supported by any reflink-capable filesystem
(btrfs, bcachefs, xfs, etc.), and update the meson header check
accordingly.

Add test/test_bcachefs.sh which creates an 8M bcachefs image on-the-fly,
exercises cross-subvolume file and directory moves, and cleans up after
itself. Skips gracefully when bcachefs kernel support is unavailable.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Fixes #526
- test/COMMON: add SKIP=77 for Meson skip exit code
- test/test_bcachefs.sh: use pre-existing image from test dir instead of
  creating one; skip if kernel bcachefs support or image is absent; guard
  against stale mounts/loop devices and leftover files from previous runs
- .github/workflows/c-cpp.yml: add bcachefs image cache/download step
- test/rmw-bcachefs-test.img.sha256sum: add checksum file for CI caching
- man/rmw.1: add BTRFS AND BCACHEFS subsection under NOTES
- Rename want_btrfs_clone option to want_ficlone
- Rename HAVE_LINUX_BTRFS macro to HAVE_FICLONE
- Rename has_btrfs_header variable to has_linux_fs_header
- Rename src/btrfs.c and src/btrfs.h to src/ficlone.c and src/ficlone.h
- Rename is_btrfs() to is_ficlone_fs() and do_btrfs_clone() to do_ficlone()
- Rename is_btrfs struct field in st_waste to is_ficlone_fs

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

@mlawren mlawren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little hard to review for two reasons:

  1. I'm not at all familiar with the codebase (but that is changing :-)
  2. The breakdown of changes and commits is not great. Code changes and build-system changs and CI changes and renames are not cleanly separated.

Since Claude is doing the work, perhaps it could be instructed to redo the steps.

But more strategically, I don't like (as an interested user / random internet commenter :-) the focus on a single bugfix for the single file case I presented. These commits do appear to enable a reflink copy for a single file. However if the target is a directory then the whole thing gets mved, which copies all the files in the tree. That is the extra work (and disk space) that a reflink is supposed to avoid.

So to my mind, the code needs to be architected a little differently, to almost re-implement mv internally by copying the directory tree and FICLONEing each file within. But perhaps that should be a patch against mv...

@mlawren
Copy link
Copy Markdown

mlawren commented Apr 28, 2026

So to my mind, the code needs to be architected a little differently, to almost re-implement mv internally by copying the directory tree and FICLONEing each file within. But perhaps that should be a patch against mv...

So, one probably should raise an issue against mv, but I just thought of something perhaps simpler. The equivalent of:

  1. cp --archive --recursive --reflink=always $SRC $DEST.tmp
  2. On failure, delete $DEST.tmp & error out
  3. rm -rf $SRC
  4. On failure, restore from $DEST.tmp & error out
  5. Rename $DEST.tmp to $DEST

The above is specific to a FICLONE filesystem, but it wouldn't take much to add a few steps to make it generic across all filesystem types. It is a bit more shelling out, but those tools are well-tested for "tree of directory" conditions.

@mlawren
Copy link
Copy Markdown

mlawren commented Apr 28, 2026

Finally, just another (somewhat unrelated) comment on code structure.

The restore and move_to_waste functions are basically doing the same thing: moving files or directories from one location to another. The generic part of those actions could be factored out to make the code smaller.

@andy5995
Copy link
Copy Markdown
Member Author

This is a little hard to review for two reasons:

1. I'm not at all familiar with the codebase (but that is changing :-)

2. The breakdown of changes and commits is not great. Code changes and build-system changs and CI changes and renames are not cleanly separated.

Originally the code was more btrfs-specific. So it makes sense to change some of the variables, the functions, and what I had in the build system. And I needed to rename btrfs.c for the purpose of this PR. Unfortunately it does make the PR more convoluted, but the code is cleaner in the long-run.

But more strategically, I don't like (as an interested user / random internet commenter :-) the focus on a single bugfix for the single file case I presented. These commits do appear to enable a reflink copy for a single file. However if the target is a directory then the whole thing gets mved, which copies all the files in the tree. That is the extra work (and disk space) that a reflink is supposed to avoid.

GNU coreutils mv has clone support, so even though 'mv' is getting called in some cases, It's not actually "copying" all the files, but cloning them (#497). Unfortunately that's not very robust because last time I checked, busybox mv doesn't support cloning. So... this all depends on what's built into mv on the user's target system.

The reason I decided to use mv like that was to avoid re-inventing the wheel by adding recursive copy/clone/delete or whatever, but I think I'll do that soon, a separate patch.

So to my mind, the code needs to be architected a little differently, to almost re-implement mv internally by copying the directory tree and FICLONEing each file within. But perhaps that should be a patch against mv...

Agreed ;)

@andy5995
Copy link
Copy Markdown
Member Author

Finally, just another (somewhat unrelated) comment on code structure.

The restore and move_to_waste functions are basically doing the same thing: moving files or directories from one location to another. The generic part of those actions could be factored out to make the code smaller.

I'll have a look.

@andy5995
Copy link
Copy Markdown
Member Author

1. `cp --archive --recursive --reflink=always $SRC $DEST.tmp`

--reflink is a GNU coreutils thing.

@andy5995 andy5995 merged commit 2c10b72 into master Apr 28, 2026
16 checks passed
@andy5995 andy5995 deleted the iss-526 branch April 28, 2026 16:49
@andy5995
Copy link
Copy Markdown
Member Author

@mlawren #529 takes care of some of what we discussed.

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.

Invalid cross-device link error on bcachefs

2 participants