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