Skip to content

bcachefs: implement online filesystem shrinking - #1073

Open
jullanggit wants to merge 85 commits into
koverstreet:masterfrom
jullanggit:shrink
Open

bcachefs: implement online filesystem shrinking#1073
jullanggit wants to merge 85 commits into
koverstreet:masterfrom
jullanggit:shrink

Conversation

@jullanggit

@jullanggit jullanggit commented Mar 2, 2026

Copy link
Copy Markdown

Implement online filesystem shrinking through reconcile. Closes #781 once done.
This is hopefully complementary to #1070, which targets offline shrink.

Goal

A robust online shrinking implementation, that automatically resumes after restarts/crashes, as shrinking is a potentially long-running operation, and supports changing the target size mid-shrink.

Current state

  • Functionality is implemented
  • Basic tests pass (2 test timing errors in ~300 test runs)

Implementation

Reuses large parts of the device remove/evacuate paths

Documentation

Inline in https://github.com/jullanggit/bcachefs/blob/shrink/fs/bcachefs/init/dev.c - tho could maybe use an update

Testing

https://github.com/jullanggit/ktest/tree/shrink

@koverstreet

Copy link
Copy Markdown
Owner

Nice work — the reconcile-based approach for online shrinking is the right direction. Some feedback:

On-disk format:

Adding target_nbuckets to struct bch_member is a reasonable approach for tracking in-progress shrinks across restarts. But appending to bch_member needs care — make sure you're checking sizeof for the member struct version in the superblock validation path so older kernels reading a newer superblock don't read garbage. (This might already be handled by the existing versioned member struct machinery, but worth verifying.)

The evacuation loop:

The tail_is_empty() / sleep / retry loop in bch2_dev_shrink() is the right structure, but there are some issues:

  • The TODO about cached data is a real problem. After reconcile evacuates, the old extents get marked cached, so tail_is_empty() (which checks backpointers) might never see them clear. You'll need to handle this — either by making evacuate fully remove the old extent rather than caching it, or by adding a pass that invalidates cached buckets in the shrink region.

  • schedule_timeout_killable(HZ/2) is a reasonable poll interval but you should probably also check c->reconcile progress or wait on an event rather than blind polling.

  • The signal handling (signal_pending → -EINTR) is good — long-running operations should be interruptible.

Allocation cutoff:

if (unlikely(ca->mi.target_nbuckets && bucket >= ca->mi.target_nbuckets)) {

This replaces bch2_bucket_nouse() — but buckets_nouse was also used for other things (marking individual buckets bad). You've removed ENOMEM_buckets_nouse and no_resize_with_buckets_nouse from the error codes. Make sure nothing else was relying on that bitmap.

bch2_ptr_bad_or_evacuating_rcu:

This inline function does a division (div_u64(ptr->offset, ca->mi.bucket_size)) in what can be a hot path. Consider whether you can precompute the cutoff sector instead.

Commented-out code:

The __bch2_dev_resize_alloc block is commented out with a TODO. Either figure out what's needed there or remove it — commented-out code shouldn't ship.

Style nits:

  • Opening braces on function definitions should be on their own line (kernel style)
  • // TODO comments are fine for WIP but should be resolved before merge
  • The _typos.toml change doesn't belong in this PR
  • Several unrelated typo fixes (sentinal→sentinel, dosen't→doesn't, minumum→minimum, elligible→eligible) — those are welcome but should be a separate commit

Testing:

Good that you have ktest tests for this: https://github.com/jullanggit/ktest/tree/shrink. Consider adding cases for:

  • Shrink interrupted by signal, then resumed
  • Shrink with concurrent heavy writes to the device being shrunk
  • Shrink that needs to move striped/EC data
  • Shrink below journal location

Overall this is solid WIP. The hard parts (cached data handling, journal, resume after crash) are acknowledged as TODOs, which is the right approach — get the happy path working first.

— ProofOfConcept

@koverstreet
koverstreet force-pushed the master branch 2 times, most recently from 6303f5b to 990d039 Compare March 14, 2026 03:32
@jullanggit

Copy link
Copy Markdown
Author

Thank you for the review! I'll continue working on this, and will ping you once I feel like another review would help.

this addition is backwards compatible because new fields are initialized
to zero, which means no pending resize, and are not read by older
kernels
also comment in outline of shrink path
…elying on ca->mi.target_nbuckets

avoids possible edge cases if device is being removed mid-shrink etc.
This is done analogous to the remove alloc info path
…nto caller

bch2_dev_remove_alloc() now does the same operations wheter it is passed
a cutoff or not. Removing usage is not directly handled by both the
shrink (as previously) and remove (new) caller.
use helper macros and add comments
In the definitive tail-empty check inside __bch2_dev_shrink(), the outer
CLASS(btree_trans, trans) from the write-buffer flush call was still alive
when tail_head_snapshot() created its own btree_trans via the same macro.
This produced two live btree_trans for the same kernel thread, and the
DEBUG BUG_ON in __bch2_trans_get() fired when it found the first trans
still on the list with trans->locked set (from __bch2_trans_relock during
the flush).

Fix by narrowing the outer trans scope with an inner brace block so the
flush trans is released before tail_head_snapshot() allocates its own.
@jullanggit

jullanggit commented Jul 15, 2026

Copy link
Copy Markdown
Author

Hey @koverstreet!
Could you maybe take another look at these changes? They now consistently pass the shrink test suite (executed the 15 tests ~20 times with in total only two small errors due to test timing) and I'd say they are behaviour-wise solid enough to be get in the hands of people, although I would maybe still recommend an experimental label.
Also please note that this is my first contribution to the kernel or a project like it so please excuse any mistakes I might have made and do inform me of them :)

@jullanggit

jullanggit commented Jul 15, 2026

Copy link
Copy Markdown
Author

There are still some things I'd like to improve / add, among them a more fuzz-style tester and copying the code over into the userspace implementation. The latter shouldn't be very hard and I'd say both of these don't stand in the way of this PR :)

@jullanggit
jullanggit marked this pull request as ready for review July 15, 2026 19:05
…hange

this was due to the bch2_dev_resize_update_target also doing validation,
but allowing 0 to go through because we also used it to reset. Now the
two tasks are split into separate functions
@koverstreet

Copy link
Copy Markdown
Owner

uhh, this is a massive series :) are you aware that we're now working in the bcachefs-tools repo? and there's a shrink PR or maybe two there

@jullanggit

Copy link
Copy Markdown
Author

Ah no I wasn't aware of that, in that case I'll prioritize porting the implementation over. The PR over there does copy most of the same commits but is on a branch I don't have access to so I can't update it. I guess I'll write @Komzpa first and if that doesn't work open another one.
For the size, this does indeed touch quite a few things, would you like me to squash the changes into a few more coherent commits, or is the final diff the main issue anyways?

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.

Support for shrinking filesystem

3 participants