[02/19] xfs: refactor log recovery item sorting into a generic dispatch structure
diff mbox series

Message ID 158752117554.2140829.4901314701479350791.stgit@magnolia
State Superseded
Headers show
Series
  • xfs: refactor log recovery
Related show

Commit Message

Darrick J. Wong April 22, 2020, 2:06 a.m. UTC
From: Darrick J. Wong <darrick.wong@oracle.com>

Create a generic dispatch structure to delegate recovery of different
log item types into various code modules.  This will enable us to move
code specific to a particular log item type out of xfs_log_recover.c and
into the log item source.

The first operation we virtualize is the log item sorting.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_log_recover.h |   39 +++++++++++++++++
 fs/xfs/xfs_buf_item.c           |   20 ++++++++-
 fs/xfs/xfs_dquot_item.c         |   10 ++++
 fs/xfs/xfs_icreate_item.c       |    6 +++
 fs/xfs/xfs_inode_item.c         |    6 +++
 fs/xfs/xfs_log_recover.c        |   88 +++++++++++++++++++++++++++------------
 6 files changed, 139 insertions(+), 30 deletions(-)

Comments

Christoph Hellwig April 25, 2020, 6:13 p.m. UTC | #1
> +
> +/* 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.
Darrick J. Wong April 27, 2020, 10:04 p.m. UTC | #2
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
Christoph Hellwig April 28, 2020, 5:11 a.m. UTC | #3
On Mon, Apr 27, 2020 at 03:04:12PM -0700, Darrick J. Wong wrote:
> > > +	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.

The addition goes back to:

commit 1588194c4a13b36badf2f55c022fc4487633f746
Author: Adam Sweeney <ajs@sgi.com>
Date:   Fri Feb 25 02:02:01 1994 +0000

    Add new prototypes and log item types.


In the imported tree, which only added the definition, and no code
related to it.  I can't find any code related to it either in random
checkpoints.
Darrick J. Wong April 28, 2020, 8:46 p.m. UTC | #4
On Mon, Apr 27, 2020 at 10:11:54PM -0700, Christoph Hellwig wrote:
> On Mon, Apr 27, 2020 at 03:04:12PM -0700, Darrick J. Wong wrote:
> > > > +	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.
> 
> The addition goes back to:
> 
> commit 1588194c4a13b36badf2f55c022fc4487633f746
> Author: Adam Sweeney <ajs@sgi.com>
> Date:   Fri Feb 25 02:02:01 1994 +0000
> 
>     Add new prototypes and log item types.
> 
> 
> In the imported tree, which only added the definition, and no code
> related to it.  I can't find any code related to it either in random
> checkpoints.

<nod> Dave told me on IRC that it was added in 1994 but never used, so
it's safe to get rid of it entirely.

--D

Patch
diff mbox series

diff --git a/fs/xfs/libxfs/xfs_log_recover.h b/fs/xfs/libxfs/xfs_log_recover.h
index 3bf671637a91..60a6afb93049 100644
--- a/fs/xfs/libxfs/xfs_log_recover.h
+++ b/fs/xfs/libxfs/xfs_log_recover.h
@@ -6,6 +6,43 @@ 
 #ifndef	__XFS_LOG_RECOVER_H__
 #define __XFS_LOG_RECOVER_H__
 
+/*
+ * Each log item type (XFS_LI_*) gets its own xlog_recover_item_type to
+ * define how recovery should work for that type of log item.
+ */
+struct xlog_recover_item;
+
+/* 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,
+};
+
+typedef enum xlog_recover_reorder (*xlog_recover_reorder_fn)(
+		struct xlog_recover_item *item);
+
+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;
+};
+
+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_intent_item_type;
+
 /*
  * Macros, structures, prototypes for internal log manager use.
  */
