* [PATCH] btrfs: remove unneeded variable: "ret" @ 2021-06-28 8:30 13145886936 2021-06-28 9:21 ` Qu Wenruo 2021-06-28 11:38 ` Qu Wenruo 0 siblings, 2 replies; 13+ messages in thread From: 13145886936 @ 2021-06-28 8:30 UTC (permalink / raw) To: clm, josef, dsterba; +Cc: linux-btrfs, linux-kernel, gushengxian, gushengxian From: gushengxian <gushengxian@yulong.com> Remove unneeded variable: "ret". Signed-off-by: gushengxian <13145886936@163.com> Signed-off-by: gushengxian <gushengxian@yulong.com> --- fs/btrfs/disk-io.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index b117dd3b8172..7e65a54b7839 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -4624,7 +4624,6 @@ static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans, struct rb_node *node; struct btrfs_delayed_ref_root *delayed_refs; struct btrfs_delayed_ref_node *ref; - int ret = 0; delayed_refs = &trans->delayed_refs; @@ -4632,7 +4631,7 @@ static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans, if (atomic_read(&delayed_refs->num_entries) == 0) { spin_unlock(&delayed_refs->lock); btrfs_debug(fs_info, "delayed_refs has NO entry"); - return ret; + return 0; } while ((node = rb_first_cached(&delayed_refs->href_root)) != NULL) { @@ -4695,7 +4694,7 @@ static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans, spin_unlock(&delayed_refs->lock); - return ret; + return 0; } static void btrfs_destroy_delalloc_inodes(struct btrfs_root *root) -- 2.25.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] btrfs: remove unneeded variable: "ret" 2021-06-28 8:30 [PATCH] btrfs: remove unneeded variable: "ret" 13145886936 @ 2021-06-28 9:21 ` Qu Wenruo 2021-06-28 9:34 ` Damien Le Moal 2021-06-28 11:38 ` Qu Wenruo 1 sibling, 1 reply; 13+ messages in thread From: Qu Wenruo @ 2021-06-28 9:21 UTC (permalink / raw) To: 13145886936, clm, josef, dsterba; +Cc: linux-btrfs, linux-kernel, gushengxian On 2021/6/28 下午4:30, 13145886936@163.com wrote: > From: gushengxian <gushengxian@yulong.com> > > Remove unneeded variable: "ret". > > Signed-off-by: gushengxian <13145886936@163.com> > Signed-off-by: gushengxian <gushengxian@yulong.com> Is this detected by some script? Mind to share the script and run it against the whole btrfs code base? Thanks, Qu > --- > fs/btrfs/disk-io.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index b117dd3b8172..7e65a54b7839 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -4624,7 +4624,6 @@ static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans, > struct rb_node *node; > struct btrfs_delayed_ref_root *delayed_refs; > struct btrfs_delayed_ref_node *ref; > - int ret = 0; > > delayed_refs = &trans->delayed_refs; > > @@ -4632,7 +4631,7 @@ static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans, > if (atomic_read(&delayed_refs->num_entries) == 0) { > spin_unlock(&delayed_refs->lock); > btrfs_debug(fs_info, "delayed_refs has NO entry"); > - return ret; > + return 0; > } > > while ((node = rb_first_cached(&delayed_refs->href_root)) != NULL) { > @@ -4695,7 +4694,7 @@ static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans, > > spin_unlock(&delayed_refs->lock); > > - return ret; > + return 0; > } > > static void btrfs_destroy_delalloc_inodes(struct btrfs_root *root) > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] btrfs: remove unneeded variable: "ret" 2021-06-28 9:21 ` Qu Wenruo @ 2021-06-28 9:34 ` Damien Le Moal 2021-06-28 9:38 ` Johannes Thumshirn 0 siblings, 1 reply; 13+ messages in thread From: Damien Le Moal @ 2021-06-28 9:34 UTC (permalink / raw) To: Qu Wenruo, 13145886936, clm, josef, dsterba; +Cc: linux-btrfs, gushengxian On 2021/06/28 18:22, Qu Wenruo wrote: > > > On 2021/6/28 下午4:30, 13145886936@163.com wrote: >> From: gushengxian <gushengxian@yulong.com> >> >> Remove unneeded variable: "ret". >> >> Signed-off-by: gushengxian <13145886936@163.com> >> Signed-off-by: gushengxian <gushengxian@yulong.com> > > Is this detected by some script? > > Mind to share the script and run it against the whole btrfs code base? make M=fs/btrfs W=1 should work. With gcc, unused variable warnings show up only with W=1. clang is different I think. > > Thanks, > Qu > >> --- >> fs/btrfs/disk-io.c | 5 ++--- >> 1 file changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c >> index b117dd3b8172..7e65a54b7839 100644 >> --- a/fs/btrfs/disk-io.c >> +++ b/fs/btrfs/disk-io.c >> @@ -4624,7 +4624,6 @@ static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans, >> struct rb_node *node; >> struct btrfs_delayed_ref_root *delayed_refs; >> struct btrfs_delayed_ref_node *ref; >> - int ret = 0; >> >> delayed_refs = &trans->delayed_refs; >> >> @@ -4632,7 +4631,7 @@ static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans, >> if (atomic_read(&delayed_refs->num_entries) == 0) { >> spin_unlock(&delayed_refs->lock); >> btrfs_debug(fs_info, "delayed_refs has NO entry"); >> - return ret; >> + return 0; >> } >> >> while ((node = rb_first_cached(&delayed_refs->href_root)) != NULL) { >> @@ -4695,7 +4694,7 @@ static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans, >> >> spin_unlock(&delayed_refs->lock); >> >> - return ret; >> + return 0; >> } >> >> static void btrfs_destroy_delalloc_inodes(struct btrfs_root *root) >> > -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] btrfs: remove unneeded variable: "ret" 2021-06-28 9:34 ` Damien Le Moal @ 2021-06-28 9:38 ` Johannes Thumshirn 2021-06-28 9:44 ` Qu Wenruo 0 siblings, 1 reply; 13+ messages in thread From: Johannes Thumshirn @ 2021-06-28 9:38 UTC (permalink / raw) To: Damien Le Moal, Qu Wenruo, 13145886936, clm, josef, dsterba Cc: linux-btrfs, gushengxian On 28/06/2021 11:34, Damien Le Moal wrote: > On 2021/06/28 18:22, Qu Wenruo wrote: >> >> >> On 2021/6/28 下午4:30, 13145886936@163.com wrote: >>> From: gushengxian <gushengxian@yulong.com> >>> >>> Remove unneeded variable: "ret". >>> >>> Signed-off-by: gushengxian <13145886936@163.com> >>> Signed-off-by: gushengxian <gushengxian@yulong.com> >> >> Is this detected by some script? >> >> Mind to share the script and run it against the whole btrfs code base? > > make M=fs/btrfs W=1 > > should work. > > With gcc, unused variable warnings show up only with W=1. clang is different I > think. Nope doesn't work here, as the variable is actually used (but not needed at all). make M=fs/btrfs W=1 just prints some warnings about improper kernel-doc formatting. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] btrfs: remove unneeded variable: "ret" 2021-06-28 9:38 ` Johannes Thumshirn @ 2021-06-28 9:44 ` Qu Wenruo 2021-06-28 9:48 ` Damien Le Moal 0 siblings, 1 reply; 13+ messages in thread From: Qu Wenruo @ 2021-06-28 9:44 UTC (permalink / raw) To: Johannes Thumshirn, Damien Le Moal, 13145886936, clm, josef, dsterba Cc: linux-btrfs, gushengxian On 2021/6/28 下午5:38, Johannes Thumshirn wrote: > On 28/06/2021 11:34, Damien Le Moal wrote: >> On 2021/06/28 18:22, Qu Wenruo wrote: >>> >>> >>> On 2021/6/28 下午4:30, 13145886936@163.com wrote: >>>> From: gushengxian <gushengxian@yulong.com> >>>> >>>> Remove unneeded variable: "ret". >>>> >>>> Signed-off-by: gushengxian <13145886936@163.com> >>>> Signed-off-by: gushengxian <gushengxian@yulong.com> >>> >>> Is this detected by some script? >>> >>> Mind to share the script and run it against the whole btrfs code base? >> >> make M=fs/btrfs W=1 >> >> should work. >> >> With gcc, unused variable warnings show up only with W=1. clang is different I >> think. > > Nope doesn't work here, as the variable is actually used (but not needed at all). > > make M=fs/btrfs W=1 just prints some warnings about improper kernel-doc formatting. > Yeah, that's why I'm asking for the script, which could be way more valuable than all these small fixes. And, again a Chinese company doing the same tons of small fixes... So nothing could change their behaviors, no matter whatever... Thanks, Qu ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] btrfs: remove unneeded variable: "ret" 2021-06-28 9:44 ` Qu Wenruo @ 2021-06-28 9:48 ` Damien Le Moal 2021-06-28 9:52 ` Qu Wenruo 2021-06-28 12:04 ` David Sterba 0 siblings, 2 replies; 13+ messages in thread From: Damien Le Moal @ 2021-06-28 9:48 UTC (permalink / raw) To: Qu Wenruo, Johannes Thumshirn, 13145886936, clm, josef, dsterba Cc: linux-btrfs, gushengxian On 2021/06/28 18:45, Qu Wenruo wrote: > > > On 2021/6/28 下午5:38, Johannes Thumshirn wrote: >> On 28/06/2021 11:34, Damien Le Moal wrote: >>> On 2021/06/28 18:22, Qu Wenruo wrote: >>>> >>>> >>>> On 2021/6/28 下午4:30, 13145886936@163.com wrote: >>>>> From: gushengxian <gushengxian@yulong.com> >>>>> >>>>> Remove unneeded variable: "ret". >>>>> >>>>> Signed-off-by: gushengxian <13145886936@163.com> >>>>> Signed-off-by: gushengxian <gushengxian@yulong.com> >>>> >>>> Is this detected by some script? >>>> >>>> Mind to share the script and run it against the whole btrfs code base? >>> >>> make M=fs/btrfs W=1 >>> >>> should work. >>> >>> With gcc, unused variable warnings show up only with W=1. clang is different I >>> think. >> >> Nope doesn't work here, as the variable is actually used (but not needed at all). >> >> make M=fs/btrfs W=1 just prints some warnings about improper kernel-doc formatting. >> > > Yeah, that's why I'm asking for the script, which could be way more > valuable than all these small fixes. > > And, again a Chinese company doing the same tons of small fixes... > So nothing could change their behaviors, no matter whatever... Keep cool ! This one is actually a good fix :) Just did a make with gcc 11 and W=2 and this warning does not show up, but there are a lot more warnings about unused macros and some "variable may be used uninitialized" in the zone code... -> Johannes ? There are lots of warnings about ffs() and other core functions not in btrfs code though. -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] btrfs: remove unneeded variable: "ret" 2021-06-28 9:48 ` Damien Le Moal @ 2021-06-28 9:52 ` Qu Wenruo 2021-06-28 12:04 ` David Sterba 1 sibling, 0 replies; 13+ messages in thread From: Qu Wenruo @ 2021-06-28 9:52 UTC (permalink / raw) To: Damien Le Moal, Johannes Thumshirn, 13145886936, clm, josef, dsterba Cc: linux-btrfs, gushengxian On 2021/6/28 下午5:48, Damien Le Moal wrote: > On 2021/06/28 18:45, Qu Wenruo wrote: >> >> >> On 2021/6/28 下午5:38, Johannes Thumshirn wrote: >>> On 28/06/2021 11:34, Damien Le Moal wrote: >>>> On 2021/06/28 18:22, Qu Wenruo wrote: >>>>> >>>>> >>>>> On 2021/6/28 下午4:30, 13145886936@163.com wrote: >>>>>> From: gushengxian <gushengxian@yulong.com> >>>>>> >>>>>> Remove unneeded variable: "ret". >>>>>> >>>>>> Signed-off-by: gushengxian <13145886936@163.com> >>>>>> Signed-off-by: gushengxian <gushengxian@yulong.com> >>>>> >>>>> Is this detected by some script? >>>>> >>>>> Mind to share the script and run it against the whole btrfs code base? >>>> >>>> make M=fs/btrfs W=1 >>>> >>>> should work. >>>> >>>> With gcc, unused variable warnings show up only with W=1. clang is different I >>>> think. >>> >>> Nope doesn't work here, as the variable is actually used (but not needed at all). >>> >>> make M=fs/btrfs W=1 just prints some warnings about improper kernel-doc formatting. >>> >> >> Yeah, that's why I'm asking for the script, which could be way more >> valuable than all these small fixes. >> >> And, again a Chinese company doing the same tons of small fixes... >> So nothing could change their behaviors, no matter whatever... > > Keep cool ! This one is actually a good fix :) Yeah, that's why I'm not that exciting. > > Just did a make with gcc 11 and W=2 and this warning does not show up, but there > are a lot more warnings about unused macros and some "variable may be used > uninitialized" in the zone code... -> Johannes ? > > There are lots of warnings about ffs() and other core functions not in btrfs > code though. Hopes those guys see this comment and do better and more structured cleanup before any of us :) Thanks, Qu ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] btrfs: remove unneeded variable: "ret" 2021-06-28 9:48 ` Damien Le Moal 2021-06-28 9:52 ` Qu Wenruo @ 2021-06-28 12:04 ` David Sterba 2021-06-28 12:13 ` Damien Le Moal 2021-06-28 12:19 ` Qu Wenruo 1 sibling, 2 replies; 13+ messages in thread From: David Sterba @ 2021-06-28 12:04 UTC (permalink / raw) To: Damien Le Moal Cc: Qu Wenruo, Johannes Thumshirn, 13145886936, clm, josef, dsterba, linux-btrfs, gushengxian On Mon, Jun 28, 2021 at 09:48:40AM +0000, Damien Le Moal wrote: > This one is actually a good fix :) It's not a good fix and has been posted several times already. What needs to be done in this function is to propagate error codes from the whole call tree starting in that function. Removing code triggering a warning is perhaps the simplest thing to make the warning go away but the right fix needs some understanding of the function and context. The patch also comes without any explanation so that does not help to back the intentions to remove it. Reveiew of such path is "Please explain". Replying to patches attempting to fix the warning (and not the code) does not seem to help, it's just pointing to the previous iterations. Everybody is free to run checkers, find the warnings and send patches, that's fine and that's how open communities work. But in this case we'll probably have to put a note in code not to touch that particular line/variable. > Just did a make with gcc 11 and W=2 and this warning does not show up, but there > are a lot more warnings about unused macros and some "variable may be used > uninitialized" in the zone code... -> Johannes ? > > There are lots of warnings about ffs() and other core functions not in btrfs > code though. That the higher W= warning levels are too noisy and have to be filtered or specific issues fixed after manual selection. We've recently added a more fine grained list of warnings that would apply only in fs/btrfs, so if you find more that are worth fixing and then enabling by default, no problem. Some set-but-not used could be useful to point to code to analyze if it's not obscuring some bug but given that thre are lots of instances in the system includes we can't enable it. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] btrfs: remove unneeded variable: "ret" 2021-06-28 12:04 ` David Sterba @ 2021-06-28 12:13 ` Damien Le Moal 2021-06-28 12:19 ` Qu Wenruo 1 sibling, 0 replies; 13+ messages in thread From: Damien Le Moal @ 2021-06-28 12:13 UTC (permalink / raw) To: dsterba Cc: Qu Wenruo, Johannes Thumshirn, 13145886936, clm, josef, dsterba, linux-btrfs, gushengxian On 2021/06/28 21:07, David Sterba wrote: > On Mon, Jun 28, 2021 at 09:48:40AM +0000, Damien Le Moal wrote: >> This one is actually a good fix :) > > It's not a good fix and has been posted several times already. What > needs to be done in this function is to propagate error codes from the > whole call tree starting in that function. Removing code triggering a > warning is perhaps the simplest thing to make the warning go away but > the right fix needs some understanding of the function and context. Thanks for clarifying. I actually did not look into the details of the patch. From a 10,000 feet view, it did seem like something OK. Obviously, it is not :) > > The patch also comes without any explanation so that does not help to > back the intentions to remove it. Reveiew of such path is "Please > explain". > > Replying to patches attempting to fix the warning (and not the code) > does not seem to help, it's just pointing to the previous iterations. > > Everybody is free to run checkers, find the warnings and send patches, > that's fine and that's how open communities work. But in this case > we'll probably have to put a note in code not to touch that particular > line/variable. > >> Just did a make with gcc 11 and W=2 and this warning does not show up, but there >> are a lot more warnings about unused macros and some "variable may be used >> uninitialized" in the zone code... -> Johannes ? >> >> There are lots of warnings about ffs() and other core functions not in btrfs >> code though. > > That the higher W= warning levels are too noisy and have to be filtered > or specific issues fixed after manual selection. We've recently added a > more fine grained list of warnings that would apply only in fs/btrfs, so > if you find more that are worth fixing and then enabling by default, no > problem. > > Some set-but-not used could be useful to point to code to analyze if > it's not obscuring some bug but given that thre are lots of instances in > the system includes we can't enable it. Yes. Agree there. I personally use W=1 and W=2 only when compile testing patches before sending. Not having these enabled by default is fine with me. And since I just noticed the warnings related to zone code with W=2, I mentioned it. Most of the W=2 warnings for btrfs are clearly not related to btrfs itself. -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] btrfs: remove unneeded variable: "ret" 2021-06-28 12:04 ` David Sterba 2021-06-28 12:13 ` Damien Le Moal @ 2021-06-28 12:19 ` Qu Wenruo 2021-06-28 12:43 ` Nikolay Borisov 2021-06-28 12:51 ` David Sterba 1 sibling, 2 replies; 13+ messages in thread From: Qu Wenruo @ 2021-06-28 12:19 UTC (permalink / raw) To: dsterba, Damien Le Moal, Qu Wenruo, Johannes Thumshirn, 13145886936, clm, josef, dsterba, linux-btrfs, gushengxian On 2021/6/28 下午8:04, David Sterba wrote: > On Mon, Jun 28, 2021 at 09:48:40AM +0000, Damien Le Moal wrote: >> This one is actually a good fix :) > > It's not a good fix and has been posted several times already. What > needs to be done in this function is to propagate error codes from the > whole call tree starting in that function. Removing code triggering a > warning is perhaps the simplest thing to make the warning go away but > the right fix needs some understanding of the function and context. > > The patch also comes without any explanation so that does not help to > back the intentions to remove it. Reveiew of such path is "Please > explain". > > Replying to patches attempting to fix the warning (and not the code) > does not seem to help, it's just pointing to the previous iterations. But in this particular patch, it's really doing proper cleanup. The truth is, the whole function, btrfs_destroy_delayed_refs(), only get called when a transaction is aborted, and to cleanup all pending delayed refs. Thus in this particular function, there is nothing to return an error, since we're already erroring out. And in fact we should use void for btrfs_destroy_delayed_refs(). Thus opposite to my initial thought, it's in fact OK for the cleanup. Just one step left to change the function to return void. And obviously, with all these comment above added to commit message. If this is from some guy with years experience, I would definitely bark at him, but this is one really looks like a newbie, thus it's more or less acceptable. Thanks, Qu > > Everybody is free to run checkers, find the warnings and send patches, > that's fine and that's how open communities work. But in this case > we'll probably have to put a note in code not to touch that particular > line/variable. > >> Just did a make with gcc 11 and W=2 and this warning does not show up, but there >> are a lot more warnings about unused macros and some "variable may be used >> uninitialized" in the zone code... -> Johannes ? >> >> There are lots of warnings about ffs() and other core functions not in btrfs >> code though. > > That the higher W= warning levels are too noisy and have to be filtered > or specific issues fixed after manual selection. We've recently added a > more fine grained list of warnings that would apply only in fs/btrfs, so > if you find more that are worth fixing and then enabling by default, no > problem. > > Some set-but-not used could be useful to point to code to analyze if > it's not obscuring some bug but given that thre are lots of instances in > the system includes we can't enable it. > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] btrfs: remove unneeded variable: "ret" 2021-06-28 12:19 ` Qu Wenruo @ 2021-06-28 12:43 ` Nikolay Borisov 2021-06-28 12:51 ` David Sterba 1 sibling, 0 replies; 13+ messages in thread From: Nikolay Borisov @ 2021-06-28 12:43 UTC (permalink / raw) To: Qu Wenruo, dsterba, Damien Le Moal, Qu Wenruo, Johannes Thumshirn, 13145886936, clm, josef, dsterba, linux-btrfs, gushengxian <snip> > > If this is from some guy with years experience, I would definitely bark > at him, but this is one really looks like a newbie, thus it's more or > less acceptable. I beg to differ, if we want to have quality code we should be imposing standards and shouldn't be doing exception just because someone is a newbie. SO even if the patch is good per-se, because removing the ret is fine, missing comprehensible rationale of why this is the case constitutes a patch which falls short of said standards. In this case feedback should be given and if the person doesn't take a note and improve on his subsequent posting they should be ignored as this is actively wasting everyone's time. > > Thanks, > Qu <snip> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] btrfs: remove unneeded variable: "ret" 2021-06-28 12:19 ` Qu Wenruo 2021-06-28 12:43 ` Nikolay Borisov @ 2021-06-28 12:51 ` David Sterba 1 sibling, 0 replies; 13+ messages in thread From: David Sterba @ 2021-06-28 12:51 UTC (permalink / raw) To: Qu Wenruo Cc: dsterba, Damien Le Moal, Qu Wenruo, Johannes Thumshirn, 13145886936, clm, josef, dsterba, linux-btrfs, gushengxian On Mon, Jun 28, 2021 at 08:19:28PM +0800, Qu Wenruo wrote: > On 2021/6/28 下午8:04, David Sterba wrote: > > On Mon, Jun 28, 2021 at 09:48:40AM +0000, Damien Le Moal wrote: > >> This one is actually a good fix :) > > > > It's not a good fix and has been posted several times already. What > > needs to be done in this function is to propagate error codes from the > > whole call tree starting in that function. Removing code triggering a > > warning is perhaps the simplest thing to make the warning go away but > > the right fix needs some understanding of the function and context. > > > > The patch also comes without any explanation so that does not help to > > back the intentions to remove it. Reveiew of such path is "Please > > explain". > > > > Replying to patches attempting to fix the warning (and not the code) > > does not seem to help, it's just pointing to the previous iterations. > > But in this particular patch, it's really doing proper cleanup. > > The truth is, the whole function, btrfs_destroy_delayed_refs(), only get > called when a transaction is aborted, and to cleanup all pending delayed > refs. > > Thus in this particular function, there is nothing to return an error, > since we're already erroring out. > > And in fact we should use void for btrfs_destroy_delayed_refs(). > > Thus opposite to my initial thought, it's in fact OK for the cleanup. > Just one step left to change the function to return void. > > And obviously, with all these comment above added to commit message. The logic about making a function void and removing the return value has some assumptions: - no BUG/BUG_ON inside the function itself - all called functions are return-value clean and either handle everything transparently or return a value - the above applies recursively to the whole call chain btrfs_destroy_delayed_refs itself contains one BUG, so that needs to be fixed. The whole problem of making it void is in the pinned rage, see eg. btrfs_error_unpin_extent_range. Nikolay did some cleanups there but there are still some BUGs left in place of error handling, namely in unpin_extent_range and I did not check completely. I understand that it's tempting to just turn the function to void, because there's nothing obviously wrong, but that's only on first sight. A proper review must follow the code and once there's unhandled error deeper in the callchain, it has to be solved first. IIRC we've cleaned most of if not all the easy cases so it may not be that easy to identify something we haven't seen yet. I'll put something to the wiki in case people are interested. There's https://btrfs.wiki.kernel.org/index.php/Project_ideas#Cleanup_projects it hasn't been up to date but it is the place where to look for such things. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] btrfs: remove unneeded variable: "ret" 2021-06-28 8:30 [PATCH] btrfs: remove unneeded variable: "ret" 13145886936 2021-06-28 9:21 ` Qu Wenruo @ 2021-06-28 11:38 ` Qu Wenruo 1 sibling, 0 replies; 13+ messages in thread From: Qu Wenruo @ 2021-06-28 11:38 UTC (permalink / raw) To: 13145886936, linux-kernel, linux-btrfs, gushengxian Hi Gu, On 2021/6/28 下午4:30, 13145886936@163.com wrote: > From: gushengxian <gushengxian@yulong.com> > > Remove unneeded variable: "ret". Considering you're really a newbie doing new contribution to the kernel, I think it's better to give you some advice to encourage you do more contribution, while also be more professional, and hopefully to help other individuals in your company to contribute. And I hope this could also be some guide for new developers to learn a trick or two from. === WHEN NOT SURE, LEARN FROM OTHERS === First thing first, if you have something not sure, like how you should setup your name and email in your git config, try to find some merged patches to learn from them. === EMAIL ADDRESS === Firstly, your mail is sent from your 163 mail box, not your company mail box. Peter Zijlstra has already mentioned this in another patch. Normally if you want to send a patch to represent your company, it's common to send using a mail address of your company "@yulong.com" (Isn't it called Coolpad Group nowadays?) For how to configure git-send-email to use the SMTP server of your company, I guess your colleague Hu Yue <huyue2@yulong.com> has more experience and you can definitely learn from him. If you want to the patch to be CCed to your personal mail box, you can use "--cc" option of git-send-email, as most reviewer just reply-to-all, thus your personal mail box will definite get the comment. This will make the SOB line much cleaner. === MAIL LIST === This is not a big deal, just something optional but really helpful for your next contribution. In this particular patch, you only need to send the patch to btrfs mail list <linux-btrfs@vger.kernel.org>, even no need to CC the maintainers. LKML is fine for your first several patches to get more comments, like this one. But when you get settled to a certain field of kernel, it's better just to send the patch to the related mail list. === FOR THE CLEANUP === As I mentioned in another thread, if you use some automatic tool or script to expose the problem, that's fine. But it would be even better to provide the tool. Fix one small problem is OK, but fixing a type of problems is really what we want. If you really just found the bug by manually scanning the code, kudos to you. But if you do more contribution, one day your time will be too precious to be spent on things like this. Just like an old Chinese saying, give a man a fish, and you feed him for a day, teach a man to finish, and you feed him for a lifetime. And since the patch itself is fine. Reviewed-by: Qu Wenruo <wqu@suse.com> And really hope you can do more and better contribution to linux kernel. Thanks, Qu > > Signed-off-by: gushengxian <13145886936@163.com> > Signed-off-by: gushengxian <gushengxian@yulong.com> > --- > fs/btrfs/disk-io.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index b117dd3b8172..7e65a54b7839 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -4624,7 +4624,6 @@ static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans, > struct rb_node *node; > struct btrfs_delayed_ref_root *delayed_refs; > struct btrfs_delayed_ref_node *ref; > - int ret = 0; > > delayed_refs = &trans->delayed_refs; > > @@ -4632,7 +4631,7 @@ static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans, > if (atomic_read(&delayed_refs->num_entries) == 0) { > spin_unlock(&delayed_refs->lock); > btrfs_debug(fs_info, "delayed_refs has NO entry"); > - return ret; > + return 0; > } > > while ((node = rb_first_cached(&delayed_refs->href_root)) != NULL) { > @@ -4695,7 +4694,7 @@ static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans, > > spin_unlock(&delayed_refs->lock); > > - return ret; > + return 0; > } > > static void btrfs_destroy_delalloc_inodes(struct btrfs_root *root) > ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-06-28 12:54 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-28 8:30 [PATCH] btrfs: remove unneeded variable: "ret" 13145886936 2021-06-28 9:21 ` Qu Wenruo 2021-06-28 9:34 ` Damien Le Moal 2021-06-28 9:38 ` Johannes Thumshirn 2021-06-28 9:44 ` Qu Wenruo 2021-06-28 9:48 ` Damien Le Moal 2021-06-28 9:52 ` Qu Wenruo 2021-06-28 12:04 ` David Sterba 2021-06-28 12:13 ` Damien Le Moal 2021-06-28 12:19 ` Qu Wenruo 2021-06-28 12:43 ` Nikolay Borisov 2021-06-28 12:51 ` David Sterba 2021-06-28 11:38 ` Qu Wenruo
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.