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;
To:     Jan Kara <jack@suse.cz>
Cc:     tytso@mit.edu, Jan Kara <jack@suse.com>,
        Horst Schirmeier <horst.schirmeier@tu-dortmund.de>,
        linux-ext4@vger.kernel.org
References: <20190408083500.66759-1-alexander.lochmann@tu-dortmund.de>
 <7827d153-f75c-89a2-1890-86e85f86c704@tu-dortmund.de>
 <14dbc946-b0c5-4165-3e6a-3cbe3c6a74b4@tu-dortmund.de>
 <20210208152750.GD30081@quack2.suse.cz>
 <02643d06-0066-a7c3-b6dd-2d190c8e0c41@tu-dortmund.de>
 <20210209120017.GB19070@quack2.suse.cz>
From:   Alexander Lochmann <alexander.lochmann@tu-dortmund.de>
Subject: Re: [RFC] Fine-grained locking documentation for jbd2 data structures
Message-ID: <a086fca9-eac8-f897-1d28-eee977d7c12d@tu-dortmund.de>
Date:   Tue, 9 Feb 2021 14:47:28 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.7.0
MIME-Version: 1.0
In-Reply-To: <20210209120017.GB19070@quack2.suse.cz>
Content-Type: multipart/signed; micalg=pgp-sha256;
 protocol="application/pgp-signature";
 boundary="4lnTMQHB5xg6nIAYKGp4egX3LFOiNlrM9"
Precedence: bulk

This is an OpenPGP/MIME signed message (RFC 4880 and 3156)
--4lnTMQHB5xg6nIAYKGp4egX3LFOiNlrM9
Content-Type: multipart/mixed; boundary="YFqjbLTaAS8ys6NEXFTugdufBqCyfkVJ9";
 protected-headers="v1"
From: Alexander Lochmann <alexander.lochmann@tu-dortmund.de>
To: Jan Kara <jack@suse.cz>
Cc: tytso@mit.edu, Jan Kara <jack@suse.com>,
 Horst Schirmeier <horst.schirmeier@tu-dortmund.de>,
 linux-ext4@vger.kernel.org
Message-ID: <a086fca9-eac8-f897-1d28-eee977d7c12d@tu-dortmund.de>
Subject: Re: [RFC] Fine-grained locking documentation for jbd2 data structures
References: <20190408083500.66759-1-alexander.lochmann@tu-dortmund.de>
 <7827d153-f75c-89a2-1890-86e85f86c704@tu-dortmund.de>
 <14dbc946-b0c5-4165-3e6a-3cbe3c6a74b4@tu-dortmund.de>
 <20210208152750.GD30081@quack2.suse.cz>
 <02643d06-0066-a7c3-b6dd-2d190c8e0c41@tu-dortmund.de>
 <20210209120017.GB19070@quack2.suse.cz>
In-Reply-To: <20210209120017.GB19070@quack2.suse.cz>

--YFqjbLTaAS8ys6NEXFTugdufBqCyfkVJ9
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: de-DE
Content-Transfer-Encoding: quoted-printable

On 09.02.21 13:00, Jan Kara wrote:
>>> Yes, although in last year, people try to convert these unlocked read=
s to
>>> READ_ONCE() or similar as otherwise the compiler is apparently allowe=
d to
>>> generate code which is not safe. But that's a different story.
>> Is this ongoing work?
>=20
> Yes, in a way. It's mostly prompted by KCSAN warnings generated by syzb=
ot
> ;).
>=20
>> Using such a macro would a) make our work much easier as we can instru=
ment
>> them, and b) would tell less experienced developers that no locking is=

>> needed.
>=20
> Yes, I agree that it has some benefit for documentation and automatic
> checkers as well. OTOH code readability is sometimes hurt by this...
>=20
>> Does the usage of READ_ONCE() imply that no lock is needed?
>=20
> No, but it does indicate there's something unusual happening with the
> variable - usually that variable write can race with this read.
>=20
>> Otherwise, one could introduce another macro for jbd2, such as #define=

>> READ_UNLOCKED() READ_ONCE(), which is more precise.
>=20
> Well, yes, but OTOH special macros for small subsystems like this are m=
aking
> more harm than good in terms of readability since people have to lookup=

> what exactly they mean anyway.
So the only option left would be a global macro such as READ_ONCE() I gue=
ss.
How hard would it be to establish such a global notation?
It would make things a lot easier for LockDoc, because we can instrument =

