Re: [PATCH v2 3/5] btrfs: Add lockdep models for the transaction states wait events

[Josef Bacik <josef@toxicpanda.com>]

On Mon, Jul 18, 2022 at 09:09:56PM -0700, Ioannis Angelakopoulos wrote:
> Add a lockdep annotation for the transaction states that have wait
> events; 1) TRANS_STATE_COMMIT_START, 2) TRANS_STATE_UNBLOCKED, 3)
> TRANS_STATE_SUPER_COMMITTED, and 4) TRANS_STATE_COMPLETED in
> fs/btrfs/transaction.c.
> 
> With the exception of the lockdep annotation for TRANS_STATE_COMMIT_START
> the transaction thread has to acquire the lockdep maps for the transaction
> states as reader after the lockdep map for num_writers is released so that
> lockdep does not complain.
> 
> 
> Signed-off-by: Ioannis Angelakopoulos <iangelak@fb.com>
> ---
>  fs/btrfs/ctree.h       | 20 +++++++++++++++
>  fs/btrfs/disk-io.c     | 17 +++++++++++++
>  fs/btrfs/transaction.c | 57 +++++++++++++++++++++++++++++++++++++-----
>  3 files changed, 88 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 586756f831e5..e6c7cafcd296 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -1097,6 +1097,7 @@ struct btrfs_fs_info {
>  
>  	struct lockdep_map btrfs_trans_num_writers_map;
>  	struct lockdep_map btrfs_trans_num_extwriters_map;
> +	struct lockdep_map btrfs_state_change_map[4];
>  
>  #ifdef CONFIG_BTRFS_FS_REF_VERIFY
>  	spinlock_t ref_verify_lock;
> @@ -1178,6 +1179,13 @@ enum {
>  	BTRFS_ROOT_UNFINISHED_DROP,
>  };
>  
> +enum btrfs_lockdep_trans_states {
> +	BTRFS_LOCKDEP_TRANS_COMMIT_START,
> +	BTRFS_LOCKDEP_TRANS_UNBLOCKED,
> +	BTRFS_LOCKDEP_TRANS_SUPER_COMMITTED,
> +	BTRFS_LOCKDEP_TRANS_COMPLETED,
> +};
> +
>  #define btrfs_might_wait_for_event(b, lock) \
>  	do { \
>  		rwsem_acquire(&b->lock##_map, 0, 0, _THIS_IP_); \
> @@ -1190,6 +1198,18 @@ enum {
>  #define btrfs_lockdep_release(b, lock) \
>  	rwsem_release(&b->lock##_map, _THIS_IP_)
>  
> +#define btrfs_might_wait_for_state(b, i) \
> +	do { \
> +		rwsem_acquire(&b->btrfs_state_change_map[i], 0, 0, _THIS_IP_); \
> +		rwsem_release(&b->btrfs_state_change_map[i], _THIS_IP_); \
> +	} while (0)
> +
> +#define btrfs_trans_state_lockdep_acquire(b, i) \
> +	rwsem_acquire_read(&b->btrfs_state_change_map[i], 0, 0, _THIS_IP_)
> +
> +#define btrfs_trans_state_lockdep_release(b, i) \
> +	rwsem_release(&b->btrfs_state_change_map[i], _THIS_IP_)
> +
>  static inline void btrfs_wake_unfinished_drop(struct btrfs_fs_info *fs_info)
>  {
>  	clear_and_wake_up_bit(BTRFS_FS_UNFINISHED_DROPS, &fs_info->flags);
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index b1193584ba49..be5cf86fa992 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -3048,6 +3048,10 @@ void btrfs_init_fs_info(struct btrfs_fs_info *fs_info)
>  {
>  	static struct lock_class_key btrfs_trans_num_writers_key;
>  	static struct lock_class_key btrfs_trans_num_extwriters_key;
> +	static struct lock_class_key btrfs_trans_commit_start_key;
> +	static struct lock_class_key btrfs_trans_unblocked_key;
> +	static struct lock_class_key btrfs_trans_sup_committed_key;
> +	static struct lock_class_key btrfs_trans_completed_key;
>  
>  	INIT_RADIX_TREE(&fs_info->fs_roots_radix, GFP_ATOMIC);
>  	INIT_RADIX_TREE(&fs_info->buffer_radix, GFP_ATOMIC);
> @@ -3084,6 +3088,19 @@ void btrfs_init_fs_info(struct btrfs_fs_info *fs_info)
>  					 "btrfs_trans_num_extwriters",
>  					 &btrfs_trans_num_extwriters_key, 0);
>  
> +	lockdep_init_map(&fs_info->btrfs_state_change_map[0],
> +					 "btrfs_trans_commit_start",
> +					 &btrfs_trans_commit_start_key, 0);
> +	lockdep_init_map(&fs_info->btrfs_state_change_map[1],
> +					 "btrfs_trans_unblocked",
> +					 &btrfs_trans_unblocked_key, 0);
> +	lockdep_init_map(&fs_info->btrfs_state_change_map[2],
> +					 "btrfs_trans_sup_commited",

s/commited/committed/

> +					 &btrfs_trans_sup_committed_key, 0);
> +	lockdep_init_map(&fs_info->btrfs_state_change_map[3],
> +					 "btrfs_trans_completed",
> +					 &btrfs_trans_completed_key, 0);
> +
>  	INIT_LIST_HEAD(&fs_info->dirty_cowonly_roots);
>  	INIT_LIST_HEAD(&fs_info->space_info);
>  	INIT_LIST_HEAD(&fs_info->tree_mod_seq_list);
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index c9751a05c029..e4efaa27ec17 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -550,6 +550,7 @@ static void wait_current_trans(struct btrfs_fs_info *fs_info)
>  		refcount_inc(&cur_trans->use_count);
>  		spin_unlock(&fs_info->trans_lock);
>  
> +		btrfs_might_wait_for_state(fs_info, BTRFS_LOCKDEP_TRANS_UNBLOCKED);
>  		wait_event(fs_info->transaction_wait,
>  			   cur_trans->state >= TRANS_STATE_UNBLOCKED ||
>  			   TRANS_ABORTED(cur_trans));
> @@ -949,6 +950,7 @@ int btrfs_wait_for_commit(struct btrfs_fs_info *fs_info, u64 transid)
>  			goto out;  /* nothing committing|committed */
>  	}
>  
> +	btrfs_might_wait_for_state(fs_info, BTRFS_LOCKDEP_TRANS_COMPLETED);
>  	wait_for_commit(cur_trans, TRANS_STATE_COMPLETED);
>  	btrfs_put_transaction(cur_trans);
>  out:
> @@ -1980,6 +1982,7 @@ void btrfs_commit_transaction_async(struct btrfs_trans_handle *trans)
>  	 * Wait for the current transaction commit to start and block
>  	 * subsequent transaction joins
>  	 */
> +	btrfs_might_wait_for_state(fs_info, BTRFS_LOCKDEP_TRANS_COMMIT_START);
>  	wait_event(fs_info->transaction_blocked_wait,
>  		   cur_trans->state >= TRANS_STATE_COMMIT_START ||
>  		   TRANS_ABORTED(cur_trans));
> @@ -2137,14 +2140,16 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>  	ktime_t interval;
>  
>  	ASSERT(refcount_read(&trans->use_count) == 1);
> +	btrfs_trans_state_lockdep_acquire(fs_info,
> +					  BTRFS_LOCKDEP_TRANS_COMMIT_START);
>  
>  	/* Stop the commit early if ->aborted is set */
>  	if (TRANS_ABORTED(cur_trans)) {
>  		ret = cur_trans->aborted;
> -		btrfs_end_transaction(trans);
> -		return ret;
> +		goto lockdep_trans_commit_start_release;
>  	}
>  
> +
>  	btrfs_trans_release_metadata(trans);
>  	trans->block_rsv = NULL;
>  
> @@ -2160,8 +2165,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>  		 */
>  		ret = btrfs_run_delayed_refs(trans, 0);
>  		if (ret) {
> -			btrfs_end_transaction(trans);
> -			return ret;
> +			goto lockdep_trans_commit_start_release;
>  		}
>  	}
>  
> @@ -2192,8 +2196,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>  		if (run_it) {
>  			ret = btrfs_start_dirty_block_groups(trans);
>  			if (ret) {
> -				btrfs_end_transaction(trans);
> -				return ret;
> +				goto lockdep_trans_commit_start_release;
>  			}
>  		}
>  	}
> @@ -2209,7 +2212,17 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>  
>  		if (trans->in_fsync)
>  			want_state = TRANS_STATE_SUPER_COMMITTED;
> +
> +		btrfs_trans_state_lockdep_release(fs_info,
> +						  BTRFS_LOCKDEP_TRANS_COMMIT_START);
>  		ret = btrfs_end_transaction(trans);
> +
> +		if (want_state == TRANS_STATE_COMPLETED)
> +			btrfs_might_wait_for_state(fs_info, BTRFS_LOCKDEP_TRANS_COMPLETED);
> +		else
> +			btrfs_might_wait_for_state(fs_info,
> +						   BTRFS_LOCKDEP_TRANS_SUPER_COMMITTED);
> +
>  		wait_for_commit(cur_trans, want_state);
>  
>  		if (TRANS_ABORTED(cur_trans))
> @@ -2222,6 +2235,8 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>  
>  	cur_trans->state = TRANS_STATE_COMMIT_START;
>  	wake_up(&fs_info->transaction_blocked_wait);
> +	btrfs_trans_state_lockdep_release(fs_info,
> +					  BTRFS_LOCKDEP_TRANS_COMMIT_START);
>  
>  	if (cur_trans->list.prev != &fs_info->trans_list) {
>  		enum btrfs_trans_state want_state = TRANS_STATE_COMPLETED;
> @@ -2235,6 +2250,12 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>  			refcount_inc(&prev_trans->use_count);
>  			spin_unlock(&fs_info->trans_lock);
>  
> +			if (want_state == TRANS_STATE_COMPLETED)
> +				btrfs_might_wait_for_state(fs_info,
> +							   BTRFS_LOCKDEP_TRANS_COMPLETED);
> +			else
> +				btrfs_might_wait_for_state(fs_info,
> +							   BTRFS_LOCKDEP_TRANS_SUPER_COMMITTED);

You do this everywhere we call wait_for_commit(), you can push these
btrfs_might_wait_for_state() into wait_for_commit and make everything a bit
cleaner.  Thanks,

Josef