Message ID | 20190318184237.20677-1-alexander.lochmann@tu-dortmund.de |
---|---|
State | New |
Headers | show |
Series | [v2] Updated locking documentation for transaction_t | expand |
On Mon 18-03-19 19:42:37, Alexander Lochmann wrote: > We used LockDoc to derive locking rules for each member > of struct transaction_t. > Based on those results, we extended the existing documentation > by more members of struct inode, and updated the existing > documentation. > > Signed-off-by: Alexander Lochmann <alexander.lochmann@tu-dortmund.de> > Signed-off-by: Horst Schirmeier <horst.schirmeier@tu-dortmund.de> Thanks! You can add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > include/linux/jbd2.h | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h > index 0f919d5fe84f..72f689746340 100644 > --- a/include/linux/jbd2.h > +++ b/include/linux/jbd2.h > @@ -534,6 +534,7 @@ struct transaction_chp_stats_s { > * The transaction keeps track of all of the buffers modified by a > * running transaction, and all of the buffers committed but not yet > * flushed to home for finished transactions. > + * (Locking Documentation improved by LockDoc) > */ > > /* > @@ -585,7 +586,8 @@ struct transaction_s > } t_state; > > /* > - * Where in the log does this transaction's commit start? [no locking] > + * Where in the log does this transaction's commit start? > + * [journal_t.j_state_lock] > */ > unsigned long t_log_start; > > @@ -647,17 +649,17 @@ struct transaction_s > unsigned long t_max_wait; > > /* > - * When transaction started > + * When transaction started [journal_t.j_state_lock] > */ > unsigned long t_start; > > /* > - * When commit was requested > + * When commit was requested [journal_t.j_state_lock] > */ > unsigned long t_requested; > > /* > - * Checkpointing stats [j_checkpoint_sem] > + * Checkpointing stats [journal_t.j_list_lock] > */ > struct transaction_chp_stats_s t_chp_stats; > > -- > 2.20.1 > >
On Mon, Mar 18, 2019 at 07:42:37PM +0100, Alexander Lochmann wrote: > /*t > - * Where in the log does this transaction's commit start? [no locking] > + * Where in the log does this transaction's commit start? > + * [journal_t.j_state_lock] > */ > unsigned long t_log_start; Well, technically, that's not quite right. It's only assigned in one location, and we hold j_state_lock, yes. But that's because we need to access journal->j_head. At the point where we set t_log_start, the transaction has already been locked down (transaction->t_state > T_LOCKED). Similarly, we happen to be holding j_state where it is currently being accessed, but it's not because we needed the lock in order to access t_log_start safely. > /* > - * When transaction started > + * When transaction started [journal_t.j_state_lock] > */ > unsigned long t_start; And again, not really. The primary place where t_start is set is when the transaction is firstt created, before it's visible anywhere else. after that, it is used exclusively by the commit thread, and so no locking is necessary. It's true that in the places where it is used, j_state_lock happens to be taken, but it's strictly not necessary. > > /* > - * When commit was requested > + * When commit was requested [journal_t.j_state_lock] > */ > unsigned long t_requested; Yes, that appears to be correct. > > /* > - * Checkpointing stats [j_checkpoint_sem] > + * Checkpointing stats [journal_t.j_list_lock] > */ > struct transaction_chp_stats_s t_chp_stats; > This appears to be correct. - Ted
Thanks, Ted, for your feedback! I'll submit a modified version. - Alex On 07.04.19 18:52, Theodore Ts'o wrote: > On Mon, Mar 18, 2019 at 07:42:37PM +0100, Alexander Lochmann wrote: >> /*t >> - * Where in the log does this transaction's commit start? [no locking] >> + * Where in the log does this transaction's commit start? >> + * [journal_t.j_state_lock] >> */ >> unsigned long t_log_start; > > Well, technically, that's not quite right. It's only assigned in one > location, and we hold j_state_lock, yes. But that's because we need > to access journal->j_head. At the point where we set t_log_start, the > transaction has already been locked down (transaction->t_state > > T_LOCKED). > > Similarly, we happen to be holding j_state where it is currently being > accessed, but it's not because we needed the lock in order to access > t_log_start safely. > >> /* >> - * When transaction started >> + * When transaction started [journal_t.j_state_lock] >> */ >> unsigned long t_start; > > And again, not really. The primary place where t_start is set is when > the transaction is firstt created, before it's visible anywhere else. > after that, it is used exclusively by the commit thread, and so no > locking is necessary. It's true that in the places where it is used, > j_state_lock happens to be taken, but it's strictly not necessary. > >> >> /* >> - * When commit was requested >> + * When commit was requested [journal_t.j_state_lock] >> */ >> unsigned long t_requested; > > Yes, that appears to be correct. > >> >> /* >> - * Checkpointing stats [j_checkpoint_sem] >> + * Checkpointing stats [journal_t.j_list_lock] >> */ >> struct transaction_chp_stats_s t_chp_stats; >> > > This appears to be correct. > > - Ted >
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h index 0f919d5fe84f..72f689746340 100644 --- a/include/linux/jbd2.h +++ b/include/linux/jbd2.h @@ -534,6 +534,7 @@ struct transaction_chp_stats_s { * The transaction keeps track of all of the buffers modified by a * running transaction, and all of the buffers committed but not yet * flushed to home for finished transactions. + * (Locking Documentation improved by LockDoc) */ /* @@ -585,7 +586,8 @@ struct transaction_s } t_state; /* - * Where in the log does this transaction's commit start? [no locking] + * Where in the log does this transaction's commit start? + * [journal_t.j_state_lock] */ unsigned long t_log_start; @@ -647,17 +649,17 @@ struct transaction_s unsigned long t_max_wait; /* - * When transaction started + * When transaction started [journal_t.j_state_lock] */ unsigned long t_start; /* - * When commit was requested + * When commit was requested [journal_t.j_state_lock] */ unsigned long t_requested; /* - * Checkpointing stats [j_checkpoint_sem] + * Checkpointing stats [journal_t.j_list_lock] */ struct transaction_chp_stats_s t_chp_stats;