| Message ID | cover.1734108739.git.beckerlee3@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | reduce boilerplate code within btrfs | expand |
在 2024/12/16 01:56, Roger L. Beckermeyer III 写道: > The goal of this patch series is to reduce boilerplate code > within btrfs. To accomplish this rb_find_add_cached() was added > to linux/include/rbtree.h. Any replaceable functions were then > replaced within btrfs. Since Peter has acknowledged the change in rbtree, the remaining part looks fine to me. The mentioned error handling bug will be fixed when I merge the series. Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu > > changelog: > updated if() statements to utilize newer error checking > resolved lock error on 0002 > edited title of patches to utilize update instead of edit > added Acked-by from Peter Zijlstra to 0001 > eliminated extra variables throughout the patch series > > Roger L. Beckermeyer III (6): > rbtree: add rb_find_add_cached() to rbtree.h > btrfs: update btrfs_add_block_group_cache() to use rb helper > btrfs: update prelim_ref_insert() to use rb helpers > btrfs: update __btrfs_add_delayed_item() to use rb helper > btrfs: update btrfs_add_chunk_map() to use rb helpers > btrfs: update tree_insert() to use rb helpers > > fs/btrfs/backref.c | 71 ++++++++++++++++++++-------------------- > fs/btrfs/block-group.c | 41 ++++++++++------------- > fs/btrfs/delayed-inode.c | 40 +++++++++------------- > fs/btrfs/delayed-ref.c | 39 +++++++++------------- > fs/btrfs/volumes.c | 39 ++++++++++------------ > include/linux/rbtree.h | 37 +++++++++++++++++++++ > 6 files changed, 141 insertions(+), 126 deletions(-) >
在 2024/12/17 07:50, Qu Wenruo 写道: > > > 在 2024/12/16 01:56, Roger L. Beckermeyer III 写道: >> The goal of this patch series is to reduce boilerplate code >> within btrfs. To accomplish this rb_find_add_cached() was added >> to linux/include/rbtree.h. Any replaceable functions were then >> replaced within btrfs. > > Since Peter has acknowledged the change in rbtree, the remaining part > looks fine to me. > > The mentioned error handling bug will be fixed when I merge the series. > > Reviewed-by: Qu Wenruo <wqu@suse.com> Well, during merge I found some extra things that you can improve in the future. - The length of each code line Although we no longer have the older strict 80 chars length limit, check_patch.pl will still warn about lines over 100 chars. Several patches triggered this. So please use check_patch.pl or just use btrfs workflow instead. - The incorrect drop of const prefix in the last patch Since the comparison function accepts one regular node and one const node, the last patch drops the second const prefix, mostly due to the factg that comp_refs() doesn't have const prefix at all for both parameters. The proper fix is to add const prefixes for involved functions, not dropping the existing const prefixes. I have also make all internal structure inside those helpers to be const. (Personally speaking I also want to check if the less() and cmp() can be converted to accept both parameters as const in the future) - Upper case for the first letter of a sentence I'm not good at English either, but at least for the commit message, the first letter of a sentence should be in upper case. - Minor code style problems IIRC others have already address the problem like: int result; result = some_function(); return result; Which can be done by a simple "return some_function();". Thanks, Qu > > Thanks, > Qu >> >> changelog: >> updated if() statements to utilize newer error checking >> resolved lock error on 0002 >> edited title of patches to utilize update instead of edit >> added Acked-by from Peter Zijlstra to 0001 >> eliminated extra variables throughout the patch series >> >> Roger L. Beckermeyer III (6): >> rbtree: add rb_find_add_cached() to rbtree.h >> btrfs: update btrfs_add_block_group_cache() to use rb helper >> btrfs: update prelim_ref_insert() to use rb helpers >> btrfs: update __btrfs_add_delayed_item() to use rb helper >> btrfs: update btrfs_add_chunk_map() to use rb helpers >> btrfs: update tree_insert() to use rb helpers >> >> fs/btrfs/backref.c | 71 ++++++++++++++++++++-------------------- >> fs/btrfs/block-group.c | 41 ++++++++++------------- >> fs/btrfs/delayed-inode.c | 40 +++++++++------------- >> fs/btrfs/delayed-ref.c | 39 +++++++++------------- >> fs/btrfs/volumes.c | 39 ++++++++++------------ >> include/linux/rbtree.h | 37 +++++++++++++++++++++ >> 6 files changed, 141 insertions(+), 126 deletions(-) >> > >
On Mon, Dec 16, 2024 at 10:07 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > > > > 在 2024/12/17 07:50, Qu Wenruo 写道: > > > > > > 在 2024/12/16 01:56, Roger L. Beckermeyer III 写道: > >> The goal of this patch series is to reduce boilerplate code > >> within btrfs. To accomplish this rb_find_add_cached() was added > >> to linux/include/rbtree.h. Any replaceable functions were then > >> replaced within btrfs. > > > > Since Peter has acknowledged the change in rbtree, the remaining part > > looks fine to me. > > > > The mentioned error handling bug will be fixed when I merge the series. > > > > Reviewed-by: Qu Wenruo <wqu@suse.com> > > Well, during merge I found some extra things that you can improve in the > future. One more thing to improve in the future: - Run fstests and check that that are no tests failing after the changes. There's at least 1 test case failing after this patchset. The patch "btrfs: update prelim_ref_insert() to use rb helpers" breaks btrfs/287: $ ./check btrfs/287 FSTYP -- btrfs PLATFORM -- Linux/x86_64 debian0 6.13.0-rc2-btrfs-next-182+ #2 SMP PREEMPT_DYNAMIC Tue Dec 17 11:02:25 WET 2024 MKFS_OPTIONS -- /dev/sdc MOUNT_OPTIONS -- /dev/sdc /home/fdmanana/btrfs-tests/scratch_1 btrfs/287 1s ... - output mismatch (see /home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad) --- tests/btrfs/287.out 2024-10-30 07:42:47.901514035 +0000 +++ /home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad 2024-12-17 15:00:35.341110069 +0000 @@ -8,82 +8,82 @@ linked 8388608/8388608 bytes at offset 16777216 XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) resolve first extent: -inode 257 offset 16777216 root 5 -inode 257 offset 8388608 root 5 inode 257 offset 0 root 5 +inode 257 offset 8388608 root 5 ... (Run 'diff -u /home/fdmanana/git/hub/xfstests/tests/btrfs/287.out /home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad' to see the entire diff) HINT: You _MAY_ be missing kernel fix: 0cad8f14d70c btrfs: fix backref walking not returning all inode refs Ran: btrfs/287 Failures: btrfs/287 Failed 1 of 1 tests So please fix the patch (or patches) and resend the updated version once fixed. Meanwhile it should be dropped from for-next. Thanks. > > - The length of each code line > Although we no longer have the older strict 80 chars length limit, > check_patch.pl will still warn about lines over 100 chars. > Several patches triggered this. > > So please use check_patch.pl or just use btrfs workflow instead. > > - The incorrect drop of const prefix in the last patch > Since the comparison function accepts one regular node and one const > node, the last patch drops the second const prefix, mostly due to > the factg that comp_refs() doesn't have const prefix at all for both > parameters. > > The proper fix is to add const prefixes for involved functions, not > dropping the existing const prefixes. > > I have also make all internal structure inside those helpers to be > const. > (Personally speaking I also want to check if the less() and cmp() can > be converted to accept both parameters as const in the future) > > - Upper case for the first letter of a sentence > I'm not good at English either, but at least for the commit message, > the first letter of a sentence should be in upper case. > > - Minor code style problems > IIRC others have already address the problem like: > > int result; > > result = some_function(); > return result; > > Which can be done by a simple "return some_function();". > > Thanks, > Qu > > > > > Thanks, > > Qu > >> > >> changelog: > >> updated if() statements to utilize newer error checking > >> resolved lock error on 0002 > >> edited title of patches to utilize update instead of edit > >> added Acked-by from Peter Zijlstra to 0001 > >> eliminated extra variables throughout the patch series > >> > >> Roger L. Beckermeyer III (6): > >> rbtree: add rb_find_add_cached() to rbtree.h > >> btrfs: update btrfs_add_block_group_cache() to use rb helper > >> btrfs: update prelim_ref_insert() to use rb helpers > >> btrfs: update __btrfs_add_delayed_item() to use rb helper > >> btrfs: update btrfs_add_chunk_map() to use rb helpers > >> btrfs: update tree_insert() to use rb helpers > >> > >> fs/btrfs/backref.c | 71 ++++++++++++++++++++-------------------- > >> fs/btrfs/block-group.c | 41 ++++++++++------------- > >> fs/btrfs/delayed-inode.c | 40 +++++++++------------- > >> fs/btrfs/delayed-ref.c | 39 +++++++++------------- > >> fs/btrfs/volumes.c | 39 ++++++++++------------ > >> include/linux/rbtree.h | 37 +++++++++++++++++++++ > >> 6 files changed, 141 insertions(+), 126 deletions(-) > >> > > > > > >
On Tue, Dec 17, 2024 at 9:09 AM Filipe Manana <fdmanana@kernel.org> wrote: > > On Mon, Dec 16, 2024 at 10:07 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > > > > > > > > 在 2024/12/17 07:50, Qu Wenruo 写道: > > > > > > > > > 在 2024/12/16 01:56, Roger L. Beckermeyer III 写道: > > >> The goal of this patch series is to reduce boilerplate code > > >> within btrfs. To accomplish this rb_find_add_cached() was added > > >> to linux/include/rbtree.h. Any replaceable functions were then > > >> replaced within btrfs. > > > > > > Since Peter has acknowledged the change in rbtree, the remaining part > > > looks fine to me. > > > > > > The mentioned error handling bug will be fixed when I merge the series. > > > > > > Reviewed-by: Qu Wenruo <wqu@suse.com> > > > > Well, during merge I found some extra things that you can improve in the > > future. > > One more thing to improve in the future: > > - Run fstests and check that that are no tests failing after the changes. > > There's at least 1 test case failing after this patchset. > The patch "btrfs: update prelim_ref_insert() to use rb helpers" breaks > btrfs/287: > > $ ./check btrfs/287 > FSTYP -- btrfs > PLATFORM -- Linux/x86_64 debian0 6.13.0-rc2-btrfs-next-182+ #2 > SMP PREEMPT_DYNAMIC Tue Dec 17 11:02:25 WET 2024 > MKFS_OPTIONS -- /dev/sdc > MOUNT_OPTIONS -- /dev/sdc /home/fdmanana/btrfs-tests/scratch_1 > > btrfs/287 1s ... - output mismatch (see > /home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad) > --- tests/btrfs/287.out 2024-10-30 07:42:47.901514035 +0000 > +++ /home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad > 2024-12-17 15:00:35.341110069 +0000 > @@ -8,82 +8,82 @@ > linked 8388608/8388608 bytes at offset 16777216 > XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > resolve first extent: > -inode 257 offset 16777216 root 5 > -inode 257 offset 8388608 root 5 > inode 257 offset 0 root 5 > +inode 257 offset 8388608 root 5 > ... > (Run 'diff -u /home/fdmanana/git/hub/xfstests/tests/btrfs/287.out > /home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad' to see > the entire diff) > > HINT: You _MAY_ be missing kernel fix: > 0cad8f14d70c btrfs: fix backref walking not returning all inode refs > > Ran: btrfs/287 > Failures: btrfs/287 > Failed 1 of 1 tests > > So please fix the patch (or patches) and resend the updated version once fixed. > Meanwhile it should be dropped from for-next. > > Thanks. > Found the problem, it's in prelim_ref_cmp in fs/btrfs/backref.c, if you invert the comparison between the parent and node it passes the test. e.g. prelim_ref_compare(ref2, ref1); instead of prelim_ref_compare(ref1, ref2); I can dig into it but I've got a couple of other things in the queue right now so it might be a little bit. prelim_ref_cmp() could do with a logic rework as well as prelim_ref_compare() is only used in 2 places within backref.c. It's just outside the scope of this patch series so I didn't dig too deep into it. > > > > > - The length of each code line > > Although we no longer have the older strict 80 chars length limit, > > check_patch.pl will still warn about lines over 100 chars. > > Several patches triggered this. > > > > So please use check_patch.pl or just use btrfs workflow instead. > > yee, I haven't built a really good workflow for kernel submissions yet. > > - The incorrect drop of const prefix in the last patch > > Since the comparison function accepts one regular node and one const > > node, the last patch drops the second const prefix, mostly due to > > the factg that comp_refs() doesn't have const prefix at all for both > > parameters. > > > > The proper fix is to add const prefixes for involved functions, not > > dropping the existing const prefixes. > > Okay, however, what do const keywords do that would require this habit? There's also a place in backref.c where I didn't implement const keywords. > > I have also make all internal structure inside those helpers to be > > const. > > (Personally speaking I also want to check if the less() and cmp() can > > be converted to accept both parameters as const in the future) > > > > - Upper case for the first letter of a sentence > > I'm not good at English either, but at least for the commit message, > > the first letter of a sentence should be in upper case. Gotcha, I can work on rewording those descriptions, just didn't seem very important compared to validating everything works properly. > > > > - Minor code style problems > > IIRC others have already address the problem like: > > > > int result; > > > > result = some_function(); > > return result; > > > > Which can be done by a simple "return some_function();". Where was this? I probably missed one when I went through these, sorry. Just to summarize, would you like me to resend the patch series with the below changes? - updated prelim_ref_compare to eliminate comparison error - rescanned with scripts/checkpatch.pl - whatever needs to be done with consts. Thanks for your time! Lee > > > > Thanks, > > Qu > > > > > > > > Thanks, > > > Qu > > >> > > >> changelog: > > >> updated if() statements to utilize newer error checking > > >> resolved lock error on 0002 > > >> edited title of patches to utilize update instead of edit > > >> added Acked-by from Peter Zijlstra to 0001 > > >> eliminated extra variables throughout the patch series > > >> > > >> Roger L. Beckermeyer III (6): > > >> rbtree: add rb_find_add_cached() to rbtree.h > > >> btrfs: update btrfs_add_block_group_cache() to use rb helper > > >> btrfs: update prelim_ref_insert() to use rb helpers > > >> btrfs: update __btrfs_add_delayed_item() to use rb helper > > >> btrfs: update btrfs_add_chunk_map() to use rb helpers > > >> btrfs: update tree_insert() to use rb helpers > > >> > > >> fs/btrfs/backref.c | 71 ++++++++++++++++++++-------------------- > > >> fs/btrfs/block-group.c | 41 ++++++++++------------- > > >> fs/btrfs/delayed-inode.c | 40 +++++++++------------- > > >> fs/btrfs/delayed-ref.c | 39 +++++++++------------- > > >> fs/btrfs/volumes.c | 39 ++++++++++------------ > > >> include/linux/rbtree.h | 37 +++++++++++++++++++++ > > >> 6 files changed, 141 insertions(+), 126 deletions(-) > > >> > > > > > > > > > >
在 2024/12/18 01:39, Filipe Manana 写道: > On Mon, Dec 16, 2024 at 10:07 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: >> >> >> >> 在 2024/12/17 07:50, Qu Wenruo 写道: >>> >>> >>> 在 2024/12/16 01:56, Roger L. Beckermeyer III 写道: >>>> The goal of this patch series is to reduce boilerplate code >>>> within btrfs. To accomplish this rb_find_add_cached() was added >>>> to linux/include/rbtree.h. Any replaceable functions were then >>>> replaced within btrfs. >>> >>> Since Peter has acknowledged the change in rbtree, the remaining part >>> looks fine to me. >>> >>> The mentioned error handling bug will be fixed when I merge the series. >>> >>> Reviewed-by: Qu Wenruo <wqu@suse.com> >> >> Well, during merge I found some extra things that you can improve in the >> future. > > One more thing to improve in the future: > > - Run fstests and check that that are no tests failing after the changes. > > There's at least 1 test case failing after this patchset. > The patch "btrfs: update prelim_ref_insert() to use rb helpers" breaks > btrfs/287: > > $ ./check btrfs/287 > FSTYP -- btrfs > PLATFORM -- Linux/x86_64 debian0 6.13.0-rc2-btrfs-next-182+ #2 > SMP PREEMPT_DYNAMIC Tue Dec 17 11:02:25 WET 2024 > MKFS_OPTIONS -- /dev/sdc > MOUNT_OPTIONS -- /dev/sdc /home/fdmanana/btrfs-tests/scratch_1 > > btrfs/287 1s ... - output mismatch (see > /home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad) > --- tests/btrfs/287.out 2024-10-30 07:42:47.901514035 +0000 > +++ /home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad > 2024-12-17 15:00:35.341110069 +0000 > @@ -8,82 +8,82 @@ > linked 8388608/8388608 bytes at offset 16777216 > XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > resolve first extent: > -inode 257 offset 16777216 root 5 > -inode 257 offset 8388608 root 5 > inode 257 offset 0 root 5 > +inode 257 offset 8388608 root 5 > ... > (Run 'diff -u /home/fdmanana/git/hub/xfstests/tests/btrfs/287.out > /home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad' to see > the entire diff) > > HINT: You _MAY_ be missing kernel fix: > 0cad8f14d70c btrfs: fix backref walking not returning all inode refs > > Ran: btrfs/287 > Failures: btrfs/287 > Failed 1 of 1 tests I thought the failure is caused by the missing fix, unfortunately I didn't notice the fix is already merged upstream. > > So please fix the patch (or patches) and resend the updated version once fixed. > Meanwhile it should be dropped from for-next. Now reverted. Thanks, Qu > > Thanks. > > >> >> - The length of each code line >> Although we no longer have the older strict 80 chars length limit, >> check_patch.pl will still warn about lines over 100 chars. >> Several patches triggered this. >> >> So please use check_patch.pl or just use btrfs workflow instead. >> >> - The incorrect drop of const prefix in the last patch >> Since the comparison function accepts one regular node and one const >> node, the last patch drops the second const prefix, mostly due to >> the factg that comp_refs() doesn't have const prefix at all for both >> parameters. >> >> The proper fix is to add const prefixes for involved functions, not >> dropping the existing const prefixes. >> >> I have also make all internal structure inside those helpers to be >> const. >> (Personally speaking I also want to check if the less() and cmp() can >> be converted to accept both parameters as const in the future) >> >> - Upper case for the first letter of a sentence >> I'm not good at English either, but at least for the commit message, >> the first letter of a sentence should be in upper case. >> >> - Minor code style problems >> IIRC others have already address the problem like: >> >> int result; >> >> result = some_function(); >> return result; >> >> Which can be done by a simple "return some_function();". >> >> Thanks, >> Qu >> >>> >>> Thanks, >>> Qu >>>> >>>> changelog: >>>> updated if() statements to utilize newer error checking >>>> resolved lock error on 0002 >>>> edited title of patches to utilize update instead of edit >>>> added Acked-by from Peter Zijlstra to 0001 >>>> eliminated extra variables throughout the patch series >>>> >>>> Roger L. Beckermeyer III (6): >>>> rbtree: add rb_find_add_cached() to rbtree.h >>>> btrfs: update btrfs_add_block_group_cache() to use rb helper >>>> btrfs: update prelim_ref_insert() to use rb helpers >>>> btrfs: update __btrfs_add_delayed_item() to use rb helper >>>> btrfs: update btrfs_add_chunk_map() to use rb helpers >>>> btrfs: update tree_insert() to use rb helpers >>>> >>>> fs/btrfs/backref.c | 71 ++++++++++++++++++++-------------------- >>>> fs/btrfs/block-group.c | 41 ++++++++++------------- >>>> fs/btrfs/delayed-inode.c | 40 +++++++++------------- >>>> fs/btrfs/delayed-ref.c | 39 +++++++++------------- >>>> fs/btrfs/volumes.c | 39 ++++++++++------------ >>>> include/linux/rbtree.h | 37 +++++++++++++++++++++ >>>> 6 files changed, 141 insertions(+), 126 deletions(-) >>>> >>> >>> >> >>
在 2024/12/18 07:06, Lee Beckermeyer 写道: > On Tue, Dec 17, 2024 at 9:09 AM Filipe Manana <fdmanana@kernel.org> wrote: >> >> On Mon, Dec 16, 2024 at 10:07 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: >>> >>> >>> >>> 在 2024/12/17 07:50, Qu Wenruo 写道: >>>> >>>> >>>> 在 2024/12/16 01:56, Roger L. Beckermeyer III 写道: >>>>> The goal of this patch series is to reduce boilerplate code >>>>> within btrfs. To accomplish this rb_find_add_cached() was added >>>>> to linux/include/rbtree.h. Any replaceable functions were then >>>>> replaced within btrfs. >>>> >>>> Since Peter has acknowledged the change in rbtree, the remaining part >>>> looks fine to me. >>>> >>>> The mentioned error handling bug will be fixed when I merge the series. >>>> >>>> Reviewed-by: Qu Wenruo <wqu@suse.com> >>> >>> Well, during merge I found some extra things that you can improve in the >>> future. >> >> One more thing to improve in the future: >> >> - Run fstests and check that that are no tests failing after the changes. >> >> There's at least 1 test case failing after this patchset. >> The patch "btrfs: update prelim_ref_insert() to use rb helpers" breaks >> btrfs/287: >> >> $ ./check btrfs/287 >> FSTYP -- btrfs >> PLATFORM -- Linux/x86_64 debian0 6.13.0-rc2-btrfs-next-182+ #2 >> SMP PREEMPT_DYNAMIC Tue Dec 17 11:02:25 WET 2024 >> MKFS_OPTIONS -- /dev/sdc >> MOUNT_OPTIONS -- /dev/sdc /home/fdmanana/btrfs-tests/scratch_1 >> >> btrfs/287 1s ... - output mismatch (see >> /home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad) >> --- tests/btrfs/287.out 2024-10-30 07:42:47.901514035 +0000 >> +++ /home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad >> 2024-12-17 15:00:35.341110069 +0000 >> @@ -8,82 +8,82 @@ >> linked 8388608/8388608 bytes at offset 16777216 >> XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) >> resolve first extent: >> -inode 257 offset 16777216 root 5 >> -inode 257 offset 8388608 root 5 >> inode 257 offset 0 root 5 >> +inode 257 offset 8388608 root 5 >> ... >> (Run 'diff -u /home/fdmanana/git/hub/xfstests/tests/btrfs/287.out >> /home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad' to see >> the entire diff) >> >> HINT: You _MAY_ be missing kernel fix: >> 0cad8f14d70c btrfs: fix backref walking not returning all inode refs >> >> Ran: btrfs/287 >> Failures: btrfs/287 >> Failed 1 of 1 tests >> >> So please fix the patch (or patches) and resend the updated version once fixed. >> Meanwhile it should be dropped from for-next. >> >> Thanks. >> > Found the problem, it's in prelim_ref_cmp in fs/btrfs/backref.c, if > you invert the comparison between the parent and node it passes the > test. e.g. prelim_ref_compare(ref2, ref1); instead of > prelim_ref_compare(ref1, ref2); > > I can dig into it but I've got a couple of other things in the queue > right now so it might be a little bit. prelim_ref_cmp() could do with > a logic rework as well as prelim_ref_compare() is only used in 2 > places within backref.c. It's just outside the scope of this patch > series so I didn't dig too deep into it. It's not the parameter order, it's the problem related to the parent/ref usage. Previously during the insert we update the parent/ref pointer during the search, but it's no longer the case, thus the whole if (exist) {} branch is wrong. >> >>> >>> - The length of each code line >>> Although we no longer have the older strict 80 chars length limit, >>> check_patch.pl will still warn about lines over 100 chars. >>> Several patches triggered this. >>> >>> So please use check_patch.pl or just use btrfs workflow instead. >>> > yee, I haven't built a really good workflow for kernel submissions yet. >>> - The incorrect drop of const prefix in the last patch >>> Since the comparison function accepts one regular node and one const >>> node, the last patch drops the second const prefix, mostly due to >>> the factg that comp_refs() doesn't have const prefix at all for both >>> parameters. >>> >>> The proper fix is to add const prefixes for involved functions, not >>> dropping the existing const prefixes. >>> > Okay, however, what do const keywords do that would require this > habit? There's also a place in backref.c where I didn't implement > const keywords. >>> I have also make all internal structure inside those helpers to be >>> const. >>> (Personally speaking I also want to check if the less() and cmp() can >>> be converted to accept both parameters as const in the future) >>> >>> - Upper case for the first letter of a sentence >>> I'm not good at English either, but at least for the commit message, >>> the first letter of a sentence should be in upper case. > Gotcha, I can work on rewording those descriptions, just didn't seem > very important compared to validating everything works properly. >>> >>> - Minor code style problems >>> IIRC others have already address the problem like: >>> >>> int result; >>> >>> result = some_function(); >>> return result; >>> >>> Which can be done by a simple "return some_function();". > Where was this? I probably missed one when I went through these, sorry. > > Just to summarize, would you like me to resend the patch series with > the below changes? > - updated prelim_ref_compare to eliminate comparison error > - rescanned with scripts/checkpatch.pl > - whatever needs to be done with consts. I'll handle the error fix. Since I have already changed the series a lot, your update will get all the existing modification lost. Thanks, Qu > > Thanks for your time! > Lee >>> >>> Thanks, >>> Qu >>> >>>> >>>> Thanks, >>>> Qu >>>>> >>>>> changelog: >>>>> updated if() statements to utilize newer error checking >>>>> resolved lock error on 0002 >>>>> edited title of patches to utilize update instead of edit >>>>> added Acked-by from Peter Zijlstra to 0001 >>>>> eliminated extra variables throughout the patch series >>>>> >>>>> Roger L. Beckermeyer III (6): >>>>> rbtree: add rb_find_add_cached() to rbtree.h >>>>> btrfs: update btrfs_add_block_group_cache() to use rb helper >>>>> btrfs: update prelim_ref_insert() to use rb helpers >>>>> btrfs: update __btrfs_add_delayed_item() to use rb helper >>>>> btrfs: update btrfs_add_chunk_map() to use rb helpers >>>>> btrfs: update tree_insert() to use rb helpers >>>>> >>>>> fs/btrfs/backref.c | 71 ++++++++++++++++++++-------------------- >>>>> fs/btrfs/block-group.c | 41 ++++++++++------------- >>>>> fs/btrfs/delayed-inode.c | 40 +++++++++------------- >>>>> fs/btrfs/delayed-ref.c | 39 +++++++++------------- >>>>> fs/btrfs/volumes.c | 39 ++++++++++------------ >>>>> include/linux/rbtree.h | 37 +++++++++++++++++++++ >>>>> 6 files changed, 141 insertions(+), 126 deletions(-) >>>>> >>>> >>>> >>> >>>
On Tue, Dec 17, 2024 at 2:49 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > > > > 在 2024/12/18 07:06, Lee Beckermeyer 写道: > > On Tue, Dec 17, 2024 at 9:09 AM Filipe Manana <fdmanana@kernel.org> wrote: > >> > >> On Mon, Dec 16, 2024 at 10:07 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > >>> > >>> > >>> > >>> 在 2024/12/17 07:50, Qu Wenruo 写道: > >>>> > >>>> > >>>> 在 2024/12/16 01:56, Roger L. Beckermeyer III 写道: > >>>>> The goal of this patch series is to reduce boilerplate code > >>>>> within btrfs. To accomplish this rb_find_add_cached() was added > >>>>> to linux/include/rbtree.h. Any replaceable functions were then > >>>>> replaced within btrfs. > >>>> > >>>> Since Peter has acknowledged the change in rbtree, the remaining part > >>>> looks fine to me. > >>>> > >>>> The mentioned error handling bug will be fixed when I merge the series. > >>>> > >>>> Reviewed-by: Qu Wenruo <wqu@suse.com> > >>> > >>> Well, during merge I found some extra things that you can improve in the > >>> future. > >> > >> One more thing to improve in the future: > >> > >> - Run fstests and check that that are no tests failing after the changes. > >> > >> There's at least 1 test case failing after this patchset. > >> The patch "btrfs: update prelim_ref_insert() to use rb helpers" breaks > >> btrfs/287: > >> > >> $ ./check btrfs/287 > >> FSTYP -- btrfs > >> PLATFORM -- Linux/x86_64 debian0 6.13.0-rc2-btrfs-next-182+ #2 > >> SMP PREEMPT_DYNAMIC Tue Dec 17 11:02:25 WET 2024 > >> MKFS_OPTIONS -- /dev/sdc > >> MOUNT_OPTIONS -- /dev/sdc /home/fdmanana/btrfs-tests/scratch_1 > >> > >> btrfs/287 1s ... - output mismatch (see > >> /home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad) > >> --- tests/btrfs/287.out 2024-10-30 07:42:47.901514035 +0000 > >> +++ /home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad > >> 2024-12-17 15:00:35.341110069 +0000 > >> @@ -8,82 +8,82 @@ > >> linked 8388608/8388608 bytes at offset 16777216 > >> XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > >> resolve first extent: > >> -inode 257 offset 16777216 root 5 > >> -inode 257 offset 8388608 root 5 > >> inode 257 offset 0 root 5 > >> +inode 257 offset 8388608 root 5 > >> ... > >> (Run 'diff -u /home/fdmanana/git/hub/xfstests/tests/btrfs/287.out > >> /home/fdmanana/git/hub/xfstests/results//btrfs/287.out.bad' to see > >> the entire diff) > >> > >> HINT: You _MAY_ be missing kernel fix: > >> 0cad8f14d70c btrfs: fix backref walking not returning all inode refs > >> > >> Ran: btrfs/287 > >> Failures: btrfs/287 > >> Failed 1 of 1 tests > >> > >> So please fix the patch (or patches) and resend the updated version once fixed. > >> Meanwhile it should be dropped from for-next. > >> > >> Thanks. > >> > > Found the problem, it's in prelim_ref_cmp in fs/btrfs/backref.c, if > > you invert the comparison between the parent and node it passes the > > test. e.g. prelim_ref_compare(ref2, ref1); instead of > > prelim_ref_compare(ref1, ref2); > > > > I can dig into it but I've got a couple of other things in the queue > > right now so it might be a little bit. prelim_ref_cmp() could do with > > a logic rework as well as prelim_ref_compare() is only used in 2 > > places within backref.c. It's just outside the scope of this patch > > series so I didn't dig too deep into it. > > It's not the parameter order, it's the problem related to the parent/ref > usage. > > Previously during the insert we update the parent/ref pointer during the > search, but it's no longer the case, thus the whole if (exist) {} branch > is wrong. > > >> > >>> > >>> - The length of each code line > >>> Although we no longer have the older strict 80 chars length limit, > >>> check_patch.pl will still warn about lines over 100 chars. > >>> Several patches triggered this. > >>> > >>> So please use check_patch.pl or just use btrfs workflow instead. > >>> > > yee, I haven't built a really good workflow for kernel submissions yet. > >>> - The incorrect drop of const prefix in the last patch > >>> Since the comparison function accepts one regular node and one const > >>> node, the last patch drops the second const prefix, mostly due to > >>> the factg that comp_refs() doesn't have const prefix at all for both > >>> parameters. > >>> > >>> The proper fix is to add const prefixes for involved functions, not > >>> dropping the existing const prefixes. > >>> > > Okay, however, what do const keywords do that would require this > > habit? There's also a place in backref.c where I didn't implement > > const keywords. > >>> I have also make all internal structure inside those helpers to be > >>> const. > >>> (Personally speaking I also want to check if the less() and cmp() can > >>> be converted to accept both parameters as const in the future) > >>> > >>> - Upper case for the first letter of a sentence > >>> I'm not good at English either, but at least for the commit message, > >>> the first letter of a sentence should be in upper case. > > Gotcha, I can work on rewording those descriptions, just didn't seem > > very important compared to validating everything works properly. > >>> > >>> - Minor code style problems > >>> IIRC others have already address the problem like: > >>> > >>> int result; > >>> > >>> result = some_function(); > >>> return result; > >>> > >>> Which can be done by a simple "return some_function();". > > Where was this? I probably missed one when I went through these, sorry. > > > > Just to summarize, would you like me to resend the patch series with > > the below changes? > > - updated prelim_ref_compare to eliminate comparison error > > - rescanned with scripts/checkpatch.pl > > - whatever needs to be done with consts. > > I'll handle the error fix. > > Since I have already changed the series a lot, your update will get all > the existing modification lost. > > Thanks, > Qu Sounds good, let me know if you need anything else on this patch series from me then. Thanks, Lee > > > > > Thanks for your time! > > Lee > >>> > >>> Thanks, > >>> Qu > >>> > >>>> > >>>> Thanks, > >>>> Qu > >>>>> > >>>>> changelog: > >>>>> updated if() statements to utilize newer error checking > >>>>> resolved lock error on 0002 > >>>>> edited title of patches to utilize update instead of edit > >>>>> added Acked-by from Peter Zijlstra to 0001 > >>>>> eliminated extra variables throughout the patch series > >>>>> > >>>>> Roger L. Beckermeyer III (6): > >>>>> rbtree: add rb_find_add_cached() to rbtree.h > >>>>> btrfs: update btrfs_add_block_group_cache() to use rb helper > >>>>> btrfs: update prelim_ref_insert() to use rb helpers > >>>>> btrfs: update __btrfs_add_delayed_item() to use rb helper > >>>>> btrfs: update btrfs_add_chunk_map() to use rb helpers > >>>>> btrfs: update tree_insert() to use rb helpers > >>>>> > >>>>> fs/btrfs/backref.c | 71 ++++++++++++++++++++-------------------- > >>>>> fs/btrfs/block-group.c | 41 ++++++++++------------- > >>>>> fs/btrfs/delayed-inode.c | 40 +++++++++------------- > >>>>> fs/btrfs/delayed-ref.c | 39 +++++++++------------- > >>>>> fs/btrfs/volumes.c | 39 ++++++++++------------ > >>>>> include/linux/rbtree.h | 37 +++++++++++++++++++++ > >>>>> 6 files changed, 141 insertions(+), 126 deletions(-) > >>>>> > >>>> > >>>> > >>> > >>> >