Re: [PATCH 1/2] xfs: refactor the buffer cancellation table helpers

[Brian Foster <bfoster@redhat.com>]

On Mon, Apr 27, 2020 at 03:52:28PM +0200, Christoph Hellwig wrote:
> Replace the somewhat convoluted use of xlog_peek_buffer_cancelled and
> xlog_check_buffer_cancelled with two obvious helpers:
> 
>  xlog_is_buffer_cancelled, which returns true if there is a buffer in
>  the cancellation table, and
>  xlog_put_buffer_cancelled, which also decrements the reference count
>  of the buffer cancellation table.
> 
> Both share a little helper to look up the entry.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_log_recover.c | 109 ++++++++++++++++++---------------------
>  1 file changed, 50 insertions(+), 59 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 11c3502b07b13..750a81b941ea4 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -1972,26 +1972,17 @@ xlog_recover_buffer_pass1(
>  	return 0;
>  }
>  
...
>  /*
> - * If the buffer is being cancelled then return 1 so that it will be cancelled,
> - * otherwise return 0.  If the buffer is actually a buffer cancel item
> - * (XFS_BLF_CANCEL is set), then decrement the refcount on the entry in the
> - * table and remove it from the table if this is the last reference.
> + * Check if there is and entry for blkno, len in the buffer cancel record table.
> + */
> +static bool
> +xlog_is_buffer_cancelled(
> +	struct xlog		*log,
> +	xfs_daddr_t		blkno,
> +	uint			len)
> +{
> +	return xlog_find_buffer_cancelled(log, blkno, len) != NULL;
> +}
> +
> +/*
> + * Check if there is and entry for blkno, len in the buffer cancel record table,

Nit:			an

> + * and decremented the reference count on it if there is one.

Nit:	  decrement

...
> @@ -2733,10 +2722,15 @@ xlog_recover_buffer_pass2(
>  	 * In this pass we only want to recover all the buffers which have
>  	 * not been cancelled and are not cancellation buffers themselves.
>  	 */
> -	if (xlog_check_buffer_cancelled(log, buf_f->blf_blkno,
> -			buf_f->blf_len, buf_f->blf_flags)) {
> -		trace_xfs_log_recover_buf_cancel(log, buf_f);
> -		return 0;
> +	if (buf_f->blf_flags & XFS_BLF_CANCEL) {
> +		if (xlog_put_buffer_cancelled(log, buf_f->blf_blkno,
> +				buf_f->blf_len))
> +			goto cancelled;
> +	} else {
> +

Extra whitespace above. With those nits fixed:

Reviewed-by: Brian Foster <bfoster@redhat.com>

> +		if (xlog_is_buffer_cancelled(log, buf_f->blf_blkno,
> +				buf_f->blf_len))
> +			goto cancelled;
>  	}
>  
>  	trace_xfs_log_recover_buf_recover(log, buf_f);
> @@ -2820,6 +2814,9 @@ xlog_recover_buffer_pass2(
>  out_release:
>  	xfs_buf_relse(bp);
>  	return error;
> +cancelled:
> +	trace_xfs_log_recover_buf_cancel(log, buf_f);
> +	return 0;
>  }
>  
>  /*
> @@ -2937,8 +2934,7 @@ xlog_recover_inode_pass2(
>  	 * Inode buffers can be freed, look out for it,
>  	 * and do not replay the inode.
>  	 */
> -	if (xlog_check_buffer_cancelled(log, in_f->ilf_blkno,
> -					in_f->ilf_len, 0)) {
> +	if (xlog_is_buffer_cancelled(log, in_f->ilf_blkno, in_f->ilf_len)) {
>  		error = 0;
>  		trace_xfs_log_recover_inode_cancel(log, in_f);
>  		goto error;
> @@ -3840,7 +3836,7 @@ xlog_recover_do_icreate_pass2(
>  
>  		daddr = XFS_AGB_TO_DADDR(mp, agno,
>  				agbno + i * igeo->blocks_per_cluster);
> -		if (xlog_check_buffer_cancelled(log, daddr, bb_per_cluster, 0))
> +		if (xlog_is_buffer_cancelled(log, daddr, bb_per_cluster))
>  			cancel_count++;
>  	}
>  
> @@ -3876,11 +3872,8 @@ xlog_recover_buffer_ra_pass2(
>  	struct xfs_buf_log_format	*buf_f = item->ri_buf[0].i_addr;
>  	struct xfs_mount		*mp = log->l_mp;
>  
> -	if (xlog_peek_buffer_cancelled(log, buf_f->blf_blkno,
> -			buf_f->blf_len, buf_f->blf_flags)) {
> +	if (xlog_is_buffer_cancelled(log, buf_f->blf_blkno, buf_f->blf_len))
>  		return;
> -	}
> -
>  	xfs_buf_readahead(mp->m_ddev_targp, buf_f->blf_blkno,
>  				buf_f->blf_len, NULL);
>  }
> @@ -3905,9 +3898,8 @@ xlog_recover_inode_ra_pass2(
>  			return;
>  	}
>  
> -	if (xlog_peek_buffer_cancelled(log, ilfp->ilf_blkno, ilfp->ilf_len, 0))
> +	if (xlog_is_buffer_cancelled(log, ilfp->ilf_blkno, ilfp->ilf_len))
>  		return;
> -
>  	xfs_buf_readahead(mp->m_ddev_targp, ilfp->ilf_blkno,
>  				ilfp->ilf_len, &xfs_inode_buf_ra_ops);
>  }
> @@ -3943,9 +3935,8 @@ xlog_recover_dquot_ra_pass2(
>  	ASSERT(dq_f->qlf_len == 1);
>  
>  	len = XFS_FSB_TO_BB(mp, dq_f->qlf_len);
> -	if (xlog_peek_buffer_cancelled(log, dq_f->qlf_blkno, len, 0))
> +	if (xlog_is_buffer_cancelled(log, dq_f->qlf_blkno, len))
>  		return;
> -
>  	xfs_buf_readahead(mp->m_ddev_targp, dq_f->qlf_blkno, len,
>  			  &xfs_dquot_buf_ra_ops);
>  }
> -- 
> 2.26.1
>