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

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Sat, Apr 25, 2020 at 11:13:07AM -0700, Christoph Hellwig wrote:
> > +
> > +/* 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?

Yes, thanks for the series fixing that.

> > +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).

Fair enough.

> > +
> > +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.

<nod>

> > +	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.

The thing I can't tell is if XFS_LI_IUNLINK is a code point reserved
from some long-ago log item that fell out, or reserved for some future
project?

Either way, this case doesn't need to be there.

> > @@ -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.

xlog_set_item_type(item); yes.

> > +		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.

Ok.

--D