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

[Alexander Lochmann <alexander.lochmann@tu-dortmund.de>]

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



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?
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.
Does the usage of READ_ONCE() imply that no lock is needed?
Otherwise, one could introduce another macro for jbd2, such as #define 
READ_UNLOCKED() READ_ONCE(), which is more precise.
  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?
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?
>>> The following members are not word-sizes. Our results also suggest that
>>> no locks are needed.
>>> Can the proposed change be applied to them as well?
>>> transaction_t
>>> - t_chp_stats: j_checkpoint_sem
> 
> Where do we read t_chp_stats outside of a lock? j_list_lock seems to be
> used pretty consistently there. Except perhaps
> __jbd2_journal_remove_checkpoint() but there we know we are already the
> only ones touching the transaction and thus its statistics.
> 
I'm sorry. That's my mistake. There's no access without a lock.
>>> jbd2_inode:
>>> - i_list: j_list_lock
> 
> And here as well. I would not complicate the locking description unless we
> really have places that access these fields without locks...
> 
Same here.

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