[v6,12/16] xfs: Add helper function xfs_attr_init_unmapstate
diff mbox series

Message ID 20200118225035.19503-13-allison.henderson@oracle.com
State New
Headers show
Series
  • xfs: Delay Ready Attributes
Related show

Commit Message

Allison Collins Jan. 18, 2020, 10:50 p.m. UTC
This patch helps to pre-simplify xfs_attr_node_removename by modularizing
the code around the transactions into helper functions.  This will make
the function easier to follow when we introduce delayed attributes.

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

Comments

Darrick J. Wong Jan. 21, 2020, 11:21 p.m. UTC | #1
On Sat, Jan 18, 2020 at 03:50:31PM -0700, Allison Collins wrote:
> This patch helps to pre-simplify xfs_attr_node_removename by modularizing
> the code around the transactions into helper functions.  This will make
> the function easier to follow when we introduce delayed attributes.
> 
> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 45 +++++++++++++++++++++++++++++++--------------
>  1 file changed, 31 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index e9d22c1..453ea59 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -1202,6 +1202,36 @@ xfs_attr_node_addname(
>  }
>  
>  /*
> + * Set up the state for unmap and set the incomplete flag

"Mark an attribute entry INCOMPLETE and save pointers to the relevant
buffers for later deletion of the entry." ?

> + */
> +STATIC int
> +xfs_attr_init_unmapstate(

I'm hung up on this name -- it marks the entry incomplete and saves some
breadcrumbs for erasing data later, which I guess is "unmap state"?
What do you think of:

xfs_attr_leaf_mark_incomplete()

> +	struct xfs_da_args	*args,
> +	struct xfs_da_state	*state)
> +{
> +	int error;
> +
> +	/*
> +	 * Fill in disk block numbers in the state structure
> +	 * so that we can get the buffers back after we commit
> +	 * several transactions in the following calls.
> +	 */
> +	error = xfs_attr_fillstate(state);
> +	if (error)
> +		return error;
> +
> +	/*
> +	 * Mark the attribute as INCOMPLETE

Has this stopped doing the bunmapi call, or was the comment inaccurate?
I'm guessing the second if it's necessary to save state to recover
buffer pointers later...?

--D

> +	 */
> +	error = xfs_attr3_leaf_setflag(args);
> +	if (error)
> +		return error;
> +
> +	return 0;
> +}
> +
> +
> +/*
>   * Remove a name from a B-tree attribute list.
>   *
>   * This will involve walking down the Btree, and may involve joining
> @@ -1233,20 +1263,7 @@ xfs_attr_node_removename(
>  	ASSERT(blk->bp != NULL);
>  	ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC);
>  	if (args->rmtblkno > 0) {
> -		/*
> -		 * Fill in disk block numbers in the state structure
> -		 * so that we can get the buffers back after we commit
> -		 * several transactions in the following calls.
> -		 */
> -		error = xfs_attr_fillstate(state);
> -		if (error)
> -			goto out;
> -
> -		/*
> -		 * Mark the attribute as INCOMPLETE, then bunmapi() the
> -		 * remote value.
> -		 */
> -		error = xfs_attr3_leaf_setflag(args);
> +		error = xfs_attr_init_unmapstate(args, state);
>  		if (error)
>  			goto out;
>  
> -- 
> 2.7.4
>
Allison Collins Jan. 22, 2020, 5:05 a.m. UTC | #2
On 1/21/20 4:21 PM, Darrick J. Wong wrote:
> On Sat, Jan 18, 2020 at 03:50:31PM -0700, Allison Collins wrote:
>> This patch helps to pre-simplify xfs_attr_node_removename by modularizing
>> the code around the transactions into helper functions.  This will make
>> the function easier to follow when we introduce delayed attributes.
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> ---
>>   fs/xfs/libxfs/xfs_attr.c | 45 +++++++++++++++++++++++++++++++--------------
>>   1 file changed, 31 insertions(+), 14 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index e9d22c1..453ea59 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -1202,6 +1202,36 @@ xfs_attr_node_addname(
>>   }
>>   
>>   /*
>> + * Set up the state for unmap and set the incomplete flag
> 
> "Mark an attribute entry INCOMPLETE and save pointers to the relevant
> buffers for later deletion of the entry." ?
I am fine with that if folks prefer.

> 
>> + */
>> +STATIC int
>> +xfs_attr_init_unmapstate(
> 
> I'm hung up on this name -- it marks the entry incomplete and saves some
> breadcrumbs for erasing data later, which I guess is "unmap state"?
> What do you think of:
> 
> xfs_attr_leaf_mark_incomplete()

I actually struggled a bit with what to call it.  I called it an init 
because in the immediately proceeding code we fall into a loop of 
bunmapi and refilling the state, rolling the transaction in between. 
But we only make this initial call once before we start doing that. 
Later when we get into the state machine this turns into multiple 
iterations of jumping in and out of this region of code.  Even though 
the while() syntax is gone, it still ends up functioning very much like 
a loop.

This helper function isnt so much necessary as much as it is an effort 
to cut down on the size of xfs_attr_node_removename which was getting 
quite lengthy.

In anycase, xfs_attr_leaf_mark_incomplete is fine with me :-)

