linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 2/2] Updated locking documentation for journal_t
@ 2021-02-10  9:57 Alexander Lochmann
  2021-02-10  9:57 ` [PATCH 1/2] Updated locking documentation for transaction_t Alexander Lochmann
  2021-02-11  9:37 ` [PATCH 2/2] Updated locking documentation for journal_t Jan Kara
  0 siblings, 2 replies; 12+ messages in thread
From: Alexander Lochmann @ 2021-02-10  9:57 UTC (permalink / raw)
  Cc: Alexander Lochmann, Horst Schirmeier, Theodore Ts'o,
	Jan Kara, linux-ext4, linux-kernel

Some members of transaction_t are allowed to be read without
any lock being held if consistency doesn't matter.
Based on LockDoc's findings, we extended the locking
documentation of those members.
Each one of them is marked with a short comment:
"no lock for quick racy checks".

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, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 18f77d9b1745..4dca33a063dd 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -768,7 +768,7 @@ enum passtype {PASS_SCAN, PASS_REVOKE, PASS_REPLAY};
 struct journal_s
 {
 	/**
-	 * @j_flags: General journaling state flags [j_state_lock]
+	 * @j_flags: General journaling state flags [j_state_lock, no lock for quick racy checks]
 	 */
 	unsigned long		j_flags;
 
@@ -808,7 +808,7 @@ struct journal_s
 	/**
 	 * @j_barrier_count:
 	 *
-	 * Number of processes waiting to create a barrier lock [j_state_lock]
+	 * Number of processes waiting to create a barrier lock [j_state_lock, no lock for quick racy checks]
 	 */
 	int			j_barrier_count;
 
@@ -821,7 +821,7 @@ struct journal_s
 	 * @j_running_transaction:
 	 *
 	 * Transactions: The current running transaction...
-	 * [j_state_lock] [caller holding open handle]
+	 * [j_state_lock, no lock for quick racy checks] [caller holding open handle]
 	 */
 	transaction_t		*j_running_transaction;
 
@@ -1033,7 +1033,7 @@ struct journal_s
 	 * @j_commit_sequence:
 	 *
 	 * Sequence number of the most recently committed transaction
-	 * [j_state_lock].
+	 * [j_state_lock, no lock for quick racy checks].
 	 */
 	tid_t			j_commit_sequence;
 
@@ -1041,7 +1041,7 @@ struct journal_s
 	 * @j_commit_request:
 	 *
 	 * Sequence number of the most recent transaction wanting commit
-	 * [j_state_lock]
+	 * [j_state_lock, no lock for quick racy checks]
 	 */
 	tid_t			j_commit_request;
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 1/2] Updated locking documentation for transaction_t
  2021-02-10  9:57 [PATCH 2/2] Updated locking documentation for journal_t Alexander Lochmann
@ 2021-02-10  9:57 ` Alexander Lochmann
  2021-02-11  9:30   ` Jan Kara
  2021-02-11  9:37 ` [PATCH 2/2] Updated locking documentation for journal_t Jan Kara
  1 sibling, 1 reply; 12+ messages in thread
From: Alexander Lochmann @ 2021-02-10  9:57 UTC (permalink / raw)
  Cc: Alexander Lochmann, Horst Schirmeier, Theodore Ts'o,
	Jan Kara, linux-ext4, linux-kernel

Some members of transaction_t are allowed to be read without
any lock being held if consistency doesn't matter.
Based on LockDoc's findings, we extended the locking
documentation of those members.
Each one of them is marked with a short comment:
"no lock for quick racy checks".

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..18f77d9b1745 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 lock for quick racy checks] */
 	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 lock for quick racy checks]
 	 */
 	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 lock for quick racy checks]
 	 */
 	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 lock for quick racy checks]
 	 */
 	struct journal_head	*t_shadow_list;
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] Updated locking documentation for transaction_t
  2021-02-10  9:57 ` [PATCH 1/2] Updated locking documentation for transaction_t Alexander Lochmann
