diff mbox series

[v13,06/10] xfs: Add xfs_attr_set_deferred and xfs_attr_remove_deferred

Message ID 20201023063435.7510-7-allison.henderson@oracle.com (mailing list archive)
State Superseded
Headers show
Series xfs: Delayed Attributes | expand

Commit Message

Allison Henderson Oct. 23, 2020, 6:34 a.m. UTC 
From: Allison Collins <allison.henderson@oracle.com>

These routines to set up and start a new deferred attribute operations.
These functions are meant to be called by any routine needing to
initiate a deferred attribute operation as opposed to the existing
inline operations. New helper function xfs_attr_item_init also added.

Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
---
 fs/xfs/libxfs/xfs_attr.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/libxfs/xfs_attr.h |  2 ++
 2 files changed, 56 insertions(+)

Comments

Darrick J. Wong Nov. 10, 2020, 8:15 p.m. UTC | #1 
On Thu, Oct 22, 2020 at 11:34:31PM -0700, Allison Henderson wrote:
> From: Allison Collins <allison.henderson@oracle.com>
> 
> These routines to set up and start a new deferred attribute operations.
> These functions are meant to be called by any routine needing to
> initiate a deferred attribute operation as opposed to the existing
> inline operations. New helper function xfs_attr_item_init also added.
> 
> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/xfs/libxfs/xfs_attr.h |  2 ++
>  2 files changed, 56 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 760383c..7fe5554 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -25,6 +25,7 @@
>  #include "xfs_trans_space.h"
>  #include "xfs_trace.h"
>  #include "xfs_attr_item.h"
> +#include "xfs_attr.h"
>  
>  /*
>   * xfs_attr.c
> @@ -643,6 +644,59 @@ xfs_attr_set(
>  	goto out_unlock;
>  }
>  
> +STATIC int
> +xfs_attr_item_init(
> +	struct xfs_da_args	*args,
> +	unsigned int		op_flags,	/* op flag (set or remove) */
> +	struct xfs_attr_item	**attr)		/* new xfs_attr_item */
> +{
> +
> +	struct xfs_attr_item	*new;
> +
> +	new = kmem_alloc_large(sizeof(struct xfs_attr_item), KM_NOFS);

I don't think we need _large allocations for struct xfs_attr_item, right?

> +	memset(new, 0, sizeof(struct xfs_attr_item));

Use kmem_zalloc and you won't have to memset.  Better yet, zalloc will
get you memory that's been pre-zeroed in the background.

> +	new->xattri_op_flags = op_flags;
> +	new->xattri_dac.da_args = args;
> +
> +	*attr = new;
> +	return 0;
> +}
> +
> +/* Sets an attribute for an inode as a deferred operation */
> +int
> +xfs_attr_set_deferred(
> +	struct xfs_da_args	*args)
> +{
> +	struct xfs_attr_item	*new;
> +	int			error = 0;
> +
> +	error = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_SET, &new);
> +	if (error)
> +		return error;
> +
> +	xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list);
> +
> +	return 0;
> +}

The changes in "xfs: enable delayed attributes" should be moved to this
patch so that these new functions immediately have callers.