> 
>> +	struct xfs_da_args	*args,
>> +	struct xfs_da_state	*state)
>> +{
>> +	int error;
>> +
>> +	/*
>> +	 * Fill in disk block numbers in the state structure
>> +	 * so that we can get the buffers back after we commit
>> +	 * several transactions in the following calls.
>> +	 */
>> +	error = xfs_attr_fillstate(state);
>> +	if (error)
>> +		return error;
>> +
>> +	/*
>> +	 * Mark the attribute as INCOMPLETE
> 
> Has this stopped doing the bunmapi call, or was the comment inaccurate?
> I'm guessing the second if it's necessary to save state to recover
> buffer pointers later...?
Right, I clipped off the comment because we dont do the bunmapi in this 
function scope.  That happens later in the loop behavior described above.

Hope that helps!

Allison

> 
> --D
> 
>> +	 */
>> +	error = xfs_attr3_leaf_setflag(args);
>> +	if (error)
>> +		return error;
>> +
>> +	return 0;
>> +}
>> +
>> +
>> +/*
>>    * Remove a name from a B-tree attribute list.
>>    *
>>    * This will involve walking down the Btree, and may involve joining
>> @@ -1233,20 +1263,7 @@ xfs_attr_node_removename(
>>   	ASSERT(blk->bp != NULL);
>>   	ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC);
>>   	if (args->rmtblkno > 0) {
>> -		/*
>> -		 * Fill in disk block numbers in the state structure
>> -		 * so that we can get the buffers back after we commit
>> -		 * several transactions in the following calls.
>> -		 */
>> -		error = xfs_attr_fillstate(state);
>> -		if (error)
>> -			goto out;
>> -
>> -		/*
>> -		 * Mark the attribute as INCOMPLETE, then bunmapi() the
>> -		 * remote value.
>> -		 */
>> -		error = xfs_attr3_leaf_setflag(args);
>> +		error = xfs_attr_init_unmapstate(args, state);
>>   		if (error)
>>   			goto out;
>>   
>> -- 
>> 2.7.4
>>

Patch
diff mbox series

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index e9d22c1..453ea59 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -1202,6 +1202,36 @@  xfs_attr_node_addname(
 }
 
 /*
+ * Set up the state for unmap and set the incomplete flag
+ */
+STATIC int
+xfs_attr_init_unmapstate(
+	struct xfs_da_args	*args,
+	struct xfs_da_state	*state)
+{
+	int error;
+
+	/*
+	 * Fill in disk block numbers in the state structure
+	 * so that we can get the buffers back after we commit
+	 * several transactions in the following calls.
+	 */
+	error = xfs_attr_fillstate(state);
+	if (error)
+		return error;
+
+	/*
+	 * Mark the attribute as INCOMPLETE
+	 */
+	error = xfs_attr3_leaf_setflag(args);
+	if (error)
+		return error;
+
+	return 0;
+}
+
+
+/*
  * Remove a name from a B-tree attribute list.
  *
  * This will involve walking down the Btree, and may involve joining
@@ -1233,20 +1263,7 @@  xfs_attr_node_removename(
 	ASSERT(blk->bp != NULL);
 	ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC);
 	if (args->rmtblkno > 0) {
-		/*
-		 * Fill in disk block numbers in the state structure
-		 * so that we can get the buffers back after we commit
-		 * several transactions in the following calls.
-		 */
-		error = xfs_attr_fillstate(state);
-		if (error)
-			goto out;
-
-		/*
-		 * Mark the attribute as INCOMPLETE, then bunmapi() the
-		 * remote value.
-		 */
-		error = xfs_attr3_leaf_setflag(args);
+		error = xfs_attr_init_unmapstate(args, state);
 		if (error)
 			goto out;