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

From: Christoph Hellwig <hch@infradead.org>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 01/21] xfs: refactor log recovery item sorting into a generic dispatch structure
Date: Wed, 29 Apr 2020 22:53:09 -0700	[thread overview]
Message-ID: <20200430055309.GA29110@infradead.org> (raw)
In-Reply-To: <158820766135.467894.13993542565087629835.stgit@magnolia>

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.

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

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

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