(Also see the reply I sent to the next patch, which will avoid weird
regressions if someone's bisect lands in the middle of this series...)

--D

> +
> +/* Removes an attribute for an inode as a deferred operation */
> +int
> +xfs_attr_remove_deferred(
> +	struct xfs_da_args	*args)
> +{
> +
> +	struct xfs_attr_item	*new;
> +	int			error;
> +
> +	error  = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_REMOVE, &new);
> +	if (error)
> +		return error;
> +
> +	xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list);
> +
> +	return 0;
> +}
> +
>  /*========================================================================
>   * External routines when attribute list is inside the inode
>   *========================================================================*/
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index 5b4a1ca..8a08411 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -307,5 +307,7 @@ bool xfs_attr_namecheck(const void *name, size_t length);
>  void xfs_delattr_context_init(struct xfs_delattr_context *dac,
>  			      struct xfs_da_args *args);
>  int xfs_attr_calc_size(struct xfs_da_args *args, int *local);
> +int xfs_attr_set_deferred(struct xfs_da_args *args);
> +int xfs_attr_remove_deferred(struct xfs_da_args *args);
>  
>  #endif	/* __XFS_ATTR_H__ */
> -- 
> 2.7.4
>
Allison Henderson Nov. 13, 2020, 1:27 a.m. UTC | #2 
On 11/10/20 1:15 PM, Darrick J. Wong wrote:
> On Thu, Oct 22, 2020 at 11:34:31PM -0700, Allison Henderson wrote:
>> From: Allison Collins <allison.henderson@oracle.com>
>>
>> These routines to set up and start a new deferred attribute operations.
>> These functions are meant to be called by any routine needing to
>> initiate a deferred attribute operation as opposed to the existing
>> inline operations. New helper function xfs_attr_item_init also added.
>>
>> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
>> ---
>>   fs/xfs/libxfs/xfs_attr.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   fs/xfs/libxfs/xfs_attr.h |  2 ++
>>   2 files changed, 56 insertions(+)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index 760383c..7fe5554 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -25,6 +25,7 @@
>>   #include "xfs_trans_space.h"
>>   #include "xfs_trace.h"
>>   #include "xfs_attr_item.h"
>> +#include "xfs_attr.h"
>>   
>>   /*
>>    * xfs_attr.c
>> @@ -643,6 +644,59 @@ xfs_attr_set(
>>   	goto out_unlock;
>>   }
>>   
>> +STATIC int
>> +xfs_attr_item_init(
>> +	struct xfs_da_args	*args,
>> +	unsigned int		op_flags,	/* op flag (set or remove) */
>> +	struct xfs_attr_item	**attr)		/* new xfs_attr_item */
>> +{
>> +
>> +	struct xfs_attr_item	*new;
>> +
>> +	new = kmem_alloc_large(sizeof(struct xfs_attr_item), KM_NOFS);
> 
> I don't think we need _large allocations for struct xfs_attr_item, right?
I will try it and see, I think it should be ok, one of the new test 
cases I'm using does try to progressively add larger and larger attrs. 
If it doesnt work, I'll make a note of it though.

> 
>> +	memset(new, 0, sizeof(struct xfs_attr_item));
> 
> Use kmem_zalloc and you won't have to memset.  Better yet, zalloc will
> get you memory that's been pre-zeroed in the background.
> 
>> +	new->xattri_op_flags = op_flags;
>> +	new->xattri_dac.da_args = args;
>> +
>> +	*attr = new;
>> +	return 0;
>> +}
>> +
>> +/* Sets an attribute for an inode as a deferred operation */
>> +int
>> +xfs_attr_set_deferred(
>> +	struct xfs_da_args	*args)
>> +{
>> +	struct xfs_attr_item	*new;
>> +	int			error = 0;
>> +
>> +	error = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_SET, &new);
>> +	if (error)
>> +		return error;
>> +
>> +	xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list);
>> +
>> +	return 0;
>> +}
> 
> The changes in "xfs: enable delayed attributes" should be moved to this
> patch so that these new functions immediately have callers.
Sure, will merge those patches together then