@@ -24,10 +61,10 @@ 
  */
 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;
 
 struct xlog_recover {
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 1545657c3ca0..bf7480e18889 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -16,7 +16,8 @@ 
 #include "xfs_trans_priv.h"
 #include "xfs_trace.h"
 #include "xfs_log.h"
-
+#include "xfs_log_priv.h"
+#include "xfs_log_recover.h"
 
 kmem_zone_t	*xfs_buf_item_zone;
 
@@ -1287,3 +1288,20 @@  xfs_buf_resubmit_failed_buffers(
 
 	return ret;
 }
+
+STATIC enum xlog_recover_reorder
+xlog_buf_reorder_fn(
+	struct xlog_recover_item	*item)
+{
+	struct xfs_buf_log_format	*buf_f = item->ri_buf[0].i_addr;
+
+	if (buf_f->blf_flags & XFS_BLF_CANCEL)
+		return XLOG_REORDER_CANCEL_LIST;
+	if (buf_f->blf_flags & XFS_BLF_INODE_BUF)
+		return XLOG_REORDER_INODE_BUFFER_LIST;
+	return XLOG_REORDER_BUFFER_LIST;
+}
+
+const struct xlog_recover_item_type xlog_buf_item_type = {
+	.reorder_fn		= xlog_buf_reorder_fn,
+};
diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
index baad1748d0d1..2558586b4d45 100644
--- a/fs/xfs/xfs_dquot_item.c
+++ b/fs/xfs/xfs_dquot_item.c
@@ -17,6 +17,8 @@ 
 #include "xfs_trans_priv.h"
 #include "xfs_qm.h"
 #include "xfs_log.h"
+#include "xfs_log_priv.h"
+#include "xfs_log_recover.h"
 
 static inline struct xfs_dq_logitem *DQUOT_ITEM(struct xfs_log_item *lip)
 {
@@ -383,3 +385,11 @@  xfs_qm_qoff_logitem_init(
 	qf->qql_flags = flags;
 	return qf;
 }
+
+const struct xlog_recover_item_type xlog_dquot_item_type = {
+	.reorder		= XLOG_REORDER_INODE_LIST,
+};
+
+const struct xlog_recover_item_type xlog_quotaoff_item_type = {
+	.reorder		= XLOG_REORDER_INODE_LIST,
+};
diff --git a/fs/xfs/xfs_icreate_item.c b/fs/xfs/xfs_icreate_item.c
index 490fee22b878..0a1ed4dc1c3d 100644
--- a/fs/xfs/xfs_icreate_item.c
+++ b/fs/xfs/xfs_icreate_item.c
@@ -11,6 +11,8 @@ 
 #include "xfs_trans_priv.h"
 #include "xfs_icreate_item.h"
 #include "xfs_log.h"
+#include "xfs_log_priv.h"
+#include "xfs_log_recover.h"
 
 kmem_zone_t	*xfs_icreate_zone;		/* inode create item zone */
 
@@ -107,3 +109,7 @@  xfs_icreate_log(
 	tp->t_flags |= XFS_TRANS_DIRTY;
 	set_bit(XFS_LI_DIRTY, &icp->ic_item.li_flags);
 }
+
+const struct xlog_recover_item_type xlog_icreate_item_type = {
+	.reorder		= XLOG_REORDER_BUFFER_LIST,
+};
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index f779cca2346f..b04e9c5330b7 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -18,6 +18,8 @@ 
 #include "xfs_buf_item.h"
 #include "xfs_log.h"
 #include "xfs_error.h"
+#include "xfs_log_priv.h"
+#include "xfs_log_recover.h"
 
 #include <linux/iversion.h>
 
@@ -843,3 +845,7 @@  xfs_inode_item_format_convert(
 	in_f->ilf_boffset = in_f32->ilf_boffset;
 	return 0;
 }
+
+const struct xlog_recover_item_type xlog_inode_item_type = {
+	.reorder		= XLOG_REORDER_INODE_LIST,
+};
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 5f803083ddc3..e7a9f899f657 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -1779,6 +1779,12 @@  xlog_clear_stale_blocks(
 	return 0;
 }
 
+/* Log intent item dispatching. */
+
+const struct xlog_recover_item_type xlog_intent_item_type = {
+	.reorder		= XLOG_REORDER_INODE_LIST,
+};
+
 /******************************************************************************
  *
  *		Log recover routines
@@ -1786,6 +1792,39 @@  xlog_clear_stale_blocks(
  ******************************************************************************
  */
 
+static const struct xlog_recover_item_type *
+xlog_item_for_type(
+	unsigned short	type)
+{
+	switch (type) {
+	case XFS_LI_ICREATE:
+		return &xlog_icreate_item_type;
+	case XFS_LI_BUF:
+		return &xlog_buf_item_type;
+	case XFS_LI_EFD:
+	case XFS_LI_EFI:
+	case XFS_LI_RUI:
+	case XFS_LI_RUD:
+	case XFS_LI_CUI:
+	case XFS_LI_CUD:
+	case XFS_LI_BUI:
+	case XFS_LI_BUD:
+		return &xlog_intent_item_type;
+	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? */
+		return NULL;
+	default:
+		/* Unknown type, go away. */
+		return NULL;
+	}
+}
+
 /*
  * Sort the log items in the transaction.
  *
@@ -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));
+		if (item->ri_type) {
+			if (item->ri_type->reorder_fn)
+				fate = item->ri_type->reorder_fn(item);
+			else
+				fate = item->ri_type->reorder;
+		}
 
-		switch (ITEM_TYPE(item)) {
-		case XFS_LI_ICREATE:
+		switch (fate) {
+		case XLOG_REORDER_BUFFER_LIST:
 			list_move_tail(&item->ri_list, &buffer_list);
 			break;
-		case XFS_LI_BUF:
-			if (buf_f->blf_flags & XFS_BLF_CANCEL) {
-				trace_xfs_log_recover_item_reorder_head(log,
-							trans, item, pass);
-				list_move(&item->ri_list, &cancel_list);
-				break;
-			}
-			if (buf_f->blf_flags & XFS_BLF_INODE_BUF) {
-				list_move(&item->ri_list, &inode_buffer_list);
-				break;
-			}
-			list_move_tail(&item->ri_list, &buffer_list);
+		case XLOG_REORDER_CANCEL_LIST:
+			trace_xfs_log_recover_item_reorder_head(log,
+					trans, item, pass);
+			list_move(&item->ri_list, &cancel_list);
 			break;
-		case XFS_LI_INODE:
-		case XFS_LI_DQUOT:
-		case XFS_LI_QUOTAOFF:
-		case XFS_LI_EFD:
-		case XFS_LI_EFI:
-		case XFS_LI_RUI:
-		case XFS_LI_RUD:
-		case XFS_LI_CUI:
-		case XFS_LI_CUD:
-		case XFS_LI_BUI:
-		case XFS_LI_BUD:
+		case XLOG_REORDER_INODE_BUFFER_LIST:
+			list_move(&item->ri_list, &inode_buffer_list);
+			break;
+		case XLOG_REORDER_INODE_LIST:
 			trace_xfs_log_recover_item_reorder_tail(log,
-							trans, item, pass);
+					trans, item, pass);
 			list_move_tail(&item->ri_list, &inode_list);
 			break;
-		default:
+		case XLOG_REORDER_UNKNOWN:
 			xfs_warn(log->l_mp,
 				"%s: unrecognized type of log operation (%d)",
 				__func__, ITEM_TYPE(item));