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;
From:   Andreas Dilger <adilger@dilger.ca>
Message-Id: <9CA2BADA-9CD9-49DC-A5E2-063C8A593E57@dilger.ca>
Content-Type: multipart/signed;
 boundary="Apple-Mail=_D6983B98-027E-4E50-A31C-08148A4552B0";
 protocol="application/pgp-signature"; micalg=pgp-sha256
Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\))
Subject: Re: [PATCH v2] ext4: Enable code path when DX_DEBUG is set
Date:   Mon, 1 Feb 2021 13:45:11 -0700
In-Reply-To: <CALD9WKzO53AXQW-qQ82VZ41H5=cGdLTUiEoz3X6BmPkb6XaTag@mail.gmail.com>
Cc:     Theodore Ts'o <tytso@mit.edu>,
        Christoph Hellwig <hch@infradead.org>,
        Nathan Chancellor <natechancellor@gmail.com>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Ext4 Developers List <linux-ext4@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        clang-built-linux@googlegroups.com
To:     Vinicius Tinti <viniciustinti@gmail.com>
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>
 <CALD9WKzO53AXQW-qQ82VZ41H5=cGdLTUiEoz3X6BmPkb6XaTag@mail.gmail.com>
Precedence: bulk


--Apple-Mail=_D6983B98-027E-4E50-A31C-08148A4552B0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=us-ascii

On Feb 1, 2021, at 11:41 AM, Vinicius Tinti <viniciustinti@gmail.com> =
wrote:
>=20
> On Mon, Feb 1, 2021 at 2:13 PM Theodore Ts'o <tytso@mit.edu> wrote:
>>=20
>> 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:
>>>>=20
>>>> DX_DEBUG is completely dead code, so either kill it off or make it =
an
>>>> actual CONFIG_* symbol through Kconfig if it seems useful.
>>>=20
>>> About the unreachable code in "if (0)" I think it could be removed.
>>> It seems to be doing an extra check.
>>>=20
>>> About the DX_DEBUG I think I can do another patch adding it to =
Kconfig
>>> as you and Nathan suggested.
>>=20
>> 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)".
>=20
> My goal is to avoid having a dead code. Three options come to mind.
>=20
> 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

>=20
> 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.
>=20
> 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.
>=20
> What do you think?
>=20
>> 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.
>=20
> Good idea. I will do that too.
>=20
>> 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.
>=20
> Agreed. For now I only want to focus on the "if (0)".
>=20
> Regards,
> Vinicius
>=20
>> Cheers,
>>=20
>>                                                - Ted


Cheers, Andreas


--Apple-Mail=_D6983B98-027E-4E50-A31C-08148A4552B0
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
	filename=signature.asc
Content-Type: application/pgp-signature;
	name=signature.asc
Content-Description: Message signed with OpenPGP

-----BEGIN PGP SIGNATURE-----
Comment: GPGTools - http://gpgtools.org

iQIzBAEBCAAdFiEEDb73u6ZejP5ZMprvcqXauRfMH+AFAmAYaFcACgkQcqXauRfM
H+DA5w/+NwuAaMctdbMXqQsHDMc+8lK3fpTgnvWNSPdfWXXowh1ubKGMfCPPAAIB
MJKNY8zp2dCTFYTX8fzJrCdzcjD45k2snHVfHZlKuI8kSsbxe9lUdVl/uKCVwgDn
LFSL7B8HN0yIAWW7gdg2EDTDuc+MNAliJQnDRX7/25dZBqV4Y7TA579NklClOO64
zoTCFYZGanXrtr0UYEQX5vGl6udy/YoftI1miH5mw73th7KxaIvgYHu0tiKTrggc
BnWS74x2JQHFmr+/6dsob/tBDsK0BE/CifcBBgY1Atqzzczjua8zQa3ew/hzLbiq
3g8xHJau+15rOIX+DwfZRWZGFOoGV4jNBtKrFULAUDEdypWxk58t7ZB7bHq/5y6k
baiMYVGBDebneGEvWU6JRSUYJ16cyLlXDtEw39iDlLk0Qz7vzGYCMXyi7wpebY6c
ZB0774XE2kWE1BlWtarIyUpXstwuysG4FwCCnp04HlYpeWGc7PdoFwykXQSTScWF
HGfbF/EYJzyxEeEKBB7Jn/UDzVU2dnUEgfVUJpTdiCZhesRMIluyKKlL75mFI4pP
2gjnnAy79AYMtHY2fiEUrjoJZvbLO6BWN+CS5TLUzcGqTmo5Qi09B5HKeH2BLV8H
EP0jf0us79YH2/pFwtxvZRnQJrDCML7cCN4VtSiIqY3iNlBpBbA=
=p4Ec
-----END PGP SIGNATURE-----

--Apple-Mail=_D6983B98-027E-4E50-A31C-08148A4552B0--