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 <[email protected]>
Signed-off-by: Horst Schirmeier <[email protected]>
---
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 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 <[email protected]>
> Signed-off-by: Horst Schirmeier <[email protected]>
Thanks! You can add:
Reviewed-by: Jan Kara <[email protected]>
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
>
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
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
>
--
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