diff mbox series

[v15,06/22] xfs: Separate xfs_attr_node_addname and xfs_attr_node_addname_work

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

Commit Message

Allison Henderson Feb. 18, 2021, 4:53 p.m. UTC
This patch separate xfs_attr_node_addname into two functions.  This will
help to make it easier to hoist parts of xfs_attr_node_addname that need
state management

Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
---
 fs/xfs/libxfs/xfs_attr.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

Brian Foster Feb. 24, 2021, 3:04 p.m. UTC | #1
On Thu, Feb 18, 2021 at 09:53:32AM -0700, Allison Henderson wrote:
> This patch separate xfs_attr_node_addname into two functions.  This will
> help to make it easier to hoist parts of xfs_attr_node_addname that need
> state management
> 
> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_attr.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 205ad26..bee8d3fb 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -54,6 +54,7 @@ STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
>  STATIC int xfs_attr_node_get(xfs_da_args_t *args);
>  STATIC int xfs_attr_node_addname(xfs_da_args_t *args);
>  STATIC int xfs_attr_node_removename(xfs_da_args_t *args);
> +STATIC int xfs_attr_node_addname_work(struct xfs_da_args *args);
>  STATIC int xfs_attr_node_hasname(xfs_da_args_t *args,
>  				 struct xfs_da_state **state);
>  STATIC int xfs_attr_fillstate(xfs_da_state_t *state);
> @@ -1059,6 +1060,25 @@ xfs_attr_node_addname(
>  			return error;
>  	}
>  
> +	error = xfs_attr_node_addname_work(args);
> +out:
> +	if (state)
> +		xfs_da_state_free(state);
> +	if (error)
> +		return error;
> +	return retval;
> +}
> +
> +
> +STATIC
> +int xfs_attr_node_addname_work(
> +	struct xfs_da_args		*args)
> +{
> +	struct xfs_da_state		*state = NULL;
> +	struct xfs_da_state_blk		*blk;
> +	int				retval = 0;
> +	int				error = 0;
> +
>  	/*
>  	 * Re-find the "old" attribute entry after any split ops. The INCOMPLETE
>  	 * flag means that we will find the "old" attr, not the "new" one.
> -- 
> 2.7.4
>
Allison Henderson Feb. 25, 2021, 6:18 a.m. UTC | #2
On 2/24/21 8:04 AM, Brian Foster wrote:
> On Thu, Feb 18, 2021 at 09:53:32AM -0700, Allison Henderson wrote:
>> This patch separate xfs_attr_node_addname into two functions.  This will
>> help to make it easier to hoist parts of xfs_attr_node_addname that need
>> state management
>>
>> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
>> ---
> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>
Alrighty, thanks!
Allison

> 
>>   fs/xfs/libxfs/xfs_attr.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index 205ad26..bee8d3fb 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -54,6 +54,7 @@ STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
>>   STATIC int xfs_attr_node_get(xfs_da_args_t *args);
>>   STATIC int xfs_attr_node_addname(xfs_da_args_t *args);
>>   STATIC int xfs_attr_node_removename(xfs_da_args_t *args);
>> +STATIC int xfs_attr_node_addname_work(struct xfs_da_args *args);
>>   STATIC int xfs_attr_node_hasname(xfs_da_args_t *args,
>>   				 struct xfs_da_state **state);
>>   STATIC int xfs_attr_fillstate(xfs_da_state_t *state);
>> @@ -1059,6 +1060,25 @@ xfs_attr_node_addname(
>>   			return error;
>>   	}
>>   
>> +	error = xfs_attr_node_addname_work(args);
>> +out:
>> +	if (state)
>> +		xfs_da_state_free(state);
>> +	if (error)
>> +		return error;
>> +	return retval;
>> +}
>> +
>> +
>> +STATIC
>> +int xfs_attr_node_addname_work(
>> +	struct xfs_da_args		*args)
>> +{
>> +	struct xfs_da_state		*state = NULL;
>> +	struct xfs_da_state_blk		*blk;
>> +	int				retval = 0;
>> +	int				error = 0;
>> +
>>   	/*
>>   	 * Re-find the "old" attribute entry after any split ops. The INCOMPLETE
>>   	 * flag means that we will find the "old" attr, not the "new" one.
>> -- 
>> 2.7.4
>>
>
Darrick J. Wong Feb. 26, 2021, 4:02 a.m. UTC | #3
On Thu, Feb 18, 2021 at 09:53:32AM -0700, Allison Henderson wrote:
> This patch separate xfs_attr_node_addname into two functions.  This will
> help to make it easier to hoist parts of xfs_attr_node_addname that need
> state management
> 
> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 205ad26..bee8d3fb 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -54,6 +54,7 @@ STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
>  STATIC int xfs_attr_node_get(xfs_da_args_t *args);
>  STATIC int xfs_attr_node_addname(xfs_da_args_t *args);
>  STATIC int xfs_attr_node_removename(xfs_da_args_t *args);
> +STATIC int xfs_attr_node_addname_work(struct xfs_da_args *args);
>  STATIC int xfs_attr_node_hasname(xfs_da_args_t *args,
>  				 struct xfs_da_state **state);
>  STATIC int xfs_attr_fillstate(xfs_da_state_t *state);
> @@ -1059,6 +1060,25 @@ xfs_attr_node_addname(
>  			return error;
>  	}
>  
> +	error = xfs_attr_node_addname_work(args);
> +out:
> +	if (state)
> +		xfs_da_state_free(state);
> +	if (error)
> +		return error;
> +	return retval;
> +}
> +
> +
> +STATIC
> +int xfs_attr_node_addname_work(

What, erm, work does this function do?  Since it survives to the end of
the patchset, I think this needs a better name (or at least needs a
comment about what it's actually supposed to do).

AFAICT you're splitting node_addname() into two functions because we're
at a transaction roll point, and this "_work" function exists to remove
the copy of the xattr key that has the "INCOMPLETE" bit set (aka the old
one), right?

--D

> +	struct xfs_da_args		*args)
> +{
> +	struct xfs_da_state		*state = NULL;
> +	struct xfs_da_state_blk		*blk;
> +	int				retval = 0;
> +	int				error = 0;
> +
>  	/*
>  	 * Re-find the "old" attribute entry after any split ops. The INCOMPLETE
>  	 * flag means that we will find the "old" attr, not the "new" one.
> -- 
> 2.7.4
>
Allison Henderson Feb. 27, 2021, 12:54 a.m. UTC | #4
On 2/25/21 9:02 PM, Darrick J. Wong wrote:
> On Thu, Feb 18, 2021 at 09:53:32AM -0700, Allison Henderson wrote:
>> This patch separate xfs_attr_node_addname into two functions.  This will
>> help to make it easier to hoist parts of xfs_attr_node_addname that need
>> state management
>>
>> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
>> ---
>>   fs/xfs/libxfs/xfs_attr.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index 205ad26..bee8d3fb 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -54,6 +54,7 @@ STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
>>   STATIC int xfs_attr_node_get(xfs_da_args_t *args);
>>   STATIC int xfs_attr_node_addname(xfs_da_args_t *args);
>>   STATIC int xfs_attr_node_removename(xfs_da_args_t *args);
>> +STATIC int xfs_attr_node_addname_work(struct xfs_da_args *args);
>>   STATIC int xfs_attr_node_hasname(xfs_da_args_t *args,
>>   				 struct xfs_da_state **state);
>>   STATIC int xfs_attr_fillstate(xfs_da_state_t *state);
>> @@ -1059,6 +1060,25 @@ xfs_attr_node_addname(
>>   			return error;
>>   	}
>>   
>> +	error = xfs_attr_node_addname_work(args);
>> +out:
>> +	if (state)
>> +		xfs_da_state_free(state);
>> +	if (error)
>> +		return error;
>> +	return retval;
>> +}
>> +
>> +
>> +STATIC
>> +int xfs_attr_node_addname_work(
> 
> What, erm, work does this function do?  Since it survives to the end of
> the patchset, I think this needs a better name (or at least needs a
> comment about what it's actually supposed to do).
To directly answer the question: it's here to help xfs_attr_set_iter not 
be any bigger than it has to. I think we likely struggled with the name 
because it's almost like it's just the "remainder" of the operation that 
doesnt need state management

> 
> AFAICT you're splitting node_addname() into two functions because we're
> at a transaction roll point, and this "_work" function exists to remove
> the copy of the xattr key that has the "INCOMPLETE" bit set (aka the old
> one), right?
Thats about right. Maybe just a quick comment?
/*
  * Removes the old xattr key marked with the INCOMPLETE bit
  */

I suppose we could consider something like 
"xfs_attr_node_addname_remv_incomplete"?  Or 
xfs_attr_node_addname_cleanup? Trying to cram it into the name maybe 
getting a bit wordy too.

Allison
> 
> --D
> 
>> +	struct xfs_da_args		*args)
>> +{
>> +	struct xfs_da_state		*state = NULL;
>> +	struct xfs_da_state_blk		*blk;
>> +	int				retval = 0;
>> +	int				error = 0;
>> +
>>   	/*
>>   	 * Re-find the "old" attribute entry after any split ops. The INCOMPLETE
>>   	 * flag means that we will find the "old" attr, not the "new" one.
>> -- 
>> 2.7.4
>>
Darrick J. Wong March 1, 2021, 6 p.m. UTC | #5
On Fri, Feb 26, 2021 at 05:54:51PM -0700, Allison Henderson wrote:
> 
> 
> On 2/25/21 9:02 PM, Darrick J. Wong wrote:
> > On Thu, Feb 18, 2021 at 09:53:32AM -0700, Allison Henderson wrote:
> > > This patch separate xfs_attr_node_addname into two functions.  This will
> > > help to make it easier to hoist parts of xfs_attr_node_addname that need
> > > state management
> > > 
> > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> > > ---
> > >   fs/xfs/libxfs/xfs_attr.c | 20 ++++++++++++++++++++
> > >   1 file changed, 20 insertions(+)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > > index 205ad26..bee8d3fb 100644
> > > --- a/fs/xfs/libxfs/xfs_attr.c
> > > +++ b/fs/xfs/libxfs/xfs_attr.c
> > > @@ -54,6 +54,7 @@ STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
> > >   STATIC int xfs_attr_node_get(xfs_da_args_t *args);
> > >   STATIC int xfs_attr_node_addname(xfs_da_args_t *args);
> > >   STATIC int xfs_attr_node_removename(xfs_da_args_t *args);
> > > +STATIC int xfs_attr_node_addname_work(struct xfs_da_args *args);
> > >   STATIC int xfs_attr_node_hasname(xfs_da_args_t *args,
> > >   				 struct xfs_da_state **state);
> > >   STATIC int xfs_attr_fillstate(xfs_da_state_t *state);
> > > @@ -1059,6 +1060,25 @@ xfs_attr_node_addname(
> > >   			return error;
> > >   	}
> > > +	error = xfs_attr_node_addname_work(args);
> > > +out:
> > > +	if (state)
> > > +		xfs_da_state_free(state);
> > > +	if (error)
> > > +		return error;
> > > +	return retval;
> > > +}
> > > +
> > > +
> > > +STATIC
> > > +int xfs_attr_node_addname_work(
> > 
> > What, erm, work does this function do?  Since it survives to the end of
> > the patchset, I think this needs a better name (or at least needs a
> > comment about what it's actually supposed to do).
> To directly answer the question: it's here to help xfs_attr_set_iter not be
> any bigger than it has to. I think we likely struggled with the name because
> it's almost like it's just the "remainder" of the operation that doesnt need
> state management
> 
> > 
> > AFAICT you're splitting node_addname() into two functions because we're
> > at a transaction roll point, and this "_work" function exists to remove
> > the copy of the xattr key that has the "INCOMPLETE" bit set (aka the old
> > one), right?
> Thats about right. Maybe just a quick comment?
> /*
>  * Removes the old xattr key marked with the INCOMPLETE bit
>  */
> 
> I suppose we could consider something like
> "xfs_attr_node_addname_remv_incomplete"?  Or xfs_attr_node_addname_cleanup?
> Trying to cram it into the name maybe getting a bit wordy too.

xfs_attr_node_addname_clear_incomplete?

--D

> 
> Allison
> > 
> > --D
> > 
> > > +	struct xfs_da_args		*args)
> > > +{
> > > +	struct xfs_da_state		*state = NULL;
> > > +	struct xfs_da_state_blk		*blk;
> > > +	int				retval = 0;
> > > +	int				error = 0;
> > > +
> > >   	/*
> > >   	 * Re-find the "old" attribute entry after any split ops. The INCOMPLETE
> > >   	 * flag means that we will find the "old" attr, not the "new" one.
> > > -- 
> > > 2.7.4
> > >
Allison Henderson March 2, 2021, 8:26 a.m. UTC | #6
On 3/1/21 11:00 AM, Darrick J. Wong wrote:
> On Fri, Feb 26, 2021 at 05:54:51PM -0700, Allison Henderson wrote:
>>
>>
>> On 2/25/21 9:02 PM, Darrick J. Wong wrote:
>>> On Thu, Feb 18, 2021 at 09:53:32AM -0700, Allison Henderson wrote:
>>>> This patch separate xfs_attr_node_addname into two functions.  This will
>>>> help to make it easier to hoist parts of xfs_attr_node_addname that need
>>>> state management
>>>>
>>>> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
>>>> ---
>>>>    fs/xfs/libxfs/xfs_attr.c | 20 ++++++++++++++++++++
>>>>    1 file changed, 20 insertions(+)
>>>>
>>>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>>>> index 205ad26..bee8d3fb 100644
>>>> --- a/fs/xfs/libxfs/xfs_attr.c
>>>> +++ b/fs/xfs/libxfs/xfs_attr.c
>>>> @@ -54,6 +54,7 @@ STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
>>>>    STATIC int xfs_attr_node_get(xfs_da_args_t *args);
>>>>    STATIC int xfs_attr_node_addname(xfs_da_args_t *args);
>>>>    STATIC int xfs_attr_node_removename(xfs_da_args_t *args);
>>>> +STATIC int xfs_attr_node_addname_work(struct xfs_da_args *args);
>>>>    STATIC int xfs_attr_node_hasname(xfs_da_args_t *args,
>>>>    				 struct xfs_da_state **state);
>>>>    STATIC int xfs_attr_fillstate(xfs_da_state_t *state);
>>>> @@ -1059,6 +1060,25 @@ xfs_attr_node_addname(
>>>>    			return error;
>>>>    	}
>>>> +	error = xfs_attr_node_addname_work(args);
>>>> +out:
>>>> +	if (state)
>>>> +		xfs_da_state_free(state);
>>>> +	if (error)
>>>> +		return error;
>>>> +	return retval;
>>>> +}
>>>> +
>>>> +
>>>> +STATIC
>>>> +int xfs_attr_node_addname_work(
>>>
>>> What, erm, work does this function do?  Since it survives to the end of
>>> the patchset, I think this needs a better name (or at least needs a
>>> comment about what it's actually supposed to do).
>> To directly answer the question: it's here to help xfs_attr_set_iter not be
>> any bigger than it has to. I think we likely struggled with the name because
>> it's almost like it's just the "remainder" of the operation that doesnt need
>> state management
>>
>>>
>>> AFAICT you're splitting node_addname() into two functions because we're
>>> at a transaction roll point, and this "_work" function exists to remove
>>> the copy of the xattr key that has the "INCOMPLETE" bit set (aka the old
>>> one), right?
>> Thats about right. Maybe just a quick comment?
>> /*
>>   * Removes the old xattr key marked with the INCOMPLETE bit
>>   */
>>
>> I suppose we could consider something like
>> "xfs_attr_node_addname_remv_incomplete"?  Or xfs_attr_node_addname_cleanup?
>> Trying to cram it into the name maybe getting a bit wordy too.
> 
> xfs_attr_node_addname_clear_incomplete?
I'm fine with that as long as everyone else is :-)

Allison
> 
> --D
> 
>>
>> Allison
>>>
>>> --D
>>>
>>>> +	struct xfs_da_args		*args)
>>>> +{
>>>> +	struct xfs_da_state		*state = NULL;
>>>> +	struct xfs_da_state_blk		*blk;
>>>> +	int				retval = 0;
>>>> +	int				error = 0;
>>>> +
>>>>    	/*
>>>>    	 * Re-find the "old" attribute entry after any split ops. The INCOMPLETE
>>>>    	 * flag means that we will find the "old" attr, not the "new" one.
>>>> -- 
>>>> 2.7.4
>>>>
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 205ad26..bee8d3fb 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -54,6 +54,7 @@  STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
 STATIC int xfs_attr_node_get(xfs_da_args_t *args);
 STATIC int xfs_attr_node_addname(xfs_da_args_t *args);
 STATIC int xfs_attr_node_removename(xfs_da_args_t *args);
+STATIC int xfs_attr_node_addname_work(struct xfs_da_args *args);
 STATIC int xfs_attr_node_hasname(xfs_da_args_t *args,
 				 struct xfs_da_state **state);
 STATIC int xfs_attr_fillstate(xfs_da_state_t *state);
@@ -1059,6 +1060,25 @@  xfs_attr_node_addname(
 			return error;
 	}
 
+	error = xfs_attr_node_addname_work(args);
+out:
+	if (state)
+		xfs_da_state_free(state);
+	if (error)
+		return error;
+	return retval;
+}
+
+
+STATIC
+int xfs_attr_node_addname_work(
+	struct xfs_da_args		*args)
+{
+	struct xfs_da_state		*state = NULL;
+	struct xfs_da_state_blk		*blk;
+	int				retval = 0;
+	int				error = 0;
+
 	/*
 	 * Re-find the "old" attribute entry after any split ops. The INCOMPLETE
 	 * flag means that we will find the "old" attr, not the "new" one.