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
