Re: [PATCH 6/6] bcachefs: rebalance_work

[Nathan Chancellor <nathan@kernel.org>]

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 <kent.overstreet@linux.dev>

<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 :)

>  	bch2_trans_iter_init(trans, &src_iter, BTREE_ID_extents, src_start,
>  			     BTREE_ITER_INTENT);
>  	bch2_trans_iter_init(trans, &dst_iter, BTREE_ID_extents, dst_start,
> @@ -352,10 +358,13 @@ s64 bch2_remap_range(struct bch_fs *c,
>  				min(src_k.k->p.offset - src_want.offset,
>  				    dst_end.offset - dst_iter.pos.offset));
>  
> -		ret = bch2_extent_update(trans, dst_inum, &dst_iter,
> -					 new_dst.k, &disk_res,
> -					 new_i_size, i_sectors_delta,
> -					 true);
> +		ret =   bch2_bkey_set_needs_rebalance(c, new_dst.k,
> +					opts.background_target,
> +					opts.background_compression) ?:
> +			bch2_extent_update(trans, dst_inum, &dst_iter,
> +					new_dst.k, &disk_res,
> +					new_i_size, i_sectors_delta,
> +					true);
>  		bch2_disk_reservation_put(c, &disk_res);
>  	}
>  	bch2_trans_iter_exit(trans, &dst_iter);
> @@ -386,7 +395,7 @@ s64 bch2_remap_range(struct bch_fs *c,
>  
>  		bch2_trans_iter_exit(trans, &inode_iter);
>  	} while (bch2_err_matches(ret2, BCH_ERR_transaction_restart));
> -
> +err:
>  	bch2_trans_put(trans);
>  	bch2_bkey_buf_exit(&new_src, c);
>  	bch2_bkey_buf_exit(&new_dst, c);

Cheers,
Nathan