Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <AAB32610-D238-4137-96DE-33655AAAB545@dilger.ca>
 <20210201003125.90257-1-viniciustinti@gmail.com> <20210201124924.GA3284018@infradead.org>
 <CALD9WKxc0kMPCHSoikko+qYk2+ZLUy73+ryKGW9qMSpyzAobLA@mail.gmail.com> <YBg20AuSC3/9w2zz@mit.edu>
In-Reply-To: <YBg20AuSC3/9w2zz@mit.edu>
From:   Vinicius Tinti <viniciustinti@gmail.com>
Date:   Mon, 1 Feb 2021 15:41:50 -0300
Message-ID: <CALD9WKzO53AXQW-qQ82VZ41H5=cGdLTUiEoz3X6BmPkb6XaTag@mail.gmail.com>
Subject: Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set
To:     "Theodore Ts'o" <tytso@mit.edu>
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
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Feb 1, 2021 at 2:13 PM Theodore Ts'o <tytso@mit.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@infradead.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.

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