diff mbox series

[1/2] Updated locking documentation for transaction_t

Message ID 20210210095740.54881-2-alexander.lochmann@tu-dortmund.de
State New
Headers show
Series [1/2] Updated locking documentation for transaction_t | expand

Commit Message

Alexander Lochmann Feb. 10, 2021, 9:57 a.m. UTC
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(-)

Comments

Jan Kara Feb. 11, 2021, 9:30 a.m. UTC | #1
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
Alexander Lochmann Feb. 11, 2021, 9:53 a.m. UTC | #2
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
Jan Kara Feb. 11, 2021, 1:13 p.m. UTC | #3
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
diff mbox series

Patch

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;