Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 3 Feb 2021 00:39:03 -0500
From:   "Theodore Ts'o" <tytso@mit.edu>
To:     Vinicius Tinti <viniciustinti@gmail.com>
Cc:     Christoph Hellwig <hch@infradead.org>,
        Andreas Dilger <adilger.kernel@dilger.ca>,
        Nathan Chancellor <natechancellor@gmail.com>,
        Nick Desaulniers <ndesaulniers@google.com>,
        linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org,
        clang-built-linux@googlegroups.com
Subject: Re: [PATCH v3] ext4: Enable code path when DX_DEBUG is set
Message-ID: <YBo29/SdgCkZZinT@mit.edu>
References: <20210202080508.GA3550351@infradead.org>
 <20210202162837.129631-1-viniciustinti@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210202162837.129631-1-viniciustinti@gmail.com>
Precedence: bulk

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