From: Christoph Hellwig <hch@infradead.org>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH v2 00/21] xfs: refactor log recovery
Date: Fri, 1 May 2020 03:15:39 -0700 [thread overview]
Message-ID: <20200501101539.GA21903@infradead.org> (raw)
In-Reply-To: <158820765488.467894.15408191148091671053.stgit@magnolia>
I've looked a bit over the total diff and finaly result and really like
it.
A few comments from that without going into the individual patches:
- I don't think the buffer cancellation table should remain in
xfs_log_recovery.c. I can either move them into a new file
as part of resending my prep series, or you could move them into
xfs_buf_item_recover.c. Let me know what you prefer.
- Should the match callback also move into struct xfs_item_ops? That
would also match iop_recover.
- Based on that we could also kill XFS_ITEM_TYPE_IS_INTENT by just
checking for iop_recover and/or iop_match.
- Setting XFS_LI_RECOVERED could also move to common code, basically
set it whenever iop_recover returns. Also we can remove the
XFS_LI_RECOVERED asserts in ->iop_recovery when the caller checks
it just before.
- we are still having a few redundant ri_type checks.
- ri_type maybe should be ri_ops?
See this patch below for my take on cleaning up the recovery ops
handling a bit:
diff --git a/fs/xfs/libxfs/xfs_log_recover.h b/fs/xfs/libxfs/xfs_log_recover.h
index ba172eb454c8f..f97946cf94f11 100644
--- a/fs/xfs/libxfs/xfs_log_recover.h
+++ b/fs/xfs/libxfs/xfs_log_recover.h
@@ -7,7 +7,7 @@
#define __XFS_LOG_RECOVER_H__
/*
- * Each log item type (XFS_LI_*) gets its own xlog_recover_item_type to
+ * Each log item type (XFS_LI_*) gets its own xlog_recover_item_ops to
* define how recovery should work for that type of log item.
*/
struct xlog_recover_item;
@@ -20,7 +20,9 @@ enum xlog_recover_reorder {
XLOG_REORDER_CANCEL_LIST,
};
-struct xlog_recover_item_type {
+struct xlog_recover_item_ops {
+ uint16_t item_type;
+
/*
* Help sort recovered log items into the order required to replay them
* correctly. Log item types that always use XLOG_REORDER_ITEM_LIST do
@@ -58,19 +60,19 @@ struct xlog_recover_item_type {
struct xlog_recover_item *item, xfs_lsn_t lsn);
};
-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;
+extern const struct xlog_recover_item_ops xlog_icreate_item_type;
+extern const struct xlog_recover_item_ops xlog_buf_item_type;
+extern const struct xlog_recover_item_ops xlog_inode_item_type;
+extern const struct xlog_recover_item_ops xlog_dquot_item_type;
+extern const struct xlog_recover_item_ops xlog_quotaoff_item_type;
+extern const struct xlog_recover_item_ops xlog_bmap_intent_item_type;
+extern const struct xlog_recover_item_ops xlog_bmap_done_item_type;
+extern const struct xlog_recover_item_ops xlog_extfree_intent_item_type;
+extern const struct xlog_recover_item_ops xlog_extfree_done_item_type;
+extern const struct xlog_recover_item_ops xlog_rmap_intent_item_type;
+extern const struct xlog_recover_item_ops xlog_rmap_done_item_type;
+extern const struct xlog_recover_item_ops xlog_refcount_intent_item_type;
+extern const struct xlog_recover_item_ops xlog_refcount_done_item_type;
/*
* Macros, structures, prototypes for internal log manager use.
@@ -93,7 +95,7 @@ typedef struct xlog_recover_item {
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;
+ const struct xlog_recover_item_ops *ri_ops;
} xlog_recover_item_t;
struct xlog_recover {
diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c
index 58f0904e4504d..952b4ce40433e 100644
--- a/fs/xfs/xfs_bmap_item.c
+++ b/fs/xfs/xfs_bmap_item.c
@@ -667,10 +667,12 @@ xlog_recover_bmap_done_commit_pass2(
return 0;
}
-const struct xlog_recover_item_type xlog_bmap_intent_item_type = {
+const struct xlog_recover_item_ops xlog_bmap_intent_item_type = {
+ .item_type = XFS_LI_BUI,
.commit_pass2_fn = xlog_recover_bmap_intent_commit_pass2,
};
-const struct xlog_recover_item_type xlog_bmap_done_item_type = {
+const struct xlog_recover_item_ops xlog_bmap_done_item_type = {
+ .item_type = XFS_LI_BUD,
.commit_pass2_fn = xlog_recover_bmap_done_commit_pass2,
};
diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c
index d324f810819df..954e0e96af5dc 100644
--- a/fs/xfs/xfs_buf_item_recover.c
+++ b/fs/xfs/xfs_buf_item_recover.c
@@ -857,7 +857,8 @@ xlog_recover_buffer_commit_pass2(
return 0;
}
-const struct xlog_recover_item_type xlog_buf_item_type = {
+const struct xlog_recover_item_ops xlog_buf_item_type = {
+ .item_type = XFS_LI_BUF,
.reorder_fn = xlog_buf_reorder_fn,
.ra_pass2_fn = xlog_recover_buffer_ra_pass2,
.commit_pass1_fn = xlog_recover_buffer_commit_pass1,
diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
index 83bd7ded9185f..6c6216bdc432c 100644
--- a/fs/xfs/xfs_dquot_item.c
+++ b/fs/xfs/xfs_dquot_item.c
@@ -527,7 +527,8 @@ xlog_recover_dquot_commit_pass2(
return 0;
}
-const struct xlog_recover_item_type xlog_dquot_item_type = {
+const struct xlog_recover_item_ops xlog_dquot_item_type = {
+ .item_type = XFS_LI_DQUOT,
.ra_pass2_fn = xlog_recover_dquot_ra_pass2,
.commit_pass2_fn = xlog_recover_dquot_commit_pass2,
};
@@ -559,6 +560,7 @@ xlog_recover_quotaoff_commit_pass1(
return 0;
}
-const struct xlog_recover_item_type xlog_quotaoff_item_type = {
+const struct xlog_recover_item_ops xlog_quotaoff_item_type = {
+ .item_type = XFS_LI_QUOTAOFF,
.commit_pass1_fn = xlog_recover_quotaoff_commit_pass1,
};
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index d6f2c88570de1..5d1fb5e05b781 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -729,10 +729,12 @@ xlog_recover_extfree_done_commit_pass2(
return 0;
}
-const struct xlog_recover_item_type xlog_extfree_intent_item_type = {
+const struct xlog_recover_item_ops xlog_extfree_intent_item_type = {
+ .item_type = XFS_LI_EFI,
.commit_pass2_fn = xlog_recover_extfree_intent_commit_pass2,
};
-const struct xlog_recover_item_type xlog_extfree_done_item_type = {
+const struct xlog_recover_item_ops xlog_extfree_done_item_type = {
+ .item_type = XFS_LI_EFD,
.commit_pass2_fn = xlog_recover_extfree_done_commit_pass2,
};
diff --git a/fs/xfs/xfs_icreate_item.c b/fs/xfs/xfs_icreate_item.c
index 602a8c91371fe..34805bdbc2e12 100644
--- a/fs/xfs/xfs_icreate_item.c
+++ b/fs/xfs/xfs_icreate_item.c
@@ -248,7 +248,8 @@ xlog_recover_do_icreate_commit_pass2(
length, be32_to_cpu(icl->icl_gen));
}
-const struct xlog_recover_item_type xlog_icreate_item_type = {
+const struct xlog_recover_item_ops xlog_icreate_item_type = {
+ .item_type = XFS_LI_ICREATE,
.reorder_fn = xlog_icreate_reorder,
.commit_pass2_fn = xlog_recover_do_icreate_commit_pass2,
};
diff --git a/fs/xfs/xfs_inode_item_recover.c b/fs/xfs/xfs_inode_item_recover.c
index 46fc8a4b9ac61..9dff80783fe12 100644
--- a/fs/xfs/xfs_inode_item_recover.c
+++ b/fs/xfs/xfs_inode_item_recover.c
@@ -393,7 +393,8 @@ xlog_recover_inode_commit_pass2(
return error;
}
-const struct xlog_recover_item_type xlog_inode_item_type = {
+const struct xlog_recover_item_ops xlog_inode_item_type = {
+ .item_type = XFS_LI_INODE,
.ra_pass2_fn = xlog_recover_inode_ra_pass2,
.commit_pass2_fn = xlog_recover_inode_commit_pass2,
};
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 09dd514a34980..e3f13866deb08 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -1828,55 +1828,35 @@ xlog_recover_insert_ail(
******************************************************************************
*/
-static int
-xlog_set_item_type(
- struct xlog_recover_item *item)
-{
- switch (ITEM_TYPE(item)) {
- case XFS_LI_ICREATE:
- item->ri_type = &xlog_icreate_item_type;
- return 0;
- case XFS_LI_BUF:
- item->ri_type = &xlog_buf_item_type;
- return 0;
- case XFS_LI_EFI:
- item->ri_type = &xlog_extfree_intent_item_type;
- return 0;
- case XFS_LI_EFD:
- item->ri_type = &xlog_extfree_done_item_type;
- return 0;
- case XFS_LI_RUI:
- item->ri_type = &xlog_rmap_intent_item_type;
- return 0;
- case XFS_LI_RUD:
- item->ri_type = &xlog_rmap_done_item_type;
- return 0;
- case XFS_LI_CUI:
- item->ri_type = &xlog_refcount_intent_item_type;
- return 0;
- case XFS_LI_CUD:
- item->ri_type = &xlog_refcount_done_item_type;
- return 0;
- case XFS_LI_BUI:
- item->ri_type = &xlog_bmap_intent_item_type;
- return 0;
- case XFS_LI_BUD:
- item->ri_type = &xlog_bmap_done_item_type;
- return 0;
- case XFS_LI_INODE:
- item->ri_type = &xlog_inode_item_type;
- return 0;
+static const struct xlog_recover_item_ops *xlog_recover_item_ops[] = {
+ &xlog_icreate_item_type,
+ &xlog_buf_item_type,
+ &xlog_extfree_intent_item_type,
+ &xlog_extfree_done_item_type,
+ &xlog_rmap_intent_item_type,
+ &xlog_rmap_done_item_type,
+ &xlog_refcount_intent_item_type,
+ &xlog_refcount_done_item_type,
+ &xlog_bmap_intent_item_type,
+ &xlog_bmap_done_item_type,
+ &xlog_inode_item_type,
#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;
+ &xlog_dquot_item_type,
+ &xlog_quotaoff_item_type,
#endif /* CONFIG_XFS_QUOTA */
- default:
- return -EFSCORRUPTED;
- }
+};
+
+static const struct xlog_recover_item_ops *
+xlog_find_item_ops(
+ struct xlog_recover_item *item)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(xlog_recover_item_ops); i++)
+ if (ITEM_TYPE(item) == xlog_recover_item_ops[i]->item_type)
+ return xlog_recover_item_ops[i];
+
+ return NULL;
}
/*
@@ -1946,8 +1926,8 @@ xlog_recover_reorder_trans(
list_for_each_entry_safe(item, n, &sort_list, ri_list) {
enum xlog_recover_reorder fate = XLOG_REORDER_ITEM_LIST;
- error = xlog_set_item_type(item);
- if (error) {
+ item->ri_ops = xlog_find_item_ops(item);
+ if (!item->ri_ops) {
xfs_warn(log->l_mp,
"%s: unrecognized type of log operation (%d)",
__func__, ITEM_TYPE(item));
@@ -1958,11 +1938,12 @@ xlog_recover_reorder_trans(
*/
if (!list_empty(&sort_list))
list_splice_init(&sort_list, &trans->r_itemq);
+ error = -EFSCORRUPTED;
break;
}
- if (item->ri_type->reorder_fn)
- fate = item->ri_type->reorder_fn(item);
+ if (item->ri_ops->reorder_fn)
+ fate = item->ri_ops->reorder_fn(item);
switch (fate) {
case XLOG_REORDER_BUFFER_LIST:
@@ -2098,46 +2079,6 @@ xlog_buf_readahead(
xfs_buf_readahead(log->l_mp->m_ddev_targp, blkno, len, ops);
}
-STATIC int
-xlog_recover_commit_pass1(
- struct xlog *log,
- struct xlog_recover *trans,
- struct xlog_recover_item *item)
-{
- trace_xfs_log_recover_item_recover(log, trans, item, XLOG_RECOVER_PASS1);
-
- if (!item->ri_type) {
- xfs_warn(log->l_mp, "%s: invalid item type (%d)",
- __func__, ITEM_TYPE(item));
- ASSERT(0);
- return -EFSCORRUPTED;
- }
- if (!item->ri_type->commit_pass1_fn)
- return 0;
- return item->ri_type->commit_pass1_fn(log, item);
-}
-
-STATIC int
-xlog_recover_commit_pass2(
- struct xlog *log,
- struct xlog_recover *trans,
- struct list_head *buffer_list,
- struct xlog_recover_item *item)
-{
- trace_xfs_log_recover_item_recover(log, trans, item, XLOG_RECOVER_PASS2);
-
- if (!item->ri_type) {
- xfs_warn(log->l_mp, "%s: invalid item type (%d)",
- __func__, ITEM_TYPE(item));
- ASSERT(0);
- return -EFSCORRUPTED;
- }
- if (!item->ri_type->commit_pass2_fn)
- return 0;
- return item->ri_type->commit_pass2_fn(log, buffer_list, item,
- trans->r_lsn);
-}
-
STATIC int
xlog_recover_items_pass2(
struct xlog *log,
@@ -2146,16 +2087,18 @@ xlog_recover_items_pass2(
struct list_head *item_list)
{
struct xlog_recover_item *item;
- int error = 0;
+ int error;
list_for_each_entry(item, item_list, ri_list) {
- error = xlog_recover_commit_pass2(log, trans,
- buffer_list, item);
+ if (!item->ri_ops->commit_pass2_fn)
+ continue;
+ error = item->ri_ops->commit_pass2_fn(log, buffer_list, item,
+ trans->r_lsn);
if (error)
return error;
}
- return error;
+ return 0;
}
/*
@@ -2187,13 +2130,16 @@ xlog_recover_commit_trans(
return error;
list_for_each_entry_safe(item, next, &trans->r_itemq, ri_list) {
+ trace_xfs_log_recover_item_recover(log, trans, item, pass);
+
switch (pass) {
case XLOG_RECOVER_PASS1:
- error = xlog_recover_commit_pass1(log, trans, item);
+ if (item->ri_ops->commit_pass1_fn)
+ error = item->ri_ops->commit_pass1_fn(log, item);
break;
case XLOG_RECOVER_PASS2:
- if (item->ri_type && item->ri_type->ra_pass2_fn)
- item->ri_type->ra_pass2_fn(log, item);
+ if (item->ri_ops->ra_pass2_fn)
+ item->ri_ops->ra_pass2_fn(log, item);
list_move_tail(&item->ri_list, &ra_list);
items_queued++;
if (items_queued >= XLOG_RECOVER_COMMIT_QUEUE_MAX) {
diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c
index 53a79dc618f76..5703d5fdf4eeb 100644
--- a/fs/xfs/xfs_refcount_item.c
+++ b/fs/xfs/xfs_refcount_item.c
@@ -690,10 +690,12 @@ xlog_recover_refcount_done_commit_pass2(
return 0;
}
-const struct xlog_recover_item_type xlog_refcount_intent_item_type = {
+const struct xlog_recover_item_ops xlog_refcount_intent_item_type = {
+ .item_type = XFS_LI_CUI,
.commit_pass2_fn = xlog_recover_refcount_intent_commit_pass2,
};
-const struct xlog_recover_item_type xlog_refcount_done_item_type = {
+const struct xlog_recover_item_ops xlog_refcount_done_item_type = {
+ .item_type = XFS_LI_CUD,
.commit_pass2_fn = xlog_recover_refcount_done_commit_pass2,
};
diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c
index cee5c61550321..12e035ff7bb2d 100644
--- a/fs/xfs/xfs_rmap_item.c
+++ b/fs/xfs/xfs_rmap_item.c
@@ -680,10 +680,12 @@ xlog_recover_rmap_done_commit_pass2(
return 0;
}
-const struct xlog_recover_item_type xlog_rmap_intent_item_type = {
+const struct xlog_recover_item_ops xlog_rmap_intent_item_type = {
+ .item_type = XFS_LI_RUI,
.commit_pass2_fn = xlog_recover_rmap_intent_commit_pass2,
};
-const struct xlog_recover_item_type xlog_rmap_done_item_type = {
+const struct xlog_recover_item_ops xlog_rmap_done_item_type = {
+ .item_type = XFS_LI_RUD,
.commit_pass2_fn = xlog_recover_rmap_done_commit_pass2,
};
next prev parent reply other threads:[~2020-05-01 10:15 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-30 0:47 [PATCH v2 00/21] xfs: refactor log recovery Darrick J. Wong
2020-04-30 0:47 ` [PATCH 01/21] xfs: refactor log recovery item sorting into a generic dispatch structure Darrick J. Wong
2020-04-30 5:53 ` Christoph Hellwig
2020-04-30 15:08 ` Darrick J. Wong
2020-04-30 18:16 ` Darrick J. Wong
2020-05-01 8:08 ` Christoph Hellwig
2020-05-01 10:40 ` Chandan Rajendra
2020-04-30 0:47 ` [PATCH 02/21] xfs: refactor log recovery item dispatch for pass2 readhead functions Darrick J. Wong
2020-05-01 12:10 ` Chandan Rajendra
2020-04-30 0:47 ` [PATCH 03/21] xfs: refactor log recovery item dispatch for pass1 commit functions Darrick J. Wong
2020-04-30 0:48 ` [PATCH 04/21] xfs: refactor log recovery buffer item dispatch for pass2 " Darrick J. Wong
2020-05-01 13:43 ` Chandan Rajendra
2020-04-30 0:48 ` [PATCH 05/21] xfs: refactor log recovery inode " Darrick J. Wong
2020-05-01 14:03 ` Chandan Rajendra
2020-04-30 0:48 ` [PATCH 06/21] xfs: refactor log recovery dquot " Darrick J. Wong
2020-05-01 14:14 ` Chandan Rajendra
2020-04-30 0:48 ` [PATCH 07/21] xfs: refactor log recovery icreate " Darrick J. Wong
2020-05-01 14:18 ` Chandan Rajendra
2020-04-30 0:48 ` [PATCH 08/21] xfs: remove log recovery quotaoff " Darrick J. Wong
2020-05-01 15:09 ` Chandan Rajendra
2020-05-01 17:41 ` Darrick J. Wong
2020-04-30 0:48 ` [PATCH 09/21] xfs: refactor log recovery EFI " Darrick J. Wong
2020-05-01 10:28 ` Christoph Hellwig
2020-05-01 17:56 ` Darrick J. Wong
2020-04-30 0:48 ` [PATCH 10/21] xfs: refactor log recovery RUI " Darrick J. Wong
2020-04-30 0:48 ` [PATCH 11/21] xfs: refactor log recovery CUI " Darrick J. Wong
2020-04-30 0:48 ` [PATCH 12/21] xfs: refactor log recovery BUI " Darrick J. Wong
2020-04-30 0:48 ` [PATCH 13/21] xfs: refactor recovered EFI log item playback Darrick J. Wong
2020-05-01 10:19 ` Christoph Hellwig
2020-05-01 17:58 ` Darrick J. Wong
2020-04-30 0:49 ` [PATCH 14/21] xfs: refactor recovered RUI " Darrick J. Wong
2020-04-30 0:49 ` [PATCH 15/21] xfs: refactor recovered CUI " Darrick J. Wong
2020-04-30 0:49 ` [PATCH 16/21] xfs: refactor recovered BUI " Darrick J. Wong
2020-04-30 0:49 ` [PATCH 17/21] xfs: refactor releasing finished intents during log recovery Darrick J. Wong
2020-04-30 0:49 ` [PATCH 18/21] xfs: refactor adding recovered intent items to the log Darrick J. Wong
2020-04-30 0:49 ` [PATCH 19/21] xfs: refactor intent item RECOVERED flag into the log item Darrick J. Wong
2020-04-30 0:49 ` [PATCH 20/21] xfs: refactor intent item iop_recover calls Darrick J. Wong
2020-04-30 0:49 ` [PATCH 21/21] xfs: remove unnecessary includes from xfs_log_recover.c Darrick J. Wong
2020-05-01 10:15 ` Christoph Hellwig [this message]
2020-05-01 16:53 ` [PATCH v2 00/21] xfs: refactor log recovery Darrick J. Wong
2020-05-01 17:03 ` Christoph Hellwig
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200501101539.GA21903@infradead.org \
--to=hch@infradead.org \
--cc=darrick.wong@oracle.com \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).