Re: [PATCH 03/19] xfs: refactor log recovery item dispatch for pass2 readhead functions

[Christoph Hellwig <hch@infradead.org>]

On Tue, Apr 21, 2020 at 07:06:21PM -0700, Darrick J. Wong wrote:
>  typedef enum xlog_recover_reorder (*xlog_recover_reorder_fn)(
>  		struct xlog_recover_item *item);
> +typedef void (*xlog_recover_ra_pass2_fn)(struct xlog *log,
> +		struct xlog_recover_item *item);

Same comment for this one - or the other two following.

> +/*
> + * This structure is used during recovery to record the buf log items which
> + * have been canceled and should not be replayed.
> + */
> +struct xfs_buf_cancel {
> +	xfs_daddr_t		bc_blkno;
> +	uint			bc_len;
> +	int			bc_refcount;
> +	struct list_head	bc_list;
> +};
> +
> +struct xfs_buf_cancel *xlog_peek_buffer_cancelled(struct xlog *log,
> +		xfs_daddr_t blkno, uint len, unsigned short flags);

None of the callers moved in this patch even needs the xfs_buf_cancel
structure, it just uses the return value as a boolean.  I think they
all should be switched to use xlog_check_buffer_cancelled instead, which
means that struct xfs_buf_cancel and xlog_peek_buffer_cancelled can stay
private in xfs_log_recovery.c (and later xfs_buf_item.c).

> +STATIC void
> +xlog_recover_buffer_ra_pass2(
> +	struct xlog                     *log,
> +	struct xlog_recover_item        *item)
> +{
> +	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)) {
> +		return;
> +	}
> +
> +	xfs_buf_readahead(mp->m_ddev_targp, buf_f->blf_blkno,
> +				buf_f->blf_len, NULL);

Why not:

	if (!xlog_peek_buffer_cancelled(log, buf_f->blf_blkno,
			buf_f->blf_len, buf_f->blf_flags)) {
		xfs_buf_readahead(mp->m_ddev_targp, buf_f->blf_blkno,
				buf_f->blf_len, NULL);
	}

> +STATIC void
> +xlog_recover_dquot_ra_pass2(
> +	struct xlog			*log,
> +	struct xlog_recover_item	*item)
> +{
> +	struct xfs_mount	*mp = log->l_mp;
> +	struct xfs_disk_dquot	*recddq;
> +	struct xfs_dq_logformat	*dq_f;
> +	uint			type;
> +	int			len;
> +
> +
> +	if (mp->m_qflags == 0)

Double empty line above.

> +	if (xlog_peek_buffer_cancelled(log, dq_f->qlf_blkno, len, 0))
> +		return;
> +
> +	xfs_buf_readahead(mp->m_ddev_targp, dq_f->qlf_blkno, len,
> +			  &xfs_dquot_buf_ra_ops);

Same comment as above, no real need for the early return here.

> +	if (xlog_peek_buffer_cancelled(log, ilfp->ilf_blkno, ilfp->ilf_len, 0))
> +		return;
> +
> +	xfs_buf_readahead(mp->m_ddev_targp, ilfp->ilf_blkno,
> +				ilfp->ilf_len, &xfs_inode_buf_ra_ops);

Here again.

> -	unsigned short			flags)
> +	unsigned short		flags)

Spurious whitespace change.

>  		case XLOG_RECOVER_PASS2:
> -			xlog_recover_ra_pass2(log, item);
> +			if (item->ri_type && item->ri_type->ra_pass2_fn)
> +				item->ri_type->ra_pass2_fn(log, item);

Shouldn't we ensure eatly on that we always have a valid ->ri_type?