[PATCH v3] Updated locking documentation for transaction_t

[PATCH v3] 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>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 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

Re: [PATCH v3] Updated locking documentation for transaction_t

[Alexander Lochmann]

Does this patch look good to you either?

- Alex

On 11.02.21 18:14, 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>
> Reviewed-by: Jan Kara <jack@suse.cz>
> ---
>  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;
>  
> 

-- 
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: [PATCH v3] Updated locking documentation for transaction_t

[Theodore Ts'o]

On Thu, Feb 11, 2021 at 06:14:10PM +0100, 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>
> Reviewed-by: Jan Kara <jack@suse.cz>

Thanks, applied.  I reflowed the comments before applying the patch.

		    	     	 	  - Ted

[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