[PATCH v2] Updated locking documentation for transaction_t

[PATCH v2] 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 inode, 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 | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 0f919d5fe84f..72f689746340 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)
  */
 
 /*
@@ -585,7 +586,8 @@ struct transaction_s
 	}			t_state;
 
 	/*
-	 * Where in the log does this transaction's commit start? [no locking]
+	 * Where in the log does this transaction's commit start?
+	 * [journal_t.j_state_lock]
 	 */
 	unsigned long		t_log_start;
 
@@ -647,17 +649,17 @@ struct transaction_s
 	unsigned long		t_max_wait;
 
 	/*
-	 * When transaction started
+	 * When transaction started [journal_t.j_state_lock]
 	 */
 	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 v2] Updated locking documentation for transaction_t

[Jan Kara]

On Mon 18-03-19 19:42:37, 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 inode, 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>

Thanks! You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  include/linux/jbd2.h | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 0f919d5fe84f..72f689746340 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)
>   */
>  
>  /*
> @@ -585,7 +586,8 @@ struct transaction_s
>  	}			t_state;
>  
>  	/*
> -	 * Where in the log does this transaction's commit start? [no locking]
> +	 * Where in the log does this transaction's commit start?
> +	 * [journal_t.j_state_lock]
>  	 */
>  	unsigned long		t_log_start;
>  
> @@ -647,17 +649,17 @@ struct transaction_s
>  	unsigned long		t_max_wait;
>  
>  	/*
> -	 * When transaction started
> +	 * When transaction started [journal_t.j_state_lock]
>  	 */
>  	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
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [v2] Updated locking documentation for transaction_t

[Theodore Ts'o]

On Mon, Mar 18, 2019 at 07:42:37PM +0100, Alexander Lochmann wrote:
>  	/*t
> -	 * Where in the log does this transaction's commit start? [no locking]
> +	 * Where in the log does this transaction's commit start?
> +	 * [journal_t.j_state_lock]
>  	 */
>  	unsigned long		t_log_start;

Well, technically, that's not quite right.  It's only assigned in one
location, and we hold j_state_lock, yes.  But that's because we need
to access journal->j_head.  At the point where we set t_log_start, the
transaction has already been locked down (transaction->t_state >
T_LOCKED).

Similarly, we happen to be holding j_state where it is currently being
accessed, but it's not because we needed the lock in order to access
t_log_start safely.

>  	/*
> -	 * When transaction started
> +	 * When transaction started [journal_t.j_state_lock]
>  	 */
>  	unsigned long		t_start;

And again, not really.  The primary place where t_start is set is when
the transaction is firstt created, before it's visible anywhere else.
after that, it is used exclusively by the commit thread, and so no
locking is necessary.  It's true that in the places where it is used,
j_state_lock happens to be taken, but it's strictly not necessary.

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

Yes, that appears to be correct.

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

This appears to be correct.

						- Ted

Re: [v2] Updated locking documentation for transaction_t

[Alexander Lochmann]

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

Thanks, Ted, for your feedback!
I'll submit a modified version.

- Alex

On 07.04.19 18:52, Theodore Ts'o wrote:
> On Mon, Mar 18, 2019 at 07:42:37PM +0100, Alexander Lochmann wrote:
>>  	/*t
>> -	 * Where in the log does this transaction's commit start? [no locking]
>> +	 * Where in the log does this transaction's commit start?
>> +	 * [journal_t.j_state_lock]
>>  	 */
>>  	unsigned long		t_log_start;
> 
> Well, technically, that's not quite right.  It's only assigned in one
> location, and we hold j_state_lock, yes.  But that's because we need
> to access journal->j_head.  At the point where we set t_log_start, the
> transaction has already been locked down (transaction->t_state >
> T_LOCKED).
> 
> Similarly, we happen to be holding j_state where it is currently being
> accessed, but it's not because we needed the lock in order to access
> t_log_start safely.
> 
>>  	/*
>> -	 * When transaction started
>> +	 * When transaction started [journal_t.j_state_lock]
>>  	 */
>>  	unsigned long		t_start;
> 
> And again, not really.  The primary place where t_start is set is when
> the transaction is firstt created, before it's visible anywhere else.
> after that, it is used exclusively by the commit thread, and so no
> locking is necessary.  It's true that in the places where it is used,
> j_state_lock happens to be taken, but it's strictly not necessary.
> 
>>  
>>  	/*
>> -	 * When commit was requested
>> +	 * When commit was requested [journal_t.j_state_lock]
>>  	 */
>>  	unsigned long		t_requested;
> 
> Yes, that appears to be correct.
> 
>>  
>>  	/*
>> -	 * Checkpointing stats [j_checkpoint_sem]
>> +	 * Checkpointing stats [journal_t.j_list_lock]
>>  	 */
>>  	struct transaction_chp_stats_s t_chp_stats;
>>
> 
> This appears to be correct.
> 
> 						- 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: 833 bytes --]

