Re: [PATCH 02/19] xfs: refactor log recovery item sorting into a generic dispatch structure

[Christoph Hellwig <hch@infradead.org>]

> +
> +/* Sorting hat for log items as they're read in. */
> +enum xlog_recover_reorder {
> +	XLOG_REORDER_UNKNOWN,
> +	XLOG_REORDER_BUFFER_LIST,
> +	XLOG_REORDER_CANCEL_LIST,
> +	XLOG_REORDER_INODE_BUFFER_LIST,
> +	XLOG_REORDER_INODE_LIST,

XLOG_REORDER_INODE_LIST seems a bit misnamed as it really is the
"misc" or "no reorder" list.  I guess the naming comes from the
local inode_list variable, but maybe we need to fix that as well?

> +typedef enum xlog_recover_reorder (*xlog_recover_reorder_fn)(
> +		struct xlog_recover_item *item);

This typedef doesn't actually seem to help with anything (neither
with just thіs patch nor the final tree).

> +
> +struct xlog_recover_item_type {
> +	/*
> +	 * These two items decide how to sort recovered log items during
> +	 * recovery.  If reorder_fn is non-NULL it will be called; otherwise,
> +	 * reorder will be used to decide.  See the comment above
> +	 * xlog_recover_reorder_trans for more details about what the values
> +	 * mean.
> +	 */
> +	enum xlog_recover_reorder	reorder;
> +	xlog_recover_reorder_fn		reorder_fn;

I'd just use reorder_fn and skip the simple field.  Just one way to do
things even if it adds a tiny amount of boilerplate code.

> +	case XFS_LI_INODE:
> +		return &xlog_inode_item_type;
> +	case XFS_LI_DQUOT:
> +		return &xlog_dquot_item_type;
> +	case XFS_LI_QUOTAOFF:
> +		return &xlog_quotaoff_item_type;
> +	case XFS_LI_IUNLINK:
> +		/* Not implemented? */

Not implemented!  I think we need a prep patch to remove this first.

> @@ -1851,41 +1890,34 @@ xlog_recover_reorder_trans(
>  
>  	list_splice_init(&trans->r_itemq, &sort_list);
>  	list_for_each_entry_safe(item, n, &sort_list, ri_list) {
> -		xfs_buf_log_format_t	*buf_f = item->ri_buf[0].i_addr;
> +		enum xlog_recover_reorder	fate = XLOG_REORDER_UNKNOWN;
> +
> +		item->ri_type = xlog_item_for_type(ITEM_TYPE(item));

I wonder if just passing the whole item to xlog_item_for_type would
make more sense.  It would then need a different name, of course.

> +		if (item->ri_type) {
> +			if (item->ri_type->reorder_fn)
> +				fate = item->ri_type->reorder_fn(item);
> +			else
> +				fate = item->ri_type->reorder;
> +		}

I think for the !item->ri_type we should immediately jump to what
currently is the XLOG_REORDER_UNKNOWN case, and thus avoid even
adding XLOG_REORDER_UNKNOWN to the enum.  The added benefit is that
any item without a reorder_fn could then be treated as on what
currently is the inode_list, but needs a btter name.