All of lore.kernel.org
 help / color / mirror / Atom feed
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
Subject: Re: [RFC] Fine-grained locking documentation for jbd2 data structures
Date: Tue, 9 Feb 2021 14:47:28 +0100	[thread overview]
Message-ID: <a086fca9-eac8-f897-1d28-eee977d7c12d@tu-dortmund.de> (raw)
In-Reply-To: <20210209120017.GB19070@quack2.suse.cz>


[-- Attachment #1.1: Type: text/plain, Size: 3283 bytes --]

On 09.02.21 13:00, Jan Kara wrote:
>>> Yes, although in last year, people try to convert these unlocked reads to
>>> READ_ONCE() or similar as otherwise the compiler is apparently allowed to
>>> generate code which is not safe. But that's a different story.
>> Is this ongoing work?
> 
> Yes, in a way. It's mostly prompted by KCSAN warnings generated by syzbot
> ;).
> 
>> Using such a macro would a) make our work much easier as we can instrument
>> them, and b) would tell less experienced developers that no locking is
>> needed.
> 
> Yes, I agree that it has some benefit for documentation and automatic
> checkers as well. OTOH code readability is sometimes hurt by this...
> 
>> Does the usage of READ_ONCE() imply that no lock is needed?
> 
> No, but it does indicate there's something unusual happening with the
> variable - usually that variable write can race with this read.
> 
>> Otherwise, one could introduce another macro for jbd2, such as #define
>> READ_UNLOCKED() READ_ONCE(), which is more precise.
> 
> Well, yes, but OTOH special macros for small subsystems like this are making
> 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 guess.
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. But
> you can *dereference* it only if you hold the j_state_lock while fetching the
> pointer and dereferencing it.
Thx.
> 
>> 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, possibly
>>> racy checks and if they indicate we may need to do something then take 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 of
>> journal_t (and transaction_t?) listed above?
> 
> Yes.
Thx. I'll submit a patch for those elements.
For now, this will improve LockDoc's results as we can add "no locks 
needed" to our config for j_flags. We check whether the observed 
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
> 
> 								Honza
> 

-- 
Technische Universität 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

  reply	other threads:[~2021-02-09 13:50 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-08  8:35 [PATCH v3] Updated locking documentation for transaction_t Alexander Lochmann
2019-06-20 20:45 ` Alexander Lochmann
2020-10-15 13:26 ` Alexander Lochmann
2020-12-03 14:04   ` Theodore Y. Ts'o
2020-12-03 14:38     ` Alexander Lochmann
2020-12-03 20:39       ` Theodore Y. Ts'o
2020-10-15 13:56 ` [RFC] Fine-grained locking documentation for jbd2 data structures Alexander Lochmann
2021-02-05 15:31   ` Alexander Lochmann
2021-02-08 15:27     ` Jan Kara
2021-02-09  9:58       ` Alexander Lochmann
2021-02-09 12:00         ` Jan Kara
2021-02-09 13:47           ` Alexander Lochmann [this message]
2021-02-09 16:48             ` Jan Kara

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a086fca9-eac8-f897-1d28-eee977d7c12d@tu-dortmund.de \
    --to=alexander.lochmann@tu-dortmund.de \
    --cc=horst.schirmeier@tu-dortmund.de \
    --cc=jack@suse.com \
    --cc=jack@suse.cz \
    --cc=linux-ext4@vger.kernel.org \
    --cc=tytso@mit.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.