@ 2021-02-11  9:30   ` Jan Kara
  2021-02-11  9:53     ` Alexander Lochmann
  2021-03-26  8:18     ` Alexander Lochmann
  0 siblings, 2 replies; 12+ messages in thread
From: Jan Kara @ 2021-02-11  9:30 UTC (permalink / raw)
  To: Alexander Lochmann
  Cc: Horst Schirmeier, Theodore Ts'o, Jan Kara, linux-ext4, linux-kernel

On Wed 10-02-21 10:57:39, Alexander Lochmann wrote:
> Some members of transaction_t are allowed to be read without
> any lock being held if consistency doesn't matter.
> Based on LockDoc's findings, we extended the locking
> documentation of those members.
> Each one of them is marked with a short comment:
> "no lock for quick racy checks".
> 
> Signed-off-by: Alexander Lochmann <alexander.lochmann@tu-dortmund.de>
> Signed-off-by: Horst Schirmeier <horst.schirmeier@tu-dortmund.de>

Thanks for the patch! Some comments below...

> ---
>  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..18f77d9b1745 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 lock for quick racy checks] */
>  	int			t_nr_buffers;

So this case is actually somewhat different now that I audited the uses.
There are two types of users - commit code (fs/jbd2/commit.c) and others.
Other users properly use j_list_lock to access t_nr_buffers. Commit code
does not use any locks because committing transaction is fully in
ownership of the jbd2 thread and all other users need to check & wait for
commit to be finished before doing anything with the transaction's 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 lock for quick racy checks]
>  	 */
>  	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 lock for quick racy checks]
>  	 */
>  	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 lock for quick racy checks]
>  	 */
>  	struct journal_head	*t_shadow_list;

The above three cases are the same as t_reserved_list.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] Updated locking documentation for journal_t
  2021-02-10  9:57 [PATCH 2/2] Updated locking documentation for journal_t Alexander Lochmann
  2021-02-10  9:57 ` [PATCH 1/2] Updated locking documentation for transaction_t Alexander Lochmann
@ 2021-02-11  9:37 ` Jan Kara
  2021-02-11  9:51   ` [PATCH v2] " Alexander Lochmann
  1 sibling, 1 reply; 12+ messages in thread
From: Jan Kara @ 2021-02-11  9:37 UTC (permalink / raw)
  To: Alexander Lochmann
  Cc: Horst Schirmeier, Theodore Ts'o, Jan Kara, linux-ext4, linux-kernel

On Wed 10-02-21 10:57:38, Alexander Lochmann wrote:
> Some members of transaction_t are allowed to be read without
> any lock being held if consistency doesn't matter.
> Based on LockDoc's findings, we extended the locking
> documentation of those members.
> Each one of them is marked with a short comment:
> "no lock for quick racy checks".
> 
> Signed-off-by: Alexander Lochmann <alexander.lochmann@tu-dortmund.de>
> Signed-off-by: Horst Schirmeier <horst.schirmeier@tu-dortmund.de>

This patch looks good. You can add:

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

								Honza

