On Tue, Apr 28, 2020 at 10:05:50AM +0200, Christoph Hellwig wrote:
> Split out a xlog_add_buffer_cancelled helper which does the low-level
> manipulation of the buffer cancelation table, and in that helper call
> xlog_find_buffer_cancelled instead of open coding it.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
> fs/xfs/xfs_log_recover.c | 114 +++++++++++++++++++--------------------
> 1 file changed, 55 insertions(+), 59 deletions(-)
>
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index fe4dad5b77a95..3bc61838266f1 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
...
> @@ -2045,6 +2015,32 @@ xlog_buf_readahead(
> xfs_buf_readahead(log->l_mp->m_ddev_targp, blkno, len, ops);
> }
>
> +/*
> + * Build up the table of buf cancel records so that we don't replay cancelled
> + * data in the second pass.
> + */
> +static int
> +xlog_recover_buffer_pass1(
> + struct xlog *log,
> + struct xlog_recover_item *item)
> +{
> + struct xfs_buf_log_format *bf = item->ri_buf[0].i_addr;
> +
> + if (!xfs_buf_log_check_iovec(&item->ri_buf[0])) {
> + xfs_err(log->l_mp, "bad buffer log item size (%d)",
> + item->ri_buf[0].i_len);
> + return -EFSCORRUPTED;
> + }
> +
> + if (!(bf->blf_flags & XFS_BLF_CANCEL))
> + trace_xfs_log_recover_buf_not_cancel(log, bf);
> + else if (xlog_add_buffer_cancelled(log, bf->blf_blkno, bf->blf_len))
> + trace_xfs_log_recover_buf_cancel_add(log, bf);
> + else
> + trace_xfs_log_recover_buf_cancel_ref_inc(log, bf);
Nit, but the function call looks buried here. Also, the boolean return
seems like overkill if it's only used to control tracepoints (and
true/false for inc/ref isn't terribly intuitive anyways).
This looks cleaner to me if it's just:
if (!XFS_BLF_CANCEL) {
trace_xfs_log_recover_buf_not_cancel(log, bf);
return 0;
}
xlog_add_buffer_cancelled(log, bf->blf_blkno, bf->blf_len);
return 0;
... and let the helper invoke the buf_cancel tracepoints and return
void. Otherwise looks like a good cleanup to me.
Brian
> + return 0;
> +}
> +
> /*
> * Perform recovery for a buffer full of inodes. In these buffers, the only
> * data which should be recovered is that which corresponds to the
> --
> 2.26.1
>
