Re: [PATCH RFC 1/2] xfs: refactor buffer logging into buffer dirtying helper

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Mon, Aug 14, 2017 at 12:54:50PM -0400, Brian Foster wrote:
> xfs_trans_log_buf() is responsible for logging the dirty segments of
> a buffer along with setting all of the necessary state on the
> transaction, buffer, bli, etc., to ensure that the associated items
> are marked as dirty and prepared for I/O. We have a couple use cases
> that need to to dirty a buffer in a transaction without actually
> logging dirty ranges of the buffer.  One existing use case is
> ordered buffers, which are currently logged with arbitrary ranges to
> accomplish this even though the content of ordered buffers is never
> written to the log. Another pending use case is to relog an already
> dirty buffer across rolled transactions within the deferred
> operations infrastructure. This is required to prevent a held
> (XFS_BLI_HOLD) buffer from pinning the tail of the log.
> 
> Refactor xfs_trans_log_buf() into a new function that contains all
> of the logic responsible to dirty the transaction, lidp, buffer and
> bli. This new function can be used in the future for the use cases
> outlined above. This patch does not introduce functional changes.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

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

--D

> ---
>  fs/xfs/xfs_trans.h     |  4 +++-
>  fs/xfs/xfs_trans_buf.c | 46 ++++++++++++++++++++++++++++++----------------
>  2 files changed, 33 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index 6bdad6f..a9c4404 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -213,7 +213,9 @@ void		xfs_trans_dquot_buf(xfs_trans_t *, struct xfs_buf *, uint);
>  void		xfs_trans_inode_alloc_buf(xfs_trans_t *, struct xfs_buf *);
>  void		xfs_trans_ichgtime(struct xfs_trans *, struct xfs_inode *, int);
>  void		xfs_trans_ijoin(struct xfs_trans *, struct xfs_inode *, uint);
> -void		xfs_trans_log_buf(xfs_trans_t *, struct xfs_buf *, uint, uint);
> +void		xfs_trans_log_buf(struct xfs_trans *, struct xfs_buf *, uint,
> +				  uint);
> +void		xfs_trans_dirty_buf(struct xfs_trans *, struct xfs_buf *);
>  void		xfs_trans_log_inode(xfs_trans_t *, struct xfs_inode *, uint);
>  
>  void		xfs_extent_free_init_defer_op(void);
> diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
> index 86987d8..58818a0 100644
> --- a/fs/xfs/xfs_trans_buf.c
> +++ b/fs/xfs/xfs_trans_buf.c
> @@ -493,25 +493,17 @@ xfs_trans_bhold_release(xfs_trans_t	*tp,
>  }
>  
>  /*
> - * This is called to mark bytes first through last inclusive of the given
> - * buffer as needing to be logged when the transaction is committed.
> - * The buffer must already be associated with the given transaction.
> - *
> - * First and last are numbers relative to the beginning of this buffer,
> - * so the first byte in the buffer is numbered 0 regardless of the
> - * value of b_blkno.
> + * Mark a buffer dirty in the transaction.
>   */
>  void
> -xfs_trans_log_buf(xfs_trans_t	*tp,
> -		  xfs_buf_t	*bp,
> -		  uint		first,
> -		  uint		last)
> +xfs_trans_dirty_buf(
> +	struct xfs_trans	*tp,
> +	struct xfs_buf		*bp)
>  {
> -	xfs_buf_log_item_t	*bip = bp->b_fspriv;
> +	struct xfs_buf_log_item	*bip = bp->b_fspriv;
>  
>  	ASSERT(bp->b_transp == tp);
>  	ASSERT(bip != NULL);
> -	ASSERT(first <= last && last < BBTOB(bp->b_length));
>  	ASSERT(bp->b_iodone == NULL ||
>  	       bp->b_iodone == xfs_buf_iodone_callbacks);
>  
> @@ -531,8 +523,6 @@ xfs_trans_log_buf(xfs_trans_t	*tp,
>  	bp->b_iodone = xfs_buf_iodone_callbacks;
>  	bip->bli_item.li_cb = xfs_buf_iodone;
>  
> -	trace_xfs_trans_log_buf(bip);
> -
>  	/*
>  	 * If we invalidated the buffer within this transaction, then
>  	 * cancel the invalidation now that we're dirtying the buffer
> @@ -545,15 +535,39 @@ xfs_trans_log_buf(xfs_trans_t	*tp,
>  		bp->b_flags &= ~XBF_STALE;
>  		bip->__bli_format.blf_flags &= ~XFS_BLF_CANCEL;
>  	}
> +	bip->bli_flags |= XFS_BLI_DIRTY | XFS_BLI_LOGGED;
>  
>  	tp->t_flags |= XFS_TRANS_DIRTY;
>  	bip->bli_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> +}
> +
> +/*
> + * This is called to mark bytes first through last inclusive of the given
> + * buffer as needing to be logged when the transaction is committed.
> + * The buffer must already be associated with the given transaction.
> + *
> + * First and last are numbers relative to the beginning of this buffer,
> + * so the first byte in the buffer is numbered 0 regardless of the
> + * value of b_blkno.
> + */
> +void
> +xfs_trans_log_buf(
> +	struct xfs_trans	*tp,
> +	struct xfs_buf		*bp,
> +	uint			first,
> +	uint			last)
> +{
> +	struct xfs_buf_log_item	*bip = bp->b_fspriv;
> +
> +	ASSERT(first <= last && last < BBTOB(bp->b_length));
> +
> +	xfs_trans_dirty_buf(tp, bp);
>  
>  	/*
>  	 * If we have an ordered buffer we are not logging any dirty range but
>  	 * it still needs to be marked dirty and that it has been logged.
>  	 */
> -	bip->bli_flags |= XFS_BLI_DIRTY | XFS_BLI_LOGGED;
> +	trace_xfs_trans_log_buf(bip);
>  	if (!(bip->bli_flags & XFS_BLI_ORDERED))
>  		xfs_buf_item_log(bip, first, last);
>  }
> -- 
> 2.9.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html