* Re: [PATCH 6/6] bcachefs: rebalance_work
[not found] ` <20231024191414.2157874-7-kent.overstreet@linux.dev>
@ 2023-11-01 17:02 ` Nathan Chancellor
2023-11-01 17:07 ` Nathan Chancellor
0 siblings, 1 reply; 3+ messages in thread
From: Nathan Chancellor @ 2023-11-01 17:02 UTC (permalink / raw
To: Kent Overstreet; +Cc: linux-bcachefs, llvm
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 6/6] bcachefs: rebalance_work
2023-11-01 17:02 ` [PATCH 6/6] bcachefs: rebalance_work Nathan Chancellor
@ 2023-11-01 17:07 ` Nathan Chancellor
2023-11-02 1:11 ` Kent Overstreet
0 siblings, 1 reply; 3+ messages in thread
From: Nathan Chancellor @ 2023-11-01 17:07 UTC (permalink / raw
To: Kent Overstreet; +Cc: linux-bcachefs, llvm
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 <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 :)
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?
diff --git a/fs/bcachefs/reflink.c b/fs/bcachefs/reflink.c
index eb31df605c2e..6e1bfe9feb59 100644
--- a/fs/bcachefs/reflink.c
+++ b/fs/bcachefs/reflink.c
@@ -255,7 +255,7 @@ s64 bch2_remap_range(struct bch_fs *c,
struct bpos dst_end = dst_start, src_end = src_start;
struct bch_io_opts opts;
struct bpos src_want;
- u64 dst_done;
+ u64 dst_done = 0;
u32 dst_snapshot, src_snapshot;
int ret = 0, ret2 = 0;
> > 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
>
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 6/6] bcachefs: rebalance_work
2023-11-01 17:07 ` Nathan Chancellor
@ 2023-11-02 1:11 ` Kent Overstreet
0 siblings, 0 replies; 3+ messages in thread
From: Kent Overstreet @ 2023-11-02 1:11 UTC (permalink / raw
To: Nathan Chancellor; +Cc: linux-bcachefs, llvm
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 <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 :)
>
> 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 :)
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-11-02 1:11 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20231024191414.2157874-1-kent.overstreet@linux.dev>
[not found] ` <20231024191414.2157874-7-kent.overstreet@linux.dev>
2023-11-01 17:02 ` [PATCH 6/6] bcachefs: rebalance_work Nathan Chancellor
2023-11-01 17:07 ` Nathan Chancellor
2023-11-02 1:11 ` Kent Overstreet
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).