> ---
>  include/linux/jbd2.h | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 18f77d9b1745..4dca33a063dd 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -768,7 +768,7 @@ enum passtype {PASS_SCAN, PASS_REVOKE, PASS_REPLAY};
>  struct journal_s
>  {
>  	/**
> -	 * @j_flags: General journaling state flags [j_state_lock]
> +	 * @j_flags: General journaling state flags [j_state_lock, no lock for quick racy checks]
>  	 */
>  	unsigned long		j_flags;
>  
> @@ -808,7 +808,7 @@ struct journal_s
>  	/**
>  	 * @j_barrier_count:
>  	 *
> -	 * Number of processes waiting to create a barrier lock [j_state_lock]
> +	 * Number of processes waiting to create a barrier lock [j_state_lock, no lock for quick racy checks]
>  	 */
>  	int			j_barrier_count;
>  
> @@ -821,7 +821,7 @@ struct journal_s
>  	 * @j_running_transaction:
>  	 *
>  	 * Transactions: The current running transaction...
> -	 * [j_state_lock] [caller holding open handle]
> +	 * [j_state_lock, no lock for quick racy checks] [caller holding open handle]
>  	 */
>  	transaction_t		*j_running_transaction;
>  
> @@ -1033,7 +1033,7 @@ struct journal_s
>  	 * @j_commit_sequence:
>  	 *
>  	 * Sequence number of the most recently committed transaction
> -	 * [j_state_lock].
> +	 * [j_state_lock, no lock for quick racy checks].
>  	 */
>  	tid_t			j_commit_sequence;
>  
> @@ -1041,7 +1041,7 @@ struct journal_s
>  	 * @j_commit_request:
>  	 *
>  	 * Sequence number of the most recent transaction wanting commit
> -	 * [j_state_lock]
> +	 * [j_state_lock, no lock for quick racy checks]
>  	 */
>  	tid_t			j_commit_request;
>  
> -- 
> 2.20.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2] Updated locking documentation for journal_t
  2021-02-11  9:37 ` [PATCH 2/2] Updated locking documentation for journal_t Jan Kara
@ 2021-02-11  9:51   ` Alexander Lochmann
  2021-03-17 20:57     ` Alexander Lochmann
  2021-04-02 15:40     ` Theodore Ts'o
  0 siblings, 2 replies; 12+ messages in thread
From: Alexander Lochmann @ 2021-02-11  9:51 UTC (permalink / raw)
  To: Jan Kara
  Cc: Horst Schirmeier, Theodore Ts'o, Jan Kara, linux-ext4, linux-kernel


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

Some members of transaction_t are allowed to be read without
any lock being held if consistency doesn't matter.
Based on LockDoc's findings, we extended the locking
documentation of those members.
Each one of them is marked with a short comment:
"no lock for quick racy checks".

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 | 10 +++++-----
  1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 18f77d9b1745..4dca33a063dd 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -768,7 +768,7 @@ enum passtype {PASS_SCAN, PASS_REVOKE, PASS_REPLAY};
  struct journal_s
  {
  	/**
-	 * @j_flags: General journaling state flags [j_state_lock]
+	 * @j_flags: General journaling state flags [j_state_lock, no lock for 
quick racy checks]
  	 */
  	unsigned long		j_flags;

@@ -808,7 +808,7 @@ struct journal_s
  	/**
  	 * @j_barrier_count:
  	 *
-	 * Number of processes waiting to create a barrier lock [j_state_lock]
+	 * Number of processes waiting to create a barrier lock [j_state_lock, 
no lock for quick racy checks]
  	 */
  	int			j_barrier_count;

@@ -821,7 +821,7 @@ struct journal_s
  	 * @j_running_transaction:
  	 *
  	 * Transactions: The current running transaction...
-	 * [j_state_lock] [caller holding open handle]
+	 * [j_state_lock, no lock for quick racy checks] [caller holding open 
handle]
  	 */
  	transaction_t		*j_running_transaction;

@@ -1033,7 +1033,7 @@ struct journal_s
  	 * @j_commit_sequence:
  	 *
  	 * Sequence number of the most recently committed transaction
-	 * [j_state_lock].
+	 * [j_state_lock, no lock for quick racy checks].
  	 */
  	tid_t			j_commit_sequence;

@@ -1041,7 +1041,7 @@ struct journal_s
  	 * @j_commit_request:
  	 *
  	 * Sequence number of the most recent transaction wanting commit
-	 * [j_state_lock]
+	 * [j_state_lock, no lock for quick racy checks]
  	 */
  	tid_t			j_commit_request;

-- 
2.20.1




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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] Updated locking documentation for transaction_t
  2021-02-11  9:30   ` Jan Kara
@ 2021-02-11  9:53     ` Alexander Lochmann
  2021-02-11 13:13       ` Jan Kara
  2021-03-26  8:18     ` Alexander Lochmann
  1 sibling, 1 reply; 12+ messages in thread
