| Message ID | 20210210095740.54881-2-alexander.lochmann@tu-dortmund.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] Updated locking documentation for transaction_t | expand |
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
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
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
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
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
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;