[PATCH v3] Updated locking documentation for transaction_t

[PATCH v3] Updated locking documentation for transaction_t

[Alexander Lochmann]

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 <alexander.lochmann@tu-dortmund.de>
Signed-off-by: Horst Schirmeier <horst.schirmeier@tu-dortmund.de>
---
 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

Re: [PATCH v3] Updated locking documentation for transaction_t

[Alexander Lochmann]

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

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 <alexander.lochmann@tu-dortmund.de>
> Signed-off-by: Horst Schirmeier <horst.schirmeier@tu-dortmund.de>
> ---
>  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


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

[PATCH v3] Updated locking documentation for transaction_t

[Alexander Lochmann]

[-- Attachment #1.1.1: Type: text/plain, Size: 147 bytes --]

Hi folks,

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

Cheers,
Alex

[-- Attachment #1.1.2: transaction_t-doc.patch --]
[-- Type: text/x-patch, Size: 1446 bytes --]

commit 13ac907c45c5da7d691f6e10972de5e56e0072c6
Author: Alexander Lochmann <alexander.lochmann@tu-dortmund.de>
Date:   Thu Oct 15 15:24:52 2020 +0200

    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 <alexander.lochmann@tu-dortmund.de>
    Signed-off-by: Horst Schirmeier <horst.schirmeier@tu-dortmund.de>

diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 08f904943ab2..a11c78e4af4e 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -532,6 +532,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)
  */
 
 /*
@@ -650,12 +651,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;
 

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

Re: [PATCH v3] Updated locking documentation for transaction_t

[Theodore Y. Ts'o]

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

Re: [PATCH v3] Updated locking documentation for transaction_t

[Alexander Lochmann]

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



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


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

Re: [PATCH v3] Updated locking documentation for transaction_t

[Theodore Y. Ts'o]

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

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

[Alexander Lochmann]

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

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


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

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

[Alexander Lochmann]

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

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

[Jan Kara]

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 <jack@suse.com>
SUSE Labs, CR

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

[Alexander Lochmann]

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

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

[Jan Kara]

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

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

[Alexander Lochmann]

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

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

[Jan Kara]

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 <jack@suse.com>
SUSE Labs, CR