diff mbox series

libxfs: check all defer ops types

Message ID 20181026161905.GH28243@magnolia (mailing list archive)
State Rejected
Headers show
Series libxfs: check all defer ops types | expand

Commit Message

Darrick J. Wong Oct. 26, 2018, 4:19 p.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

Make sure we have all the defer ops types loaded before proceeding.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libxfs/init.c      |    5 +++++
 libxfs/xfs_defer.c |   12 ++++++++++++
 libxfs/xfs_defer.h |    1 +
 3 files changed, 18 insertions(+)

Comments

Eric Sandeen Oct. 26, 2018, 7:02 p.m. UTC | #1 
On 10/26/18 11:19 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> 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


> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  libxfs/init.c      |    5 +++++
>  libxfs/xfs_defer.c |   12 ++++++++++++
>  libxfs/xfs_defer.h |    1 +
>  3 files changed, 18 insertions(+)
> 
> diff --git a/libxfs/init.c b/libxfs/init.c
> index 750b8f20..d1fdc3a8 100644
> --- a/libxfs/init.c
> +++ b/libxfs/init.c
> @@ -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();
>  
> diff --git a/libxfs/xfs_defer.c b/libxfs/xfs_defer.c
> index 5a6da0f3..cd3a8ef3 100644
> --- a/libxfs/xfs_defer.c
> +++ b/libxfs/xfs_defer.c
> @@ -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;
> +}
> diff --git a/libxfs/xfs_defer.h b/libxfs/xfs_defer.h
> index 2584a5b9..bc7ae7ff 100644
> --- a/libxfs/xfs_defer.h
> +++ b/libxfs/xfs_defer.h
> @@ -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__ */
>
Eric Sandeen Oct. 26, 2018, 7:11 p.m. UTC | #2 
On 10/26/18 2:02 PM, Eric Sandeen wrote:
> On 10/26/18 11:19 AM, Darrick J. Wong wrote:
>> From: Darrick J. Wong <darrick.wong@oracle.com>
>>
>> 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?

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 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,
};
Darrick J. Wong Oct. 26, 2018, 7:18 p.m. UTC | #3 
On Fri, Oct 26, 2018 at 02:11:47PM -0500, Eric Sandeen wrote:
> 
> 
> On 10/26/18 2:02 PM, Eric Sandeen wrote:
> > On 10/26/18 11:19 AM, Darrick J. Wong wrote:
> >> From: Darrick J. Wong <darrick.wong@oracle.com>
> >>
> >> 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?
> 
> 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 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 struct
xfs_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
> 
> 
> 
>
Christoph Hellwig Oct. 26, 2018, 7:24 p.m. UTC | #4 
On Fri, Oct 26, 2018 at 02:11:47PM -0500, Eric Sandeen wrote:
> 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);
}
Eric Sandeen Oct. 26, 2018, 7:26 p.m. UTC | #5 
On 10/26/18 2:24 PM, Christoph Hellwig wrote:
> On Fri, Oct 26, 2018 at 02:11:47PM -0500, Eric Sandeen wrote:
>> 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);
> }
> 

Uh, yeah, that's what I was trying but pitifully failing to come
up with, thanks.  :)

-Eric
Christoph Hellwig Oct. 26, 2018, 7:26 p.m. UTC | #6 
On Fri, Oct 26, 2018 at 12:18:51PM -0700, Darrick J. Wong wrote:
> 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.

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.
diff mbox series

Patch

diff --git a/libxfs/init.c b/libxfs/init.c
index 750b8f20..d1fdc3a8 100644
--- a/libxfs/init.c
+++ b/libxfs/init.c
@@ -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();
 
diff --git a/libxfs/xfs_defer.c b/libxfs/xfs_defer.c
index 5a6da0f3..cd3a8ef3 100644
--- a/libxfs/xfs_defer.c
+++ b/libxfs/xfs_defer.c
@@ -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;
+}
diff --git a/libxfs/xfs_defer.h b/libxfs/xfs_defer.h
index 2584a5b9..bc7ae7ff 100644
--- a/libxfs/xfs_defer.h
+++ b/libxfs/xfs_defer.h
@@ -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__ */