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

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

On Thu, Apr 30, 2020 at 08:08:21AM -0700, Darrick J. Wong wrote:
> On Wed, Apr 29, 2020 at 10:53:09PM -0700, Christoph Hellwig wrote:
> > On Wed, Apr 29, 2020 at 05:47:41PM -0700, Darrick J. Wong wrote:
> > > +extern const struct xlog_recover_item_type xlog_icreate_item_type;
> > > +extern const struct xlog_recover_item_type xlog_buf_item_type;
> > > +extern const struct xlog_recover_item_type xlog_inode_item_type;
> > > +extern const struct xlog_recover_item_type xlog_dquot_item_type;
> > > +extern const struct xlog_recover_item_type xlog_quotaoff_item_type;
> > > +extern const struct xlog_recover_item_type xlog_bmap_intent_item_type;
> > > +extern const struct xlog_recover_item_type xlog_bmap_done_item_type;
> > > +extern const struct xlog_recover_item_type xlog_extfree_intent_item_type;
> > > +extern const struct xlog_recover_item_type xlog_extfree_done_item_type;
> > > +extern const struct xlog_recover_item_type xlog_rmap_intent_item_type;
> > > +extern const struct xlog_recover_item_type xlog_rmap_done_item_type;
> > > +extern const struct xlog_recover_item_type xlog_refcount_intent_item_type;
> > > +extern const struct xlog_recover_item_type xlog_refcount_done_item_type;
> > 
> > I'd prefer if we didn't have to expose these structures, but had a
> > xlog_register_recovery_item helper that just adds them to a list or
> > array.
> 
> I can look into making a register function and do lookups, but that's
> a lot of indirection to save ~15 or so externs.

Ok, so I looked into this, and I don't know of a good way to avoid
exporting 14 *somethings*.  If we require a xlog_register_recovery_item
call to link the item types when the module loads, something has to call
that registration function.  That can be an __init function in each item
recovery source file, but now we have to export all fourteen of those
functions so that we can call them from init_xfs_fs.  Unless you've got
a better suggestion, I don't think this is worth the effort.

--D

> > >  typedef struct xlog_recover_item {
> > >  	struct list_head	ri_list;
> > > -	int			ri_type;
> > >  	int			ri_cnt;	/* count of regions found */
> > >  	int			ri_total;	/* total regions */
> > >  	xfs_log_iovec_t		*ri_buf;	/* ptr to regions buffer */
> > > +	const struct xlog_recover_item_type *ri_type;
> > >  } xlog_recover_item_t;
> > 
> > Btw, killing the xlog_recover_item_t typedef might be a worthwhile prep
> > patch.
> 
> Hrm, ok....
> 
> > > --- a/fs/xfs/xfs_buf_item.c
> > > +++ b/fs/xfs/xfs_buf_item.c
> > > @@ -17,7 +17,6 @@
> > >  #include "xfs_trace.h"
> > >  #include "xfs_log.h"
> > >  
> > > -
> > >  kmem_zone_t	*xfs_buf_item_zone;
> > >  
> > >  static inline struct xfs_buf_log_item *BUF_ITEM(struct xfs_log_item *lip)
> > 
> > Spurious whitespace change in a file not otherwise touched.
> 
> Fixed.
> 
> > >  
> > > @@ -107,3 +109,14 @@ xfs_icreate_log(
> > >  	tp->t_flags |= XFS_TRANS_DIRTY;
> > >  	set_bit(XFS_LI_DIRTY, &icp->ic_item.li_flags);
> > >  }
> > > +
> > > +static enum xlog_recover_reorder
> > > +xlog_icreate_reorder(
> > > +		struct xlog_recover_item *item)
> > > +{
> > > +	return XLOG_REORDER_BUFFER_LIST;
> > > +}
> > 
> > It might be worth to throw in a comment why icreate items got to
> > the buffer list.
> > 
> > > +		return 0;
> > > +#ifdef CONFIG_XFS_QUOTA
> > > +	case XFS_LI_DQUOT:
> > > +		item->ri_type = &xlog_dquot_item_type;
> > > +		return 0;
> > > +	case XFS_LI_QUOTAOFF:
> > > +		item->ri_type = &xlog_quotaoff_item_type;
> > > +		return 0;
> > > +#endif /* CONFIG_XFS_QUOTA */
> > > +	default:
> > > +		return -EFSCORRUPTED;
> > > +	}
> > > +}
> > 
> > Quote recovery support currently is unconditionalẏ  Making it
> > conditional on CONFIG_XFS_QUOTA means a kernel without that config
> > will now fail to recover a file system with quota updates in the log.
> 
> Heh, I hadn't realized that quota recovery always works even if quotas
> are disabled.  Ok, xfs_dquot_recovery.c it is...
> 
> --D