| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <jwob7wwk6eijpsusqfuqllnmo4e5wj7a7lwnbmcp5exgrpkget@b4lo6d4yy5ya>
Date: Sun, 17 Nov 2024 20:32:19 -0500
From: Kent Overstreet <kent.overstreet@...ux.dev>
To: Nathan Chancellor <nathan@...nel.org>
Cc: Piotr Zalewski <pZ010001011111@...ton.me>, ndesaulniers@...gle.com,
morbo@...gle.com, justinstitt@...gle.com, linux-bcachefs@...r.kernel.org,
linux-kernel@...r.kernel.org, llvm@...ts.linux.dev, skhan@...uxfoundation.org
Subject: Re: [PATCH] bcachefs: initialize local variables in
bch2_evacuate_bucket
On Sun, Nov 17, 2024 at 06:00:40PM -0700, Nathan Chancellor wrote:
> On Sun, Nov 17, 2024 at 11:48:23PM +0000, Piotr Zalewski wrote:
> > Compiling bcachefs sources with LLVM triggers uninitialized variables
> > warnings.
> >
> > Signed-off-by: Piotr Zalewski <pZ010001011111@...ton.me>
> > ---
> > fs/bcachefs/move.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/bcachefs/move.c b/fs/bcachefs/move.c
> > index 8c032ef3a567..94cac498d372 100644
> > --- a/fs/bcachefs/move.c
> > +++ b/fs/bcachefs/move.c
> > @@ -674,8 +674,8 @@ int bch2_evacuate_bucket(struct moving_context *ctxt,
> > struct bkey_buf sk;
> > struct bkey_s_c k;
> > struct data_update_opts data_opts;
> > - unsigned dirty_sectors, bucket_size;
> > - u64 fragmentation;
> > + unsigned dirty_sectors = 0, bucket_size = 0;
> > + u64 fragmentation = 0;
>
> Shouldn't these just be removed altogether...? They are only used in a
> trace event and now only zero.
Actually, I think we want to bring them back; being able to understand
the behaviour of these background tasks is important whenever we have to
debug something.
Hasn't come up as much with copygc recently, but it has with rebalance
and I was just improving the rebalance tracepoint.
(The copygc issue that does come up is that it behaves poorly with a
multi device tiering filesystem where it's rebalance's job to move data
off a full foreground device, not copygc, but that doesn't hit this
codepath).
But - fragmentation is not really a "evacuate_bucket()" concern, that's
a generic codepath, so I think I'll do something simpler here and then
later we can add a dedicated copygc tracepoint, and that can include
information about the fragmentation LRU that copygc looks at.
So...
>From 6e1a9826444612f49e3667a0c0f4cd592dad9948 Mon Sep 17 00:00:00 2001
From: Kent Overstreet <kent.overstreet@...ux.dev>
Date: Sun, 17 Nov 2024 20:27:34 -0500
Subject: [PATCH] bcachefs: Fix evacuate_bucket tracepoint
86a494c8eef9 ("bcachefs: Kill bch2_get_next_backpointer()") dropped some
things the tracepoint emitted because bch2_evacuate_bucket() no longer
looks at the alloc key - but we did want at least some of that.
We still no longer look at the alloc key so we can't report on the
fragmentation number, but that's a direct function of dirty_sectors and
a copygc concern anyways - copygc should get its own tracepoint that
includes information from the fragmentation LRU.
But we can report on the number of sectors we moved and the bucket size.
Signed-off-by: Kent Overstreet <kent.overstreet@...ux.dev>
diff --git a/fs/bcachefs/move.c b/fs/bcachefs/move.c
index a98a0893a03b..07064872a697 100644
--- a/fs/bcachefs/move.c
+++ b/fs/bcachefs/move.c
@@ -674,8 +674,7 @@ int bch2_evacuate_bucket(struct moving_context *ctxt,
struct bkey_buf sk;
struct bkey_s_c k;
struct data_update_opts data_opts;
- unsigned dirty_sectors = 0, bucket_size = 0;
- u64 fragmentation = 0;
+ unsigned sectors_moved = 0;
struct bkey_buf last_flushed;
int ret = 0;
@@ -750,15 +749,18 @@ int bch2_evacuate_bucket(struct moving_context *ctxt,
data_opts.target = io_opts.background_target;
data_opts.rewrite_ptrs = 0;
- unsigned i = 0;
- bkey_for_each_ptr(bch2_bkey_ptrs_c(k), ptr) {
- if (ptr->dev == bucket.inode) {
- data_opts.rewrite_ptrs |= 1U << i;
- if (ptr->cached) {
+ unsigned i = 0, sectors = 0;
+ const union bch_extent_entry *entry;
+ struct extent_ptr_decoded p;
+ bkey_for_each_ptr_decode(k.k, bch2_bkey_ptrs_c(k), p, entry) {
+ if (p.ptr.dev == bucket.inode) {
+ if (p.ptr.cached) {
bch2_trans_iter_exit(trans, &iter);
goto next;
}
+ data_opts.rewrite_ptrs |= 1U << i;
}
+ sectors = ptr_disk_sectors(k.k->size, p);
i++;
}
@@ -777,7 +779,8 @@ int bch2_evacuate_bucket(struct moving_context *ctxt,
goto err;
if (ctxt->stats)
- atomic64_add(k.k->size, &ctxt->stats->sectors_seen);
+ atomic64_add(sectors, &ctxt->stats->sectors_seen);
+ sectors_moved += sectors;
} else {
struct btree *b;
@@ -808,12 +811,13 @@ int bch2_evacuate_bucket(struct moving_context *ctxt,
atomic64_add(sectors, &ctxt->stats->sectors_seen);
atomic64_add(sectors, &ctxt->stats->sectors_moved);
}
+ sectors_moved += btree_sectors(c);
}
next:
bch2_btree_iter_advance(&bp_iter);
}
- trace_evacuate_bucket(c, &bucket, dirty_sectors, bucket_size, fragmentation, ret);
+ trace_evacuate_bucket(c, &bucket, sectors_moved, ca->mi.bucket_size, ret);
err:
bch2_trans_iter_exit(trans, &bp_iter);
bch2_dev_put(ca);
diff --git a/fs/bcachefs/trace.h b/fs/bcachefs/trace.h
index 5597b9d6297f..2d5932d2881e 100644
--- a/fs/bcachefs/trace.h
+++ b/fs/bcachefs/trace.h
@@ -848,8 +848,8 @@ TRACE_EVENT(move_data,
TRACE_EVENT(evacuate_bucket,
TP_PROTO(struct bch_fs *c, struct bpos *bucket,
unsigned sectors, unsigned bucket_size,
- u64 fragmentation, int ret),
- TP_ARGS(c, bucket, sectors, bucket_size, fragmentation, ret),
+ int ret),
+ TP_ARGS(c, bucket, sectors, bucket_size, ret),
TP_STRUCT__entry(
__field(dev_t, dev )
@@ -857,7 +857,6 @@ TRACE_EVENT(evacuate_bucket,
__field(u64, bucket )
__field(u32, sectors )
__field(u32, bucket_size )
- __field(u64, fragmentation )
__field(int, ret )
),
@@ -867,15 +866,14 @@ TRACE_EVENT(evacuate_bucket,
__entry->bucket = bucket->offset;
__entry->sectors = sectors;
__entry->bucket_size = bucket_size;
- __entry->fragmentation = fragmentation;
__entry->ret = ret;
),
- TP_printk("%d,%d %llu:%llu sectors %u/%u fragmentation %llu ret %i",
+ TP_printk("%d,%d %llu:%llu sectors %u/%u ret %i",
MAJOR(__entry->dev), MINOR(__entry->dev),
__entry->member, __entry->bucket,
__entry->sectors, __entry->bucket_size,
- __entry->fragmentation, __entry->ret)
+ __entry->ret)
);
TRACE_EVENT(copygc,
Powered by blists - more mailing lists