libxfs: check all defer ops types
PATCH 1/1 libxfs: check all defer ops types
From: Darrick J. Wong <[email protected]>
Make sure we have all the defer ops types loaded before proceeding.
--- libxfs/init.c | 5 +++++ libxfs/xfs_defer.c | 12 ++++++++++++ libxfs/xfs_defer.h | 1 + 3 files changed, 18 insertions(+)
libxfs/init.c +5 -0
@@ -253,6 +253,11 @@ libxfs_init(libxfs_init_t *a)xfs_rmap_update_init_defer_op ();xfs_refcount_update_init_defer_op ();xfs_bmap_update_init_defer_op (); +if (!xfs_defer_check_all_present ()) { +fprintf (stderr,_ ("%s: defer ops not all there\n" ), + progname); + goto done; + }radix_tree_init ();
libxfs/xfs_defer.c +12 -0
@@ -545,3 +545,15 @@ xfs_defer_move(xfs_defer_reset (stp); } + +/* Make sure we filled out all the defer_ops types. */ +bool +xfs_defer_check_all_present (void ) +{ +int x; + +for (x =0 ; x <XFS_DEFER_OPS_TYPE_MAX ; x++) +if (defer_op_types[x] ==NULL ) +return false; +return true; +}
libxfs/xfs_defer.h +1 -0
@@ -57,5 +57,6 @@ struct xfs_defer_op_type { };void xfs_defer_init_op_type (const struct xfs_defer_op_type *type); +bool xfs_defer_check_all_present (void );#endif /* __XFS_DEFER_H__ */
Quoted 3 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
Make sure we have all the defer ops types loaded before proceeding.
So, the only reason we have this problem is that userspace doesn't exactly share the code which handles all the defer ops.
I think this really needs to be a BUILD_BUG_ON if anything, by the time we get to runtime it's really too late, and a weird thing to run at every invocation.
The only thing I can think of is to make an array containing each of the xfs_defer_op_types, and iterate over it in some sort of xfs_defer_ops_init(), then we can compare the size of that array to XFS_DEFER_OPS_TYPE_MAX with a BUILD_BUG_ON. Worth it?
-Eric
Quoted 56 lines
Signed-off-by Darrick J. Wong <[email protected]>--- libxfs/init.c | 5 +++++ libxfs/xfs_defer.c | 12 ++++++++++++ libxfs/xfs_defer.h | 1 + 3 files changed, 18 insertions(+)libxfs/init.c +5
-0@@ -253,6 +253,11 @@ libxfs_init(libxfs_init_t *a)xfs_rmap_update_init_defer_op ();xfs_refcount_update_init_defer_op ();xfs_bmap_update_init_defer_op (); +if (!xfs_defer_check_all_present ()) { +fprintf (stderr,_ ("%s: defer ops not all there\n" ), + progname); + goto done; + }radix_tree_init ();libxfs/xfs_defer.c +12
-0@@ -545,3 +545,15 @@ xfs_defer_move(xfs_defer_reset (stp); } + +/* Make sure we filled out all the defer_ops types. */ +bool +xfs_defer_check_all_present (void ) +{ +int x; + +for (x =0 ; x <XFS_DEFER_OPS_TYPE_MAX ; x++) +if (defer_op_types[x] ==NULL ) +return false; +return true; +}libxfs/xfs_defer.h +1
-0@@ -57,5 +57,6 @@ struct xfs_defer_op_type { };void xfs_defer_init_op_type (const struct xfs_defer_op_type *type); +bool xfs_defer_check_all_present (void );#endif /* __XFS_DEFER_H__ */
Quoted 8 lines · Eric Sandeen
Quoted 3 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
Make sure we have all the defer ops types loaded before proceeding.
So, the only reason we have this problem is that userspace doesn't exactly share the code which handles all the defer ops.
Quoted 8 lines
I think this really needs to be a
BUILD_BUG_ONif anything, by the time we get to runtime it's really too late, and a weird thing to run at every invocation.The only thing I can think of is to make an array containing each of the
xfs_defer_op_types, and iterate over it in some sort ofxfs_defer_ops_init(), then we can compare the size of that array toXFS_DEFER_OPS_TYPE_MAXwith aBUILD_BUG_ON. Worth it?
Uh, or just make a statically initialized array?
I guess nothing about this directly enforces that the ->type number is in the right slot of the array.
static const structxfs_defer_op_type *defer_op_types[XFS_DEFER_OPS_TYPE_MAX] = {
xfs_bmap_update_defer_type, xfs_refcount_update_defer_type, xfs_rmap_update_defer_type, xfs_extent_free_defer_type, xfs_agfl_free_defer_type,
};
Quoted 24 lines · Eric Sandeen
Quoted 8 lines · Eric Sandeen
Quoted 3 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
Make sure we have all the defer ops types loaded before proceeding.
So, the only reason we have this problem is that userspace doesn't exactly share the code which handles all the defer ops.
Quoted 8 lines
I think this really needs to be a
BUILD_BUG_ONif anything, by the time we get to runtime it's really too late, and a weird thing to run at every invocation.The only thing I can think of is to make an array containing each of the
xfs_defer_op_types, and iterate over it in some sort ofxfs_defer_ops_init(), then we can compare the size of that array toXFS_DEFER_OPS_TYPE_MAXwith aBUILD_BUG_ON. Worth it?Uh, or just make a statically initialized array?
I guess nothing about this directly enforces that the ->type number is in the right slot of the array.
Quoted 8 lines
static const struct
xfs_defer_op_type*defer_op_types[XFS_DEFER_OPS_TYPE_MAX] = {
xfs_bmap_update_defer_type,xfs_refcount_update_defer_type,xfs_rmap_update_defer_type,xfs_extent_free_defer_type,xfs_agfl_free_defer_type,
};
I was thinking more:
static const structxfs_defer_op_type defer_op_types[XFS_DEFER_OPS_TYPE_MAX] = {
[XFS_DEFER_OPS_TYPE_AGFL_FREE] = {
.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,
.log_item = xfs_extent_free_log_item,
.create_done = xfs_extent_free_create_done,
.finish_item = xfs_agfl_free_finish_item,
.cancel_item = xfs_extent_free_cancel_item,
},
...
};
The downside of this is that now we have to declare all those functions somewhere so that the fs/xfs/ and libxfs/defer_item.c can match the prototypes. But that's probably better than what we have now, which is a big gaping hole in xfsprogs 4.18.
--D
Quoted 4 lines
Quoted 3 lines · Darrick J. Wong
The downside of this is that now we have to declare all those functions somewhere so that the
fs/xfs/andlibxfs/defer_item.ccan match the prototypes.
I don't think that is worth the pain vs the version I posted. I don't think removing a single indirection really buys us so much as to expose all the internals.
Quoted 8 lines · Eric Sandeen
static const struct
xfs_defer_op_type*defer_op_types[XFS_DEFER_OPS_TYPE_MAX] = {
xfs_bmap_update_defer_type,xfs_refcount_update_defer_type,xfs_rmap_update_defer_type,xfs_extent_free_defer_type,xfs_agfl_free_defer_type,
};
I think this should be:
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,
};
somefunc()
{
BUILD_BUG_ON(ARRAY_SIZE(defer_op_types) != XFS_DEFER_OPS_TYPE_MAX);
}Quoted 20 lines · Christoph Hellwig
Quoted 8 lines · Eric Sandeen
static const struct
xfs_defer_op_type*defer_op_types[XFS_DEFER_OPS_TYPE_MAX] = {
xfs_bmap_update_defer_type,xfs_refcount_update_defer_type,xfs_rmap_update_defer_type,xfs_extent_free_defer_type,xfs_agfl_free_defer_type,
};I think this should be:
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,
};
Quoted 5 lines
somefunc(){ BUILD_BUG_ON(ARRAY_SIZE(defer_op_types) != XFS_DEFER_OPS_TYPE_MAX); }
Uh, yeah, that's what I was trying but pitifully failing to come up with, thanks. :)
-Eric