Re: [PATCH v2] Updated locking documentation for transaction_t

[Jan Kara]

On Thu 11-02-21 14:54:23, Alexander Lochmann wrote:
> Some members of transaction_t are allowed to be read without
> any lock being held if accessed from the correct context.
> We used LockDoc's findings to determine those members.
> Each member of them is marked with a short comment:
> "no lock needed for jbd2 thread".
> 
> Signed-off-by: Alexander Lochmann <alexander.lochmann@tu-dortmund.de>
> Signed-off-by: Horst Schirmeier <horst.schirmeier@tu-dortmund.de>

Thanks. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  include/linux/jbd2.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 99d3cd051ac3..1f19d19f6435 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -594,18 +594,18 @@ struct transaction_s
>  	 */
>  	unsigned long		t_log_start;
>  
> -	/* Number of buffers on the t_buffers list [j_list_lock] */
> +	/* Number of buffers on the t_buffers list [j_list_lock, no locks needed for jbd2 thread] */
>  	int			t_nr_buffers;
>  
>  	/*
>  	 * Doubly-linked circular list of all buffers reserved but not yet
> -	 * modified by this transaction [j_list_lock]
> +	 * modified by this transaction [j_list_lock, no locks needed for jbd2 thread]
>  	 */
>  	struct journal_head	*t_reserved_list;
>  
>  	/*
>  	 * Doubly-linked circular list of all metadata buffers owned by this
> -	 * transaction [j_list_lock]
> +	 * transaction [j_list_lock, no locks needed for jbd2 thread]
>  	 */
>  	struct journal_head	*t_buffers;
>  
> @@ -631,7 +631,7 @@ struct transaction_s
>  	/*
>  	 * Doubly-linked circular list of metadata buffers being shadowed by log
>  	 * IO.  The IO buffers on the iobuf list and the shadow buffers on this
> -	 * list match each other one for one at all times. [j_list_lock]
> +	 * list match each other one for one at all times. [j_list_lock, no locks needed for jbd2 thread]
>  	 */
>  	struct journal_head	*t_shadow_list;
>  
> -- 
> 2.20.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v2] Updated locking documentation for transaction_t

[Alexander Lochmann]

Some members of transaction_t are allowed to be read without
any lock being held if accessed from the correct context.
We used LockDoc's findings to determine those members.
Each member of them is marked with a short comment:
"no lock needed for jbd2 thread".

Signed-off-by: Alexander Lochmann <alexander.lochmann@tu-dortmund.de>
Signed-off-by: Horst Schirmeier <horst.schirmeier@tu-dortmund.de>
---
 include/linux/jbd2.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 99d3cd051ac3..1f19d19f6435 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -594,18 +594,18 @@ struct transaction_s
 	 */
 	unsigned long		t_log_start;
 
-	/* Number of buffers on the t_buffers list [j_list_lock] */
+	/* Number of buffers on the t_buffers list [j_list_lock, no locks needed for jbd2 thread] */
 	int			t_nr_buffers;
 
 	/*
 	 * Doubly-linked circular list of all buffers reserved but not yet
-	 * modified by this transaction [j_list_lock]
+	 * modified by this transaction [j_list_lock, no locks needed for jbd2 thread]
 	 */
 	struct journal_head	*t_reserved_list;
 
 	/*
 	 * Doubly-linked circular list of all metadata buffers owned by this
-	 * transaction [j_list_lock]
+	 * transaction [j_list_lock, no locks needed for jbd2 thread]
 	 */
 	struct journal_head	*t_buffers;
 
@@ -631,7 +631,7 @@ struct transaction_s
 	/*
 	 * Doubly-linked circular list of metadata buffers being shadowed by log
 	 * IO.  The IO buffers on the iobuf list and the shadow buffers on this
-	 * list match each other one for one at all times. [j_list_lock]
+	 * list match each other one for one at all times. [j_list_lock, no locks needed for jbd2 thread]
 	 */
 	struct journal_head	*t_shadow_list;
 
-- 
2.20.1