* [PATCH 0/2] libxfs: defer ops idiotproofing
@ 2018-10-29 18:26 Darrick J. Wong
2018-10-29 18:26 ` [PATCH 1/2] libxfs: idiotproof defer op type configuration Darrick J. Wong
2018-10-29 18:26 ` [PATCH 2/2] libxfs: streamline defer op type handling Darrick J. Wong
0 siblings, 2 replies; 3+ messages in thread
From: Darrick J. Wong @ 2018-10-29 18:26 UTC (permalink / raw)
To: sandeen, darrick.wong; +Cc: linux-xfs
Hi all,
Here are a couple of idiotproofing fixes for xfsprogs. We forgot to
port the defered AGFL free stuff to xfsprogs, which lead to sporadic
crashes in xfs_repair... way back in 4.18. :(
To prevent a recurrence, we now require the libxfs client to export the
(currently five) defer_ops type structures so that we can link the whole
mess together at build time instead of dynamically assembling the types
at run time.
The second patch removes now-unnecessary fields from the defer ops type
structure and streamlines some of the xfs_defer.c code.
Comments and questions are, as always, welcome.
--D
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH 1/2] libxfs: idiotproof defer op type configuration
2018-10-29 18:26 [PATCH 0/2] libxfs: defer ops idiotproofing Darrick J. Wong
@ 2018-10-29 18:26 ` Darrick J. Wong
2018-10-29 18:26 ` [PATCH 2/2] libxfs: streamline defer op type handling Darrick J. Wong
1 sibling, 0 replies; 3+ messages in thread
From: Darrick J. Wong @ 2018-10-29 18:26 UTC (permalink / raw)
To: sandeen, darrick.wong; +Cc: linux-xfs
From: Darrick J. Wong <darrick.wong@oracle.com>
Recently, we forgot to port a new defer op type to xfsprogs, which
caused us some userspace pain. Reorganize the way we make libxfs
clients supply defer op type information so that all type information
has to be provided at build time instead of risky runtime dynamic
configuration.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
include/xfs_trans.h | 5 -----
libxfs/defer_item.c | 39 +++++----------------------------------
libxfs/init.c | 5 -----
libxfs/xfs_defer.c | 17 ++++++++---------
libxfs/xfs_defer.h | 6 +++++-
5 files changed, 18 insertions(+), 54 deletions(-)
diff --git a/include/xfs_trans.h b/include/xfs_trans.h
index 31df1172..9e44d18e 100644
--- a/include/xfs_trans.h
+++ b/include/xfs_trans.h
@@ -152,9 +152,4 @@ libxfs_trans_read_buf(
return libxfs_trans_read_buf_map(mp, tp, btp, &map, 1, flags, bpp, ops);
}
-void xfs_extent_free_init_defer_op(void);
-void xfs_rmap_update_init_defer_op(void);
-void xfs_refcount_update_init_defer_op(void);
-void xfs_bmap_update_init_defer_op(void);
-
#endif /* __XFS_TRANS_H__ */
diff --git a/libxfs/defer_item.c b/libxfs/defer_item.c
index 4f7d5499..d36f9d9e 100644
--- a/libxfs/defer_item.c
+++ b/libxfs/defer_item.c
@@ -107,7 +107,7 @@ xfs_extent_free_cancel_item(
kmem_free(free);
}
-static const struct xfs_defer_op_type xfs_extent_free_defer_type = {
+const struct xfs_defer_op_type xfs_extent_free_defer_type = {
.type = XFS_DEFER_OPS_TYPE_FREE,
.diff_items = xfs_extent_free_diff_items,
.create_intent = xfs_extent_free_create_intent,
@@ -150,7 +150,7 @@ xfs_agfl_free_finish_item(
}
/* sub-type with special handling for AGFL deferred frees */
-static const struct xfs_defer_op_type xfs_agfl_free_defer_type = {
+const struct xfs_defer_op_type xfs_agfl_free_defer_type = {
.type = XFS_DEFER_OPS_TYPE_AGFL_FREE,
.diff_items = xfs_extent_free_diff_items,
.create_intent = xfs_extent_free_create_intent,
@@ -161,14 +161,6 @@ static const struct xfs_defer_op_type xfs_agfl_free_defer_type = {
.cancel_item = xfs_extent_free_cancel_item,
};
-/* Register the deferred op type. */
-void
-xfs_extent_free_init_defer_op(void)
-{
- xfs_defer_init_op_type(&xfs_extent_free_defer_type);
- xfs_defer_init_op_type(&xfs_agfl_free_defer_type);
-}
-
/* Reverse Mapping */
/* Sort rmap intents by AG. */
@@ -270,7 +262,7 @@ xfs_rmap_update_cancel_item(
kmem_free(rmap);
}
-static const struct xfs_defer_op_type xfs_rmap_update_defer_type = {
+const struct xfs_defer_op_type xfs_rmap_update_defer_type = {
.type = XFS_DEFER_OPS_TYPE_RMAP,
.diff_items = xfs_rmap_update_diff_items,
.create_intent = xfs_rmap_update_create_intent,
@@ -282,13 +274,6 @@ static const struct xfs_defer_op_type xfs_rmap_update_defer_type = {
.cancel_item = xfs_rmap_update_cancel_item,
};
-/* Register the deferred op type. */
-void
-xfs_rmap_update_init_defer_op(void)
-{
- xfs_defer_init_op_type(&xfs_rmap_update_defer_type);
-}
-
/* Reference Counting */
/* Sort refcount intents by AG. */
@@ -398,7 +383,7 @@ xfs_refcount_update_cancel_item(
kmem_free(refc);
}
-static const struct xfs_defer_op_type xfs_refcount_update_defer_type = {
+const struct xfs_defer_op_type xfs_refcount_update_defer_type = {
.type = XFS_DEFER_OPS_TYPE_REFCOUNT,
.diff_items = xfs_refcount_update_diff_items,
.create_intent = xfs_refcount_update_create_intent,
@@ -410,13 +395,6 @@ static const struct xfs_defer_op_type xfs_refcount_update_defer_type = {
.cancel_item = xfs_refcount_update_cancel_item,
};
-/* Register the deferred op type. */
-void
-xfs_refcount_update_init_defer_op(void)
-{
- xfs_defer_init_op_type(&xfs_refcount_update_defer_type);
-}
-
/* Inode Block Mapping */
/* Sort bmap intents by inode. */
@@ -510,7 +488,7 @@ xfs_bmap_update_cancel_item(
kmem_free(bmap);
}
-static const struct xfs_defer_op_type xfs_bmap_update_defer_type = {
+const struct xfs_defer_op_type xfs_bmap_update_defer_type = {
.type = XFS_DEFER_OPS_TYPE_BMAP,
.diff_items = xfs_bmap_update_diff_items,
.create_intent = xfs_bmap_update_create_intent,
@@ -520,10 +498,3 @@ static const struct xfs_defer_op_type xfs_bmap_update_defer_type = {
.finish_item = xfs_bmap_update_finish_item,
.cancel_item = xfs_bmap_update_cancel_item,
};
-
-/* Register the deferred op type. */
-void
-xfs_bmap_update_init_defer_op(void)
-{
- xfs_defer_init_op_type(&xfs_bmap_update_defer_type);
-}
diff --git a/libxfs/init.c b/libxfs/init.c
index 10dcbf3a..7c6548a8 100644
--- a/libxfs/init.c
+++ b/libxfs/init.c
@@ -248,11 +248,6 @@ libxfs_init(libxfs_init_t *a)
fd = -1;
flags = (a->isreadonly | a->isdirect);
- xfs_extent_free_init_defer_op();
- xfs_rmap_update_init_defer_op();
- xfs_refcount_update_init_defer_op();
- xfs_bmap_update_init_defer_op();
-
radix_tree_init();
if (a->volname) {
diff --git a/libxfs/xfs_defer.c b/libxfs/xfs_defer.c
index 5a6da0f3..68fe07a4 100644
--- a/libxfs/xfs_defer.c
+++ b/libxfs/xfs_defer.c
@@ -170,7 +170,13 @@
* reoccur.
*/
-static const struct xfs_defer_op_type *defer_op_types[XFS_DEFER_OPS_TYPE_MAX];
+static const struct xfs_defer_op_type *defer_op_types[] = {
+ [XFS_DEFER_OPS_TYPE_BMAP] = &xfs_bmap_update_defer_type,
+ [XFS_DEFER_OPS_TYPE_REFCOUNT] = &xfs_refcount_update_defer_type,
+ [XFS_DEFER_OPS_TYPE_RMAP] = &xfs_rmap_update_defer_type,
+ [XFS_DEFER_OPS_TYPE_FREE] = &xfs_extent_free_defer_type,
+ [XFS_DEFER_OPS_TYPE_AGFL_FREE] = &xfs_agfl_free_defer_type,
+};
/*
* For each pending item in the intake list, log its intent item and the
@@ -486,6 +492,7 @@ xfs_defer_add(
struct xfs_defer_pending *dfp = NULL;
ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
+ BUILD_BUG_ON(ARRAY_SIZE(defer_op_types) != XFS_DEFER_OPS_TYPE_MAX);
/*
* Add the item to a pending item at the end of the intake list.
@@ -515,14 +522,6 @@ xfs_defer_add(
dfp->dfp_count++;
}
-/* Initialize a deferred operation list. */
-void
-xfs_defer_init_op_type(
- const struct xfs_defer_op_type *type)
-{
- defer_op_types[type->type] = type;
-}
-
/*
* Move deferred ops from one transaction to another and reset the source to
* initial state. This is primarily used to carry state forward across
diff --git a/libxfs/xfs_defer.h b/libxfs/xfs_defer.h
index 2584a5b9..0a4b88e3 100644
--- a/libxfs/xfs_defer.h
+++ b/libxfs/xfs_defer.h
@@ -56,6 +56,10 @@ struct xfs_defer_op_type {
void (*log_item)(struct xfs_trans *, void *, struct list_head *);
};
-void xfs_defer_init_op_type(const struct xfs_defer_op_type *type);
+extern const struct xfs_defer_op_type xfs_bmap_update_defer_type;
+extern const struct xfs_defer_op_type xfs_refcount_update_defer_type;
+extern const struct xfs_defer_op_type xfs_rmap_update_defer_type;
+extern const struct xfs_defer_op_type xfs_extent_free_defer_type;
+extern const struct xfs_defer_op_type xfs_agfl_free_defer_type;
#endif /* __XFS_DEFER_H__ */
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH 2/2] libxfs: streamline defer op type handling
2018-10-29 18:26 [PATCH 0/2] libxfs: defer ops idiotproofing Darrick J. Wong
2018-10-29 18:26 ` [PATCH 1/2] libxfs: idiotproof defer op type configuration Darrick J. Wong
@ 2018-10-29 18:26 ` Darrick J. Wong
1 sibling, 0 replies; 3+ messages in thread
From: Darrick J. Wong @ 2018-10-29 18:26 UTC (permalink / raw)
To: sandeen, darrick.wong; +Cc: linux-xfs
From: Darrick J. Wong <darrick.wong@oracle.com>
There's no need to bundle a pointer to the defer op type into the defer
op control structure. Instead, store the defer op type enum, which
enables us to shorten some of the lines.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
libxfs/defer_item.c | 5 -----
libxfs/xfs_defer.c | 50 +++++++++++++++++++++++++++-----------------------
libxfs/xfs_defer.h | 31 +++++++++++++++----------------
3 files changed, 42 insertions(+), 44 deletions(-)
diff --git a/libxfs/defer_item.c b/libxfs/defer_item.c
index d36f9d9e..2ebc12bd 100644
--- a/libxfs/defer_item.c
+++ b/libxfs/defer_item.c
@@ -108,7 +108,6 @@ xfs_extent_free_cancel_item(
}
const struct xfs_defer_op_type xfs_extent_free_defer_type = {
- .type = XFS_DEFER_OPS_TYPE_FREE,
.diff_items = xfs_extent_free_diff_items,
.create_intent = xfs_extent_free_create_intent,
.abort_intent = xfs_extent_free_abort_intent,
@@ -151,7 +150,6 @@ xfs_agfl_free_finish_item(
/* sub-type with special handling for AGFL deferred frees */
const struct xfs_defer_op_type xfs_agfl_free_defer_type = {
- .type = XFS_DEFER_OPS_TYPE_AGFL_FREE,
.diff_items = xfs_extent_free_diff_items,
.create_intent = xfs_extent_free_create_intent,
.abort_intent = xfs_extent_free_abort_intent,
@@ -263,7 +261,6 @@ xfs_rmap_update_cancel_item(
}
const struct xfs_defer_op_type xfs_rmap_update_defer_type = {
- .type = XFS_DEFER_OPS_TYPE_RMAP,
.diff_items = xfs_rmap_update_diff_items,
.create_intent = xfs_rmap_update_create_intent,
.abort_intent = xfs_rmap_update_abort_intent,
@@ -384,7 +381,6 @@ xfs_refcount_update_cancel_item(
}
const struct xfs_defer_op_type xfs_refcount_update_defer_type = {
- .type = XFS_DEFER_OPS_TYPE_REFCOUNT,
.diff_items = xfs_refcount_update_diff_items,
.create_intent = xfs_refcount_update_create_intent,
.abort_intent = xfs_refcount_update_abort_intent,
@@ -489,7 +485,6 @@ xfs_bmap_update_cancel_item(
}
const struct xfs_defer_op_type xfs_bmap_update_defer_type = {
- .type = XFS_DEFER_OPS_TYPE_BMAP,
.diff_items = xfs_bmap_update_diff_items,
.create_intent = xfs_bmap_update_create_intent,
.abort_intent = xfs_bmap_update_abort_intent,
diff --git a/libxfs/xfs_defer.c b/libxfs/xfs_defer.c
index 68fe07a4..d9d42713 100644
--- a/libxfs/xfs_defer.c
+++ b/libxfs/xfs_defer.c
@@ -189,15 +189,15 @@ xfs_defer_create_intents(
{
struct list_head *li;
struct xfs_defer_pending *dfp;
+ const struct xfs_defer_op_type *ops;
list_for_each_entry(dfp, &tp->t_dfops, dfp_list) {
- dfp->dfp_intent = dfp->dfp_type->create_intent(tp,
- dfp->dfp_count);
+ ops = defer_op_types[dfp->dfp_type];
+ dfp->dfp_intent = ops->create_intent(tp, dfp->dfp_count);
trace_xfs_defer_create_intent(tp->t_mountp, dfp);
- list_sort(tp->t_mountp, &dfp->dfp_work,
- dfp->dfp_type->diff_items);
+ list_sort(tp->t_mountp, &dfp->dfp_work, ops->diff_items);
list_for_each(li, &dfp->dfp_work)
- dfp->dfp_type->log_item(tp, dfp->dfp_intent, li);
+ ops->log_item(tp, dfp->dfp_intent, li);
}
}
@@ -208,14 +208,16 @@ xfs_defer_trans_abort(
struct list_head *dop_pending)
{
struct xfs_defer_pending *dfp;
+ const struct xfs_defer_op_type *ops;
trace_xfs_defer_trans_abort(tp, _RET_IP_);
/* Abort intent items that don't have a done item. */
list_for_each_entry(dfp, dop_pending, dfp_list) {
+ ops = defer_op_types[dfp->dfp_type];
trace_xfs_defer_pending_abort(tp->t_mountp, dfp);
if (dfp->dfp_intent && !dfp->dfp_done) {
- dfp->dfp_type->abort_intent(dfp->dfp_intent);
+ ops->abort_intent(dfp->dfp_intent);
dfp->dfp_intent = NULL;
}
}
@@ -319,18 +321,20 @@ xfs_defer_cancel_list(
struct xfs_defer_pending *pli;
struct list_head *pwi;
struct list_head *n;
+ const struct xfs_defer_op_type *ops;
/*
* Free the pending items. Caller should already have arranged
* for the intent items to be released.
*/
list_for_each_entry_safe(dfp, pli, dop_list, dfp_list) {
+ ops = defer_op_types[dfp->dfp_type];
trace_xfs_defer_cancel_list(mp, dfp);
list_del(&dfp->dfp_list);
list_for_each_safe(pwi, n, &dfp->dfp_work) {
list_del(pwi);
dfp->dfp_count--;
- dfp->dfp_type->cancel_item(pwi);
+ ops->cancel_item(pwi);
}
ASSERT(dfp->dfp_count == 0);
kmem_free(dfp);
@@ -354,7 +358,7 @@ xfs_defer_finish_noroll(
struct list_head *n;
void *state;
int error = 0;
- void (*cleanup_fn)(struct xfs_trans *, void *, int);
+ const struct xfs_defer_op_type *ops;
LIST_HEAD(dop_pending);
ASSERT((*tp)->t_flags & XFS_TRANS_PERM_LOG_RES);
@@ -377,18 +381,18 @@ xfs_defer_finish_noroll(
/* Log an intent-done item for the first pending item. */
dfp = list_first_entry(&dop_pending, struct xfs_defer_pending,
dfp_list);
+ ops = defer_op_types[dfp->dfp_type];
trace_xfs_defer_pending_finish((*tp)->t_mountp, dfp);
- dfp->dfp_done = dfp->dfp_type->create_done(*tp, dfp->dfp_intent,
+ dfp->dfp_done = ops->create_done(*tp, dfp->dfp_intent,
dfp->dfp_count);
- cleanup_fn = dfp->dfp_type->finish_cleanup;
/* Finish the work items. */
state = NULL;
list_for_each_safe(li, n, &dfp->dfp_work) {
list_del(li);
dfp->dfp_count--;
- error = dfp->dfp_type->finish_item(*tp, li,
- dfp->dfp_done, &state);
+ error = ops->finish_item(*tp, li, dfp->dfp_done,
+ &state);
if (error == -EAGAIN) {
/*
* Caller wants a fresh transaction;
@@ -404,8 +408,8 @@ xfs_defer_finish_noroll(
* xfs_defer_cancel will take care of freeing
* all these lists and stuff.
*/
- if (cleanup_fn)
- cleanup_fn(*tp, state, error);
+ if (ops->finish_cleanup)
+ ops->finish_cleanup(*tp, state, error);
goto out;
}
}
@@ -417,20 +421,19 @@ xfs_defer_finish_noroll(
* a Fresh Transaction while Finishing
* Deferred Work" above.
*/
- dfp->dfp_intent = dfp->dfp_type->create_intent(*tp,
+ dfp->dfp_intent = ops->create_intent(*tp,
dfp->dfp_count);
dfp->dfp_done = NULL;
list_for_each(li, &dfp->dfp_work)
- dfp->dfp_type->log_item(*tp, dfp->dfp_intent,
- li);
+ ops->log_item(*tp, dfp->dfp_intent, li);
} else {
/* Done with the dfp, free it. */
list_del(&dfp->dfp_list);
kmem_free(dfp);
}
- if (cleanup_fn)
- cleanup_fn(*tp, state, error);
+ if (ops->finish_cleanup)
+ ops->finish_cleanup(*tp, state, error);
}
out:
@@ -490,6 +493,7 @@ xfs_defer_add(
struct list_head *li)
{
struct xfs_defer_pending *dfp = NULL;
+ const struct xfs_defer_op_type *ops;
ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
BUILD_BUG_ON(ARRAY_SIZE(defer_op_types) != XFS_DEFER_OPS_TYPE_MAX);
@@ -502,15 +506,15 @@ xfs_defer_add(
if (!list_empty(&tp->t_dfops)) {
dfp = list_last_entry(&tp->t_dfops,
struct xfs_defer_pending, dfp_list);
- if (dfp->dfp_type->type != type ||
- (dfp->dfp_type->max_items &&
- dfp->dfp_count >= dfp->dfp_type->max_items))
+ ops = defer_op_types[dfp->dfp_type];
+ if (dfp->dfp_type != type ||
+ (ops->max_items && dfp->dfp_count >= ops->max_items))
dfp = NULL;
}
if (!dfp) {
dfp = kmem_alloc(sizeof(struct xfs_defer_pending),
KM_SLEEP | KM_NOFS);
- dfp->dfp_type = defer_op_types[type];
+ dfp->dfp_type = type;
dfp->dfp_intent = NULL;
dfp->dfp_done = NULL;
dfp->dfp_count = 0;
diff --git a/libxfs/xfs_defer.h b/libxfs/xfs_defer.h
index 0a4b88e3..7c28d760 100644
--- a/libxfs/xfs_defer.h
+++ b/libxfs/xfs_defer.h
@@ -8,20 +8,6 @@
struct xfs_defer_op_type;
-/*
- * Save a log intent item and a list of extents, so that we can replay
- * whatever action had to happen to the extent list and file the log done
- * item.
- */
-struct xfs_defer_pending {
- const struct xfs_defer_op_type *dfp_type; /* function pointers */
- struct list_head dfp_list; /* pending items */
- void *dfp_intent; /* log intent item */
- void *dfp_done; /* log done item */
- struct list_head dfp_work; /* work items */
- unsigned int dfp_count; /* # extent items */
-};
-
/*
* Header for deferred operation list.
*/
@@ -34,6 +20,20 @@ enum xfs_defer_ops_type {
XFS_DEFER_OPS_TYPE_MAX,
};
+/*
+ * Save a log intent item and a list of extents, so that we can replay
+ * whatever action had to happen to the extent list and file the log done
+ * item.
+ */
+struct xfs_defer_pending {
+ struct list_head dfp_list; /* pending items */
+ struct list_head dfp_work; /* work items */
+ void *dfp_intent; /* log intent item */
+ void *dfp_done; /* log done item */
+ unsigned int dfp_count; /* # extent items */
+ enum xfs_defer_ops_type dfp_type;
+};
+
void xfs_defer_add(struct xfs_trans *tp, enum xfs_defer_ops_type type,
struct list_head *h);
int xfs_defer_finish_noroll(struct xfs_trans **tp);
@@ -43,8 +43,6 @@ void xfs_defer_move(struct xfs_trans *dtp, struct xfs_trans *stp);
/* Description of a deferred type. */
struct xfs_defer_op_type {
- enum xfs_defer_ops_type type;
- unsigned int max_items;
void (*abort_intent)(void *);
void *(*create_done)(struct xfs_trans *, void *, unsigned int);
int (*finish_item)(struct xfs_trans *, struct list_head *, void *,
@@ -54,6 +52,7 @@ struct xfs_defer_op_type {
int (*diff_items)(void *, struct list_head *, struct list_head *);
void *(*create_intent)(struct xfs_trans *, uint);
void (*log_item)(struct xfs_trans *, void *, struct list_head *);
+ unsigned int max_items;
};
extern const struct xfs_defer_op_type xfs_bmap_update_defer_type;
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-10-30 3:16 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-29 18:26 [PATCH 0/2] libxfs: defer ops idiotproofing Darrick J. Wong
2018-10-29 18:26 ` [PATCH 1/2] libxfs: idiotproof defer op type configuration Darrick J. Wong
2018-10-29 18:26 ` [PATCH 2/2] libxfs: streamline defer op type handling Darrick J. Wong
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).