On Wed, Nov 01, 2023 at 10:07:43AM -0700, Nathan Chancellor wrote: > On Wed, Nov 01, 2023 at 10:02:55AM -0700, Nathan Chancellor wrote: > > Hi Kent, > > > > On Tue, Oct 24, 2023 at 03:14:11PM -0400, Kent Overstreet wrote: > > > This adds a new btree, rebalance_work, to eliminate scanning required > > > for finding extents that need work done on them in the background - i.e. > > > for the background_target and background_compression options. > > > > > > rebalance_work is a bitset btree, where a KEY_TYPE_set corresponds to an > > > extent in the extents or reflink btree at the same pos. > > > > > > A new extent field is added, bch_extent_rebalance, which indicates that > > > this extent has work that needs to be done in the background - and which > > > options to use. This allows per-inode options to be propagated to > > > indirect extents - at least in some circumstances. In this patch, > > > changing IO options on a file will not propagate the new options to > > > indirect extents pointed to by that file. > > > > > > Updating (setting/clearing) the rebalance_work btree is done by the > > > extent trigger, which looks at the bch_extent_rebalance field. > > > > > > Scanning is still requrired after changing IO path options - either just > > > for a given inode, or for the whole filesystem. We indicate that > > > scanning is required by adding a KEY_TYPE_cookie key to the > > > rebalance_work btree: the cookie counter is so that we can detect that > > > scanning is still required when an option has been flipped mid-way > > > through an existing scan. > > > > > > Future possible work: > > > - Propagate options to indirect extents when being changed > > > - Add other IO path options - nr_replicas, ec, to rebalance_work so > > > they can be applied in the background when they change > > > - Add a counter, for bcachefs fs usage output, showing the pending > > > amount of rebalance work: we'll probably want to do this after the > > > disk space accounting rewrite (moving it to a new btree) > > > > > > Signed-off-by: Kent Overstreet <[email protected]> > > > > <snip> > > > > > diff --git a/fs/bcachefs/reflink.c b/fs/bcachefs/reflink.c > > > index 540c78cd4b0c..dbbdf1955f76 100644 > > > --- a/fs/bcachefs/reflink.c > > > +++ b/fs/bcachefs/reflink.c > > > @@ -7,6 +7,7 @@ > > > #include "inode.h" > > > #include "io_misc.h" > > > #include "io_write.h" > > > +#include "rebalance.h" > > > #include "reflink.h" > > > #include "subvolume.h" > > > #include "super-io.h" > > > @@ -252,6 +253,7 @@ s64 bch2_remap_range(struct bch_fs *c, > > > struct bpos dst_start = POS(dst_inum.inum, dst_offset); > > > struct bpos src_start = POS(src_inum.inum, src_offset); > > > struct bpos dst_end = dst_start, src_end = src_start; > > > + struct bch_io_opts opts; > > > struct bpos src_want; > > > u64 dst_done; > > > u32 dst_snapshot, src_snapshot; > > > @@ -269,6 +271,10 @@ s64 bch2_remap_range(struct bch_fs *c, > > > bch2_bkey_buf_init(&new_src); > > > trans = bch2_trans_get(c); > > > > > > + ret = bch2_inum_opts_get(trans, src_inum, &opts); > > > + if (ret) > > > + goto err; > > > + > > > > Not sure if this has been reported or fixed yet but this appears to > > introduce a valid clang warning: > > > > fs/bcachefs/reflink.c:275:6: error: variable 'dst_done' is used > > uninitialized whenever 'if' condition is true > > [-Werror,-Wsometimes-uninitialized] > > 275 | if (ret) > > | ^~~ > > fs/bcachefs/reflink.c:405:9: note: uninitialized use occurs here > > 405 | return dst_done ?: ret ?: ret2; > > | ^~~~~~~~ > > fs/bcachefs/reflink.c:275:2: note: remove the 'if' if its condition is > > always false > > 275 | if (ret) > > | ^~~~~~~~ > > 276 | goto err; > > | ~~~~~~~~ > > fs/bcachefs/reflink.c:258:14: note: initialize the variable 'dst_done' to > > silence this warning > > 258 | u64 dst_done; > > | ^ > > | = 0 > > 1 error generated. > > > > I tried to reason my way through a patch but I am a little lost, hence > > just the report :) > > Actually, it just seems like dst_done should be explicitly zero > initialized, so that ret is used as the return value. Don't know why I > was as confused as I was :) would you like a formal patch or to just > squash it in?
Squashed it in, thanks :)
