On Feb 1, 2021, at 11:41 AM, Vinicius Tinti [off-list ref] wrote:

On Mon, Feb 1, 2021 at 2:13 PM Theodore Ts'o [off-list ref] wrote:

quoted

On Mon, Feb 01, 2021 at 01:15:29PM -0300, Vinicius Tinti wrote:

quoted

On Mon, Feb 1, 2021 at 9:49 AM Christoph Hellwig [off-list ref] wrote:

quoted

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?

quoted

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.

quoted

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

quoted

Cheers,

                                               - Ted

Cheers, Andreas