| Message ID | 20210202162837.129631-1-viniciustinti@gmail.com |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | [v3] ext4: Enable code path when DX_DEBUG is set | expand |
On Tue, Feb 02, 2021 at 04:28:37PM +0000, Vinicius Tinti wrote: > Clang with -Wunreachable-code-aggressive is being used to try to find > unreachable code that could cause potential bugs. There is no plan to > enable it by default. > > The following code was detected as unreachable: > > fs/ext4/namei.c:831:17: warning: code will never be executed > [-Wunreachable-code] > unsigned n = count - 1; > ^~~~~ > fs/ext4/namei.c:830:7: note: silence by adding parentheses to mark code as > explicitly dead > if (0) { // linear search cross check > ^ > /* DISABLES CODE */ ( ) > > This has been present since commit ac27a0ec112a ("[PATCH] ext4: initial > copy of files from ext3") and fs/ext3 had it present at the beginning of > git history. It has not been changed since. > > This patch moves the code to a new function `htree_rep_invariant_check` > which only performs the check when DX_DEBUG is set. This allows the > function to be used in other parts of the code. > > Suggestions from: Andreas, Christoph, Nathan, Nick and Ted. > > Signed-off-by: Vinicius Tinti <viniciustinti@gmail.com> Thanks, applied, although I rewrote the commit description: ext4: factor out htree rep invariant check This patch moves some debugging code which is used to validate the hash tree node when doing a binary search of an htree node into a separate function, which is disabled by default (since it is only used by developers when they are modifying the htree code paths). In addition to cleaning up the code to make it more maintainable, it silences a Clang compiler warning when -Wunreachable-code-aggressive is enabled. (There is no plan to enable this warning by default, since there it has far too many false positives; nevertheless, this commit reduces one of the many false positives by one.) The original description buried the lede, in terms of the primary reason why I think the change was worthwhile (although I know you have different priorities than mine :-). Thanks for working to find a way to improve the code in a way that makes both of us happy! - Ted
On Wed, Feb 3, 2021 at 2:39 AM Theodore Ts'o <tytso@mit.edu> wrote: > > On Tue, Feb 02, 2021 at 04:28:37PM +0000, Vinicius Tinti wrote: > > Clang with -Wunreachable-code-aggressive is being used to try to find > > unreachable code that could cause potential bugs. There is no plan to > > enable it by default. > > > > The following code was detected as unreachable: > > > > fs/ext4/namei.c:831:17: warning: code will never be executed > > [-Wunreachable-code] > > unsigned n = count - 1; > > ^~~~~ > > fs/ext4/namei.c:830:7: note: silence by adding parentheses to mark code as > > explicitly dead > > if (0) { // linear search cross check > > ^ > > /* DISABLES CODE */ ( ) > > > > This has been present since commit ac27a0ec112a ("[PATCH] ext4: initial > > copy of files from ext3") and fs/ext3 had it present at the beginning of > > git history. It has not been changed since. > > > > This patch moves the code to a new function `htree_rep_invariant_check` > > which only performs the check when DX_DEBUG is set. This allows the > > function to be used in other parts of the code. > > > > Suggestions from: Andreas, Christoph, Nathan, Nick and Ted. > > > > Signed-off-by: Vinicius Tinti <viniciustinti@gmail.com> > > Thanks, applied, although I rewrote the commit description: > > ext4: factor out htree rep invariant check > > This patch moves some debugging code which is used to validate the > hash tree node when doing a binary search of an htree node into a > separate function, which is disabled by default (since it is only used > by developers when they are modifying the htree code paths). > > In addition to cleaning up the code to make it more maintainable, it > silences a Clang compiler warning when -Wunreachable-code-aggressive > is enabled. (There is no plan to enable this warning by default, since > there it has far too many false positives; nevertheless, this commit > reduces one of the many false positives by one.) > > The original description buried the lede, in terms of the primary > reason why I think the change was worthwhile (although I know you have > different priorities than mine :-). > > Thanks for working to find a way to improve the code in a way that > makes both of us happy! Thanks for the feedback. And thanks for all the ones who reviewed. > - Ted
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index cf652ba3e74d..a6e28b4b5a95 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -731,6 +731,29 @@ struct stats dx_show_entries(struct dx_hash_info *hinfo, struct inode *dir, (space/bcount)*100/blocksize); return (struct stats) { names, space, bcount}; } + +/* + * Linear search cross check + */ +static inline void htree_rep_invariant_check(struct dx_entry *at, + struct dx_entry *target, + u32 hash, unsigned int n) +{ + while (n--) { + dxtrace(printk(KERN_CONT ",")); + if (dx_get_hash(++at) > hash) { + at--; + break; + } + } + ASSERT(at == target - 1); +} +#else /* DX_DEBUG */ +static inline void htree_rep_invariant_check(struct dx_entry *at, + struct dx_entry *target, + u32 hash, unsigned int n) +{ +} #endif /* DX_DEBUG */ /* @@ -827,20 +850,7 @@ dx_probe(struct ext4_filename *fname, struct inode *dir, p = m + 1; } - if (0) { // linear search cross check - unsigned n = count - 1; - at = entries; - while (n--) - { - dxtrace(printk(KERN_CONT ",")); - if (dx_get_hash(++at) > hash) - { - at--; - break; - } - } - ASSERT(at == p - 1); - } + htree_rep_invariant_check(entries, p, hash, count - 1); at = p - 1; dxtrace(printk(KERN_CONT " %x->%u\n",
Clang with -Wunreachable-code-aggressive is being used to try to find unreachable code that could cause potential bugs. There is no plan to enable it by default. The following code was detected as unreachable: fs/ext4/namei.c:831:17: warning: code will never be executed [-Wunreachable-code] unsigned n = count - 1; ^~~~~ fs/ext4/namei.c:830:7: note: silence by adding parentheses to mark code as explicitly dead if (0) { // linear search cross check ^ /* DISABLES CODE */ ( ) This has been present since commit ac27a0ec112a ("[PATCH] ext4: initial copy of files from ext3") and fs/ext3 had it present at the beginning of git history. It has not been changed since. This patch moves the code to a new function `htree_rep_invariant_check` which only performs the check when DX_DEBUG is set. This allows the function to be used in other parts of the code. Suggestions from: Andreas, Christoph, Nathan, Nick and Ted. Signed-off-by: Vinicius Tinti <viniciustinti@gmail.com> --- fs/ext4/namei.c | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-)