such a macro, and therefore can annotate those accesses.>
> Definitely. The simplest case is: You can fetch
> journal->j_running_transaction pointer any time without any problem. Bu=
t
> you can *dereference* it only if you hold the j_state_lock while fetchi=
ng the
> pointer and dereferencing it.
Thx.
>=20
>> So sometimes requiring the lock is just the least
>>> problematic solution - there's always the tradeoff between the speed =
and
>>> simplicity.
>>>
>>>>> All of the above members have word size, i.e., int, long, or ptr.
>>>>> Is it therefore safe to split the locking documentation as follows?=

>>>>> @j_flags: General journaling state flags [r:nolocks, w:j_state_lock=
]
>>>
>>> I've checked the code and we usually use unlocked reads for quick, po=
ssibly
>>> racy checks and if they indicate we may need to do something then tak=
e the
>>> lock and do a reliable check. This is quite common pattern, not sure =
how to
>>> best document this. Maybe like [j_state_lock, no lock for quick racy =
checks]?
>>>
>> Yeah, I'm fine with that. Does this rule apply for the other members o=
f
>> journal_t (and transaction_t?) listed above?
>=20
> Yes.
Thx. I'll submit a patch for those elements.
For now, this will improve LockDoc's results as we can add "no locks=20
needed" to our config for j_flags. We check whether the observed=20
accesses match the documented locking rules.
LockDoc will accept both results "j_list_lock" and "no locks needed" for =

reading j_flags.
However, real faulty unlocked accesses will be concealed. :-(

- Alex
>=20
> 								Honza
>=20

--=20
Technische Universit=C3=A4t Dortmund
Alexander Lochmann                PGP key: 0xBC3EF6FD
Otto-Hahn-Str. 16                 phone:  +49.231.7556141
D-44227 Dortmund                  fax:    +49.231.7556116
http://ess.cs.tu-dortmund.de/Staff/al


--YFqjbLTaAS8ys6NEXFTugdufBqCyfkVJ9--

--4lnTMQHB5xg6nIAYKGp4egX3LFOiNlrM9
Content-Type: application/pgp-signature; name="OpenPGP_signature.asc"
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename="OpenPGP_signature"

-----BEGIN PGP SIGNATURE-----

wsF5BAABCAAjFiEElhZsUHzVP0dbkjCRWT7tBbw+9v0FAmAiknEFAwAAAAAACgkQWT7tBbw+9v0W
/A//S8Y6xiF/FRtvMLw1Zco8yJLoj7zu8zCnfdnd1UhAjR/d1s7LN8MFT+Xp2UVYN2acPwIGEU9V
T+AhUtKqTfq8IZTbUz1mULsbQUWDBDjoy+uFEy288Z3vBVEfWz2kkgk5DRohQgUzHIftSGO24uVt
5fZ3yhv6f3EAIZHk4Qw7CjA0f4HR3eVWpvKXDF0QXE/as4gA6hSr4qDPDdqsfHkkbDdPk554dXIx
IzNGub5j51Jw6UTSskWWbf7axFPW85KXXicfoZYVWXt2g0/DH1joxINwNLJU5AG/g7V9dbIYO180
jcbckYbBayZLl9BS8AviQZV6w7wRFzY9SFrPl4kEdJnNroPtIxpHBZ5SQnZFSFAKe2uP/4fEzrmm
1FX1O+66/88iKnbQBhWLBM/l5lMEQb+/N+Ya57El+e/GKJyY0qTSFaRWVXxJGEGaaUHo/iuYDDaE
pkQt+1Xt9ep/sX7cm+7itrMs8mHqP2ji2mJGe2Oo0g39esoBOX64s0ZJ7cLz8gXYTgn5gP2td4M+
lXgGHAgj+Jrip+kNj+3uv3J3Oe+PYJmurhdXrZcIYjH4POuo51zC5xjb8fRny4CyPwFiAlw0Z1Bc
lf+OJDqYaZXI3jgDruYGaG96kpAeGOkZfcXkzyaDS768iSHg5ifNhoMmVGPqV4WnN9+B2vj4AstB
8r8=
=WZPP
-----END PGP SIGNATURE-----

--4lnTMQHB5xg6nIAYKGp4egX3LFOiNlrM9--