* [PATCH 0/2] eliminate the uninitialized compilation warning
@ 2024-04-19 7:48 Hongbo Li
2024-04-19 7:48 ` [PATCH 1/2] bcachefs: eliminate the uninitialized compilation warning in bch2_reconstruct_snapshots Hongbo Li
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Hongbo Li @ 2024-04-19 7:48 UTC (permalink / raw)
To: kent.overstreet, bfoster; +Cc: linux-bcachefs, lihongbo22
Eliminating the uninitialized compilation warning in bcachefs-tools.
Hongbo Li (2):
bcachefs: eliminate the uninitialized compilation warning in
bch2_reconstruct_snapshots
bcachefs: eliminate the uninitialized compilation warning in
__do_six_trylock
fs/bcachefs/six.c | 6 ++----
fs/bcachefs/snapshot.c | 5 ++++-
2 files changed, 6 insertions(+), 5 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/2] bcachefs: eliminate the uninitialized compilation warning in bch2_reconstruct_snapshots 2024-04-19 7:48 [PATCH 0/2] eliminate the uninitialized compilation warning Hongbo Li @ 2024-04-19 7:48 ` Hongbo Li 2024-04-25 3:34 ` Kent Overstreet 2024-04-19 7:48 ` [PATCH 2/2] bcachefs: eliminate the uninitialized compilation warning in __do_six_trylock Hongbo Li 2024-04-24 1:14 ` [PATCH 0/2] eliminate the uninitialized compilation warning Hongbo Li 2 siblings, 1 reply; 9+ messages in thread From: Hongbo Li @ 2024-04-19 7:48 UTC (permalink / raw) To: kent.overstreet, bfoster; +Cc: linux-bcachefs, lihongbo22 [BUG] When compiling the bcachefs-tools, the following compilation warning is reported: libbcachefs/snapshot.c: In function ‘bch2_reconstruct_snapshots’: libbcachefs/snapshot.c:915:19: warning: ‘tree_id’ may be used uninitialized in this function [-Wmaybe-uninitialized] 915 | snapshot->v.tree = cpu_to_le32(tree_id); libbcachefs/snapshot.c:903:6: note: ‘tree_id’ was declared here 903 | u32 tree_id; | ^~~~~~~ [CAUSE] This is a false alert, because @tree_id is changed in bch2_snapshot_tree_create after it returns 0. And if this function returns other value, @tree_id wouldn't be used. Thus there should be nothing wrong in logical. [FIX] Although the report itself is a false alert, we can still make it more explicit by: - check the input parameter 'u32 *tree_id' with WARN_ON_ONCE - initialize @tree_id to U32_MAX - add extra WARN_ON_ONCE to make sure @tree_id is updated Fixes: a292be3b68f3 ("bcachefs: Reconstruct missing snapshot nodes") Signed-off-by: Hongbo Li <lihongbo22@huawei.com> --- fs/bcachefs/snapshot.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/fs/bcachefs/snapshot.c b/fs/bcachefs/snapshot.c index 0b26dee17a5a..cad3408903b2 100644 --- a/fs/bcachefs/snapshot.c +++ b/fs/bcachefs/snapshot.c @@ -78,6 +78,7 @@ __bch2_snapshot_tree_create(struct btree_trans *trans) static int bch2_snapshot_tree_create(struct btree_trans *trans, u32 root_id, u32 subvol_id, u32 *tree_id) { + WARN_ON_ONCE(!tree_id); struct bkey_i_snapshot_tree *n_tree = __bch2_snapshot_tree_create(trans); @@ -900,7 +901,7 @@ static int check_snapshot_exists(struct btree_trans *trans, u32 id) if (bch2_snapshot_equiv(c, id)) return 0; - u32 tree_id; + u32 tree_id = U32_MAX; int ret = bch2_snapshot_tree_create(trans, id, 0, &tree_id); if (ret) return ret; @@ -910,6 +911,8 @@ static int check_snapshot_exists(struct btree_trans *trans, u32 id) if (ret) return ret; + /* bch2_snapshot_tree_create returned 0, @tree_id must be updated. */ + WARN_ON_ONCE(tree_id == U32_MAX); bkey_snapshot_init(&snapshot->k_i); snapshot->k.p = POS(0, id); snapshot->v.tree = cpu_to_le32(tree_id); -- 2.34.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] bcachefs: eliminate the uninitialized compilation warning in bch2_reconstruct_snapshots 2024-04-19 7:48 ` [PATCH 1/2] bcachefs: eliminate the uninitialized compilation warning in bch2_reconstruct_snapshots Hongbo Li @ 2024-04-25 3:34 ` Kent Overstreet 2024-04-25 3:55 ` Hongbo Li 0 siblings, 1 reply; 9+ messages in thread From: Kent Overstreet @ 2024-04-25 3:34 UTC (permalink / raw) To: Hongbo Li; +Cc: bfoster, linux-bcachefs On Fri, Apr 19, 2024 at 03:48:50PM +0800, Hongbo Li wrote: > [BUG] > When compiling the bcachefs-tools, the following compilation warning > is reported: > libbcachefs/snapshot.c: In function ‘bch2_reconstruct_snapshots’: > libbcachefs/snapshot.c:915:19: warning: ‘tree_id’ may be used uninitialized in this function [-Wmaybe-uninitialized] > 915 | snapshot->v.tree = cpu_to_le32(tree_id); > libbcachefs/snapshot.c:903:6: note: ‘tree_id’ was declared here > 903 | u32 tree_id; > | ^~~~~~~ > > [CAUSE] > This is a false alert, because @tree_id is changed in > bch2_snapshot_tree_create after it returns 0. And if this function > returns other value, @tree_id wouldn't be used. Thus there should > be nothing wrong in logical. > > [FIX] > Although the report itself is a false alert, we can still make it more > explicit by: > - check the input parameter 'u32 *tree_id' with WARN_ON_ONCE > - initialize @tree_id to U32_MAX > - add extra WARN_ON_ONCE to make sure @tree_id is updated > > Fixes: a292be3b68f3 ("bcachefs: Reconstruct missing snapshot nodes") > Signed-off-by: Hongbo Li <lihongbo22@huawei.com> > --- > fs/bcachefs/snapshot.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/fs/bcachefs/snapshot.c b/fs/bcachefs/snapshot.c > index 0b26dee17a5a..cad3408903b2 100644 > --- a/fs/bcachefs/snapshot.c > +++ b/fs/bcachefs/snapshot.c > @@ -78,6 +78,7 @@ __bch2_snapshot_tree_create(struct btree_trans *trans) > static int bch2_snapshot_tree_create(struct btree_trans *trans, > u32 root_id, u32 subvol_id, u32 *tree_id) > { > + WARN_ON_ONCE(!tree_id); There's no point checking for a null pointer like that; the oops from a null ptr deref gives us exactly the same information. > struct bkey_i_snapshot_tree *n_tree = > __bch2_snapshot_tree_create(trans); > > @@ -900,7 +901,7 @@ static int check_snapshot_exists(struct btree_trans *trans, u32 id) > if (bch2_snapshot_equiv(c, id)) > return 0; > > - u32 tree_id; > + u32 tree_id = U32_MAX; Just initialize it to 0. 0 is an invalid tree ID, so it'll be caught by snapshot_tree_invalid() if it's not set. > int ret = bch2_snapshot_tree_create(trans, id, 0, &tree_id); > if (ret) > return ret; > @@ -910,6 +911,8 @@ static int check_snapshot_exists(struct btree_trans *trans, u32 id) > if (ret) > return ret; > > + /* bch2_snapshot_tree_create returned 0, @tree_id must be updated. */ > + WARN_ON_ONCE(tree_id == U32_MAX); Nix this as well. > bkey_snapshot_init(&snapshot->k_i); > snapshot->k.p = POS(0, id); > snapshot->v.tree = cpu_to_le32(tree_id); > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] bcachefs: eliminate the uninitialized compilation warning in bch2_reconstruct_snapshots 2024-04-25 3:34 ` Kent Overstreet @ 2024-04-25 3:55 ` Hongbo Li 2024-04-25 17:08 ` Kent Overstreet 0 siblings, 1 reply; 9+ messages in thread From: Hongbo Li @ 2024-04-25 3:55 UTC (permalink / raw) To: Kent Overstreet; +Cc: bfoster, linux-bcachefs On 2024/4/25 11:34, Kent Overstreet wrote: > On Fri, Apr 19, 2024 at 03:48:50PM +0800, Hongbo Li wrote: >> [BUG] >> When compiling the bcachefs-tools, the following compilation warning >> is reported: >> libbcachefs/snapshot.c: In function ‘bch2_reconstruct_snapshots’: >> libbcachefs/snapshot.c:915:19: warning: ‘tree_id’ may be used uninitialized in this function [-Wmaybe-uninitialized] >> 915 | snapshot->v.tree = cpu_to_le32(tree_id); >> libbcachefs/snapshot.c:903:6: note: ‘tree_id’ was declared here >> 903 | u32 tree_id; >> | ^~~~~~~ >> >> [CAUSE] >> This is a false alert, because @tree_id is changed in >> bch2_snapshot_tree_create after it returns 0. And if this function >> returns other value, @tree_id wouldn't be used. Thus there should >> be nothing wrong in logical. >> >> [FIX] >> Although the report itself is a false alert, we can still make it more >> explicit by: >> - check the input parameter 'u32 *tree_id' with WARN_ON_ONCE >> - initialize @tree_id to U32_MAX >> - add extra WARN_ON_ONCE to make sure @tree_id is updated >> >> Fixes: a292be3b68f3 ("bcachefs: Reconstruct missing snapshot nodes") >> Signed-off-by: Hongbo Li <lihongbo22@huawei.com> >> --- >> fs/bcachefs/snapshot.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/fs/bcachefs/snapshot.c b/fs/bcachefs/snapshot.c >> index 0b26dee17a5a..cad3408903b2 100644 >> --- a/fs/bcachefs/snapshot.c >> +++ b/fs/bcachefs/snapshot.c >> @@ -78,6 +78,7 @@ __bch2_snapshot_tree_create(struct btree_trans *trans) >> static int bch2_snapshot_tree_create(struct btree_trans *trans, >> u32 root_id, u32 subvol_id, u32 *tree_id) >> { >> + WARN_ON_ONCE(!tree_id); > > There's no point checking for a null pointer like that; the oops from a > null ptr deref gives us exactly the same information. > >> struct bkey_i_snapshot_tree *n_tree = >> __bch2_snapshot_tree_create(trans); >> >> @@ -900,7 +901,7 @@ static int check_snapshot_exists(struct btree_trans *trans, u32 id) >> if (bch2_snapshot_equiv(c, id)) >> return 0; >> >> - u32 tree_id; >> + u32 tree_id = U32_MAX; > > Just initialize it to 0. 0 is an invalid tree ID, so it'll be caught by > snapshot_tree_invalid() if it's not set. > >> int ret = bch2_snapshot_tree_create(trans, id, 0, &tree_id); >> if (ret) >> return ret; >> @@ -910,6 +911,8 @@ static int check_snapshot_exists(struct btree_trans *trans, u32 id) >> if (ret) >> return ret; >> >> + /* bch2_snapshot_tree_create returned 0, @tree_id must be updated. */ >> + WARN_ON_ONCE(tree_id == U32_MAX); > > Nix this as well. This is to ensure that the tree_id is modified. > >> bkey_snapshot_init(&snapshot->k_i); >> snapshot->k.p = POS(0, id); >> snapshot->v.tree = cpu_to_le32(tree_id); >> -- >> 2.34.1 >> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] bcachefs: eliminate the uninitialized compilation warning in bch2_reconstruct_snapshots 2024-04-25 3:55 ` Hongbo Li @ 2024-04-25 17:08 ` Kent Overstreet 2024-04-26 1:31 ` Hongbo Li 0 siblings, 1 reply; 9+ messages in thread From: Kent Overstreet @ 2024-04-25 17:08 UTC (permalink / raw) To: Hongbo Li; +Cc: bfoster, linux-bcachefs On Thu, Apr 25, 2024 at 11:55:02AM +0800, Hongbo Li wrote: > > > On 2024/4/25 11:34, Kent Overstreet wrote: > > On Fri, Apr 19, 2024 at 03:48:50PM +0800, Hongbo Li wrote: > > > [BUG] > > > When compiling the bcachefs-tools, the following compilation warning > > > is reported: > > > libbcachefs/snapshot.c: In function ‘bch2_reconstruct_snapshots’: > > > libbcachefs/snapshot.c:915:19: warning: ‘tree_id’ may be used uninitialized in this function [-Wmaybe-uninitialized] > > > 915 | snapshot->v.tree = cpu_to_le32(tree_id); > > > libbcachefs/snapshot.c:903:6: note: ‘tree_id’ was declared here > > > 903 | u32 tree_id; > > > | ^~~~~~~ > > > > > > [CAUSE] > > > This is a false alert, because @tree_id is changed in > > > bch2_snapshot_tree_create after it returns 0. And if this function > > > returns other value, @tree_id wouldn't be used. Thus there should > > > be nothing wrong in logical. > > > > > > [FIX] > > > Although the report itself is a false alert, we can still make it more > > > explicit by: > > > - check the input parameter 'u32 *tree_id' with WARN_ON_ONCE > > > - initialize @tree_id to U32_MAX > > > - add extra WARN_ON_ONCE to make sure @tree_id is updated > > > > > > Fixes: a292be3b68f3 ("bcachefs: Reconstruct missing snapshot nodes") > > > Signed-off-by: Hongbo Li <lihongbo22@huawei.com> > > > --- > > > fs/bcachefs/snapshot.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/fs/bcachefs/snapshot.c b/fs/bcachefs/snapshot.c > > > index 0b26dee17a5a..cad3408903b2 100644 > > > --- a/fs/bcachefs/snapshot.c > > > +++ b/fs/bcachefs/snapshot.c > > > @@ -78,6 +78,7 @@ __bch2_snapshot_tree_create(struct btree_trans *trans) > > > static int bch2_snapshot_tree_create(struct btree_trans *trans, > > > u32 root_id, u32 subvol_id, u32 *tree_id) > > > { > > > + WARN_ON_ONCE(!tree_id); > > > > There's no point checking for a null pointer like that; the oops from a > > null ptr deref gives us exactly the same information. > > > > > struct bkey_i_snapshot_tree *n_tree = > > > __bch2_snapshot_tree_create(trans); > > > @@ -900,7 +901,7 @@ static int check_snapshot_exists(struct btree_trans *trans, u32 id) > > > if (bch2_snapshot_equiv(c, id)) > > > return 0; > > > - u32 tree_id; > > > + u32 tree_id = U32_MAX; > > > > Just initialize it to 0. 0 is an invalid tree ID, so it'll be caught by > > snapshot_tree_invalid() if it's not set. > > > > > int ret = bch2_snapshot_tree_create(trans, id, 0, &tree_id); > > > if (ret) > > > return ret; > > > @@ -910,6 +911,8 @@ static int check_snapshot_exists(struct btree_trans *trans, u32 id) > > > if (ret) > > > return ret; > > > + /* bch2_snapshot_tree_create returned 0, @tree_id must be updated. */ > > > + WARN_ON_ONCE(tree_id == U32_MAX); > > > > Nix this as well. > This is to ensure that the tree_id is modified. It's still not a good assertion; it's checking something simple and purely local, and we've got other checks that will fire later - it's not guarding against undefined behaviour. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] bcachefs: eliminate the uninitialized compilation warning in bch2_reconstruct_snapshots 2024-04-25 17:08 ` Kent Overstreet @ 2024-04-26 1:31 ` Hongbo Li 0 siblings, 0 replies; 9+ messages in thread From: Hongbo Li @ 2024-04-26 1:31 UTC (permalink / raw) To: Kent Overstreet; +Cc: bfoster, linux-bcachefs On 2024/4/26 1:08, Kent Overstreet wrote: > On Thu, Apr 25, 2024 at 11:55:02AM +0800, Hongbo Li wrote: >> >> >> On 2024/4/25 11:34, Kent Overstreet wrote: >>> On Fri, Apr 19, 2024 at 03:48:50PM +0800, Hongbo Li wrote: >>>> [BUG] >>>> When compiling the bcachefs-tools, the following compilation warning >>>> is reported: >>>> libbcachefs/snapshot.c: In function ‘bch2_reconstruct_snapshots’: >>>> libbcachefs/snapshot.c:915:19: warning: ‘tree_id’ may be used uninitialized in this function [-Wmaybe-uninitialized] >>>> 915 | snapshot->v.tree = cpu_to_le32(tree_id); >>>> libbcachefs/snapshot.c:903:6: note: ‘tree_id’ was declared here >>>> 903 | u32 tree_id; >>>> | ^~~~~~~ >>>> >>>> [CAUSE] >>>> This is a false alert, because @tree_id is changed in >>>> bch2_snapshot_tree_create after it returns 0. And if this function >>>> returns other value, @tree_id wouldn't be used. Thus there should >>>> be nothing wrong in logical. >>>> >>>> [FIX] >>>> Although the report itself is a false alert, we can still make it more >>>> explicit by: >>>> - check the input parameter 'u32 *tree_id' with WARN_ON_ONCE >>>> - initialize @tree_id to U32_MAX >>>> - add extra WARN_ON_ONCE to make sure @tree_id is updated >>>> >>>> Fixes: a292be3b68f3 ("bcachefs: Reconstruct missing snapshot nodes") >>>> Signed-off-by: Hongbo Li <lihongbo22@huawei.com> >>>> --- >>>> fs/bcachefs/snapshot.c | 5 ++++- >>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/fs/bcachefs/snapshot.c b/fs/bcachefs/snapshot.c >>>> index 0b26dee17a5a..cad3408903b2 100644 >>>> --- a/fs/bcachefs/snapshot.c >>>> +++ b/fs/bcachefs/snapshot.c >>>> @@ -78,6 +78,7 @@ __bch2_snapshot_tree_create(struct btree_trans *trans) >>>> static int bch2_snapshot_tree_create(struct btree_trans *trans, >>>> u32 root_id, u32 subvol_id, u32 *tree_id) >>>> { >>>> + WARN_ON_ONCE(!tree_id); >>> >>> There's no point checking for a null pointer like that; the oops from a >>> null ptr deref gives us exactly the same information. >>> >>>> struct bkey_i_snapshot_tree *n_tree = >>>> __bch2_snapshot_tree_create(trans); >>>> @@ -900,7 +901,7 @@ static int check_snapshot_exists(struct btree_trans *trans, u32 id) >>>> if (bch2_snapshot_equiv(c, id)) >>>> return 0; >>>> - u32 tree_id; >>>> + u32 tree_id = U32_MAX; >>> >>> Just initialize it to 0. 0 is an invalid tree ID, so it'll be caught by >>> snapshot_tree_invalid() if it's not set. >>> >>>> int ret = bch2_snapshot_tree_create(trans, id, 0, &tree_id); >>>> if (ret) >>>> return ret; >>>> @@ -910,6 +911,8 @@ static int check_snapshot_exists(struct btree_trans *trans, u32 id) >>>> if (ret) >>>> return ret; >>>> + /* bch2_snapshot_tree_create returned 0, @tree_id must be updated. */ >>>> + WARN_ON_ONCE(tree_id == U32_MAX); >>> >>> Nix this as well. >> This is to ensure that the tree_id is modified. > > It's still not a good assertion; it's checking something simple and > purely local, and we've got other checks that will fire later - it's not > guarding against undefined behaviour. > Thank you, I will revise the code based on your reviews. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] bcachefs: eliminate the uninitialized compilation warning in __do_six_trylock 2024-04-19 7:48 [PATCH 0/2] eliminate the uninitialized compilation warning Hongbo Li 2024-04-19 7:48 ` [PATCH 1/2] bcachefs: eliminate the uninitialized compilation warning in bch2_reconstruct_snapshots Hongbo Li @ 2024-04-19 7:48 ` Hongbo Li 2024-04-25 3:39 ` Kent Overstreet 2024-04-24 1:14 ` [PATCH 0/2] eliminate the uninitialized compilation warning Hongbo Li 2 siblings, 1 reply; 9+ messages in thread From: Hongbo Li @ 2024-04-19 7:48 UTC (permalink / raw) To: kent.overstreet, bfoster; +Cc: linux-bcachefs, lihongbo22 When compiling the bcachefs-tools, the following compilation warning is reported: libbcachefs/six.c: In function ‘__do_six_trylock’: libbcachefs/six.c:90:12: warning: ‘old’ may be used uninitialized in this function [-Wmaybe-uninitialized] 90 | if (!(old & SIX_LOCK_HELD_intent)) { | ~~~~~^~~~~~~~~~~~~~~~~~~~~~~ This is also a false altert. Only when @type=SIX_LOCK_write and @try=false are passed in __do_six_trylock, the second condition branch would enter which does not initialize the @old variable. But six_set_owner will not use @old if @type is not SIX_LOCK_intent. There should be nothing wrong in logical too. Although the report itself is a false alert, we can elimate the unitialize compilation warning by assigning @old in front. Fixes: 84a37cbf62e0 ("six locks: Wakeup now takes lock on behalf of waiter") Signed-off-by: Hongbo Li <lihongbo22@huawei.com> --- fs/bcachefs/six.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/fs/bcachefs/six.c b/fs/bcachefs/six.c index 3a494c5d1247..9f782e4e3ca9 100644 --- a/fs/bcachefs/six.c +++ b/fs/bcachefs/six.c @@ -118,11 +118,11 @@ static int __do_six_trylock(struct six_lock *lock, enum six_lock_type type, struct task_struct *task, bool try) { int ret; - u32 old; + u32 old = atomic_read(&lock->state); EBUG_ON(type == SIX_LOCK_write && lock->owner != task); EBUG_ON(type == SIX_LOCK_write && - (try != !(atomic_read(&lock->state) & SIX_LOCK_HELD_write))); + (try != !(old & SIX_LOCK_HELD_write))); /* * Percpu reader mode: @@ -157,7 +157,6 @@ static int __do_six_trylock(struct six_lock *lock, enum six_lock_type type, smp_mb(); - old = atomic_read(&lock->state); ret = !(old & l[type].lock_fail); this_cpu_sub(*lock->readers, !ret); @@ -182,7 +181,6 @@ static int __do_six_trylock(struct six_lock *lock, enum six_lock_type type, ret = -1 - SIX_LOCK_read; } } else { - old = atomic_read(&lock->state); do { ret = !(old & l[type].lock_fail); if (!ret || (type == SIX_LOCK_write && !try)) { -- 2.34.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] bcachefs: eliminate the uninitialized compilation warning in __do_six_trylock 2024-04-19 7:48 ` [PATCH 2/2] bcachefs: eliminate the uninitialized compilation warning in __do_six_trylock Hongbo Li @ 2024-04-25 3:39 ` Kent Overstreet 0 siblings, 0 replies; 9+ messages in thread From: Kent Overstreet @ 2024-04-25 3:39 UTC (permalink / raw) To: Hongbo Li; +Cc: bfoster, linux-bcachefs On Fri, Apr 19, 2024 at 03:48:51PM +0800, Hongbo Li wrote: > When compiling the bcachefs-tools, the following compilation warning > is reported: > libbcachefs/six.c: In function ‘__do_six_trylock’: > libbcachefs/six.c:90:12: warning: ‘old’ may be used uninitialized in this function [-Wmaybe-uninitialized] > 90 | if (!(old & SIX_LOCK_HELD_intent)) { > | ~~~~~^~~~~~~~~~~~~~~~~~~~~~~ > > This is also a false altert. Only when @type=SIX_LOCK_write and @try=false > are passed in __do_six_trylock, the second condition branch would enter > which does not initialize the @old variable. But six_set_owner will not > use @old if @type is not SIX_LOCK_intent. There should be nothing wrong > in logical too. > > Although the report itself is a false alert, we can elimate the unitialize > compilation warning by assigning @old in front. > > Fixes: 84a37cbf62e0 ("six locks: Wakeup now takes lock on behalf of waiter") > Signed-off-by: Hongbo Li <lihongbo22@huawei.com> > --- > fs/bcachefs/six.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/fs/bcachefs/six.c b/fs/bcachefs/six.c > index 3a494c5d1247..9f782e4e3ca9 100644 > --- a/fs/bcachefs/six.c > +++ b/fs/bcachefs/six.c > @@ -118,11 +118,11 @@ static int __do_six_trylock(struct six_lock *lock, enum six_lock_type type, > struct task_struct *task, bool try) > { > int ret; > - u32 old; > + u32 old = atomic_read(&lock->state); > > EBUG_ON(type == SIX_LOCK_write && lock->owner != task); > EBUG_ON(type == SIX_LOCK_write && > - (try != !(atomic_read(&lock->state) & SIX_LOCK_HELD_write))); > + (try != !(old & SIX_LOCK_HELD_write))); > > /* > * Percpu reader mode: > @@ -157,7 +157,6 @@ static int __do_six_trylock(struct six_lock *lock, enum six_lock_type type, > > smp_mb(); > > - old = atomic_read(&lock->state); Nope, this is wrong. That smp_mb() is there for a reason. > ret = !(old & l[type].lock_fail); > > this_cpu_sub(*lock->readers, !ret); > @@ -182,7 +181,6 @@ static int __do_six_trylock(struct six_lock *lock, enum six_lock_type type, > ret = -1 - SIX_LOCK_read; > } > } else { > - old = atomic_read(&lock->state); > do { > ret = !(old & l[type].lock_fail); > if (!ret || (type == SIX_LOCK_write && !try)) { > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] eliminate the uninitialized compilation warning 2024-04-19 7:48 [PATCH 0/2] eliminate the uninitialized compilation warning Hongbo Li 2024-04-19 7:48 ` [PATCH 1/2] bcachefs: eliminate the uninitialized compilation warning in bch2_reconstruct_snapshots Hongbo Li 2024-04-19 7:48 ` [PATCH 2/2] bcachefs: eliminate the uninitialized compilation warning in __do_six_trylock Hongbo Li @ 2024-04-24 1:14 ` Hongbo Li 2 siblings, 0 replies; 9+ messages in thread From: Hongbo Li @ 2024-04-24 1:14 UTC (permalink / raw) To: kent.overstreet, bfoster; +Cc: linux-bcachefs These are two compilation warning in bcachefs-tools, the patches may be useful. Thanks, Hongbo. On 2024/4/19 15:48, Hongbo Li wrote: > Eliminating the uninitialized compilation warning in bcachefs-tools. > > Hongbo Li (2): > bcachefs: eliminate the uninitialized compilation warning in > bch2_reconstruct_snapshots > bcachefs: eliminate the uninitialized compilation warning in > __do_six_trylock > > fs/bcachefs/six.c | 6 ++---- > fs/bcachefs/snapshot.c | 5 ++++- > 2 files changed, 6 insertions(+), 5 deletions(-) > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-04-26 1:32 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-04-19 7:48 [PATCH 0/2] eliminate the uninitialized compilation warning Hongbo Li 2024-04-19 7:48 ` [PATCH 1/2] bcachefs: eliminate the uninitialized compilation warning in bch2_reconstruct_snapshots Hongbo Li 2024-04-25 3:34 ` Kent Overstreet 2024-04-25 3:55 ` Hongbo Li 2024-04-25 17:08 ` Kent Overstreet 2024-04-26 1:31 ` Hongbo Li 2024-04-19 7:48 ` [PATCH 2/2] bcachefs: eliminate the uninitialized compilation warning in __do_six_trylock Hongbo Li 2024-04-25 3:39 ` Kent Overstreet 2024-04-24 1:14 ` [PATCH 0/2] eliminate the uninitialized compilation warning Hongbo Li
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 NNTP newsgroup(s).