Messages in this thread | | | Date | Mon, 11 Mar 2019 17:15:25 +0100 | From | Jan Kara <> | Subject | Re: [PATCH 1/1] Updated locking documentation for struct journal_t |
| |
On Fri 08-03-19 16:06:05, Alexander Lochmann wrote: > We used LockDoc to derive locking rules for each member > of struct journal_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> > --- > include/linux/jbd2.h | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-)
Thanks for the patch! Some comments below.
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h > index 0f919d5fe84f..c1ba44ca515a 100644 > --- a/include/linux/jbd2.h > +++ b/include/linux/jbd2.h > @@ -585,7 +585,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 +648,17 @@ struct transaction_s > unsigned long t_max_wait; > > /* > - * When transaction started > + * When transaction started [j_state_lock] > */ > unsigned long t_start; > > /* > - * When commit was requested > + * When commit was requested [j_state_lock] > */ > unsigned long t_requested; > > /* > - * Checkpointing stats [j_checkpoint_sem] > + * Checkpointing stats [j_list_lock] > */ > struct transaction_chp_stats_s t_chp_stats; >
These look correct to me.
> @@ -762,6 +764,7 @@ struct journal_s > > /** > * @j_sb_buffer: The first part of the superblock buffer. > + * [j_checkpoint_mutex] > */ > struct buffer_head *j_sb_buffer;
This isn't true. j_sb_buffer is just initialized in journal_init_common() and never changed. So there's no lock protecting it.
> @@ -1015,6 +1018,7 @@ struct journal_s > * @j_commit_interval: > * > * What is the maximum transaction lifetime before we begin a commit? > + * [super_block->s_umount] > */ > unsigned long j_commit_interval;
Ditto here.
> @@ -1032,7 +1036,7 @@ struct journal_s > * @j_revoke: > * > * The revoke table - maintains the list of revoked blocks in the > - * current transaction. > + * current transaction. [j_state_lock] > */ > struct jbd2_revoke_table_s *j_revoke; >
This is also not true. The contents of the revoke table itself is protected by j_revoke_lock. The pointer to current revoke table (i.e. j_revoke itself) is changed only when the currently running transation is in T_SWITCH state - a special state where no updates can be happening anymore and new transaction cannot be running yet either.
Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR
| |