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?
