2019-04-08 08:35:22

by Alexander Lochmann

[permalink] [raw]
Subject: [PATCH v3] Updated locking documentation for transaction_t

We used LockDoc to derive locking rules for each member
of struct transaction_t.
Based on those results, we extended the existing documentation
by more members of struct transaction_t, and updated the existing
documentation.

Signed-off-by: Alexander Lochmann <[email protected]>
Signed-off-by: Horst Schirmeier <[email protected]>
---
include/linux/jbd2.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 0f919d5fe84f..34b728e2b702 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -534,6 +534,7 @@ struct transaction_chp_stats_s {
* The transaction keeps track of all of the buffers modified by a
* running transaction, and all of the buffers committed but not yet
* flushed to home for finished transactions.
+ * (Locking Documentation improved by LockDoc)
*/

/*
@@ -652,12 +653,12 @@ struct transaction_s
unsigned long t_start;

/*
- * When commit was requested
+ * When commit was requested [journal_t.j_state_lock]
*/
unsigned long t_requested;

/*
- * Checkpointing stats [j_checkpoint_sem]
+ * Checkpointing stats [journal_t.j_list_lock]
*/
struct transaction_chp_stats_s t_chp_stats;

--
2.20.1



2019-06-20 21:31:38

by Alexander Lochmann

[permalink] [raw]
Subject: Re: [PATCH v3] Updated locking documentation for transaction_t

Hi Ted,

Have you had the chance to review the most recent version of the patch?
Does it look reasonable to you?

Cheers,
Alex

On 08.04.19 10:35, Alexander Lochmann wrote:
> We used LockDoc to derive locking rules for each member
> of struct transaction_t.
> Based on those results, we extended the existing documentation
> by more members of struct transaction_t, and updated the existing
> documentation.
>
> Signed-off-by: Alexander Lochmann <[email protected]>
> Signed-off-by: Horst Schirmeier <[email protected]>
> ---
> include/linux/jbd2.h | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 0f919d5fe84f..34b728e2b702 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -534,6 +534,7 @@ struct transaction_chp_stats_s {
> * The transaction keeps track of all of the buffers modified by a
> * running transaction, and all of the buffers committed but not yet
> * flushed to home for finished transactions.
> + * (Locking Documentation improved by LockDoc)
> */
>
> /*
> @@ -652,12 +653,12 @@ struct transaction_s
> unsigned long t_start;
>
> /*
> - * When commit was requested
> + * When commit was requested [journal_t.j_state_lock]
> */
> unsigned long t_requested;
>
> /*
> - * Checkpointing stats [j_checkpoint_sem]
> + * Checkpointing stats [journal_t.j_list_lock]
> */
> struct transaction_chp_stats_s t_chp_stats;
>
>

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


Attachments:
signature.asc (849.00 B)
OpenPGP digital signature

2020-10-15 13:57:21

by Alexander Lochmann

[permalink] [raw]
Subject: [RFC] Fine-grained locking documentation for jbd2 data structures

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

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
jbd2_inode:
- i_list: j_list_lock

Cheers,
Alex

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


Attachments:
signature.asc (849.00 B)
OpenPGP digital signature

2020-10-15 14:39:17

by Alexander Lochmann

[permalink] [raw]
Subject: [PATCH v3] Updated locking documentation for transaction_t

Hi folks,

I've updated the lock documentation according to our finding for
transaction_t.
Does this patch look good to you?

Cheers,
Alex


Attachments:
transaction_t-doc.patch (1.41 kB)
signature.asc (849.00 B)
OpenPGP digital signature
Download all attachments

2020-12-03 14:06:41

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH v3] Updated locking documentation for transaction_t

On Thu, Oct 15, 2020 at 03:26:28PM +0200, Alexander Lochmann wrote:
> Hi folks,
>
> I've updated the lock documentation according to our finding for
> transaction_t.
> Does this patch look good to you?

I updated the annotations to match with the local usage, e.g:

* When commit was requested [journal_t.j_state_lock]

became:

* When commit was requested [j_state_lock]

Otherwise, looks good. Thanks for the patch!

- Ted

2020-12-03 15:37:09

by Alexander Lochmann

[permalink] [raw]
Subject: Re: [PATCH v3] Updated locking documentation for transaction_t



On 03.12.20 15:04, Theodore Y. Ts'o wrote:
> On Thu, Oct 15, 2020 at 03:26:28PM +0200, Alexander Lochmann wrote:
>> Hi folks,
>>
>> I've updated the lock documentation according to our finding for
>> transaction_t.
>> Does this patch look good to you?
>
> I updated the annotations to match with the local usage, e.g:
>
> * When commit was requested [journal_t.j_state_lock]
>
> became:
>
> * When commit was requested [j_state_lock]What do you mean by local usage?
The annotations of other members of transaction_t?

Shouldn't the annotation look like this?
[t_journal->j_state_lock]
It would be more precise.
>
> Otherwise, looks good. Thanks for the patch!
Thanks!

- Alex
>
> - Ted
>

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


Attachments:
OpenPGP_signature (855.00 B)
OpenPGP digital signature

2020-12-03 20:43:03

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH v3] Updated locking documentation for transaction_t