From: Alexander Lochmann @ 2021-02-11  9:53 UTC (permalink / raw)
  To: Jan Kara
  Cc: Horst Schirmeier, Theodore Ts'o, Jan Kara, linux-ext4, linux-kernel


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



On 11.02.21 10:30, Jan Kara wrote:
>>   	 */
>>   	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 lock for quick racy checks] */
>>   	int			t_nr_buffers;
> 
> So this case is actually somewhat different now that I audited the uses.
> There are two types of users - commit code (fs/jbd2/commit.c) and others.
> Other users properly use j_list_lock to access t_nr_buffers. Commit code
> does not use any locks because committing transaction is fully in
> ownership of the jbd2 thread and all other users need to check & wait for
> commit to be finished before doing anything with the transaction's buffers.
Mhm I see.
What about '[..., no locks needed for jbd2 thread]'?

How do the others wait for the commit to be finished?

- 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: 840 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] Updated locking documentation for transaction_t
  2021-02-11  9:53     ` Alexander Lochmann
@ 2021-02-11 13:13       ` Jan Kara
  0 siblings, 0 replies; 12+ messages in thread
From: Jan Kara @ 2021-02-11 13:13 UTC (permalink / raw)
  To: Alexander Lochmann
  Cc: Jan Kara, Horst Schirmeier, Theodore Ts'o, Jan Kara,
	linux-ext4, linux-kernel

On Thu 11-02-21 10:53:51, Alexander Lochmann wrote:
> 
> 
> On 11.02.21 10:30, Jan Kara wrote:
> > >   	 */
> > >   	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 lock for quick racy checks] */
> > >   	int			t_nr_buffers;
> > 
> > So this case is actually somewhat different now that I audited the uses.
> > There are two types of users - commit code (fs/jbd2/commit.c) and others.
> > Other users properly use j_list_lock to access t_nr_buffers. Commit code
> > does not use any locks because committing transaction is fully in
> > ownership of the jbd2 thread and all other users need to check & wait for
> > commit to be finished before doing anything with the transaction's buffers.
> Mhm I see.
> What about '[..., no locks needed for jbd2 thread]'?

Sounds good to me.

> How do the others wait for the commit to be finished?

