2021-02-10 10:02:02

by Alexander Lochmann

[permalink] [raw]
Subject: [PATCH 1/2] Updated locking documentation for transaction_t

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 <[email protected]>
Signed-off-by: Horst Schirmeier <[email protected]>
---
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


2021-02-11 09:37:24

by Jan Kara

[permalink] [raw]
Subject: Re: [PATCH 1/2] Updated locking documentation for transaction_t

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 <[email protected]>
> Signed-off-by: Horst Schirmeier <[email protected]>

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 <[email protected]>
SUSE Labs, CR

2021-02-11 09:58:06

by Alexander Lochmann

[permalink] [raw]
Subject: Re: [PATCH 1/2] Updated locking documentation for transaction_t



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


Attachments:
OpenPGP_signature (855.00 B)
OpenPGP digital signature

2021-02-11 13:19:34

by Jan Kara

[permalink] [raw]
Subject: Re: [PATCH 1/2] Updated locking documentation for transaction_t

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 <[email protected]>
SUSE Labs, CR