On Thu, Dec 03, 2020 at 03:38:40PM +0100, Alexander Lochmann wrote:
>
>
> On 03.12.20 15:04, Theodore Y. Ts'o wrote:
> > On Thu, Oct 15, 2020 at 03:26:28PM +0200, Alexander Lochmann wrote:
> > > Hi folks,
> > >
> > > I've updated the lock documentation according to our finding for
> > > transaction_t.
> > > Does this patch look good to you?
> >
> > I updated the annotations to match with the local usage, e.g:
> >
> > * When commit was requested [journal_t.j_state_lock]
> >
> > became:
> >
> > * When commit was requested [j_state_lock]What do you mean by local usage?
> The annotations of other members of transaction_t?

Yes, I'd like the annotations of the other objects to be consistent,
and just use j_state_lock, j_list_lock, etc., for the other annotations.

> Shouldn't the annotation look like this?
> [t_journal->j_state_lock]
> It would be more precise.

It's more precise, but it's also unnecessary in this case, since all
of the elements of the journal have a j_ prefix, elements of a
transaction_t have a t_ prefix, etc. There is also no other structure
element which has a j_state_lock name *other* than in journal_t.

Cheers,

- Ted

2021-02-05 19:15:31

by Alexander Lochmann

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

Hi Ted, hi Jack,

have you had the chance to review our results?

Cheers,
Alex

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.
> 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]
>
> 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
> jbd2_inode:
> - i_list: j_list_lock
>
> Cheers,
> Alex
>

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

2021-02-08 15:38:38

by Jan Kara

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

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. 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). 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]?

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

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

Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR

2021-02-09 10:03:55

by Alexander Lochmann

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



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


Attachments:
OpenPGP_signature (855.00 B)
OpenPGP digital signature

2021-02-09 12:04:34

by Jan Kara

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

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 <[email protected]>
SUSE Labs, CR

2021-02-09 13:53:20

by Alexander Lochmann

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

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


Attachments:
OpenPGP_signature (855.00 B)
OpenPGP digital signature

2021-02-09 16:53:40

by Jan Kara

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

On Tue 09-02-21 14:47:28, Alexander Lochmann wrote:
> 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.>

Well, READ_ONCE() macro already exists in the kernel and is used in quite
some places. Generally, there's concensus that newly added unlocked
accesses should use that macro. But the amount of already existing unlocked
accesses in the kernel is large and the problem is mostly theoretical / for
machine checkers so there's some resistance to the churn generated by
global search-and-replace approach. But as I said we are moving in that
direction and eventually get there.

Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR