Re: [PATCH 13/15] xfs: simplify xfs_trans_getsb

From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 13/15] xfs: simplify xfs_trans_getsb
Date: Tue, 1 Sep 2020 09:58:21 -0700	[thread overview]
Message-ID: <20200901165821.GF6096@magnolia> (raw)
In-Reply-To: <20200901155018.2524-14-hch@lst.de>

On Tue, Sep 01, 2020 at 05:50:16PM +0200, Christoph Hellwig wrote:
> Remove the mp argument as this function is only called in transaction
> context, and open code xfs_getsb given that the function already accesses
> the buffer pointer in the mount point directly.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks pretty straightforward to me,

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/libxfs/xfs_sb.c |  4 ++--
>  fs/xfs/xfs_trans.c     |  2 +-
>  fs/xfs/xfs_trans.h     |  2 +-
>  fs/xfs/xfs_trans_buf.c | 46 ++++++++++++++----------------------------
>  4 files changed, 19 insertions(+), 35 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index ae9aaf1f34bfcc..15d03d96753769 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -954,7 +954,7 @@ xfs_log_sb(
>  	struct xfs_trans	*tp)
>  {
>  	struct xfs_mount	*mp = tp->t_mountp;
> -	struct xfs_buf		*bp = xfs_trans_getsb(tp, mp);
> +	struct xfs_buf		*bp = xfs_trans_getsb(tp);
>  
>  	mp->m_sb.sb_icount = percpu_counter_sum(&mp->m_icount);
>  	mp->m_sb.sb_ifree = percpu_counter_sum(&mp->m_ifree);
> @@ -1084,7 +1084,7 @@ xfs_sync_sb_buf(
>  	if (error)
>  		return error;
>  
> -	bp = xfs_trans_getsb(tp, mp);
> +	bp = xfs_trans_getsb(tp);
>  	xfs_log_sb(tp);
>  	xfs_trans_bhold(tp, bp);
>  	xfs_trans_set_sync(tp);
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index ed72867b1a1937..ca18a040336a1d 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -468,7 +468,7 @@ xfs_trans_apply_sb_deltas(
>  	xfs_buf_t	*bp;
>  	int		whole = 0;
>  
> -	bp = xfs_trans_getsb(tp, tp->t_mountp);
> +	bp = xfs_trans_getsb(tp);
>  	sbp = bp->b_addr;
>  
>  	/*
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index b752501818d257..f46534b7523698 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -209,7 +209,7 @@ xfs_trans_read_buf(
>  				      flags, bpp, ops);
>  }
>  
> -struct xfs_buf	*xfs_trans_getsb(xfs_trans_t *, struct xfs_mount *);
> +struct xfs_buf	*xfs_trans_getsb(struct xfs_trans *);
>  
>  void		xfs_trans_brelse(xfs_trans_t *, struct xfs_buf *);
>  void		xfs_trans_bjoin(xfs_trans_t *, struct xfs_buf *);
> diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
> index 11cd666cd99a63..42d63b830cb9c8 100644
> --- a/fs/xfs/xfs_trans_buf.c
> +++ b/fs/xfs/xfs_trans_buf.c
> @@ -166,50 +166,34 @@ xfs_trans_get_buf_map(
>  }
>  
>  /*
> - * Get and lock the superblock buffer of this file system for the
> - * given transaction.
> - *
> - * We don't need to use incore_match() here, because the superblock
> - * buffer is a private buffer which we keep a pointer to in the
> - * mount structure.
> + * Get and lock the superblock buffer for the given transaction.
>   */
> -xfs_buf_t *
> +struct xfs_buf *
>  xfs_trans_getsb(
> -	xfs_trans_t		*tp,
> -	struct xfs_mount	*mp)
> +	struct xfs_trans	*tp)
>  {
> -	xfs_buf_t		*bp;
> -	struct xfs_buf_log_item	*bip;
> +	struct xfs_buf		*bp = tp->t_mountp->m_sb_bp;
>  
>  	/*
> -	 * Default to just trying to lock the superblock buffer
> -	 * if tp is NULL.
> +	 * Just increment the lock recursion count if the buffer is already
> +	 * attached to this transaction.
>  	 */
> -	if (tp == NULL)
> -		return xfs_getsb(mp);
> -
> -	/*
> -	 * If the superblock buffer already has this transaction
> -	 * pointer in its b_fsprivate2 field, then we know we already
> -	 * have it locked.  In this case we just increment the lock
> -	 * recursion count and return the buffer to the caller.
> -	 */
> -	bp = mp->m_sb_bp;
>  	if (bp->b_transp == tp) {
> -		bip = bp->b_log_item;
> +		struct xfs_buf_log_item	*bip = bp->b_log_item;
> +
>  		ASSERT(bip != NULL);
>  		ASSERT(atomic_read(&bip->bli_refcount) > 0);
>  		bip->bli_recur++;
> +
>  		trace_xfs_trans_getsb_recur(bip);
> -		return bp;
> -	}
> +	} else {
> +		xfs_buf_lock(bp);
> +		xfs_buf_hold(bp);
> +		_xfs_trans_bjoin(tp, bp, 1);
>  
> -	bp = xfs_getsb(mp);
> -	if (bp == NULL)
> -		return NULL;
> +		trace_xfs_trans_getsb(bp->b_log_item);
> +	}
>  
> -	_xfs_trans_bjoin(tp, bp, 1);
> -	trace_xfs_trans_getsb(bp->b_log_item);
>  	return bp;
>  }
>  
> -- 
> 2.28.0
> 