> 
> (Also see the reply I sent to the next patch, which will avoid weird
> regressions if someone's bisect lands in the middle of this series...)
> 
> --D
> 
>> +
>> +/* Removes an attribute for an inode as a deferred operation */
>> +int
>> +xfs_attr_remove_deferred(
>> +	struct xfs_da_args	*args)
>> +{
>> +
>> +	struct xfs_attr_item	*new;
>> +	int			error;
>> +
>> +	error  = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_REMOVE, &new);
>> +	if (error)
>> +		return error;
>> +
>> +	xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list);
>> +
>> +	return 0;
>> +}
>> +
>>   /*========================================================================
>>    * External routines when attribute list is inside the inode
>>    *========================================================================*/
>> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
>> index 5b4a1ca..8a08411 100644
>> --- a/fs/xfs/libxfs/xfs_attr.h
>> +++ b/fs/xfs/libxfs/xfs_attr.h
>> @@ -307,5 +307,7 @@ bool xfs_attr_namecheck(const void *name, size_t length);
>>   void xfs_delattr_context_init(struct xfs_delattr_context *dac,
>>   			      struct xfs_da_args *args);
>>   int xfs_attr_calc_size(struct xfs_da_args *args, int *local);
>> +int xfs_attr_set_deferred(struct xfs_da_args *args);
>> +int xfs_attr_remove_deferred(struct xfs_da_args *args);
>>   
>>   #endif	/* __XFS_ATTR_H__ */
>> -- 
>> 2.7.4
>>
Darrick J. Wong Nov. 14, 2020, 2:03 a.m. UTC | #3 
On Thu, Nov 12, 2020 at 06:27:38PM -0700, Allison Henderson wrote:
> 
> 
> On 11/10/20 1:15 PM, Darrick J. Wong wrote:
> > On Thu, Oct 22, 2020 at 11:34:31PM -0700, Allison Henderson wrote:
> > > From: Allison Collins <allison.henderson@oracle.com>
> > > 
> > > These routines to set up and start a new deferred attribute operations.
> > > These functions are meant to be called by any routine needing to
> > > initiate a deferred attribute operation as opposed to the existing
> > > inline operations. New helper function xfs_attr_item_init also added.
> > > 
> > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> > > ---
> > >   fs/xfs/libxfs/xfs_attr.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >   fs/xfs/libxfs/xfs_attr.h |  2 ++
> > >   2 files changed, 56 insertions(+)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > > index 760383c..7fe5554 100644
> > > --- a/fs/xfs/libxfs/xfs_attr.c
> > > +++ b/fs/xfs/libxfs/xfs_attr.c
> > > @@ -25,6 +25,7 @@
> > >   #include "xfs_trans_space.h"
> > >   #include "xfs_trace.h"
> > >   #include "xfs_attr_item.h"
> > > +#include "xfs_attr.h"
> > >   /*
> > >    * xfs_attr.c
> > > @@ -643,6 +644,59 @@ xfs_attr_set(
> > >   	goto out_unlock;
> > >   }
> > > +STATIC int
> > > +xfs_attr_item_init(
> > > +	struct xfs_da_args	*args,
> > > +	unsigned int		op_flags,	/* op flag (set or remove) */
> > > +	struct xfs_attr_item	**attr)		/* new xfs_attr_item */
> > > +{
> > > +
> > > +	struct xfs_attr_item	*new;
> > > +
> > > +	new = kmem_alloc_large(sizeof(struct xfs_attr_item), KM_NOFS);
> > 
> > I don't think we need _large allocations for struct xfs_attr_item, right?
> I will try it and see, I think it should be ok, one of the new test cases
> I'm using does try to progressively add larger and larger attrs. If it
> doesnt work, I'll make a note of it though.

Ok.  Note that kmem_alloc will only return heap objects from memory
that's directly addressable by the kerneli (and can't be larger than a
page), whereas kmem_alloc_large can fall back to virtually mapped
memory, which is scarce on 32-bit systems.

--D

