On Mon, Feb 01, 2021 at 01:15:29PM -0300, Vinicius Tinti wrote:
> On Mon, Feb 1, 2021 at 9:49 AM Christoph Hellwig <[email protected]> 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)".
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.
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.
Cheers,
- Ted
On Mon, Feb 1, 2021 at 2:13 PM Theodore Ts'o <[email protected]> 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 <[email protected]> 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.
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