Re: [PATCH] btrfs: reduce the scope of the tree log mutex during transaction commit

[Josef Bacik <josef@toxicpanda.com>]

On Wed, Nov 10, 2021 at 10:05:21AM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> In the transaction commit path we are acquiring the tree log mutex too
> early and we have a stale comment because:
> 
> 1) It mentions a function named btrfs_commit_tree_roots(), which does not
>    exists anymore, it was the old name of commit_cowonly_roots(), renamed
>    a very long time ago by commit 5d4f98a28c7d33 ("Btrfs: Mixed back
>    reference  (FORWARD ROLLING FORMAT CHANGE)"));
> 
> 2) It mentions that we need to acquire the tree log mutex at that point
>    to ensure we have no running log writers. That is not correct anymore,
>    for many years at least, since we are guaranteed that we do not have
>    any log writers at that point simply because we have set the state of
>    the transaction to TRANS_STATE_COMMIT_DOING and have waited for all
>    writers to complete - meaning no one can log until we change the state
>    of the transaction to TRANS_STATE_UNBLOCKED. Any attempts to join the
>    transaction or start a new one will block until we do that state
>    transition;
> 
> 3) The comment mentions a "trans mutex" which doesn't exists since 2011,
>    commit a4abeea41adf ("Btrfs: kill trans_mutex") removed it;
> 
> 4) The current use of the tree log mutex is to ensure proper serialization
>    of super block writes - if someone started a new transaction and uses it
>    for logging, it will wait for the previous transaction to write its
>    super block before writing the super block when attempting to sync the
>    log.
> 
> So acquire the tree log mutex only when it's absolutely needed, before
> setting the transaction state to TRANS_STATE_UNBLOCKED, fix and move the
> stale comment, add some assertions and new comments where appropriate.
> 
> Also, this has no effect on concurrency or performance, since the new
> start of the critical section is still when the transaction is in the
> state TRANS_STATE_COMMIT_DOING.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef