Date:   Mon, 1 Feb 2021 13:45:11 -0700
From:   Andreas Dilger <adilger@...ger.ca>
To:     Vinicius Tinti <viniciustinti@...il.com>
Cc:     Theodore Ts'o <tytso@....edu>,
        Christoph Hellwig <hch@...radead.org>,
        Nathan Chancellor <natechancellor@...il.com>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Ext4 Developers List <linux-ext4@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        clang-built-linux@...glegroups.com
Subject: Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set

On Feb 1, 2021, at 11:41 AM, Vinicius Tinti <viniciustinti@...il.com> wrote:
> 
> On Mon, Feb 1, 2021 at 2:13 PM Theodore Ts'o <tytso@....edu> wrote:
>> 
>> On Mon, Feb 01, 2021 at 01:15:29PM -0300, Vinicius Tinti wrote:
>>> On Mon, Feb 1, 2021 at 9:49 AM Christoph Hellwig <hch@...radead.org> wrote:
>>>> 
>>>> DX_DEBUG is completely dead code, so either kill it off or make it an
>>>> actual CONFIG_* symbol through Kconfig if it seems useful.
>>> 
>>> About the unreachable code in "if (0)" I think it could be removed.
>>> It seems to be doing an extra check.
>>> 
>>> About the DX_DEBUG I think I can do another patch adding it to Kconfig
>>> as you and Nathan suggested.
>> 
>> Yes, it's doing another check which is useful in terms of early
>> detection of bugs when a developer has the code open for
>> modifications.  It slows down performance under normal circumstances,
>> and assuming the code is bug-free(tm), it's entirely unnecessary ---
>> which is why it's under an "if (0)".
> 
> My goal is to avoid having a dead code. Three options come to mind.
> 
> The first would be to add another #ifdef SOMETHING (suggest a name).
> But this doesn't remove the code and someone could enable it by accident.

I don't see anything wrong with the original suggestion to use "DX_DEBUG".
If we want to get rid of "if (0)" in the code it could be changed like:

#define DX_DEBUG 0
#if DX_DEBUG
#define dxtrace(command) command
#else
#define dxtrace(command)
#endif

Then in the code check this directly (and fix the //-style comment also):

	if (DX_DEBUG) { /* linear search cross check */
		:
	}

That will hopefully avoid the "dead code" warning, while keeping the code
visible for syntax checking by the compiler (unlike if it was under #ifdef).

Alternately, if we want to keep the "#ifdef DX_DEBUG" check intact:

#ifdef DX_DEBUG
#define dxtrace(command) command
#define DX_DEBUG_CROSSCHECK true
#else
#define dxtrace(command)
#define DX_DEBUG_CROSSCHECK false
#endif

	if (DX_DEBUG_CROSSCHECK) { /* linear search cross check */
		:
	}

Cheers, Andreas

> 
> The second would be to add the code in a block comment. Then document
> that it is for checking an invariant. This will make it harder to cause
> problems.
> 
> The third would be to remove the code and explain the invariant in a block
> comment. Maybe add a pseudo code too in the comment.
> 
> What do you think?
> 
>> However, if there *is* a bug, having an early detection that the
>> representation invariant of the data structure has been violated can
>> be useful in root causing a bug.  This would probably be clearer if
>> the code was pulled out into a separate function with comments
>> explaining that this is a rep invariant check.
> 
> Good idea. I will do that too.
> 
>> The main thing about DX_DEBUG right now is that it is **super**
>> verbose.  Unwary users who enable it.... will be sorry.  If we want to
>> make it to be a first-class feature enabled via CONFIG_EXT4_DEBUG, we
>> should convert all of the dx_trace calls to use pr_debug so they are
>> enabled only if dynamic debug enables those pr_debug() statements.
>> And this should absolutely be a separate patch.
> 
> Agreed. For now I only want to focus on the "if (0)".
> 
> Regards,
> Vinicius
> 
>> Cheers,
>> 
>>                                                - Ted


Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)

Powered by blists - more mailing lists