Well, usually they just don't touch buffers belonging to the committing
transation, they just store in b_next_transaction that after commit is
done, buffer should be added to the currently running transaction. There
are some exceptions though - e.g. jbd2_journal_invalidatepage() (called
from truncate code) which returns EBUSY in some rare cases and we use
jbd2_log_wait_commit() in ext4_wait_for_tail_page_commit() to wait for
commit to be done before we know it is safe to destroy the buffer.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] Updated locking documentation for journal_t
  2021-02-11  9:51   ` [PATCH v2] " Alexander Lochmann
@ 2021-03-17 20:57     ` Alexander Lochmann
  2021-04-02 15:40     ` Theodore Ts'o
  1 sibling, 0 replies; 12+ messages in thread
From: Alexander Lochmann @ 2021-03-17 20:57 UTC (permalink / raw)
  To: Jan Kara
  Cc: Horst Schirmeier, Theodore Ts'o, Jan Kara, linux-ext4, linux-kernel

Does this patch look good to you?
Might it be ready to be merged?

- Alex

On 11.02.21 10:51, Alexander Lochmann wrote:
> Some members of transaction_t are allowed to be read without
> any lock being held if consistency doesn't matter.
> Based on LockDoc's findings, we extended the locking
> documentation of those members.
> Each one of them is marked with a short comment:
> "no lock for quick racy checks".
> 
> 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 | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 18f77d9b1745..4dca33a063dd 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -768,7 +768,7 @@ enum passtype {PASS_SCAN, PASS_REVOKE, PASS_REPLAY};
>  struct journal_s
>  {
>      /**
> -     * @j_flags: General journaling state flags [j_state_lock]
> +     * @j_flags: General journaling state flags [j_state_lock, no lock
> for quick racy checks]
>       */
>      unsigned long        j_flags;
> 
> @@ -808,7 +808,7 @@ struct journal_s
>      /**
>       * @j_barrier_count:
>       *
> -     * Number of processes waiting to create a barrier lock [j_state_lock]
> +     * Number of processes waiting to create a barrier lock
> [j_state_lock, no lock for quick racy checks]
>       */
>      int            j_barrier_count;
> 
> @@ -821,7 +821,7 @@ struct journal_s
>       * @j_running_transaction:
>       *
>       * Transactions: The current running transaction...
> -     * [j_state_lock] [caller holding open handle]
> +     * [j_state_lock, no lock for quick racy checks] [caller holding
> open handle]
>       */
>      transaction_t        *j_running_transaction;
> 
> @@ -1033,7 +1033,7 @@ struct journal_s
>       * @j_commit_sequence:
>       *
>       * Sequence number of the most recently committed transaction
> -     * [j_state_lock].
> +     * [j_state_lock, no lock for quick racy checks].
>       */
>      tid_t            j_commit_sequence;
> 
> @@ -1041,7 +1041,7 @@ struct journal_s
>       * @j_commit_request:
>       *
>       * Sequence number of the most recent transaction wanting commit
> -     * [j_state_lock]
> +     * [j_state_lock, no lock for quick racy checks]
>       */
>      tid_t            j_commit_request;
> 

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] Updated locking documentation for transaction_t
  2021-02-11  9:30   ` Jan Kara
  2021-02-11  9:53     ` Alexander Lochmann
@ 2021-03-26  8:18     ` Alexander Lochmann
  2021-03-29 10:14       ` Jan Kara
  1 sibling, 1 reply; 12+ messages in thread
From: Alexander Lochmann @ 2021-03-26  8:18 UTC (permalink / raw)
  To: Jan Kara
  Cc: Horst Schirmeier, Theodore Ts'o, Jan Kara, linux-ext4, linux-kernel

On 11.02.21 10:30, Jan Kara wrote:
>> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
>> index 99d3cd051ac3..18f77d9b1745 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 lock for quick racy checks] */
>>  	int			t_nr_buffers;
> 
> So this case is actually somewhat different now that I audited the uses.
> There are two types of users - commit code (fs/jbd2/commit.c) and others.
> Other users properly use j_list_lock to access t_nr_buffers. Commit code
> does not use any locks because committing transaction is fully in
> ownership of the jbd2 thread and all other users need to check & wait for
> commit to be finished before doing anything with the transaction's buffers.

I'm still trying understand how thinks work:
Accesses to transaction_t might occur from different contexts. Thus,
locks are necessary. If it comes to the commit phase, every other
context has to wait until jbd2 thread has done its work. Therefore, jbd2
thread does not need any locks to access a transaction_t (or just parts
of it?) during commit phase.
Is that correct?

If so: I was thinking whether it make sense to ignore all memory
accesses to a transaction_t (or parts of it) that happen in the commit
phase. They deliberately ignore the locking policy, and would confuse
our approach.

Is the commit phase performed by jbd2_journal_commit_transaction()?
We would add this function to our blacklist for transaction_t.

- 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] Updated locking documentation for transaction_t
  2021-03-26  8:18     ` Alexander Lochmann