> > 
> > > +	memset(new, 0, sizeof(struct xfs_attr_item));
> > 
> > Use kmem_zalloc and you won't have to memset.  Better yet, zalloc will
> > get you memory that's been pre-zeroed in the background.
> > 
> > > +	new->xattri_op_flags = op_flags;
> > > +	new->xattri_dac.da_args = args;
> > > +
> > > +	*attr = new;
> > > +	return 0;
> > > +}
> > > +
> > > +/* Sets an attribute for an inode as a deferred operation */
> > > +int
> > > +xfs_attr_set_deferred(
> > > +	struct xfs_da_args	*args)
> > > +{
> > > +	struct xfs_attr_item	*new;
> > > +	int			error = 0;
> > > +
> > > +	error = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_SET, &new);
> > > +	if (error)
> > > +		return error;
> > > +
> > > +	xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list);
> > > +
> > > +	return 0;
> > > +}
> > 
> > The changes in "xfs: enable delayed attributes" should be moved to this
> > patch so that these new functions immediately have callers.
> Sure, will merge those patches together then
> 
> > 
> > (Also see the reply I sent to the next patch, which will avoid weird
> > regressions if someone's bisect lands in the middle of this series...)
> > 
> > --D
> > 
> > > +
> > > +/* Removes an attribute for an inode as a deferred operation */
> > > +int
> > > +xfs_attr_remove_deferred(
> > > +	struct xfs_da_args	*args)
> > > +{
> > > +
> > > +	struct xfs_attr_item	*new;
> > > +	int			error;
> > > +
> > > +	error  = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_REMOVE, &new);
> > > +	if (error)
> > > +		return error;
> > > +
> > > +	xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >   /*========================================================================
> > >    * External routines when attribute list is inside the inode
> > >    *========================================================================*/
> > > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> > > index 5b4a1ca..8a08411 100644
> > > --- a/fs/xfs/libxfs/xfs_attr.h
> > > +++ b/fs/xfs/libxfs/xfs_attr.h
> > > @@ -307,5 +307,7 @@ bool xfs_attr_namecheck(const void *name, size_t length);
> > >   void xfs_delattr_context_init(struct xfs_delattr_context *dac,
> > >   			      struct xfs_da_args *args);
> > >   int xfs_attr_calc_size(struct xfs_da_args *args, int *local);
> > > +int xfs_attr_set_deferred(struct xfs_da_args *args);
> > > +int xfs_attr_remove_deferred(struct xfs_da_args *args);
> > >   #endif	/* __XFS_ATTR_H__ */
> > > -- 
> > > 2.7.4
> > >
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 760383c..7fe5554 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -25,6 +25,7 @@ 
 #include "xfs_trans_space.h"
 #include "xfs_trace.h"
 #include "xfs_attr_item.h"
+#include "xfs_attr.h"
 
 /*
  * xfs_attr.c
@@ -643,6 +644,59 @@  xfs_attr_set(
 	goto out_unlock;
 }
 
+STATIC int
+xfs_attr_item_init(
+	struct xfs_da_args	*args,
+	unsigned int		op_flags,	/* op flag (set or remove) */
+	struct xfs_attr_item	**attr)		/* new xfs_attr_item */
+{
+
+	struct xfs_attr_item	*new;
+
+	new = kmem_alloc_large(sizeof(struct xfs_attr_item), KM_NOFS);
+	memset(new, 0, sizeof(struct xfs_attr_item));
+	new->xattri_op_flags = op_flags;
+	new->xattri_dac.da_args = args;
+
+	*attr = new;
+	return 0;
+}
+
+/* Sets an attribute for an inode as a deferred operation */
+int
+xfs_attr_set_deferred(
+	struct xfs_da_args	*args)
+{
+	struct xfs_attr_item	*new;
+	int			error = 0;
+
+	error = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_SET, &new);
+	if (error)
+		return error;
+
+	xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list);
+
+	return 0;
+}
+
+/* Removes an attribute for an inode as a deferred operation */
+int
+xfs_attr_remove_deferred(
+	struct xfs_da_args	*args)
+{
+
+	struct xfs_attr_item	*new;
+	int			error;
+
+	error  = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_REMOVE, &new);
+	if (error)
+		return error;
+
+	xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list);
+
+	return 0;
+}
+
 /*========================================================================
  * External routines when attribute list is inside the inode
  *========================================================================*/
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index 5b4a1ca..8a08411 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -307,5 +307,7 @@  bool xfs_attr_namecheck(const void *name, size_t length);
 void xfs_delattr_context_init(struct xfs_delattr_context *dac,
 			      struct xfs_da_args *args);
 int xfs_attr_calc_size(struct xfs_da_args *args, int *local);
+int xfs_attr_set_deferred(struct xfs_da_args *args);
+int xfs_attr_remove_deferred(struct xfs_da_args *args);
 
 #endif	/* __XFS_ATTR_H__ */