Re: [RFC] Fine-grained locking documentation for jbd2 data structures

[Jan Kara <jack@suse.cz>]

On Tue 09-02-21 10:58:48, Alexander Lochmann wrote:
> 
> 
> On 08.02.21 16:27, Jan Kara wrote:
> > Hi Alexander!
> > 
> > On Fri 05-02-21 16:31:54, Alexander Lochmann wrote:
> > > have you had the chance to review our results?
> > 
> > It fell through the cracks I guess. Thanks for pinging. Let me have a look.
> > 
> > > On 15.10.20 15:56, Alexander Lochmann wrote:
> > > > Hi folks,
> > > > 
> > > > when comparing our generated locking documentation with the current
> > > > documentation
> > > > located in include/linux/jbd2.h, I found some inconsistencies. (Our
> > > > approach: https://dl.acm.org/doi/10.1145/3302424.3303948)
> > > > According to the official documentation, the following members should be
> > > > read using a lock:
> > > > journal_t
> > > > - j_flags: j_state_lock
> > > > - j_barrier_count: j_state_lock
> > > > - j_running_transaction: j_state_lock
> > > > - j_commit_sequence: j_state_lock
> > > > - j_commit_request: j_state_lock
> > > > transactiont_t
> > > > - t_nr_buffers: j_list_lock
> > > > - t_buffers: j_list_lock
> > > > - t_reserved_list: j_list_lock
> > > > - t_shadow_list: j_list_lock
> > > > jbd2_inode
> > > > - i_transaction: j_list_lock
> > > > - i_next_transaction: j_list_lock
> > > > - i_flags: j_list_lock
> > > > - i_dirty_start: j_list_lock
> > > > - i_dirty_start: j_list_lock
> > > > 
> > > > However, our results say that no locks are needed at all for *reading*
> > > > those members.
> > > >  From what I know, it is common wisdom that word-sized data types can be
> > > > read without any lock in the Linux kernel.
> > 
> > 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.

>  Also note
> > that although reading that particular word may be safe without any other
> > locks, the lock still may be needed to safely interpret the value in the
> > context of other fetched values (e.g., due to consistency among multiple
> > structure members).
> Just a side quest: Do you have an example for such a situation?

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.

> 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.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR