diff mbox series

[03/18] xfs: make xattri_leaf_bp more useful

Message ID 20220509004138.762556-4-david@fromorbit.com (mailing list archive)
State Accepted, archived
Headers show
Series XFS: LARP state machine and recovery rework | expand

Commit Message

Dave Chinner May 9, 2022, 12:41 a.m. UTC 
From: Dave Chinner <dchinner@redhat.com>

We currently set it and hold it when converting from short to leaf
form, then release it only to immediately look it back up again
to do the leaf insert.

Do a bit of refactoring to xfs_attr_leaf_try_add() to avoid this
messy handling of the newly allocated leaf buffer.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Allison Henderson<allison.henderson@oracle.com>
---
 fs/xfs/libxfs/xfs_attr.c | 50 +++++++++++++++++++++++++---------------
 1 file changed, 32 insertions(+), 18 deletions(-)

Comments

Darrick J. Wong May 10, 2022, 10:58 p.m. UTC | #1 
On Mon, May 09, 2022 at 10:41:23AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> We currently set it and hold it when converting from short to leaf
> form, then release it only to immediately look it back up again
> to do the leaf insert.
> 
> Do a bit of refactoring to xfs_attr_leaf_try_add() to avoid this
> messy handling of the newly allocated leaf buffer.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Allison Henderson<allison.henderson@oracle.com>

Took me a while to figure this out, but looks fine to me.
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_attr.c | 50 +++++++++++++++++++++++++---------------
>  1 file changed, 32 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index b3d918195160..a4b0b20a3bab 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -319,7 +319,15 @@ xfs_attr_leaf_addname(
>  	int			error;
>  
>  	if (xfs_attr_is_leaf(dp)) {
> +
> +		/*
> +		 * Use the leaf buffer we may already hold locked as a result of
> +		 * a sf-to-leaf conversion. The held buffer is no longer valid
> +		 * after this call, regardless of the result.
> +		 */
>  		error = xfs_attr_leaf_try_add(args, attr->xattri_leaf_bp);
> +		attr->xattri_leaf_bp = NULL;
> +
>  		if (error == -ENOSPC) {
>  			error = xfs_attr3_leaf_to_node(args);
>  			if (error)
> @@ -341,6 +349,8 @@ xfs_attr_leaf_addname(
>  		}
>  		next_state = XFS_DAS_FOUND_LBLK;
>  	} else {
> +		ASSERT(!attr->xattri_leaf_bp);
> +
>  		error = xfs_attr_node_addname_find_attr(attr);
>  		if (error)
>  			return error;
> @@ -396,12 +406,6 @@ xfs_attr_set_iter(
>  		 */
>  		if (xfs_attr_is_shortform(dp))
>  			return xfs_attr_sf_addname(attr);
> -		if (attr->xattri_leaf_bp != NULL) {
> -			xfs_trans_bhold_release(args->trans,
> -						attr->xattri_leaf_bp);
> -			attr->xattri_leaf_bp = NULL;
> -		}
> -
>  		return xfs_attr_leaf_addname(attr);
>  
>  	case XFS_DAS_FOUND_LBLK:
> @@ -992,18 +996,31 @@ xfs_attr_leaf_try_add(
>  	struct xfs_da_args	*args,
>  	struct xfs_buf		*bp)
>  {
> -	int			retval;
> +	int			error;
>  
>  	/*
> -	 * Look up the given attribute in the leaf block.  Figure out if
> -	 * the given flags produce an error or call for an atomic rename.
> +	 * If the caller provided a buffer to us, it is locked and held in
> +	 * the transaction because it just did a shortform to leaf conversion.
> +	 * Hence we don't need to read it again. Otherwise read in the leaf
> +	 * buffer.
>  	 */
> -	retval = xfs_attr_leaf_hasname(args, &bp);
> -	if (retval != -ENOATTR && retval != -EEXIST)
> -		return retval;
> -	if (retval == -ENOATTR && (args->attr_flags & XATTR_REPLACE))
> +	if (bp) {
> +		xfs_trans_bhold_release(args->trans, bp);
> +	} else {
> +		error = xfs_attr3_leaf_read(args->trans, args->dp, 0, &bp);
> +		if (error)
> +			return error;
> +	}
> +
> +	/*
> +	 * Look up the xattr name to set the insertion point for the new xattr.
> +	 */
> +	error = xfs_attr3_leaf_lookup_int(bp, args);
> +	if (error != -ENOATTR && error != -EEXIST)
>  		goto out_brelse;
> -	if (retval == -EEXIST) {
> +	if (error == -ENOATTR && (args->attr_flags & XATTR_REPLACE))
> +		goto out_brelse;
> +	if (error == -EEXIST) {
>  		if (args->attr_flags & XATTR_CREATE)
>  			goto out_brelse;
>  
> @@ -1023,14 +1040,11 @@ xfs_attr_leaf_try_add(
>  		args->rmtvaluelen = 0;
>  	}
>  
> -	/*
> -	 * Add the attribute to the leaf block
> -	 */
>  	return xfs_attr3_leaf_add(bp, args);
>  
>  out_brelse:
>  	xfs_trans_brelse(args->trans, bp);
> -	return retval;
> +	return error;
>  }
>  
>  /*
> -- 
> 2.35.1
>
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index b3d918195160..a4b0b20a3bab 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -319,7 +319,15 @@  xfs_attr_leaf_addname(
 	int			error;
 
 	if (xfs_attr_is_leaf(dp)) {
+
+		/*
+		 * Use the leaf buffer we may already hold locked as a result of
+		 * a sf-to-leaf conversion. The held buffer is no longer valid
+		 * after this call, regardless of the result.
+		 */
 		error = xfs_attr_leaf_try_add(args, attr->xattri_leaf_bp);
+		attr->xattri_leaf_bp = NULL;
+
 		if (error == -ENOSPC) {
 			error = xfs_attr3_leaf_to_node(args);
 			if (error)
@@ -341,6 +349,8 @@  xfs_attr_leaf_addname(
 		}
 		next_state = XFS_DAS_FOUND_LBLK;
 	} else {
+		ASSERT(!attr->xattri_leaf_bp);
+
 		error = xfs_attr_node_addname_find_attr(attr);
 		if (error)
 			return error;
@@ -396,12 +406,6 @@  xfs_attr_set_iter(
 		 */
 		if (xfs_attr_is_shortform(dp))
 			return xfs_attr_sf_addname(attr);
-		if (attr->xattri_leaf_bp != NULL) {
-			xfs_trans_bhold_release(args->trans,
-						attr->xattri_leaf_bp);
-			attr->xattri_leaf_bp = NULL;
-		}
-
 		return xfs_attr_leaf_addname(attr);
 
 	case XFS_DAS_FOUND_LBLK:
@@ -992,18 +996,31 @@  xfs_attr_leaf_try_add(
 	struct xfs_da_args	*args,
 	struct xfs_buf		*bp)
 {
-	int			retval;
+	int			error;
 
 	/*
-	 * Look up the given attribute in the leaf block.  Figure out if
-	 * the given flags produce an error or call for an atomic rename.
+	 * If the caller provided a buffer to us, it is locked and held in
+	 * the transaction because it just did a shortform to leaf conversion.
+	 * Hence we don't need to read it again. Otherwise read in the leaf
+	 * buffer.
 	 */
-	retval = xfs_attr_leaf_hasname(args, &bp);
-	if (retval != -ENOATTR && retval != -EEXIST)
-		return retval;
-	if (retval == -ENOATTR && (args->attr_flags & XATTR_REPLACE))
+	if (bp) {
+		xfs_trans_bhold_release(args->trans, bp);
+	} else {
+		error = xfs_attr3_leaf_read(args->trans, args->dp, 0, &bp);
+		if (error)
+			return error;
+	}
+
+	/*
+	 * Look up the xattr name to set the insertion point for the new xattr.
+	 */
+	error = xfs_attr3_leaf_lookup_int(bp, args);
+	if (error != -ENOATTR && error != -EEXIST)
 		goto out_brelse;
-	if (retval == -EEXIST) {
+	if (error == -ENOATTR && (args->attr_flags & XATTR_REPLACE))
+		goto out_brelse;
+	if (error == -EEXIST) {
 		if (args->attr_flags & XATTR_CREATE)
 			goto out_brelse;
 
@@ -1023,14 +1040,11 @@  xfs_attr_leaf_try_add(
 		args->rmtvaluelen = 0;
 	}
 
-	/*
-	 * Add the attribute to the leaf block
-	 */
 	return xfs_attr3_leaf_add(bp, args);
 
 out_brelse:
 	xfs_trans_brelse(args->trans, bp);
-	return retval;
+	return error;
 }
 
 /*