@ 2021-03-29 10:14       ` Jan Kara
  0 siblings, 0 replies; 12+ messages in thread
From: Jan Kara @ 2021-03-29 10:14 UTC (permalink / raw)
  To: Alexander Lochmann
  Cc: Jan Kara, Horst Schirmeier, Theodore Ts'o, Jan Kara,
	linux-ext4, linux-kernel

On Fri 26-03-21 09:18:45, Alexander Lochmann wrote:
> On 11.02.21 10:30, Jan Kara wrote:
> >> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> >> index 99d3cd051ac3..18f77d9b1745 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 lock for quick racy checks] */
> >>  	int			t_nr_buffers;
> > 
> > So this case is actually somewhat different now that I audited the uses.
> > There are two types of users - commit code (fs/jbd2/commit.c) and others.
> > Other users properly use j_list_lock to access t_nr_buffers. Commit code
> > does not use any locks because committing transaction is fully in
> > ownership of the jbd2 thread and all other users need to check & wait for
> > commit to be finished before doing anything with the transaction's buffers.
> 
> I'm still trying understand how thinks work:
> Accesses to transaction_t might occur from different contexts. Thus,
> locks are necessary. If it comes to the commit phase, every other
> context has to wait until jbd2 thread has done its work. Therefore, jbd2
> thread does not need any locks to access a transaction_t (or just parts
> of it?) during commit phase.
> Is that correct?

Yes, that is correct.

> If so: I was thinking whether it make sense to ignore all memory
> accesses to a transaction_t (or parts of it) that happen in the commit
> phase. They deliberately ignore the locking policy, and would confuse
> our approach.
> 
> Is the commit phase performed by jbd2_journal_commit_transaction()?
> We would add this function to our blacklist for transaction_t.

Yes, commit phase is implemented by jbd2_journal_commit_transaction() and
the functions it calls.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] Updated locking documentation for journal_t
  2021-02-11  9:51   ` [PATCH v2] " Alexander Lochmann
  2021-03-17 20:57     ` Alexander Lochmann
@ 2021-04-02 15:40     ` Theodore Ts'o
  1 sibling, 0 replies; 12+ messages in thread
From: Theodore Ts'o @ 2021-04-02 15:40 UTC (permalink / raw)
  To: Alexander Lochmann
  Cc: Jan Kara, Horst Schirmeier, Jan Kara, linux-ext4, linux-kernel

On Thu, Feb 11, 2021 at 10:51:55AM +0100, Alexander Lochmann wrote:
> Some members of transaction_t are allowed to be read without
> any lock being held if consistency doesn't matter.
> Based on LockDoc's findings, we extended the locking
> documentation of those members.
> Each one of them is marked with a short comment:
> "no lock for quick racy checks".
> 
> 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 had to fix up the patch, which was mailer-damaged,
and I also reflowed the comments.

					- Ted

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/2] Updated locking documentation for transaction_t
@ 2021-02-10  9:56 Alexander Lochmann
  0 siblings, 0 replies; 12+ messages in thread
From: Alexander Lochmann @ 2021-02-10  9:56 UTC (permalink / raw)
  Cc: Alexander Lochmann, Horst Schirmeier, Theodore Ts'o,
	Jan Kara, linux-ext4, linux-kernel

Some members of transaction_t are allowed to be read without
any lock being held if consistency doesn't matter.
Based on LockDoc's findings, we extended the locking
documentation of those members.
Each one of them is marked with a short comment:
"no lock for quick racy checks".

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..18f77d9b1745 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 lock for quick racy checks] */
 	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 lock for quick racy checks]
 	 */
 	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 lock for quick racy checks]
 	 */
 	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 lock for quick racy checks]
 	 */
 	struct journal_head	*t_shadow_list;
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-04-02 15:40 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-10  9:57 [PATCH 2/2] Updated locking documentation for journal_t Alexander Lochmann
2021-02-10  9:57 ` [PATCH 1/2] Updated locking documentation for transaction_t Alexander Lochmann
2021-02-11  9:30   ` Jan Kara
2021-02-11  9:53     ` Alexander Lochmann
2021-02-11 13:13       ` Jan Kara
2021-03-26  8:18     ` Alexander Lochmann
2021-03-29 10:14       ` Jan Kara
2021-02-11  9:37 ` [PATCH 2/2] Updated locking documentation for journal_t Jan Kara
2021-02-11  9:51   ` [PATCH v2] " Alexander Lochmann
2021-03-17 20:57     ` Alexander Lochmann
2021-04-02 15:40     ` Theodore Ts'o
  -- strict thread matches above, loose matches on Subject: below --
2021-02-10  9:56 [PATCH 1/2] Updated locking documentation for transaction_t Alexander Lochmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).