lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sun, 7 Apr 2019 12:52:06 -0400
From:   "Theodore Ts'o" <tytso@....edu>
To:     Alexander Lochmann <alexander.lochmann@...dortmund.de>
Cc:     Horst Schirmeier <horst.schirmeier@...dortmund.de>,
        Jan Kara <jack@...e.com>, linux-ext4@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [v2] Updated locking documentation for transaction_t

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

Powered by blists - more mailing lists